JsEngine::{SetEventCallback,RemoveEventCallback,TriggerEvent} should be thread safe.

[sergz]

Background

Beside the fact that user can call these methods from different threads, it can happen that while a user calls (or calls indirectly by calling corresponding methods of FilterEngine), e.g. SetEventCallback some background operation calls TriggerEvent and they both access JsEngine::eventCallbacks while the former modifies it.

What to change

Use JsContext instance as synchronization sentry in these methods.

[eric@adblockplus.org]

Review ​https://codereview.adblockplus.org/29369479/ uses std::mutex for synchronization.

Using JsContext would gratuitously convoluted at best. The only synchronization mechanism in JsContext is its member v8::Locker, which only synchronizes v8 engine operations. It would be possible to use v8::Locker by evaluating some JS code (to get into the engine and avail ourselves of its lock) and then calling back out it back to our code. It's much, much easier just to use standard C++ synchronization (with C++11).

[sergz]

I agree that JsContext is too much and we should rather use only v8::Locker.

The approach with using of std::mutex has its own advantages, however creating the issue I thought that v8::Locker might be more efficient here during the main work of application because we usually configure callbacks only once, mostly before running of JS code and experience calls of TriggerEvent many times later. With v8::Locker TriggerEvent might be a bit faster because v8::Locker firstly checks whether it's already locked by current thread what is indeed happening all the time for TriggerEvent because it's called only from JavaScript which has already acquired that lock in the current thread. So, acquiring of std::mutex here can be a bit heavier operation. Though, I'm not sure whether we can actually observer the impact of different approaches.

[abpbot]

A commit referencing this issue has landed:
​Issue 4694 - JsEngine::{SetEventCallback,RemoveEventCallback,TriggerEvent} should be thread safe.

[sergz]

@Eric, I think now ​https://codereview.adblockplus.org/29369479/ is rather a refactoring and should be a part of another issue or as noissue, so I'm removing it from the field "Review URL(s)". JIC, I don't have objections about extracting the functionality into a separate class.