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

src/JsEngine.cpp

Index: src/JsEngine.cpp
diff --git a/src/JsEngine.cpp b/src/JsEngine.cpp
index 657242ee38bb7ef14184a213ea515d3bd510a8ab..f874cf97c7486193aa51c1ee55a6b8c45ebf1adf 100644
--- a/src/JsEngine.cpp
+++ b/src/JsEngine.cpp
@@ -117,19 +117,27 @@ AdblockPlus::JsValuePtr AdblockPlus::JsEngine::Evaluate(const std::string& sourc
 void AdblockPlus::JsEngine::SetEventCallback(const std::string& eventName,
     AdblockPlus::JsEngine::EventCallback callback)
 {
+  std::lock_guard<std::mutex> lock(eventCallbacksMutex);
   eventCallbacks[eventName] = callback;
 }
 
 void AdblockPlus::JsEngine::RemoveEventCallback(const std::string& eventName)
 {
+  std::lock_guard<std::mutex> lock(eventCallbacksMutex);
   eventCallbacks.erase(eventName);
 }
 
 void AdblockPlus::JsEngine::TriggerEvent(const std::string& eventName, AdblockPlus::JsValueList& params)
 {
-  EventMap::iterator it = eventCallbacks.find(eventName);
-  if (it != eventCallbacks.end())
-    it->second(params);
+  EventCallback callback;
+  {
+    std::lock_guard<std::mutex> lock(eventCallbacksMutex);
+    auto it = eventCallbacks.find(eventName);
+    if (it == eventCallbacks.end())
+      return;
+    callback = it->second;
+  }
+  callback(params);

[Oleksandr, 2017/03/20 20:00:42]

Nit: Maybe check if callback is not null JIC?

[sergei, 2017/03/20 20:08:13]

What about such check in SetEventCallback? Maybe even removing of existing
callback if an empty value is passed into SetEventCallback, though I would like
to avoid such dualism, it definitely should not be a documented way while there
is RemoveEventCallback.

[sergei, 2017/03/20 20:23:24]

Actually, I think it should be in another commit.

[Oleksandr, 2017/03/20 20:25:25]

Fair enough. It isn't that big of an issue IMO, so, again, LGTM.

 }
 
 void AdblockPlus::JsEngine::Gc()