[Extensions Bindings] Notify of event unregistration on invalidation

extensions/renderer/event_emitter.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "extensions/renderer/event_emitter.h"
 
 #include <algorithm>
 
 #include "gin/object_template_builder.h"
 #include "gin/per_context_data.h"
@@ skipping 34 matching lines @@
   for (const auto& listener : listeners) {
     v8::TryCatch try_catch(context->GetIsolate());
     // SetVerbose() means the error will still get logged, which is what we
     // want. We don't let it bubble up any further to prevent it from being
     // surfaced in e.g. JS code that triggered the event.
     try_catch.SetVerbose(true);
     run_js_.Run(listener, context, args->size(), args->data());
   }
 }
 
+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
   // the status quo, but we should revisit this.
   if (!arguments->GetNext(&listener))
     return;
 
   v8::Local<v8::Object> holder;
   CHECK(arguments->GetHolder(&holder));
   CHECK(!holder.IsEmpty());
   v8::Local<v8::Context> context = holder->CreationContext();
   if (!gin::PerContextData::From(context))
     return;
 
   if (!HasListener(listener)) {
     listeners_.push_back(
         v8::Global<v8::Function>(arguments->isolate(), listener));
     if (listeners_.size() == 1) {
       listeners_changed_.Run(binding::EventListenersChanged::HAS_LISTENERS,
                              context);
     }
   }
 }
 
 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))
     return;
 
   auto iter = std::find(listeners_.begin(), listeners_.end(), listener);
   if (iter != listeners_.end()) {
     listeners_.erase(iter);
     if (listeners_.empty()) {
       v8::Local<v8::Object> holder;
       CHECK(arguments->GetHolder(&holder));
       CHECK(!holder.IsEmpty());
       v8::Local<v8::Context> context = holder->CreationContext();
       listeners_changed_.Run(binding::EventListenersChanged::NO_LISTENERS,
                              context);
     }
   }
 }
 
 bool EventEmitter::HasListener(v8::Local<v8::Function> listener) {
   return std::find(listeners_.begin(), listeners_.end(), listener) !=
          listeners_.end();
 }
 
 bool EventEmitter::HasListeners() {
   return !listeners_.empty();
 }
 
 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());
   v8::Local<v8::Context> context =
       arguments->isolate()->GetCurrentContext();
   std::vector<v8::Local<v8::Value>> v8_args;
   if (arguments->Length() > 0) {
     // Converting to v8::Values should never fail.
     CHECK(arguments->GetRemaining(&v8_args));
   }
   Fire(context, &v8_args);
 }
 
 }  // namespace extensions