Opened on 01/22/2015 at 01:43:05 PM
#1849 new change
The current usage of jsEngine->SetEventCallback from FilterEngine can be dangerous
Reported by: | sergz | Assignee: | |
---|---|---|---|
Priority: | Unknown | Milestone: | |
Module: | Libadblockplus | Keywords: | |
Cc: | Blocked By: | ||
Blocking: | Platform: | Unknown | |
Ready: | no | Confidential: | no |
Tester: | Verified working: | ||
Review URL(s): |
Description
For example:
void FilterEngine::SetFilterChangeCallback(FilterEngine::FilterChangeCallback callback) { jsEngine->SetEventCallback("filterChange", std::tr1::bind(&FilterEngine::FilterChanged, this, callback, std::tr1::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 triggers the event on JsEngine then we will try to access FilterEngine::jsEngine which is already not available from FilterEngine::FilterChanged.
I guess we don't see it in ABP engine for IE because the exit from the process is similar to killing it. I'm not sure about this particular case with FilterEngine::FilterChanged but in general the issue is that We start some async operation which is being executed in another thread, for example WebRequestThread which holds AdblockPlus::JsEnginePtr jsEngine. Then the main thread destroys FilterEngine. The web request finishes downloading and calls js function which triggers the event.
What to change
Firstly I would like to review the threading model, it would be great if we can fix it by design.