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..3cf56ec0479bb89c591d34c3b1c477793416cd13 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->AddInvalidationObserver(
+      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 extensions can create and attach events themselves. Very
+  // 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,15 @@ 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 */);
+  }
+  DCHECK(attached_event_names_.empty())
+      << "Events cannot be attached during invalidation";

[Devlin, 2015/04/10 22:00:24]

nit of nits: The error kinda sounds weird to me.  How about instead "All events
must be detached after invalidation" or something?  (Right now, it sounds like
you're ensuring that no event is attached during the course of invalidation).

[not at google - send to devlin, 2015/04/10 22:31:45]

Actually it's always re-entrancy bugs that I am worried about, and why I take
the copy. The check was to guard against that, not DetachEvent accidentally not
clearing |attached_event_names_|.

+}
+
 }  // namespace extensions