[Extensions Bindings] Notify of event unregistration on invalidation

extensions/renderer/api_event_handler.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/api_event_handler.h"
 
 #include <algorithm>
 #include <map>
 #include <memory>
 #include <vector>
@@ skipping 10 matching lines @@
 
 namespace extensions {
 
 namespace {
 
 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
   // destruction, this should always be valid.
   v8::Isolate* isolate;
 
   // A map from event name -> event emitter.
   std::map<std::string, v8::Global<v8::Object>> event_data;
 };
 
@@ skipping 67 matching lines @@
     v8_args.push_back(converter->ToV8Value(arg.get(), context));
 
   EventEmitter* emitter = nullptr;
   gin::Converter<EventEmitter*>::FromV8(
       context->GetIsolate(), iter->second.Get(context->GetIsolate()), &emitter);
   CHECK(emitter);
 
   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) {
   gin::PerContextData* per_context_data = gin::PerContextData::From(context);
   DCHECK(per_context_data);
   APIEventPerContextData* data = static_cast<APIEventPerContextData*>(
       per_context_data->GetUserData(kExtensionAPIEventPerContextKey));
   DCHECK(data);
 
   auto iter = data->event_data.find(event_name);
   DCHECK(iter != data->event_data.end());
   EventEmitter* emitter = nullptr;
   gin::Converter<EventEmitter*>::FromV8(
       context->GetIsolate(), iter->second.Get(context->GetIsolate()), &emitter);
   CHECK(emitter);
   return emitter->listeners()->size();
 }
 
 }  // namespace extensions