[Extensions Bindings] Notify of event unregistration on invalidation

extensions/renderer/event_emitter.cc

Index: extensions/renderer/event_emitter.cc
diff --git a/extensions/renderer/event_emitter.cc b/extensions/renderer/event_emitter.cc
index 96eab80b41dbe84b29b28b8d0c17eed8d1de25c7..a9c44d0ed1feabbe36e351f9969f8c2f692e4614 100644
--- a/extensions/renderer/event_emitter.cc
+++ b/extensions/renderer/event_emitter.cc
@@ -52,7 +52,15 @@ void EventEmitter::Fire(v8::Local<v8::Context> context,
   }
 }
 
+void EventEmitter::Invalidate() {
+  valid_ = false;
+  listeners_.clear();
+}
+
 void EventEmitter::AddListener(gin::Arguments* arguments) {
+  if (!valid_)

[jbroman, 2017/03/01 17:49:47]

This won't catch such objects which don't are created after the fact. If we're
worried about listeners being added after the context has begun to be torn down
(not sure whether there's a way to run script as that happens -- hope not), then
we should be equally worried about event instances that haven't yet been created
(but will be created by an accessor when they are first reached). For that
reason I'd expect this state to be checked on the PerContextData, rather than
being a per-emitter bit.

[Devlin, 2017/03/01 19:03:50]

Yeah, I still don't have a complete grasp of all the weirdness that can happen.
:)

Re new event emitters: we currently have a DCHECK(per_context_data) in
CreateEventInstance() that should prevent new emitters from being created once
tear down has started and the PerContextData is destroyed.  Depending on if we
see crashes, we may need to change it to a graceful handling.

Re this check: my (very holey) understanding was that sometimes contexts (or
objects from the context) can leak even after willReleaseScriptContext() is
called by blink.  In that case, an object like a specific event, if held onto by
another object, could maybe be called after the fact.  That's what this is
guarding against.  Is that not a concern?

[jbroman, 2017/03/01 19:26:34]

Well, that's slightly later than WillReleaseScriptContext, but I don't think
script can run in between (basically I'd have to audit every override of
WillReleaseScriptContext to make sure none of them run script -- but doing so
seems insane, so I'll assume not).
Ah yes, you're quite correct. My bad. :)

If you are going to keep this as a bit on the emitters themselves as opposed to
on the context (fair enough), would you mind a comment in
APIEventHandler::InvalidateContext explaining that no more event emitters get
created after that point?

[Devlin, 2017/03/02 17:26:32]

Done.  Added comments in InvalidateContext() and also AddListener() to explain
what we're guarding against here.

+    return;
+
   v8::Local<v8::Function> listener;
   // TODO(devlin): For some reason, we don't throw an error when someone calls
   // add/removeListener with no argument. We probably should. For now, keep
@@ -78,6 +86,9 @@ void EventEmitter::AddListener(gin::Arguments* arguments) {
 }
 
 void EventEmitter::RemoveListener(gin::Arguments* arguments) {
+  if (!valid_)

[jbroman, 2017/03/01 17:49:47]

Isn't |listeners_| empty in this case anyhow?

[Devlin, 2017/03/01 19:03:50]

Yep, but this seemed more obvious to readers.  If you prefer, I can remove it
and insert a comment instead.  (Although both may be moot with above?)

+    return;
+
   v8::Local<v8::Function> listener;
   // See comment in AddListener().
   if (!arguments->GetNext(&listener))
@@ -107,6 +118,8 @@ bool EventEmitter::HasListeners() {
 }
 
 void EventEmitter::Dispatch(gin::Arguments* arguments) {
+  DCHECK(valid_);

[jbroman, 2017/03/01 19:26:34]

Should this be a DCHECK or an early return? Scripts can presently call this
(with no bug in our code), right?

[Devlin, 2017/03/02 17:26:32]

Oh, right, we had to expose this.  Forgot about that.  (Ideally, this *should*
only be called by our own code, so it *shouldn't* happen after the context is
torn down - but the old system exposes it on the public object.)

Gracefully handling.

+
   if (listeners_.empty())
     return;
   v8::HandleScope handle_scope(arguments->isolate());