Split out the extensions gin::Runner implementation into a separate class.

The gin::Runner should be destroyed when the JS context is so that its
weak pointers are invalidated and any outstanding callbacks that
interact with the JS context can detect its destruction. Currently,
ScriptContext implements gin::Runner but is not immediately destroyed
when the JS context is. This CL moves this gin::Runner implementation
into a separate class which is destroyed when the ScriptContext is
invalidated.

Review URL: https://codereview.chromium.org/831423005

Cr-Commit-Position: refs/heads/master@{#311358}

extensions/BUILD.gn

@@ -250,6 +250,7 @@ if (false) {
       "renderer/mojo/keep_alive_client_unittest.cc",
       "renderer/safe_builtins_unittest.cc",
       "renderer/script_context_set_unittest.cc",
+      "renderer/script_context_unittest.cc",
       "renderer/utils_unittest.cc",
       "test/extensions_unittests_main.cc",
     ]

extensions/extensions.gyp

@@ -1289,6 +1289,7 @@
         'renderer/module_system_unittest.cc',
         'renderer/safe_builtins_unittest.cc',
         'renderer/script_context_set_unittest.cc',
+        'renderer/script_context_unittest.cc',
         'renderer/utils_unittest.cc',
         'test/extensions_unittests_main.cc',
       ],

extensions/renderer/module_system_test.cc

@@ -156,7 +156,7 @@ ModuleSystemTestEnvironment::ModuleSystemTestEnvironment(v8::Isolate* isolate)
 }
 
 ModuleSystemTestEnvironment::~ModuleSystemTestEnvironment() {
-  if (context_)
+  if (context_->is_valid())
     context_->v8_context()->Exit();
 }
 
@@ -197,7 +197,7 @@ void ModuleSystemTestEnvironment::ShutdownGin() {
 
 void ModuleSystemTestEnvironment::ShutdownModuleSystem() {
   context_->v8_context()->Exit();
-  context_.reset();
+  context_->Invalidate();
 }
 
 v8::Handle<v8::Object> ModuleSystemTestEnvironment::CreateGlobal(

extensions/renderer/script_context.cc

@@ -56,6 +56,24 @@ std::string GetContextTypeDescriptionString(Feature::Context context_type) {
 
 }  // namespace
 
+// A gin::Runner that delegates to its ScriptContext.
+class ScriptContext::Runner : public gin::Runner {
+ public:
+  explicit Runner(ScriptContext* context);
+
+  // gin::Runner overrides.
+  void Run(const std::string& source,
+           const std::string& resource_name) override;
+  v8::Handle<v8::Value> Call(v8::Handle<v8::Function> function,
+                             v8::Handle<v8::Value> receiver,
+                             int argc,
+                             v8::Handle<v8::Value> argv[]) override;
+  gin::ContextHolder* GetContextHolder() override;
+
+ private:
+  ScriptContext* context_;
+};
+
 ScriptContext::ScriptContext(const v8::Handle<v8::Context>& v8_context,
                              blink::WebFrame* web_frame,
                              const Extension* extension,
@@ -70,7 +88,8 @@ ScriptContext::ScriptContext(const v8::Handle<v8::Context>& v8_context,
       effective_context_type_(effective_context_type),
       safe_builtins_(this),
       isolate_(v8_context->GetIsolate()),
-      url_(web_frame_ ? GetDataSourceURLForFrame(web_frame_) : GURL()) {
+      url_(web_frame_ ? GetDataSourceURLForFrame(web_frame_) : GURL()),
+      runner_(new Runner(this)) {
   VLOG(1) << "Created context:\n"
           << "  extension id: " << GetExtensionID() << "\n"
           << "  frame:        " << web_frame_ << "\n"
@@ -80,7 +99,7 @@ ScriptContext::ScriptContext(const v8::Handle<v8::Context>& v8_context,
           << (effective_extension_.get() ? effective_extension_->id() : "")
           << "  effective context type: "
           << GetEffectiveContextTypeDescription();
-  gin::PerContextData::From(v8_context)->set_runner(this);
+  gin::PerContextData::From(v8_context)->set_runner(runner_.get());
 }
 
 ScriptContext::~ScriptContext() {
@@ -98,6 +117,7 @@ void ScriptContext::Invalidate() {
     module_system_->Invalidate();
   web_frame_ = NULL;
   v8_context_.reset();
+  runner_.reset();
 }
 
 const std::string& ScriptContext::GetExtensionID() const {
@@ -257,23 +277,6 @@ void ScriptContext::OnResponseReceived(const std::string& name,
       << *v8::String::Utf8Value(retval);
 }
 
-void ScriptContext::Run(const std::string& source,
-                        const std::string& resource_name) {
-  module_system_->RunString(source, resource_name);
-}
-
-v8::Handle<v8::Value> ScriptContext::Call(v8::Handle<v8::Function> function,
-                                          v8::Handle<v8::Value> receiver,
-                                          int argc,
-                                          v8::Handle<v8::Value> argv[]) {
-  return CallFunction(function, argc, argv);
-}
-
-gin::ContextHolder* ScriptContext::GetContextHolder() {
-  v8::HandleScope handle_scope(isolate());
-  return gin::PerContextData::From(v8_context())->context_holder();
-}
-
 void ScriptContext::SetContentCapabilities(
     const APIPermissionSet& permissions) {
   content_capabilities_ = permissions;
@@ -293,4 +296,24 @@ bool ScriptContext::HasAPIPermission(APIPermission::ID permission) const {
   return false;
 }
 
+ScriptContext::Runner::Runner(ScriptContext* context) : context_(context) {
+}
+void ScriptContext::Runner::Run(const std::string& source,
+                                const std::string& resource_name) {
+  context_->module_system()->RunString(source, resource_name);
+}
+
+v8::Handle<v8::Value> ScriptContext::Runner::Call(
+    v8::Handle<v8::Function> function,
+    v8::Handle<v8::Value> receiver,
+    int argc,
+    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

extensions/renderer/script_context.h

@@ -32,7 +32,7 @@ namespace extensions {
 class Extension;
 
 // Extensions wrapper for a v8 context.
-class ScriptContext : public RequestSender::Source, public gin::Runner {
+class ScriptContext : public RequestSender::Source {
  public:
   ScriptContext(const v8::Handle<v8::Context>& context,
                 blink::WebFrame* frame,
@@ -142,15 +142,6 @@ class ScriptContext : public RequestSender::Source, public gin::Runner {
                           const base::ListValue& response,
                           const std::string& error) override;
 
-  // gin::Runner overrides.
-  void Run(const std::string& source,
-           const std::string& resource_name) override;
-  v8::Handle<v8::Value> Call(v8::Handle<v8::Function> function,
-                             v8::Handle<v8::Value> receiver,
-                             int argc,
-                             v8::Handle<v8::Value> argv[]) override;
-  gin::ContextHolder* GetContextHolder() override;
-
   // Grants a set of content capabilities to this context.
   void SetContentCapabilities(const APIPermissionSet& permissions);
 
@@ -165,6 +156,8 @@ class ScriptContext : public RequestSender::Source, public gin::Runner {
   ScopedPersistent<v8::Context> v8_context_;
 
  private:
+  class Runner;
+
   // The WebFrame associated with this context. This can be NULL because this
   // object can outlive is destroyed asynchronously.
   blink::WebFrame* web_frame_;
@@ -197,6 +190,8 @@ class ScriptContext : public RequestSender::Source, public gin::Runner {
 
   GURL url_;
 
+  scoped_ptr<Runner> runner_;
+
   DISALLOW_COPY_AND_ASSIGN(ScriptContext);
 };
 

extensions/renderer/script_context_unittest.cc

+// Copyright 2015 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/module_system_test.h"
+#include "extensions/renderer/script_context.h"
+#include "gin/per_context_data.h"
+#include "gin/runner.h"
+
+namespace extensions {
+
+using ScriptContextTest = ModuleSystemTest;
+
+TEST_F(ScriptContextTest, GinRunnerLifetime) {
+  ExpectNoAssertionsMade();
+  base::WeakPtr<gin::Runner> weak_runner =
+      gin::PerContextData::From(env()->context()->v8_context())
+          ->runner()
+          ->GetWeakPtr();
+  env()->ShutdownModuleSystem();
+  EXPECT_FALSE(weak_runner);
+}
+
+}  // namespace extensions