[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..a92da8b6f8e1defff0c2d50bbe7fc073814b180d 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,72 @@ 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);
+
+  v8::Local<v8::Object> result;
+  {
+    v8::TryCatch try_catch(isolate);
+    try_catch.SetVerbose(true);
+    v8::Local<v8::Array> v8_responses = v8::Array::New(isolate);

[jbroman, 2017/06/06 18:29:10]

super-nit: length is already known; pass it to v8::Array::New?

[Devlin, 2017/06/09 18:20:14]

Thanks!  Done.

+    for (size_t i = 0; i < listener_responses.size(); ++i) {
+      v8::TryCatch try_catch(isolate);
+      try_catch.SetVerbose(true);

[jbroman, 2017/06/06 18:29:09]

You already have a TryCatch a few lines up, outside the loop. Delete this one?

[Devlin, 2017/06/09 18:20:14]

Done.

+      if (!v8_responses->Set(context, i, listener_responses[i].Get(isolate))

[jbroman, 2017/06/06 18:29:09]

warning: v8::Object::Set can invoke author script, since the array prototype
chain can be messed with. Possibly fine (script can run in DispatchImpl, too),
but we use CreateDataProperty in the similar Blink code.

[Devlin, 2017/06/09 18:20:14]

Done.  Also converted to CHECK(CreateDataProperty().ToChecked()) since this
should never fail, right?  Or is there some unusual way that, since the listener
response is author-supplied, assigning it in an array could throw?

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

I can't imagine how it could fail; sgtm.

+               .IsJust()) {
+        return;
+      }
+    }
+
+    result = gin::DataObjectBuilder(isolate)

[jbroman, 2017/06/06 18:29:09]

The current code does the slightly quirky thing where it returns undefined
rather than an object if there are no results other than undefined. Intentional
change?

[Devlin, 2017/06/09 18:20:14]

Semi-intentional, but probably not worth it.  Updated to match current behavior.

+                 .Set("results", v8_responses.As<v8::Value>())
+                 .Build();
+  }
+  arguments->Return(result.As<v8::Value>());

[jbroman, 2017/06/06 18:29:09]

nit: is the upcast needed here?

[Devlin, 2017/06/09 18:20:14]

I thought so, but apparently not.  Removed.

+}
+
+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);
+
+  for (const auto& listener : listeners) {
+    v8::TryCatch try_catch(context->GetIsolate());

[jbroman, 2017/06/06 18:29:09]

nit: Lift the TryCatch outside the loop?

[Devlin, 2017/06/09 18:20:14]

Done.  To confirm, can verbose try-catches handle multiple errors?

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

Yeah, I'm pretty sure they report it at the time that it would propagate to the
TryCatch, not in ~TryCatch.

+    // 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);
+
+    if (run_sync) {
+      DCHECK(out_values);
+      v8::Global<v8::Value> result =
+          run_js_sync_.Run(listener, context, args->size(), args->data());
+      if (!result.IsEmpty())

[jbroman, 2017/06/06 18:29:09]

This seems different from what the JS does. Unless an exception is thrown,
synchronously running script should always yield a value, even if that value is
undefined (which is what the JS seems to check for):

https://chromium.googlesource.com/chromium/src/+/67095448eb212b9f8e09f0715156...

[Devlin, 2017/06/09 18:20:14]

The check for empty was deliberate, since some listeners can throw errors, but
we should also be checking for undefined here.  I've added that check, and
expanded the test.

+        out_values->push_back(std::move(result));
+    } else {
+      run_js_.Run(listener, context, args->size(), args->data());
+    }
+  }
 }
 
 }  // namespace extensions