Issue #4694 - Add mutex protection to JS event handling

Issue #4694 - Add mutex protection to JS event handling

Encapsulate event map in a new class `EventManager`, which also contains a
mutex to eliminate data races on the map. Forward the event management methods
in `JsEngine` to corresponding methods on the internal manager instance.

[Eric, 2016-12-21 19:39:13]

This change set does not change the binding of "_triggerEvent" on the global
object. For code hygiene, its callback function ought be moved from
"GlobalJsObject.cpp" where it is now and into "Event.h". That change is out of
scope for #4694 and can be done in a separate commit.

[sergei, 2017-03-15 12:17:21]

Could you please rebase it on current master? It's already required for android.

https://codereview.adblockplus.org/29369479/diff/29369487/src/Event.cpp
File src/Event.cpp (right):

https://codereview.adblockplus.org/29369479/diff/29369487/src/Event.cpp#newco...
src/Event.cpp:35: * A triggered event runs with mutex unlocked.
I don't think we need this comment.

https://codereview.adblockplus.org/29369479/diff/29369487/src/Event.cpp#newco...
src/Event.cpp:41: // Find the callback with mutex locked
this comment is superfluous.

https://codereview.adblockplus.org/29369479/diff/29369487/src/Event.cpp#newco...
src/Event.cpp:50: // Execute the callback with mutex unlocked
this comment is superfluous.

https://codereview.adblockplus.org/29369479/diff/29369487/src/Event.h
File src/Event.h (right):

https://codereview.adblockplus.org/29369479/diff/29369487/src/Event.h#newcode1
src/Event.h:1: /*
I think the file should be called EventManager.

https://codereview.adblockplus.org/29369479/diff/29369487/src/Event.h#newcode3
src/Event.h:3: * Copyright (C) 2006-2016 Eyeo GmbH
It's already 2017.

https://codereview.adblockplus.org/29369479/diff/29369487/src/Event.h#newcode44
src/Event.h:44: std::mutex m;
Don't mind to call it mutex instead of m?

https://codereview.adblockplus.org/29369479/diff/29369487/src/Event.h#newcode46
src/Event.h:46: typedef std::unique_lock<std::mutex> UniqueLockType;
Why not to use std::lock_guard?

https://codereview.adblockplus.org/29369479/diff/29369487/src/Event.h#newcode49
src/Event.h:49: EventManager() {}; // = default;
I still don't see any reason to have such code and this comment.

https://codereview.adblockplus.org/29369479/diff/29369487/src/Event.h#newcode60
src/Event.h:60: void Set(const std::string& eventName,
AdblockPlus::JsEngine::EventCallback callback);
It's better to use const reference for callback argument.

https://codereview.adblockplus.org/29369479/diff/29369487/src/Event.h#newcode64
src/Event.h:64: * The event need not be present.
Actually it won't trigger any error if there is nothing with such name.

https://codereview.adblockplus.org/29369479/diff/29369487/src/Event.h#newcode71
src/Event.h:71: * Trigger an event and execute its callback synchronously.
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.

https://codereview.adblockplus.org/29369479/diff/29369487/src/Event.h#newcode77
src/Event.h:77: void Trigger(const std::string& eventName,
AdblockPlus::JsValueList& args);
What about adding const modifier to this method?

https://codereview.adblockplus.org/29369479/diff/29369487/src/Event.h#newcode77
src/Event.h:77: void Trigger(const std::string& eventName,
AdblockPlus::JsValueList& args);
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.

https://codereview.adblockplus.org/29369479/diff/29369487/src/JsEngineInternal.h
File src/JsEngineInternal.h (right):

https://codereview.adblockplus.org/29369479/diff/29369487/src/JsEngineInterna...
src/JsEngineInternal.h:83: EventManager eventMan;
what about "ager"?

[Eric, 2017-03-29 18:25:01]

On 2017/03/15 12:17:21, sergei wrote:
> Could you please rebase it on current master? It's already required for
android.

No, that's impossible.

You waited twelve weeks to make a first response to this. Had you put any
attention to what I was doing then, you would know that there are *four*
previous reviews that precede this one, and upon which this one depends. You
waited six weeks even to respond to the first one, and then you wanted to bicker
about code that was already removed in later work. I moved on to work on
something else.

So I can't rebase this to master because it was never based on master to begin
with.

In the interim, there have been too many changes to even consider trying to
rebase the whole branch leading to this. That's part of the price of a "quick
hack".

[sergei, 2017-03-30 12:56:14]

On 2017/03/29 18:25:01, Eric wrote:
> On 2017/03/15 12:17:21, sergei wrote:
> > Could you please rebase it on current master? It's already required for
> android.
> 
> No, that's impossible.
It is possible. Anyway it's not important anymore because it's already landed,
though I find it still good to move eventCallbacks with its mutex and methods
into a separate class, see comment in issue.
> 
> You waited twelve weeks to make a first response to this. Had you put any
> attention to what I was doing then, you would know that there are *four*
> previous reviews that precede this one, and upon which this one depends. You
> waited six weeks even to respond to the first one, and then you wanted to
bicker
> about code that was already removed in later work. I moved on to work on
> something else.
Of course I have seen all your reviews but it makes no sense to comment all of
them because the appearance of them will be significantly changed while fixing
things in reviews going before.
> 
> So I can't rebase this to master because it was never based on master to begin
> with.
I don't see anything in description what requires a functionality from other
codereviews, so it was absolutely possible to rebase it on master.
> 
> In the interim, there have been too many changes to even consider trying to
> rebase the whole branch leading to this. That's part of the price of a "quick
> hack".
I think that there is actually no necessity to rebase the whole branch, we just
need to pick only required functionality from it.

[Eric, 2017-03-30 13:11:18]

On 2017/03/30 12:56:14, sergei wrote:
> I think that there is actually no necessity to rebase the whole branch, we
just
> need to pick only required functionality from it.

If it didn't depend on code that was in the prior commits, then it wouldn't be
necessary to rebase the branch.

What you're asking for is a rewrite, not a rebase.

Next time don't wait three months.