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.

Attachments (0)

Change History (0)

Add Comment

Modify Ticket

Change Properties
Action
as new .
as The resolution will be set. Next status will be 'closed'.
to The owner will be changed from (none).
Next status will be 'reviewing'.
 
Note: See TracTickets for help on using tickets.