Opened on 12/02/2016 at 07:09:55 PM
#4693 new change
Race condition: methods of FilterEngine can be called by JsEngine after destroying of former
| Reported by: | sergz | Assignee: | |
|---|---|---|---|
| Priority: | P4 | Milestone: | |
| Module: | Libadblockplus | Keywords: | |
| Cc: | fhd | Blocked By: | |
| Blocking: | Platform: | Unknown / Cross platform | |
| Ready: | no | Confidential: | no |
| Tester: | Unknown | Verified working: | no |
| Review URL(s): | |||
Description
Background
The current usage of jsEngine->SetEventCallback from FilterEngine can lead to data race and race condition.
Let's considre the following code:
void FilterEngine::SetFilterChangeCallback(FilterEngine::FilterChangeCallback callback)
{
jsEngine->SetEventCallback("filterChange", std::bind(&FilterEngine::FilterChanged,
this, callback, std::placeholders::_1));
}
void FilterEngine::FilterChanged(FilterEngine::FilterChangeCallback callback, JsValueList& params)
{
std::string action(params.size() >= 1 && !params[0]->IsNull() ? params[0]->AsString() : "");
JsValuePtr item(params.size() >= 2 ? params[1] : jsEngine->NewValue(false));
callback(action, item);
}
If FilterEngine is destroyed and somebody (for instance our background thread) triggers the event on JsEngine then we will try to access FilterEngine::jsEngine which is already not available from FilterEngine::FilterChanged because there is already no that instance of FilterEngine. It can easily happen in tests where we destroy FilterEngine.
It seems the best option is to
fhd: Maybe we should somehow "own" the JsEngine instance from FilterEngine then, and make it unusable when FilterEngine is destroyed?
because it makes no sense to only remove (of course having {Set,Remove}EventCallback thread-safe) installed callbacks from JsEngine while having the rest code (JS) still running.
