[Extensions Bindings] Return result from event dispatch

The JS bindings return an array of results from a call to dispatch(),
containing the results returned by each listener. This is primarily
used, it seems, to detect whether or not a message port will be used
asynchronously through the onMessage event. Because of this, it is
(for now) necessary to also support in native bindings.

Add functionality to return the results from a JS dispatch() call on an
event. Since this is called directly from JS, we should know that
running JS synchronously from that point is safe.

Add tests for returning the result of dispatch() as well as end-to-end
tests for asynchronously responding to a message.

BUG=653596

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

chrome/test/data/extensions/api_test/native_bindings/extension/background.js

@@ -57,6 +57,7 @@ var tests = [
   },
   function testMessaging() {
     var tabId;
+
     var createPort = function() {
       chrome.test.assertTrue(!!tabId);
       var port = chrome.tabs.connect(tabId);
@@ -64,16 +65,22 @@ var tests = [
       port.onMessage.addListener(message => {
         chrome.test.assertEq('content script', message);
         port.disconnect();
-        chrome.test.succeed();
+        chrome.tabs.sendMessage(tabId, 'async bounce', function(response) {
+          chrome.test.assertEq('bounced', response);
+          chrome.test.succeed();
+        });
       });
       port.postMessage('background page');
     };
 
-    chrome.runtime.onMessage.addListener((message, sender, sendResponse) => {
+    chrome.runtime.onMessage.addListener(function listener(
+        message, sender, sendResponse) {
       chrome.test.assertEq('startFlow', message);
       createPort();
       sendResponse('started');
+      chrome.runtime.onMessage.removeListener(listener);
     });
+
     var url = 'http://localhost:' + portNumber +
               '/native_bindings/extension/messaging_test.html';
     chrome.tabs.create({url: url}, function(tab) {

chrome/test/data/extensions/api_test/native_bindings/extension/content_script.js

@@ -2,11 +2,23 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-chrome.runtime.onConnect.addListener((port) => {
+chrome.runtime.onConnect.addListener(function listener(port) {
   port.onMessage.addListener((message) => {
     chrome.test.assertEq('background page', message);
     port.postMessage('content script');
   });
+  chrome.runtime.onConnect.removeListener(listener);
+});
+
+chrome.runtime.onMessage.addListener(
+    function listener(message, sender, sendResponse) {
+  chrome.test.assertEq('async bounce', message);
+  chrome.runtime.onMessage.removeListener(listener);
+  // Respond asynchronously.
+  setTimeout(() => { sendResponse('bounced'); }, 0);
+  // When returning a result asynchronously, the listener must return true -
+  // otherwise the channel is immediately closed.
+  return true;
 });
 
 chrome.runtime.sendMessage('startFlow', function(response) {

extensions/renderer/BUILD.gn

@@ -326,6 +326,7 @@ source_set("unit_tests") {
     "argument_spec_builder.h",
     "argument_spec_unittest.cc",
     "declarative_event_unittest.cc",
+    "event_emitter_unittest.cc",
     "event_unittest.cc",
     "gc_callback_unittest.cc",
     "json_schema_unittest.cc",

extensions/renderer/api_binding_unittest.cc

@@ -164,6 +164,7 @@ class APIBindingUnittest : public APIBindingTest {
       availability_callback_ = base::Bind(&AllowAllFeatures);
     event_handler_ = base::MakeUnique<APIEventHandler>(
         base::Bind(&RunFunctionOnGlobalAndIgnoreResult),
+        base::Bind(&RunFunctionOnGlobalAndReturnHandle),
         base::Bind(&OnEventListenersChanged));
     binding_ = base::MakeUnique<APIBinding>(
         kBindingName, binding_functions_.get(), binding_types_.get(),

extensions/renderer/api_bindings_system.cc

@@ -22,7 +22,7 @@ APIBindingsSystem::APIBindingsSystem(
     : type_reference_map_(base::Bind(&APIBindingsSystem::InitializeType,
                                      base::Unretained(this))),
       request_handler_(send_request, call_js, std::move(last_error)),
-      event_handler_(call_js, event_listeners_changed),
+      event_handler_(call_js, call_js_sync, event_listeners_changed),
       call_js_(call_js),
       call_js_sync_(call_js_sync),
       get_api_schema_(get_api_schema),

extensions/renderer/api_event_handler.cc

@@ -108,8 +108,11 @@ void DispatchEvent(const v8::FunctionCallbackInfo<v8::Value>& info) {
 
 APIEventHandler::APIEventHandler(
     const binding::RunJSFunction& call_js,
+    const binding::RunJSFunctionSync& call_js_sync,
     const EventListenersChangedMethod& listeners_changed)
-    : call_js_(call_js), listeners_changed_(listeners_changed) {}
+    : call_js_(call_js),
+      call_js_sync_(call_js_sync),
+      listeners_changed_(listeners_changed) {}
 APIEventHandler::~APIEventHandler() {}
 
 v8::Local<v8::Object> APIEventHandler::CreateEventInstance(
@@ -139,9 +142,10 @@ v8::Local<v8::Object> APIEventHandler::CreateEventInstance(
         base::MakeUnique<UnfilteredEventListeners>(updated, max_listeners);
   }
 
-  gin::Handle<EventEmitter> emitter_handle = gin::CreateHandle(
-      context->GetIsolate(),
-      new EventEmitter(supports_filters, std::move(listeners), call_js_));
+  gin::Handle<EventEmitter> emitter_handle =
+      gin::CreateHandle(context->GetIsolate(),
+                        new EventEmitter(supports_filters, std::move(listeners),
+                                         call_js_, call_js_sync_));
   CHECK(!emitter_handle.IsEmpty());
   v8::Local<v8::Value> emitter_value = emitter_handle.ToV8();
   CHECK(emitter_value->IsObject());
@@ -161,9 +165,10 @@ v8::Local<v8::Object> APIEventHandler::CreateAnonymousEventInstance(
   std::unique_ptr<APIEventListeners> listeners =
       base::MakeUnique<UnfilteredEventListeners>(
           base::Bind(&DoNothingOnListenersChanged), binding::kNoListenerMax);
-  gin::Handle<EventEmitter> emitter_handle = gin::CreateHandle(
-      context->GetIsolate(),
-      new EventEmitter(supports_filters, std::move(listeners), call_js_));
+  gin::Handle<EventEmitter> emitter_handle =
+      gin::CreateHandle(context->GetIsolate(),
+                        new EventEmitter(supports_filters, std::move(listeners),
+                                         call_js_, call_js_sync_));
   CHECK(!emitter_handle.IsEmpty());
   v8::Local<v8::Object> emitter_object = emitter_handle.ToV8().As<v8::Object>();
   data->anonymous_emitters.push_back(

extensions/renderer/api_event_handler.h

@@ -39,6 +39,7 @@ class APIEventHandler {
                           v8::Local<v8::Context>)>;
 
   APIEventHandler(const binding::RunJSFunction& call_js,
+                  const binding::RunJSFunctionSync& call_js_sync,
                   const EventListenersChangedMethod& listeners_changed);
   ~APIEventHandler();
 
@@ -96,6 +97,7 @@ class APIEventHandler {
  private:
   // Method to run a given v8::Function. Curried in for testing.
   binding::RunJSFunction call_js_;
+  binding::RunJSFunctionSync call_js_sync_;
 
   EventListenersChangedMethod listeners_changed_;
 

extensions/renderer/api_event_handler_unittest.cc

@@ -40,6 +40,7 @@ class APIEventHandlerTest : public APIBindingTest {
     APIBindingTest::SetUp();
     handler_ = base::MakeUnique<APIEventHandler>(
         base::Bind(&RunFunctionOnGlobalAndIgnoreResult),
+        base::Bind(&RunFunctionOnGlobalAndReturnHandle),
         base::Bind(&DoNothingOnEventListenersChanged));
   }
 
@@ -556,6 +557,7 @@ TEST_F(APIEventHandlerTest, TestEventListenersThrowingExceptions) {
 
   SetHandler(base::MakeUnique<APIEventHandler>(
       base::Bind(run_js_and_expect_error),
+      base::Bind(&RunFunctionOnGlobalAndReturnHandle),
       base::Bind(&DoNothingOnEventListenersChanged)));
 
   v8::HandleScope handle_scope(isolate());
@@ -627,7 +629,8 @@ TEST_F(APIEventHandlerTest, TestEventListenersThrowingExceptions) {
 TEST_F(APIEventHandlerTest, CallbackNotifications) {
   MockEventChangeHandler change_handler;
   SetHandler(base::MakeUnique<APIEventHandler>(
-      base::Bind(&RunFunctionOnGlobalAndIgnoreResult), change_handler.Get()));
+      base::Bind(&RunFunctionOnGlobalAndIgnoreResult),
+      base::Bind(&RunFunctionOnGlobalAndReturnHandle), change_handler.Get()));
 
   v8::HandleScope handle_scope(isolate());
 
@@ -914,6 +917,7 @@ TEST_F(APIEventHandlerTest, TestCreateCustomEvent) {
 
   MockEventChangeHandler change_handler;
   APIEventHandler handler(base::Bind(&RunFunctionOnGlobalAndIgnoreResult),
+                          base::Bind(&RunFunctionOnGlobalAndReturnHandle),
                           change_handler.Get());
 
   v8::Local<v8::Object> event = handler.CreateAnonymousEventInstance(context);
@@ -962,6 +966,7 @@ TEST_F(APIEventHandlerTest, TestCreateCustomEventWithCyclicDependency) {
 
   MockEventChangeHandler change_handler;
   APIEventHandler handler(base::Bind(&RunFunctionOnGlobalAndIgnoreResult),
+                          base::Bind(&RunFunctionOnGlobalAndReturnHandle),
                           change_handler.Get());
 
   v8::Local<v8::Object> event = handler.CreateAnonymousEventInstance(context);
@@ -988,6 +993,7 @@ TEST_F(APIEventHandlerTest, TestUnmanagedEvents) {
          v8::Local<v8::Context> context) { ADD_FAILURE(); };
 
   APIEventHandler handler(base::Bind(&RunFunctionOnGlobalAndIgnoreResult),
+                          base::Bind(&RunFunctionOnGlobalAndReturnHandle),
                           base::Bind(fail_on_notified));
 
   const char kEventName[] = "alpha";

extensions/renderer/event_emitter.cc

@@ -7,6 +7,7 @@
 #include <algorithm>
 
 #include "extensions/renderer/api_event_listeners.h"
+#include "gin/data_object_builder.h"
 #include "gin/object_template_builder.h"
 #include "gin/per_context_data.h"
 
@@ -16,10 +17,12 @@ gin::WrapperInfo EventEmitter::kWrapperInfo = {gin::kEmbedderNativeGin};
 
 EventEmitter::EventEmitter(bool supports_filters,
                            std::unique_ptr<APIEventListeners> listeners,
-                           const binding::RunJSFunction& run_js)
+                           const binding::RunJSFunction& run_js,
+                           const binding::RunJSFunctionSync& run_js_sync)
     : supports_filters_(supports_filters),
       listeners_(std::move(listeners)),
-      run_js_(run_js) {}
+      run_js_(run_js),
+      run_js_sync_(run_js_sync) {}
 
 EventEmitter::~EventEmitter() {}
 
@@ -40,18 +43,8 @@ gin::ObjectTemplateBuilder EventEmitter::GetObjectTemplateBuilder(
 void EventEmitter::Fire(v8::Local<v8::Context> context,
                         std::vector<v8::Local<v8::Value>>* args,
                         const EventFilteringInfo* filter) {
-  // Note that |listeners_| can be modified during handling.
-  std::vector<v8::Local<v8::Function>> listeners =
-      listeners_->GetListeners(filter, context);
-
-  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());
-  }
+  bool run_sync = false;
+  DispatchImpl(context, args, filter, run_sync, nullptr);
 }
 
 void EventEmitter::Invalidate(v8::Local<v8::Context> context) {
@@ -127,11 +120,79 @@ void EventEmitter::Dispatch(gin::Arguments* arguments) {
 
   if (listeners_->GetNumListeners() == 0)
     return;
-  v8::HandleScope handle_scope(arguments->isolate());
-  v8::Local<v8::Context> context =
-      arguments->isolate()->GetCurrentContext();
+
+  v8::Isolate* isolate = arguments->isolate();
+  v8::HandleScope handle_scope(isolate);
+  v8::Local<v8::Context> context = isolate->GetCurrentContext();
   std::vector<v8::Local<v8::Value>> v8_args = arguments->GetAll();
-  Fire(context, &v8_args, nullptr);
+
+  // Dispatch() is called from JS, and sometimes expects a return value of an
+  // array with entries for each of the results of the listeners. Since this is
+  // directly from JS, we know it should be safe to call synchronously and use
+  // the return result, so we don't use Fire().
+  // TODO(devlin): It'd be nice to refactor anything expecting a result here so
+  // we don't have to have this special logic, especially since script could
+  // potentially tweak the result object through prototype manipulation (which
+  // also means we should never use this for security decisions).
+  bool run_sync = true;
+  std::vector<v8::Global<v8::Value>> listener_responses;
+  DispatchImpl(context, &v8_args, nullptr, run_sync, &listener_responses);
+
+  if (!listener_responses.size()) {
+    // Return nothing if there are no responses. This is the behavior of the
+    // current JS implementation.
+    return;
+  }
+
+  v8::Local<v8::Object> result;
+  {
+    v8::TryCatch try_catch(isolate);
+    try_catch.SetVerbose(true);
+    v8::Local<v8::Array> v8_responses =
+        v8::Array::New(isolate, listener_responses.size());
+    for (size_t i = 0; i < listener_responses.size(); ++i) {
+      // TODO(devlin): With more than 2^32 - 2 listeners, this can get nasty.
+      // We shouldn't reach that point, but it would be good to add enforcement.
+      CHECK(v8_responses
+                ->CreateDataProperty(context, i,
+                                     listener_responses[i].Get(isolate))
+                .ToChecked());
+    }
+
+    result = gin::DataObjectBuilder(isolate)
+                 .Set("results", v8_responses.As<v8::Value>())
+                 .Build();
+  }
+  arguments->Return(result);
+}
+
+void EventEmitter::DispatchImpl(
+    v8::Local<v8::Context> context,
+    std::vector<v8::Local<v8::Value>>* args,
+    const EventFilteringInfo* filter,
+    bool run_sync,
+    std::vector<v8::Global<v8::Value>>* out_values) {
+  // Note that |listeners_| can be modified during handling.
+  std::vector<v8::Local<v8::Function>> listeners =
+      listeners_->GetListeners(filter, context);
+
+  v8::Isolate* isolate = context->GetIsolate();
+  v8::TryCatch try_catch(isolate);
+  // 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);
+  for (const auto& listener : listeners) {
+    if (run_sync) {
+      DCHECK(out_values);
+      v8::Global<v8::Value> result =
+          run_js_sync_.Run(listener, context, args->size(), args->data());
+      if (!result.IsEmpty() && !result.Get(isolate)->IsUndefined())
+        out_values->push_back(std::move(result));
+    } else {
+      run_js_.Run(listener, context, args->size(), args->data());
+    }
+  }
 }
 
 }  // namespace extensions

extensions/renderer/event_emitter.h

@@ -26,7 +26,8 @@ class EventEmitter final : public gin::Wrappable<EventEmitter> {
  public:
   EventEmitter(bool supports_filters,
                std::unique_ptr<APIEventListeners> listeners,
-               const binding::RunJSFunction& run_js);
+               const binding::RunJSFunction& run_js,
+               const binding::RunJSFunctionSync& run_js_sync);
   ~EventEmitter() override;
 
   static gin::WrapperInfo kWrapperInfo;
@@ -55,6 +56,15 @@ class EventEmitter final : public gin::Wrappable<EventEmitter> {
   bool HasListeners();
   void Dispatch(gin::Arguments* arguments);
 
+  // Notifies the listeners of an event with the given |args|. If |run_sync| is
+  // true, runs JS synchronously and populates |out_values| with the results of
+  // the listeners.
+  void DispatchImpl(v8::Local<v8::Context> context,
+                    std::vector<v8::Local<v8::Value>>* args,
+                    const EventFilteringInfo* filter,
+                    bool run_sync,
+                    std::vector<v8::Global<v8::Value>>* out_values);
+
   // Whether or not this object is still valid; false upon context release.
   // When invalid, no listeners can be added or removed.
   bool valid_ = true;
@@ -65,6 +75,7 @@ class EventEmitter final : public gin::Wrappable<EventEmitter> {
   std::unique_ptr<APIEventListeners> listeners_;
 
   binding::RunJSFunction run_js_;
+  binding::RunJSFunctionSync run_js_sync_;
 
   DISALLOW_COPY_AND_ASSIGN(EventEmitter);
 };

extensions/renderer/event_emitter_unittest.cc

+// 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.
+
+#include "extensions/renderer/event_emitter.h"
+
+#include "base/bind.h"
+#include "base/memory/ptr_util.h"
+#include "base/values.h"
+#include "extensions/renderer/api_binding_test.h"
+#include "extensions/renderer/api_binding_test_util.h"
+#include "extensions/renderer/api_event_listeners.h"
+#include "gin/handle.h"
+
+namespace extensions {
+namespace {
+
+void DoNothingOnListenerChange(binding::EventListenersChanged changed,
+                               const base::DictionaryValue* filter,
+                               bool was_manual,
+                               v8::Local<v8::Context> context) {}
+
+}  // namespace
+
+using EventEmitterUnittest = APIBindingTest;
+
+TEST_F(EventEmitterUnittest, TestDispatchMethod) {
+  v8::HandleScope handle_scope(isolate());
+  v8::Local<v8::Context> context = MainContext();
+
+  auto listeners = base::MakeUnique<UnfilteredEventListeners>(
+      base::Bind(&DoNothingOnListenerChange), binding::kNoListenerMax);
+
+  // The test util methods enforce that functions always throw or always don't
+  // throw, but we test listeners that do both. Provide implementations for
+  // running functions that don't enforce throw behavior.
+  auto run_js_sync = [](v8::Local<v8::Function> function,
+                        v8::Local<v8::Context> context, int argc,
+                        v8::Local<v8::Value> argv[]) {
+    v8::Global<v8::Value> global_result;
+    v8::Local<v8::Value> result;
+    if (function->Call(context, context->Global(), argc, argv).ToLocal(&result))
+      global_result.Reset(context->GetIsolate(), result);
+    return global_result;
+  };
+
+  auto run_js = [](v8::Local<v8::Function> function,
+                   v8::Local<v8::Context> context, int argc,
+                   v8::Local<v8::Value> argv[]) {
+    ignore_result(function->Call(context, context->Global(), argc, argv));
+  };
+
+  gin::Handle<EventEmitter> event = gin::CreateHandle(
+      isolate(), new EventEmitter(false, std::move(listeners),
+                                  base::Bind(run_js), base::Bind(run_js_sync)));
+
+  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);
+
+  auto add_listener = [context, v8_event,
+                       add_listener_function](base::StringPiece listener) {
+    v8::Local<v8::Function> listener_function =
+        FunctionFromString(context, listener);
+    v8::Local<v8::Value> args[] = {v8_event, listener_function};
+    RunFunction(add_listener_function, context, arraysize(args), args);
+  };
+
+  const char kListener1[] =
+      "(function() {\n"
+      "  this.eventArgs1 = Array.from(arguments);\n"
+      "  return 'listener1';\n"
+      "})";
+  add_listener(kListener1);
+  const char kListener2[] =
+      "(function() {\n"
+      "  this.eventArgs2 = Array.from(arguments);\n"
+      "  return {listener: 'listener2'};\n"
+      "})";
+  add_listener(kListener2);
+  // Listener3 throws, but shouldn't stop the event from reaching other
+  // listeners.
+  const char kListener3[] =
+      "(function() {\n"
+      "  this.eventArgs3 = Array.from(arguments);\n"
+      "  throw new Error('hahaha');\n"
+      "})";
+  add_listener(kListener3);
+  // Returning undefined should not be added to the array of results from
+  // dispatch.
+  const char kListener4[] =
+      "(function() {\n"
+      "  this.eventArgs4 = Array.from(arguments);\n"
+      "})";
+  add_listener(kListener4);
+
+  const char kDispatch[] =
+      "(function(event) {\n"
+      "  return event.dispatch('arg1', 2);\n"
+      "})";
+  v8::Local<v8::Value> dispatch_args[] = {v8_event};
+  v8::Local<v8::Value> dispatch_result =
+      RunFunctionOnGlobal(FunctionFromString(context, kDispatch), context,
+                          arraysize(dispatch_args), dispatch_args);
+
+  const char kExpectedEventArgs[] = "[\"arg1\",2]";
+  for (const char* property :
+       {"eventArgs1", "eventArgs2", "eventArgs3", "eventArgs4"}) {
+    EXPECT_EQ(kExpectedEventArgs, GetStringPropertyFromObject(
+                                      context->Global(), context, property));
+  }
+  EXPECT_EQ("{\"results\":[\"listener1\",{\"listener\":\"listener2\"}]}",
+            V8ToString(dispatch_result, context));
+}
+
+}  // namespace extensions