[Extensions Bindings] Return result from event dispatch

extensions/renderer/event_emitter.cc

Index: extensions/renderer/event_emitter.cc
diff --git a/extensions/renderer/event_emitter.cc b/extensions/renderer/event_emitter.cc
index ccd4303cedcfcc2c42be654d8d7273d48f7936dd..2ed895752a48c52e017b003d7a035a4328d4b79f 100644
--- a/extensions/renderer/event_emitter.cc
+++ b/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,77 @@ 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) {
+      CHECK(v8_responses
+                ->CreateDataProperty(context, i,

[jbroman, 2017/06/09 19:56:36]

Can there ever be more than 2^32 - 2 listeners? If so, you'll have trouble here
and above. (I'm....pretty sure there cannot be.)

[Devlin, 2017/06/09 20:26:21]

There shouldn't be, but I've added a TODO to enforce this.

+                                     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