Issue #4694 - Add mutex protection to JS event handling

src/Event.h

Index: src/Event.h
===================================================================
new file mode 100644
--- /dev/null
+++ b/src/Event.h
@@ -0,0 +1,80 @@
+/*

[sergei, 2017/03/15 12:17:20]

I think the file should be called EventManager.

+ * This file is part of Adblock Plus <https://adblockplus.org/>,
+ * Copyright (C) 2006-2016 Eyeo GmbH

[sergei, 2017/03/15 12:17:20]

It's already 2017.

+ *
+ * Adblock Plus is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 3 as
+ * published by the Free Software Foundation.
+ *
+ * Adblock Plus is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#if !defined(EVENT_H)
+#define EVENT_H
+
+#include <map>
+#include <mutex>
+#include <v8.h>
+#include <AdblockPlus/JsEngine.h>
+
+/**
+ * Event manager class encapsulates an atomic map from names to event functions.
+ *
+ * The present version of this class has a simple event registry.
+ * Only one callback function may be registered per event.
+ *
+ * Event methods are synchronized on the event registry, not event execution.
+ * Once the callback function of an event executes it will continue to run
+ *   even if the event is deleted before the callback finishes.
+ */
+class EventManager
+{
+  /**
+   * Map from event names to callback functions.
+   *
+   */
+  std::map<std::string, AdblockPlus::JsEngine::EventCallback> eventMap;
+
+  std::mutex m;

[sergei, 2017/03/15 12:17:20]

Don't mind to call it mutex instead of m?

+
+  typedef std::unique_lock<std::mutex> UniqueLockType;

[sergei, 2017/03/15 12:17:20]

Why not to use std::lock_guard?

+
+public:
+  EventManager() {}; // = default;

[sergei, 2017/03/15 12:17:20]

I still don't see any reason to have such code and this comment.

+
+  /**
+    * Set the callback function for an event.
+    * Create event if not present; ovewrite callback if it is.
+    * @param eventName
+    *   Event name. Any string is permitted here. 
+    *   It's not limited to valid JavaScript identifiers.
+    * @param callback
+    *   Function called when event triggers
+    */
+  void Set(const std::string& eventName, AdblockPlus::JsEngine::EventCallback callback);

[sergei, 2017/03/15 12:17:20]

It's better to use const reference for callback argument.

+
+  /**
+    * Remove an event and its callback function.
+    * The event need not be present.

[sergei, 2017/03/15 12:17:20]

Actually it won't trigger any error if there is nothing with such name.

+    * @param eventName
+    *   Event name
+    */
+  void Remove(const std::string& eventName);
+
+  /**
+    * Trigger an event and execute its callback synchronously.

[sergei, 2017/03/15 12:17:20]

What does "execute its callback synchronously" mean? Only the access to internal
collection is serialized but the execution is beyond it.

For the sake of generated documentation I would say that this method executes
corresponding callback and does not lock the internal collection during the
execution of callback.

+    * @param eventName
+    *   Event name
+    * @param args
+    *   The callback function runs with these arguments passed.
+    */
+  void Trigger(const std::string& eventName, AdblockPlus::JsValueList& args);

[sergei, 2017/03/15 12:17:20]

What about adding const to args? It's clear that underlying JS objects can be
changed and it's actually expected in almost all places in libadblockplus but it
is still useful to know that we are not changing args as a collection.

[sergei, 2017/03/15 12:17:20]

What about adding const modifier to this method?

+};
+
+#endif