[Extension Bindings] Test event listeners throwing exceptions

Listeners can throw exceptions (as with any third-party code). Ensure
that we have tests for this. When a listener throws an exception, it
should be logged to the console, but not bubbled up to external try-
catches (so that JS code that may have triggered the event doesn't
receive it). This is the same as we do for web events.

BUG=653596

Review-Url: https://codereview.chromium.org/2613093002
Cr-Commit-Position: refs/heads/master@{#442759}

extensions/renderer/api_event_handler_unittest.cc

@@ -5,6 +5,7 @@
 #include "extensions/renderer/api_event_handler.h"
 
 #include "base/bind.h"
+#include "base/callback_helpers.h"
 #include "base/memory/ptr_util.h"
 #include "base/values.h"
 #include "extensions/renderer/api_binding_test.h"
@@ -436,7 +437,6 @@ TEST_F(APIEventHandlerTest, RemovingListenersWhileHandlingEvent) {
   v8::Local<v8::Object> event =
       handler.CreateEventInstance(kEventName, context);
   ASSERT_FALSE(event.IsEmpty());
-
   {
     // Cache the event object on the global in order to allow for easy removal.
     v8::Local<v8::Function> set_event_on_global =
@@ -485,4 +485,84 @@ TEST_F(APIEventHandlerTest, RemovingListenersWhileHandlingEvent) {
   // notified. Investigate what we currently do in JS-style bindings.
 }
 
+// Test an event listener throwing an exception.
+TEST_F(APIEventHandlerTest, TestEventListenersThrowingExceptions) {
+  // The default test util methods (RunFunction*) assume no errors will ever
+  // be encountered. Instead, use an implementation that allows errors.
+  auto run_js_and_expect_error = [](v8::Local<v8::Function> function,
+                                    v8::Local<v8::Context> context, int argc,
+                                    v8::Local<v8::Value> argv[]) {
+    v8::MaybeLocal<v8::Value> maybe_result =
+        function->Call(context, context->Global(), argc, argv);
+    v8::Global<v8::Value> result;
+    v8::Local<v8::Value> local;
+    if (maybe_result.ToLocal(&local))
+      result.Reset(context->GetIsolate(), local);
+  };
+
+  v8::HandleScope handle_scope(isolate());
+  v8::Local<v8::Context> context = ContextLocal();
+
+  APIEventHandler handler(base::Bind(run_js_and_expect_error));
+  const char kEventName[] = "alpha";
+  v8::Local<v8::Object> event =
+      handler.CreateEventInstance(kEventName, context);
+  ASSERT_FALSE(event.IsEmpty());
+
+  bool did_throw = false;
+  auto message_listener = [](v8::Local<v8::Message> message,
+                             v8::Local<v8::Value> data) {
+    ASSERT_FALSE(data.IsEmpty());
+    ASSERT_TRUE(data->IsExternal());
+    bool* did_throw = static_cast<bool*>(data.As<v8::External>()->Value());
+    *did_throw = true;
+    EXPECT_EQ("Uncaught Error: Event handler error",
+              gin::V8ToString(message->Get()));
+  };
+
+  isolate()->AddMessageListener(message_listener,
+                                v8::External::New(isolate(), &did_throw));
+  base::ScopedClosureRunner remove_message_listener(base::Bind(
+      [](v8::Isolate* isolate, v8::MessageCallback listener) {
+        isolate->RemoveMessageListeners(listener);
+      },
+      isolate(), message_listener));
+
+  // A listener that will throw an exception. We guarantee that we throw the
+  // exception first so that we don't rely on event listener ordering.
+  const char kListenerFunction[] =
+      "(function() {\n"
+      "  if (!this.didThrow) {\n"
+      "    this.didThrow = true;\n"
+      "    throw new Error('Event handler error');\n"
+      "  }\n"
+      "  this.eventArgs = Array.from(arguments);\n"
+      "});";
+
+  const char kAddListenerFunction[] =
+      "(function(event, listener) { event.addListener(listener); })";
+  v8::Local<v8::Function> add_listener_function =
+      FunctionFromString(context, kAddListenerFunction);
+
+  for (int i = 0; i < 2; ++i) {
+    v8::Local<v8::Function> listener =
+        FunctionFromString(context, kListenerFunction);
+    v8::Local<v8::Value> argv[] = {event, listener};
+    RunFunctionOnGlobal(add_listener_function, context, arraysize(argv), argv);
+  }
+  EXPECT_EQ(2u, handler.GetNumEventListenersForTesting(kEventName, context));
+
+  std::unique_ptr<base::ListValue> event_args = ListValueFromString("[42]");
+  ASSERT_TRUE(event_args);
+  handler.FireEventInContext(kEventName, context, *event_args);
+
+  // An exception should have been thrown by the first listener and the second
+  // listener should have recorded the event arguments.
+  EXPECT_EQ("true", GetStringPropertyFromObject(context->Global(), context,
+                                                "didThrow"));
+  EXPECT_EQ("[42]", GetStringPropertyFromObject(context->Global(), context,
+                                                "eventArgs"));
+  EXPECT_TRUE(did_throw);
+}
+
 }  // namespace extensions

extensions/renderer/event_emitter.cc

@@ -41,8 +41,14 @@ void EventEmitter::Fire(v8::Local<v8::Context> context,
   for (const auto& listener : listeners_)
     listeners.push_back(listener.Get(context->GetIsolate()));
 
-  for (const auto& listener : listeners)
+  for (const auto& listener : listeners) {
+    v8::TryCatch try_catch(context->GetIsolate());
+    // SetVerbose() means the error will still get logged, which is what we
+    // want. We don't let it bubble up any further to prevent it from being
+    // surfaced in e.g. JS code that triggered the event.
+    try_catch.SetVerbose(true);
     run_js_.Run(listener, context, args->size(), args->data());
+  }
 }
 
 void EventEmitter::AddListener(gin::Arguments* arguments) {

extensions/renderer/native_extension_bindings_system_unittest.cc

@@ -216,6 +216,46 @@ TEST_F(NativeExtensionBindingsSystemUnittest, Basic) {
   EXPECT_TRUE(request_keep_awake->IsFunction());
 }
 
+TEST_F(NativeExtensionBindingsSystemUnittest, Events) {
+  scoped_refptr<Extension> extension =
+      CreateExtension("foo", {"idle", "power"});
+  RegisterExtension(extension->id());
+
+  v8::HandleScope handle_scope(isolate());
+  v8::Local<v8::Context> context = ContextLocal();
+
+  ScriptContext* script_context = CreateScriptContext(
+      context, extension.get(), Feature::BLESSED_EXTENSION_CONTEXT);
+  script_context->set_url(extension->url());
+
+  bindings_system()->UpdateBindingsForContext(script_context);
+
+  {
+    const char kAddStateChangedListeners[] =
+        "(function() {\n"
+        "  chrome.idle.onStateChanged.addListener(function() {\n"
+        "    this.didThrow = true;\n"
+        "    throw new Error('Error!!!');\n"
+        "  });\n"
+        "  chrome.idle.onStateChanged.addListener(function(newState) {\n"
+        "    this.newState = newState;\n"
+        "  });\n"
+        "});";
+
+    v8::Local<v8::Function> add_listeners =
+        FunctionFromString(context, kAddStateChangedListeners);
+    RunFunctionOnGlobal(add_listeners, context, 0, nullptr);
+  }
+
+  bindings_system()->DispatchEventInContext(
+      "idle.onStateChanged", ListValueFromString("['idle']").get(), nullptr,
+      script_context);
+  EXPECT_EQ("\"idle\"", GetStringPropertyFromObject(context->Global(), context,
+                                                    "newState"));
+  EXPECT_EQ("true", GetStringPropertyFromObject(context->Global(), context,
+                                                "didThrow"));
+}
+
 // Tests that referencing the same API multiple times returns the same object;
 // i.e. chrome.foo === chrome.foo.
 TEST_F(NativeExtensionBindingsSystemUnittest, APIObjectsAreEqual) {