Remove stash_client.js dependency on unload_event.js

unload_event is problematic because it runs JS at inopportune times, in the
main world. This allows arbitrary web content to run JS at a time when
certain invariants in Blink may not hold true.

Unfortunately, the mojo version of the serial API depends on stash_client's
current implementation. Fortunately, it's not actually used in release yet.

BUG=537660

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

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

10 files changed, 17 insertions, 69 deletions

diff --git a/extensions/renderer/api/serial/serial_api_unittest.cc b/extensions/renderer/api/serial/serial_api_unittest.cc
index 8ba15f0..f90fe76 100644
--- a/extensions/renderer/api/serial/serial_api_unittest.cc
+++ b/extensions/renderer/api/serial/serial_api_unittest.cc
@@ -682,21 +682,22 @@ TEST_F(SerialApiTest, SendUnknownConnectionId) {
   RunTest("serial_unittest.js", "testSendUnknownConnectionId");
 }
 
-TEST_F(SerialApiTest, StashAndRestoreDuringEcho) {
+// Note: these tests are disabled, since there is no good story for persisting
+// the stashed handles when an extension process is shut down. See
+// https://crbug.com/538774
+TEST_F(SerialApiTest, DISABLED_StashAndRestoreDuringEcho) {
   ASSERT_NO_FATAL_FAILURE(RunTest("serial_unittest.js", "testSendAndStash"));
-  env()->context()->DispatchOnUnloadEvent();
   scoped_ptr<ModuleSystemTestEnvironment> new_env(CreateEnvironment());
   ApiTestEnvironment new_api_test_env(new_env.get());
   PrepareEnvironment(&new_api_test_env, stash_backend_.get());
   new_api_test_env.RunTest("serial_unittest.js", "testRestoreAndReceive");
 }
 
-TEST_F(SerialApiTest, StashAndRestoreDuringEchoError) {
+TEST_F(SerialApiTest, DISABLED_StashAndRestoreDuringEchoError) {
   io_handler_ =
       new ReceiveErrorTestIoHandler(device::serial::RECEIVE_ERROR_DEVICE_LOST);
   ASSERT_NO_FATAL_FAILURE(
       RunTest("serial_unittest.js", "testRestoreAndReceiveErrorSetUp"));
-  env()->context()->DispatchOnUnloadEvent();
   scoped_ptr<ModuleSystemTestEnvironment> new_env(CreateEnvironment());
   ApiTestEnvironment new_api_test_env(new_env.get());
   PrepareEnvironment(&new_api_test_env, stash_backend_.get());
@@ -706,7 +707,6 @@ TEST_F(SerialApiTest, StashAndRestoreDuringEchoError) {
 TEST_F(SerialApiTest, StashAndRestoreNoConnections) {
   ASSERT_NO_FATAL_FAILURE(
       RunTest("serial_unittest.js", "testStashNoConnections"));
-  env()->context()->DispatchOnUnloadEvent();
   io_handler_ = nullptr;
   scoped_ptr<ModuleSystemTestEnvironment> new_env(CreateEnvironment());
   ApiTestEnvironment new_api_test_env(new_env.get());
diff --git a/extensions/renderer/dispatcher.cc b/extensions/renderer/dispatcher.cc
index fa475ca..fa73ccc 100644
--- a/extensions/renderer/dispatcher.cc
+++ b/extensions/renderer/dispatcher.cc
@@ -427,7 +427,6 @@ void Dispatcher::WillReleaseScriptContext(
   if (!context)
     return;
 
-  context->DispatchOnUnloadEvent();
   // TODO(kalman): Make |request_sender| use |context->AddInvalidationObserver|.
   // In fact |request_sender_| should really be owned by ScriptContext.
   request_sender_->InvalidateSource(context);
@@ -619,7 +618,6 @@ std::vector<std::pair<std::string, int> > Dispatcher::GetJsResources() {
                      IDR_BROWSER_TEST_ENVIRONMENT_SPECIFIC_BINDINGS_JS));
   resources.push_back(std::make_pair("uncaught_exception_handler",
                                      IDR_UNCAUGHT_EXCEPTION_HANDLER_JS));
-  resources.push_back(std::make_pair("unload_event", IDR_UNLOAD_EVENT_JS));
   resources.push_back(std::make_pair("utils", IDR_UTILS_JS));
   resources.push_back(std::make_pair("webRequest",
                                      IDR_WEB_REQUEST_CUSTOM_BINDINGS_JS));
diff --git a/extensions/renderer/mojo/stash_client_unittest.cc b/extensions/renderer/mojo/stash_client_unittest.cc
index ec3dc3f..f8076cd 100644
--- a/extensions/renderer/mojo/stash_client_unittest.cc
+++ b/extensions/renderer/mojo/stash_client_unittest.cc
@@ -39,7 +39,6 @@ class StashClientTest : public ApiTestBase {
 // Test that stashing and restoring work correctly.
 TEST_F(StashClientTest, StashAndRestore) {
   ASSERT_NO_FATAL_FAILURE(RunTest("stash_client_unittest.js", "testStash"));
-  env()->context()->DispatchOnUnloadEvent();
   scoped_ptr<ModuleSystemTestEnvironment> restore_test_env(CreateEnvironment());
   ApiTestEnvironment restore_environment(restore_test_env.get());
   PrepareEnvironment(&restore_environment);
diff --git a/extensions/renderer/resources/extensions_renderer_resources.grd b/extensions/renderer/resources/extensions_renderer_resources.grd
index d3fcb7d..8a08705 100644
--- a/extensions/renderer/resources/extensions_renderer_resources.grd
+++ b/extensions/renderer/resources/extensions_renderer_resources.grd
@@ -55,7 +55,6 @@
       <include name="IDR_STASH_MOJOM_JS" file="${mojom_root}\extensions\common\mojo\stash.mojom.js" use_base_dir="false" type="BINDATA" />
       <include name="IDR_TEST_CUSTOM_BINDINGS_JS" file="test_custom_bindings.js" type="BINDATA" />
       <include name="IDR_UNCAUGHT_EXCEPTION_HANDLER_JS" file="uncaught_exception_handler.js" type="BINDATA" />
-      <include name="IDR_UNLOAD_EVENT_JS" file="unload_event.js" type="BINDATA" />
       <include name="IDR_UTILS_JS" file="utils.js" type="BINDATA" />
       <include name="IDR_WEB_VIEW_ACTION_REQUESTS_JS" file="guest_view/web_view/web_view_action_requests.js" type="BINDATA" />
       <include name="IDR_WEB_VIEW_API_METHODS_JS" file="guest_view/web_view/web_view_api_methods.js" type="BINDATA" />
diff --git a/extensions/renderer/resources/stash_client.js b/extensions/renderer/resources/stash_client.js
index 110e700c..b7f50fe 100644
--- a/extensions/renderer/resources/stash_client.js
+++ b/extensions/renderer/resources/stash_client.js
@@ -19,8 +19,6 @@ define('stash_client', [
   var service = new stashMojom.StashService.proxyClass(new routerModule.Router(
       serviceProvider.connectToService(stashMojom.StashService.name)));
 
-  var unloadEvent = require('unload_event');
-
   /**
    * A callback invoked to obtain objects to stash from a particular client.
    * @callback module:stash_client.StashCallback
@@ -149,7 +147,7 @@ define('stash_client', [
     });
   }
 
-  unloadEvent.addListener(function() {
+  function saveStashForTesting() {
     Promise.all($Array.map(clients, function(client) {
       return client.serialize();
     })).then(function(stashedObjects) {
@@ -160,10 +158,11 @@ define('stash_client', [
       });
       service.addToStash(flattenedObjectsToStash);
     });
-  });
+  }
 
   return {
     registerClient: registerClient,
     retrieve: retrieve,
+    saveStashForTesting, saveStashForTesting,
   };
 });
diff --git a/extensions/renderer/resources/unload_event.js b/extensions/renderer/resources/unload_event.js
deleted file mode 100644
index 753c2f0..0000000
--- a/extensions/renderer/resources/unload_event.js
+++ /dev/null
@@ -1,33 +0,0 @@
-// 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.
-
-// Special unload event. We don't use the DOM unload because that slows down
-// tab shutdown. On the other hand, onUnload might not always fire, since
-// Chrome will terminate renderers on shutdown (SuddenTermination).
-
-// Implement a primitive subset of the Event interface as needed, since if this
-// was to use the real event object there would be a circular dependency.
-var listeners = [];
-
-exports.addListener = function(listener) {
-  $Array.push(listeners, listener);
-};
-
-exports.removeListener = function(listener) {
-  for (var i = 0; i < listeners.length; ++i) {
-    if (listeners[i] == listener) {
-      $Array.splice(listeners, i, 1);
-      return;
-    }
-  }
-};
-
-exports.wasDispatched = false;
-
-// dispatch() is called from C++.
-exports.dispatch = function() {
-  exports.wasDispatched = true;
-  for (var i = 0; i < listeners.length; ++i)
-    listeners[i]();
-};
diff --git a/extensions/renderer/script_context.cc b/extensions/renderer/script_context.cc
index e49a3f0..462e30a 100644
--- a/extensions/renderer/script_context.cc
+++ b/extensions/renderer/script_context.cc
@@ -232,13 +232,6 @@ void ScriptContext::DispatchEvent(const char* event_name,
       kEventBindings, "dispatchEvent", arraysize(argv), argv);
 }
 
-void ScriptContext::DispatchOnUnloadEvent() {
-  DCHECK(thread_checker_.CalledOnValidThread());
-  v8::HandleScope handle_scope(isolate());
-  v8::Context::Scope context_scope(v8_context());
-  module_system_->CallModuleMethod("unload_event", "dispatch");
-}
-
 std::string ScriptContext::GetContextTypeDescription() const {
   DCHECK(thread_checker_.CalledOnValidThread());
   return GetContextTypeDescriptionString(context_type_);
diff --git a/extensions/renderer/script_context.h b/extensions/renderer/script_context.h
index b58b71e..a345d07 100644
--- a/extensions/renderer/script_context.h
+++ b/extensions/renderer/script_context.h
@@ -118,9 +118,6 @@ class ScriptContext : public RequestSender::Source {
 
   void DispatchEvent(const char* event_name, v8::Local<v8::Array> args) const;
 
-  // Fires the onunload event on the unload_event module.
-  void DispatchOnUnloadEvent();
-
   // Returns the availability of the API |api_name|.
   Feature::Availability GetAvailability(const std::string& api_name);
 
diff --git a/extensions/renderer/script_context_set.cc b/extensions/renderer/script_context_set.cc
index a9877dd..f3a1111 100644
--- a/extensions/renderer/script_context_set.cc
+++ b/extensions/renderer/script_context_set.cc
@@ -125,9 +125,8 @@ void ScriptContextSet::ForEach(
 std::set<ScriptContext*> ScriptContextSet::OnExtensionUnloaded(
     const std::string& extension_id) {
   std::set<ScriptContext*> removed;
-  ForEach(extension_id,
-          base::Bind(&ScriptContextSet::DispatchOnUnloadEventAndRemove,
-                     base::Unretained(this), &removed));
+  ForEach(extension_id, base::Bind(&ScriptContextSet::RecordAndRemove,
+                                   base::Unretained(this), &removed));
   return removed;
 }
 
@@ -217,12 +216,10 @@ Feature::Context ScriptContextSet::ClassifyJavaScriptContext(
   return Feature::WEB_PAGE_CONTEXT;
 }
 
-void ScriptContextSet::DispatchOnUnloadEventAndRemove(
-    std::set<ScriptContext*>* out,
-    ScriptContext* context) {
-  context->DispatchOnUnloadEvent();
-  Remove(context);  // deleted asynchronously
-  out->insert(context);
+void ScriptContextSet::RecordAndRemove(std::set<ScriptContext*>* removed,
+                                       ScriptContext* context) {
+  removed->insert(context);
+  Remove(context);  // Note: context deletion is deferred to the message loop.
 }
 
 }  // namespace extensions
diff --git a/extensions/renderer/script_context_set.h b/extensions/renderer/script_context_set.h
index be58fe5..8ca3dbf 100644
--- a/extensions/renderer/script_context_set.h
+++ b/extensions/renderer/script_context_set.h
@@ -124,10 +124,9 @@ class ScriptContextSet {
       const GURL& url,
       const blink::WebSecurityOrigin& origin);
 
-  // Calls Remove on |context| then appends |context| to |out|.
-  // This is a helper designed to be used by OnExtensionUnloaded with ForEach.
-  void DispatchOnUnloadEventAndRemove(std::set<ScriptContext*>* out,
-                                      ScriptContext* context);
+  // Helper for OnExtensionUnloaded().
+  void RecordAndRemove(std::set<ScriptContext*>* removed,
+                       ScriptContext* context);
 
   // Weak reference to all installed Extensions that are also active in this
   // process.