Move the event attach/detach logic on unload from event.js to

extensions/renderer/event_bindings.cc

Index: extensions/renderer/event_bindings.cc
diff --git a/extensions/renderer/event_bindings.cc b/extensions/renderer/event_bindings.cc
index 8a005aaec229225b10432440a1a09772ee8af28b..3858f7b89db2d9faa60f0ff6f2ec07d644ae8656 100644
--- a/extensions/renderer/event_bindings.cc
+++ b/extensions/renderer/event_bindings.cc
@@ -143,12 +143,10 @@ bool RemoveFilter(const std::string& event_name,
 
 EventBindings::EventBindings(Dispatcher* dispatcher, ScriptContext* context)
     : ObjectBackedNativeHandler(context), dispatcher_(dispatcher) {
-  RouteFunction(
-      "AttachEvent",
-      base::Bind(&EventBindings::AttachEvent, base::Unretained(this)));
-  RouteFunction(
-      "DetachEvent",
-      base::Bind(&EventBindings::DetachEvent, base::Unretained(this)));
+  RouteFunction("AttachEvent", base::Bind(&EventBindings::AttachEventHandler,
+                                          base::Unretained(this)));
+  RouteFunction("DetachEvent", base::Bind(&EventBindings::DetachEventHandler,
+                                          base::Unretained(this)));
   RouteFunction(
       "AttachFilteredEvent",
       base::Bind(&EventBindings::AttachFilteredEvent, base::Unretained(this)));
@@ -158,21 +156,36 @@ EventBindings::EventBindings(Dispatcher* dispatcher, ScriptContext* context)
   RouteFunction("MatchAgainstEventFilter",
                 base::Bind(&EventBindings::MatchAgainstEventFilter,
                            base::Unretained(this)));
+
+  // It's safe to use base::Unretained here because |context| will always
+  // outlive us.
+  context->AddInvalidateObserver(
+      base::Bind(&EventBindings::OnInvalidated, base::Unretained(this)));
 }
 
 EventBindings::~EventBindings() {}
 
-// Attach an event name to an object.
-void EventBindings::AttachEvent(
+void EventBindings::AttachEventHandler(
     const v8::FunctionCallbackInfo<v8::Value>& args) {
   CHECK_EQ(1, args.Length());
   CHECK(args[0]->IsString());
+  AttachEvent(*v8::String::Utf8Value(args[0]));
+}
 
-  std::string event_name = *v8::String::Utf8Value(args[0]);
-
+void EventBindings::AttachEvent(const std::string& event_name) {
+  // This method throws an exception if it returns false.
   if (!dispatcher_->CheckContextAccessToExtensionAPI(event_name, context()))
     return;
 
+  // Record the attachment for this context so that events can be detached when
+  // the context is destroyed.
+  //
+  // Ideally we'd CHECK that it's not already attached, however that's not
+  // possible because extension can create and attach events themselves. Very

[Devlin, 2015/04/10 18:10:40]

nit: extensions

[not at google - send to devlin, 2015/04/10 18:39:01]

Done.

+  // silly, but that's the way it is. For an example of this, see
+  // chrome/test/data/extensions/api_test/events/background.js.
+  attached_event_names_.insert(event_name);
+
   const std::string& extension_id = context()->GetExtensionID();
   if (IncrementEventListenerCount(context(), event_name) == 1) {
     content::RenderThread::Get()->Send(new ExtensionHostMsg_AddListener(
@@ -189,16 +202,20 @@ void EventBindings::AttachEvent(
   }
 }
 
-void EventBindings::DetachEvent(
+void EventBindings::DetachEventHandler(
     const v8::FunctionCallbackInfo<v8::Value>& args) {
   CHECK_EQ(2, args.Length());
   CHECK(args[0]->IsString());
   CHECK(args[1]->IsBoolean());
+  DetachEvent(*v8::String::Utf8Value(args[0]), args[1]->BooleanValue());
+}
 
-  std::string event_name = *v8::String::Utf8Value(args[0]);
-  bool is_manual = args[1]->BooleanValue();
+void EventBindings::DetachEvent(const std::string& event_name, bool is_manual) {
+  // See comment in AttachEvent().
+  attached_event_names_.erase(event_name);
 
   const std::string& extension_id = context()->GetExtensionID();
+
   if (DecrementEventListenerCount(context(), event_name) == 0) {
     content::RenderThread::Get()->Send(new ExtensionHostMsg_RemoveListener(
         extension_id, context()->GetURL(), event_name));
@@ -324,4 +341,13 @@ scoped_ptr<EventMatcher> EventBindings::ParseEventMatcher(
       context()->GetRenderView()->GetRoutingID()));
 }
 
+void EventBindings::OnInvalidated() {
+  // Detach all attached events that weren't attached. Iterate over a copy
+  // because it will be mutated.
+  std::set<std::string> attached_event_names_safe = attached_event_names_;
+  for (const std::string& event_name : attached_event_names_safe) {
+    DetachEvent(event_name, false /* is_manual */);
+  }

[Devlin, 2015/04/10 18:10:39]

For kicks and grins, can we DCHECK(attached_event_names_.empty()) here?

[not at google - send to devlin, 2015/04/10 18:39:01]

Done.

+}
+
 }  // namespace extensions