[Extension Bindings] Test event listeners throwing exceptions

extensions/renderer/api_event_handler_unittest.cc

Index: extensions/renderer/api_event_handler_unittest.cc
diff --git a/extensions/renderer/api_event_handler_unittest.cc b/extensions/renderer/api_event_handler_unittest.cc
index ce1993779b74cf469e82d8be4e1bce1cd76ee3c5..21fd381b3608f0b4cc52de2660090b67ed735057 100644
--- a/extensions/renderer/api_event_handler_unittest.cc
+++ b/extensions/renderer/api_event_handler_unittest.cc
@@ -447,4 +447,65 @@ TEST_F(APIEventHandlerTest, TestDispatchFromJs) {
                 context->Global(), context, "eventArgs"));
 }
 
+// Test an event listener throwing an exception.
+TEST_F(APIEventHandlerTest, TestEventListenersThrowingExceptions) {
+  auto run_js_and_expect_error = [](v8::Local<v8::Function> function,

[jbroman, 2017/01/05 19:52:51]

Isn't it the RunJSFunction that's supposed to be catching exceptions (which this
test one doesn't, but I think the production one does)? Without that, it looks
to me like the exception does escape from FireEventInContext, which I wouldn't
have expected.

[Devlin, 2017/01/05 21:01:46]

No, RunJSFunction doesn't always catch exceptions (and doesn't in the prod
version).  The test version usually does, but that's really just to catch
unintentional exceptions (since we have RunFunction and
RunFunctionAndExpectError).  The complication is that we can't pass either one
of those in here, since it only sometimes expects an error.  Does that make
sense?

I've also added a basic event test (including dispatching to listeners and
throwing exceptions) to the NativeExtensionBindingsSystem, which uses the prod
run js function.

[jbroman, 2017/01/05 22:22:14]

Should we be catching exceptions in the event dispatch logic, then? Blink does
catch exceptions during event listeners.
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/...

I _think_ here they'll end up propagating to script, which seems weird.

[Devlin, 2017/01/10 21:28:07]

Fixed (thanks for the help!).  Note that we still need this helper function,
since the test utils (RunFunction et al) expect no exception ever thrown, but I
think the test demonstrates the correct behavior.

+                                    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());
+
+  // 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"));
+}
+
 }  // namespace extensions