[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 4d5891f337087eb6928d6901617254dfd6b2160a..430f96c7170da03b7d86e7feb9a58b52a28608f8 100644
--- a/extensions/renderer/api_event_handler_unittest.cc
+++ b/extensions/renderer/api_event_handler_unittest.cc
@@ -436,7 +436,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 +484,83 @@ 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*>(v8::Local<v8::External>::Cast(data)->Value());

[jbroman, 2017/01/10 23:17:22]

super-nit: using Local::As instead of Local::Cast makes this fit on one line.

bool* did_throw = static_cast<bool*>(data.As<v8::External>()->Value());

[Devlin, 2017/01/10 23:26:01]

heh fair enough.  Done.

+    *did_throw = true;
+    EXPECT_EQ("Uncaught Error: Event handler error",
+              gin::V8ToString(message->Get()));
+  };
+
+  isolate()->AddMessageListener(message_listener,
+                                v8::External::New(isolate(), &did_throw));

[jbroman, 2017/01/10 23:17:22]

nit: Hrm...I guess this is probably fine, but it'd be a little unfortunate if
this had already gone out of scope before something (maybe in the teardown)
triggered a message.

One way would be a scoped object that removed itself as a listener when it went
out of scope.


class ScopedMessageListener {
 public:
  ScopedMessageListener(v8::Isolate* isolate) : isolate_(isolate) {
    isolate->AddMessageListener(&ScopedMessageListener::OnMessage,
v8::External::New(isolate, this));
  }
  ~ScopedMessageListener() {
isolate_->RemoveMessageListeners(&ScopedMessageListener::OnMessage); }
  static void OnMessage(v8::Local<v8::Message> message, v8::Local<v8::Value>
data) {
    ASSERT_FALSE(data.IsEmpty());
    ASSERT_TRUE(data->IsExternal());
    auto* self =
static_cast<ScopedMessageListener*>(data.As<v8::External>()->Value());
    self->message_ = gin::V8ToString(message->Get());
  }
  const std::string& message() { return message_; }
 private:
  v8::Isolate* isolate_;
  std::string message_;
};

ScopedMessageListener listener(isolate);
// do stuff
EXPECT_EQ("Uncaught Error: Event handler error", listener.message());


Since this is just a one-off, ScopedClosureRunner might be fine (lacking one for
lambdas):


isolate()->AddMessageListener(message_listener,
                              v8::External::New(isolate(), &did_throw));
base::ScopedClosureRunner scoped_remove(base::Bind([](v8::Isolate* isolate,
v8::MessageCallback callback) {
  isolate->RemoveMessageListeners(callback);
}, isolate, callback));


Or we could live with the mild risk of unsafety here. Since it's just a unit
test, I don't feel strongly.

[Devlin, 2017/01/10 23:26:01]

Using the ScopedClosureRunner sgtm; done.

+
+  // 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