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

extensions/renderer/script_context.cc

Index: extensions/renderer/script_context.cc
diff --git a/extensions/renderer/script_context.cc b/extensions/renderer/script_context.cc
index e5677aadb10597498e0b4d5019bc8cc4d084fd8f..03e9e14977cff39c1fdf47068eaffecc678ab839 100644
--- a/extensions/renderer/script_context.cc
+++ b/extensions/renderer/script_context.cc
@@ -80,7 +80,8 @@ ScriptContext::ScriptContext(const v8::Handle<v8::Context>& v8_context,
                              Feature::Context context_type,
                              const Extension* effective_extension,
                              Feature::Context effective_context_type)
-    : v8_context_(v8_context->GetIsolate(), v8_context),
+    : is_valid_(true),
+      v8_context_(v8_context->GetIsolate(), v8_context),
       web_frame_(web_frame),
       extension_(extension),
       context_type_(context_type),
@@ -107,17 +108,33 @@ ScriptContext::~ScriptContext() {
           << "  extension id: " << GetExtensionID() << "\n"
           << "  effective extension id: "
           << (effective_extension_.get() ? effective_extension_->id() : "");
-  Invalidate();
+  if (is_valid_)
+    Invalidate();
 }
 
 void ScriptContext::Invalidate() {
-  if (!is_valid())
-    return;
+  CHECK(is_valid_);
+  is_valid_ = false;
+
+  // TODO(kalman): Make ModuleSystem use AddInvalidateObserver.

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

Wait, didn't you?

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

No I made EventBindings.

+  // Ownership graph is a bit weird here.
   if (module_system_)
     module_system_->Invalidate();
-  web_frame_ = NULL;
-  v8_context_.Reset();
+
+  // Swap |invalidate_observers_| to a local variable to clear it, and to make
+  // sure it's not mutated as we iterate.

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

I mean, I guess swap is pretty cheap - but how would it be modified as we run? 
There's no "RemoveInvalidateObserver" (and couldn't really be, since closure
comparison is kinda hard).

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

I might be missing something... but if an InvalidationObserver calls
AddInvalidationObserver, it would be insane, but it would also be a bug. I
generally have misgivings about running observer callbacks over a live set of
observers. I'll add the neat little DCHECK you suggested though.

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

Yeah, that would be a bug if anyone did that - but totally 100% insane for them
to do so, so I like the DCHECK rather than silently pretending it's okay :P

+  std::vector<base::Closure> observers;
+  observers.swap(invalidate_observers_);
+  for (const base::Closure& observer : observers) {
+    observer.Run();
+  }
+
   runner_.reset();
+  v8_context_.Reset();
+}
+
+void ScriptContext::AddInvalidateObserver(const base::Closure& observer) {
+  invalidate_observers_.push_back(observer);
 }
 
 const std::string& ScriptContext::GetExtensionID() const {
@@ -144,7 +161,7 @@ v8::Local<v8::Value> ScriptContext::CallFunction(
   v8::Context::Scope scope(v8_context());
 
   blink::WebScopedMicrotaskSuppression suppression;
-  if (!is_valid()) {
+  if (!is_valid_) {
     return handle_scope.Escape(
         v8::Local<v8::Primitive>(v8::Undefined(isolate())));
   }