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.