[Extensions Bindings] Notify of event unregistration on invalidation

extensions/renderer/api_event_handler.cc

Index: extensions/renderer/api_event_handler.cc
diff --git a/extensions/renderer/api_event_handler.cc b/extensions/renderer/api_event_handler.cc
index 2391607e67ab5955be1ae634c9df4379cd6c6252..52979cdad6b1008ae29eac1e3e64aa5905a1dc89 100644
--- a/extensions/renderer/api_event_handler.cc
+++ b/extensions/renderer/api_event_handler.cc
@@ -28,18 +28,8 @@ const char kExtensionAPIEventPerContextKey[] = "extension_api_events";
 struct APIEventPerContextData : public base::SupportsUserData::Data {
   APIEventPerContextData(v8::Isolate* isolate) : isolate(isolate) {}
   ~APIEventPerContextData() override {
-    v8::HandleScope scope(isolate);
-    // We explicitly clear the event data map here to remove all references to
-    // v8 objects. This helps us avoid cycles in v8 where an event listener
-    // could hold a reference to the event, which in turn holds the reference
-    // to the listener.
-    for (const auto& pair : event_data) {
-      EventEmitter* emitter = nullptr;
-      gin::Converter<EventEmitter*>::FromV8(
-          isolate, pair.second.Get(isolate), &emitter);
-      CHECK(emitter);
-      emitter->listeners()->clear();
-    }
+    DCHECK(event_data.empty())
+        << "|event_data| should have been cleared by InvalidateContext()";
   }
 
   // The associated v8::Isolate. Since this object is cleaned up at context
@@ -127,6 +117,35 @@ void APIEventHandler::FireEventInContext(const std::string& event_name,
   emitter->Fire(context, &v8_args);
 }
 
+void APIEventHandler::InvalidateContext(v8::Local<v8::Context> context) {
+  gin::PerContextData* per_context_data = gin::PerContextData::From(context);
+  DCHECK(per_context_data);
+  APIEventPerContextData* data = static_cast<APIEventPerContextData*>(
+      per_context_data->GetUserData(kExtensionAPIEventPerContextKey));
+  if (!data)
+    return;
+
+  v8::Isolate* isolate = context->GetIsolate();
+  v8::HandleScope scope(isolate);
+  for (const auto& pair : data->event_data) {
+    EventEmitter* emitter = nullptr;
+    gin::Converter<EventEmitter*>::FromV8(isolate, pair.second.Get(isolate),
+                                          &emitter);
+    CHECK(emitter);
+    emitter->Invalidate();
+    // When the context is shut down, all listeners are removed.
+    listeners_changed_.Run(
+        pair.first, binding::EventListenersChanged::NO_LISTENERS, context);
+  }
+
+  data->event_data.clear();

[lazyboy, 2017/03/03 22:14:19]

FYI: If there's a chance that |data->event_data| can be accessed by lines
131-137 before this clear()...
I'd be careful and consider setting event_data aside first using std::swap() and
then iterating them on line 130. If not, then ignore.

[Devlin, 2017/03/06 19:11:06]

It shouldn't be possible, but I've added a comment documenting that.

+
+  // InvalidateContext() is called shortly (and, theoretically, synchronously)
+  // before the PerContextData is deleted. We have a check that guarantees that
+  // no new EventEmitters are created after the PerContextData is deleted, so
+  // no new emitters should be created after this point.
+}
+
 size_t APIEventHandler::GetNumEventListenersForTesting(
     const std::string& event_name,
     v8::Local<v8::Context> context) {