[Extension Bindings] Test event listeners throwing exceptions

extensions/renderer/api_event_handler_unittest.cc

 // Copyright 2016 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/api_event_handler.h"
 
 #include "base/bind.h"
 #include "base/memory/ptr_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/values.h"
@@ skipping 429 matching lines @@
   {
     v8::Local<v8::Value> argv[] = {event};
     RunFunctionOnGlobal(fire_event_function, context, arraysize(argv), argv);
   }
 
   EXPECT_EQ("[42,\"foo\",{\"bar\":\"baz\"}]",
             GetStringPropertyFromObject(
                 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