[Extensions Bindings] Account for listeners destroying the context

An event listener could destroy the context when it's notified (e.g.,
if it does window.close()). We need to account for this case. Check
after each listener is executed whether the context is still valid.

Add native bindings-specific unittests and a JS/Native bindings
browsertest.

Bug: 653596

Change-Id: I47e789800ca90935457781695d3e8c3de7aae317
Reviewed-on: https://chromium-review.googlesource.com/818528Reviewed-by: Jeremy Roman <jbroman@chromium.org>
Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org>
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#524466}

chrome/browser/extensions/extension_bindings_apitest.cc

@@ -14,6 +14,7 @@
 #include "chrome/common/chrome_switches.h"
 #include "chrome/test/base/ui_test_utils.h"
 #include "content/public/browser/browser_thread.h"
+#include "content/public/browser/render_frame_host.h"
 #include "content/public/test/browser_test_utils.h"
 #include "extensions/browser/extension_host.h"
 #include "extensions/browser/process_manager.h"
@@ -370,6 +371,58 @@ IN_PROC_BROWSER_TEST_P(ExtensionBindingsApiTest,
   ASSERT_TRUE(catcher.GetNextResult()) << catcher.message();
 }
 
+IN_PROC_BROWSER_TEST_P(ExtensionBindingsApiTest,
+                       ExtensionListenersRemoveContext) {
+  const Extension* extension = LoadExtension(
+      test_data_dir_.AppendASCII("bindings/listeners_destroy_context"));
+  ASSERT_TRUE(extension);
+
+  ExtensionTestMessageListener listener("ready", true);
+
+  // Navigate to a web page with an iframe (the iframe is title1.html).
+  GURL main_frame_url = embedded_test_server()->GetURL("a.com", "/iframe.html");
+  ui_test_utils::NavigateToURL(browser(), main_frame_url);
+
+  content::WebContents* tab =
+      browser()->tab_strip_model()->GetActiveWebContents();
+
+  content::RenderFrameHost* main_frame = tab->GetMainFrame();
+  content::RenderFrameHost* subframe = ChildFrameAt(main_frame, 0);
+  content::RenderFrameDeletedObserver subframe_deleted(subframe);
+
+  // Wait for the extension's content script to be ready.
+  ASSERT_TRUE(listener.WaitUntilSatisfied());
+
+  // It's actually critical to the test that these frames are in the same
+  // process, because otherwise a crash in the iframe wouldn't be detectable
+  // (since we rely on JS execution in the main frame to tell if the renderer
+  // crashed - see comment below).
+  content::RenderProcessHost* main_frame_process = main_frame->GetProcess();
+  EXPECT_EQ(main_frame_process, subframe->GetProcess());
+
+  ExtensionTestMessageListener failure_listener("failed", false);
+
+  // Tell the extension to register listeners that will remove the iframe, and
+  // trigger them.
+  listener.Reply("go!");
+
+  // The frame will be deleted.
+  subframe_deleted.WaitUntilDeleted();
+
+  // Unfortunately, we don't have a good way of checking if something crashed
+  // after the frame was removed. WebContents::IsCrashed() seems like it should
+  // work, but is insufficient. Instead, use JS execution as the source of
+  // true.
+  EXPECT_FALSE(tab->IsCrashed());
+  EXPECT_EQ(main_frame_url, main_frame->GetLastCommittedURL());
+  EXPECT_EQ(main_frame_process, main_frame->GetProcess());
+  bool renderer_valid = false;
+  EXPECT_TRUE(content::ExecuteScriptAndExtractBool(
+      main_frame, "domAutomationController.send(true);", &renderer_valid));
+  EXPECT_TRUE(renderer_valid);
+  EXPECT_FALSE(failure_listener.was_satisfied());
+}
+
 // Run core bindings API tests with both native and JS-based bindings. This
 // ensures we have some minimum level of coverage while in the experimental
 // phase, when native bindings may be enabled on trunk but not at 100% stable.

chrome/test/data/extensions/api_test/bindings/listeners_destroy_context/content_script.js

+// Copyright 2017 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.
+
+let closed = false;
+function getDestructiveListener() {
+  return function() {
+    if (closed) {
+      chrome.test.sendMessage('failed');
+    } else {
+      closed = true;
+      parent.document.body.removeChild(
+          parent.document.body.querySelector('iframe'));
+    }
+  };
+}
+
+chrome.test.sendMessage('ready', function() {
+  // Queue up a number of listeners, and then trigger them (by changing a
+  // storage value).
+  chrome.storage.onChanged.addListener(getDestructiveListener());
+  chrome.storage.onChanged.addListener(getDestructiveListener());
+  chrome.storage.onChanged.addListener(getDestructiveListener());
+  chrome.storage.local.set({foo: 'bar'});
+});

chrome/test/data/extensions/api_test/bindings/listeners_destroy_context/manifest.json

+{
+  "name": "Test listeners destroying context",
+  "description": "Listeners that destroy a context.",
+  "manifest_version": 2,
+  "version": "0.1",
+  "content_scripts": [{
+    "matches": ["*://*.a.com/title*.html"],
+    "js": ["content_script.js"],
+    "all_frames": true
+  }],
+  "permissions": ["storage"]
+}

extensions/renderer/bindings/api_event_handler.h

@@ -69,6 +69,8 @@ class APIEventHandler {
 
   // Notifies all listeners of the event with the given |event_name| in the
   // specified |context|, sending the included |arguments|.
+  // Warning: This runs arbitrary JS code, so the |context| may be invalidated
+  // after this!
   void FireEventInContext(const std::string& event_name,
                           v8::Local<v8::Context> context,
                           const base::ListValue& arguments,

extensions/renderer/bindings/api_request_handler.h

@@ -73,6 +73,8 @@ class APIRequestHandler {
   // Responds to the request with the given |request_id|, calling the callback
   // with the given |response| arguments.
   // Invalid ids are ignored.
+  // Warning: This can run arbitrary JS code, so the |context| may be
+  // invalidated after this!
   void CompleteRequest(int request_id,
                        const base::ListValue& response,
                        const std::string& error);

extensions/renderer/bindings/event_emitter.cc

@@ -166,10 +166,16 @@ v8::Local<v8::Value> EventEmitter::DispatchSync(
     // (through e.g. calling alert() or print()). That should suspend this
     // message loop as well (though a nested message loop will run). This is a
     // bit ugly, but should hopefully be safe.
+    v8::MaybeLocal<v8::Value> maybe_result = js_runner->RunJSFunctionSync(
+        listener, context, args->size(), args->data());
+
+    // Any of the listeners could invalidate the context. If that happens,
+    // bail out.
+    if (!binding::IsContextValid(context))
+      return v8::Undefined(isolate);
+
     v8::Local<v8::Value> listener_result;
-    if (js_runner
-            ->RunJSFunctionSync(listener, context, args->size(), args->data())
-            .ToLocal(&listener_result)) {
+    if (maybe_result.ToLocal(&listener_result)) {
       if (!listener_result->IsUndefined()) {
         CHECK(
             results

extensions/renderer/bindings/event_emitter.h

@@ -37,6 +37,9 @@ class EventEmitter final : public gin::Wrappable<EventEmitter> {
   gin::ObjectTemplateBuilder GetObjectTemplateBuilder(
       v8::Isolate* isolate) final;
 
+  // Fires the event to any listeners.
+  // Warning: This can run arbitrary JS code, so the |context| may be
+  // invalidated after this!
   void Fire(v8::Local<v8::Context> context,
             std::vector<v8::Local<v8::Value>>* args,
             const EventFilteringInfo* filter);

extensions/renderer/bindings/event_emitter_unittest.cc

@@ -25,7 +25,22 @@ void DoNothingOnListenerChange(binding::EventListenersChanged changed,
 
 }  // namespace
 
-using EventEmitterUnittest = APIBindingTest;
+class EventEmitterUnittest : public APIBindingTest {
+ public:
+  EventEmitterUnittest() = default;
+  ~EventEmitterUnittest() override = default;
+
+  // A helper method to dispose of a context and set a flag.
+  void DisposeContextWrapper(bool* did_invalidate,
+                             v8::Local<v8::Context> context) {
+    EXPECT_FALSE(*did_invalidate);
+    *did_invalidate = true;
+    DisposeContext(context);
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(EventEmitterUnittest);
+};
 
 TEST_F(EventEmitterUnittest, TestDispatchMethod) {
   v8::HandleScope handle_scope(isolate());
@@ -112,4 +127,60 @@ TEST_F(EventEmitterUnittest, TestDispatchMethod) {
               testing::StartsWith("Error in event handler: Error: hahaha"));
 }
 
+// Test dispatching an event when the first listener invalidates the context.
+// Nothing should break, and we shouldn't continue to dispatch the event.
+TEST_F(EventEmitterUnittest, ListenersDestroyingContext) {
+  v8::HandleScope handle_scope(isolate());
+  v8::Local<v8::Context> context = MainContext();
+
+  struct ListenerClosureData {
+    EventEmitterUnittest& test;
+    bool did_invalidate_context;
+  } closure_data = {*this, false};
+
+  // A wrapper that just calls DisposeContextWrapper() on the curried in data.
+  auto listener_wrapper = [](const v8::FunctionCallbackInfo<v8::Value>& info) {
+    ASSERT_TRUE(info.Data()->IsExternal());
+    auto& data = *static_cast<ListenerClosureData*>(
+        info.Data().As<v8::External>()->Value());
+    data.test.DisposeContextWrapper(&data.did_invalidate_context,
+                                    info.GetIsolate()->GetCurrentContext());
+  };
+
+  auto listeners = std::make_unique<UnfilteredEventListeners>(
+      base::BindRepeating(&DoNothingOnListenerChange), binding::kNoListenerMax,
+      true);
+  ExceptionHandler exception_handler(base::BindRepeating(
+      [](v8::Local<v8::Context> context, const std::string& error) {}));
+  gin::Handle<EventEmitter> event = gin::CreateHandle(
+      isolate(),
+      new EventEmitter(false, std::move(listeners), &exception_handler));
+
+  v8::Local<v8::Value> v8_event = event.ToV8();
+
+  const char kAddListener[] =
+      "(function(event, listener) { event.addListener(listener); })";
+  v8::Local<v8::Function> add_listener_function =
+      FunctionFromString(context, kAddListener);
+
+  // Queue up three listeners. The first triggered will invalidate the context.
+  // The others should never be triggered.
+  constexpr size_t kNumListeners = 3;
+  for (size_t i = 0; i < kNumListeners; ++i) {
+    v8::Local<v8::Function> listener =
+        v8::Function::New(context, listener_wrapper,
+                          v8::External::New(isolate(), &closure_data))
+            .ToLocalChecked();
+    v8::Local<v8::Value> args[] = {v8_event, listener};
+    RunFunction(add_listener_function, context, arraysize(args), args);
+  }
+
+  EXPECT_EQ(kNumListeners, event->GetNumListeners());
+
+  std::vector<v8::Local<v8::Value>> args;
+  event->Fire(context, &args, nullptr);
+
+  EXPECT_TRUE(closure_data.did_invalidate_context);
+}
+
 }  // namespace extensions