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

extensions/renderer/script_context.cc

 // Copyright 2014 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/script_context.h"
 
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/strings/string_split.h"
 #include "base/strings/string_util.h"
@@ skipping 62 matching lines @@
  private:
   ScriptContext* context_;
 };
 
 ScriptContext::ScriptContext(const v8::Handle<v8::Context>& v8_context,
                              blink::WebFrame* web_frame,
                              const Extension* extension,
                              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),
       effective_extension_(effective_extension),
       effective_context_type_(effective_context_type),
       safe_builtins_(this),
       isolate_(v8_context->GetIsolate()),
       url_(web_frame_ ? GetDataSourceURLForFrame(web_frame_) : GURL()),
       runner_(new Runner(this)) {
   VLOG(1) << "Created context:\n"
           << "  extension id: " << GetExtensionID() << "\n"
           << "  frame:        " << web_frame_ << "\n"
           << "  URL:          " << GetURL() << "\n"
           << "  context type: " << GetContextTypeDescription() << "\n"
           << "  effective extension id: "
           << (effective_extension_.get() ? effective_extension_->id() : "")
           << "  effective context type: "
           << GetEffectiveContextTypeDescription();
   gin::PerContextData::From(v8_context)->set_runner(runner_.get());
 }
 
 ScriptContext::~ScriptContext() {
   VLOG(1) << "Destroyed context for extension\n"
           << "  extension id: " << GetExtensionID() << "\n"
           << "  effective extension id: "
           << (effective_extension_.get() ? effective_extension_->id() : "");
-  Invalidate();
+  CHECK(!is_valid_) << "ScriptContexts must be invalidated before destruction";

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

I'm liking all these CHECKs.  Hopefully it's not already broken to where they
trigger.

 }
 
 void ScriptContext::Invalidate() {
-  if (!is_valid())
-    return;
+  CHECK(is_valid_);
+  is_valid_ = false;
+
+  // TODO(kalman): Make ModuleSystem use AddInvalidationObserver.
+  // Ownership graph is a bit weird here.
   if (module_system_)
     module_system_->Invalidate();

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

You're getting a compile error here (Invalidate() is protected on ModuleSystem),
so it might almost be worth doing this in this patch.  Is there a complication?

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

Fixed in person (I meant to friend ModuleSystemTestEnvironment; ScriptContext is
already friend'd).

-  web_frame_ = NULL;
+
+  // Swap |invalidate_observers_| to a local variable to clear it, and to make
+  // sure it's not mutated as we iterate.
+  std::vector<base::Closure> observers;
+  observers.swap(invalidate_observers_);
+  for (const base::Closure& observer : observers) {
+    observer.Run();
+  }
+  DCHECK(invalidate_observers_.empty())
+      << "Invalidation observers cannot be added during invalidation";
+
+  runner_.reset();
   v8_context_.Reset();
-  runner_.reset();
+}
+
+void ScriptContext::AddInvalidationObserver(const base::Closure& observer) {
+  invalidate_observers_.push_back(observer);
 }
 
 const std::string& ScriptContext::GetExtensionID() const {
   return extension_.get() ? extension_->id() : base::EmptyString();
 }
 
 content::RenderView* ScriptContext::GetRenderView() const {
   if (web_frame_ && web_frame_->view())
     return content::RenderView::FromWebView(web_frame_->view());
   return NULL;
 }
 
 content::RenderFrame* ScriptContext::GetRenderFrame() const {
   if (web_frame_)
     return content::RenderFrame::FromWebFrame(web_frame_);
   return NULL;
 }
 
 v8::Local<v8::Value> ScriptContext::CallFunction(
     v8::Handle<v8::Function> function,
     int argc,
     v8::Handle<v8::Value> argv[]) const {
   v8::EscapableHandleScope handle_scope(isolate());
   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())));
   }
 
   v8::Handle<v8::Object> global = v8_context()->Global();
   if (!web_frame_)
     return handle_scope.Escape(function->Call(global, argc, argv));
   return handle_scope.Escape(
       v8::Local<v8::Value>(web_frame_->callFunctionEvenIfScriptDisabled(
           function, global, argc, argv)));
@@ skipping 153 matching lines @@
     v8::Handle<v8::Value> argv[]) {
   return context_->CallFunction(function, argc, argv);
 }
 
 gin::ContextHolder* ScriptContext::Runner::GetContextHolder() {
   v8::HandleScope handle_scope(context_->isolate());
   return gin::PerContextData::From(context_->v8_context())->context_holder();
 }
 
 }  // namespace extensions