The current usage of jsEngine->SetEventCallback from FilterEngine can be dangerous

[sergz]

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.