[Extensions Bindings] Return result from event dispatch

[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}
Committed: https://chromium.googlesource.com/chromium/src/+/a3c4e75c6feb70b9af379067bf2daa7c9ef92b62

[Devlin, 2017-06-02 16:10:32]

The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-02 16:10:51]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-02 16:29:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-02 16:29:48]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Devlin, 2017-06-02 22:21:47]

The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-02 22:22:30]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-02 23:53:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-02 23:53:37]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Devlin, 2017-06-03 01:13:55]

Description was changed from

==========
[Extensions Bindings] Return result from event
BUG=
==========

to

==========
[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
==========

[Devlin, 2017-06-03 01:14:22]

rdevlin.cronin@chromium.org changed reviewers:
+ jbroman@chromium.org, lazyboy@chromium.org

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

https://codereview.chromium.org/2921013002/diff/40001/extensions/renderer/eve...
File extensions/renderer/event_emitter.cc (right):

https://codereview.chromium.org/2921013002/diff/40001/extensions/renderer/eve...
extensions/renderer/event_emitter.cc:145: v8::Local<v8::Array> v8_responses =
v8::Array::New(isolate);
super-nit: length is already known; pass it to v8::Array::New?

https://codereview.chromium.org/2921013002/diff/40001/extensions/renderer/eve...
extensions/renderer/event_emitter.cc:148: try_catch.SetVerbose(true);
You already have a TryCatch a few lines up, outside the loop. Delete this one?

https://codereview.chromium.org/2921013002/diff/40001/extensions/renderer/eve...
extensions/renderer/event_emitter.cc:149: if (!v8_responses->Set(context, i,
listener_responses[i].Get(isolate))
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.

https://codereview.chromium.org/2921013002/diff/40001/extensions/renderer/eve...
extensions/renderer/event_emitter.cc:155: result =
gin::DataObjectBuilder(isolate)
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?

https://codereview.chromium.org/2921013002/diff/40001/extensions/renderer/eve...
extensions/renderer/event_emitter.cc:159:
arguments->Return(result.As<v8::Value>());
nit: is the upcast needed here?

https://codereview.chromium.org/2921013002/diff/40001/extensions/renderer/eve...
extensions/renderer/event_emitter.cc:173: v8::TryCatch
try_catch(context->GetIsolate());
nit: Lift the TryCatch outside the loop?

https://codereview.chromium.org/2921013002/diff/40001/extensions/renderer/eve...
extensions/renderer/event_emitter.cc:183: if (!result.IsEmpty())
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:05]

The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run

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

https://codereview.chromium.org/2921013002/diff/40001/extensions/renderer/eve...
File extensions/renderer/event_emitter.cc (right):

https://codereview.chromium.org/2921013002/diff/40001/extensions/renderer/eve...
extensions/renderer/event_emitter.cc:145: v8::Local<v8::Array> v8_responses =
v8::Array::New(isolate);
On 2017/06/06 18:29:10, jbroman wrote:
> super-nit: length is already known; pass it to v8::Array::New?

Thanks!  Done.

https://codereview.chromium.org/2921013002/diff/40001/extensions/renderer/eve...
extensions/renderer/event_emitter.cc:148: try_catch.SetVerbose(true);
On 2017/06/06 18:29:09, jbroman wrote:
> You already have a TryCatch a few lines up, outside the loop. Delete this one?

Done.

https://codereview.chromium.org/2921013002/diff/40001/extensions/renderer/eve...
extensions/renderer/event_emitter.cc:149: if (!v8_responses->Set(context, i,
listener_responses[i].Get(isolate))
On 2017/06/06 18:29:09, jbroman wrote:
> 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.

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?

https://codereview.chromium.org/2921013002/diff/40001/extensions/renderer/eve...
extensions/renderer/event_emitter.cc:155: result =
gin::DataObjectBuilder(isolate)
On 2017/06/06 18:29:09, jbroman wrote:
> 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?

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

https://codereview.chromium.org/2921013002/diff/40001/extensions/renderer/eve...
extensions/renderer/event_emitter.cc:159:
arguments->Return(result.As<v8::Value>());
On 2017/06/06 18:29:09, jbroman wrote:
> nit: is the upcast needed here?

I thought so, but apparently not.  Removed.

https://codereview.chromium.org/2921013002/diff/40001/extensions/renderer/eve...
extensions/renderer/event_emitter.cc:173: v8::TryCatch
try_catch(context->GetIsolate());
On 2017/06/06 18:29:09, jbroman wrote:
> nit: Lift the TryCatch outside the loop?

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

https://codereview.chromium.org/2921013002/diff/40001/extensions/renderer/eve...
extensions/renderer/event_emitter.cc:183: if (!result.IsEmpty())
On 2017/06/06 18:29:09, jbroman wrote:
> 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...

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.

[commit-bot: I haz the power, 2017-06-09 18:20:27]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-09 19:53:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-09 19:53:36]

Dry run: This issue passed the CQ dry run.

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

lgtm

https://codereview.chromium.org/2921013002/diff/40001/extensions/renderer/eve...
File extensions/renderer/event_emitter.cc (right):

https://codereview.chromium.org/2921013002/diff/40001/extensions/renderer/eve...
extensions/renderer/event_emitter.cc:149: if (!v8_responses->Set(context, i,
listener_responses[i].Get(isolate))
On 2017/06/09 at 18:20:14, Devlin (sheriff Jun 9 - 12) wrote:
> On 2017/06/06 18:29:09, jbroman wrote:
> > 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.
> 
> 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?

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

https://codereview.chromium.org/2921013002/diff/40001/extensions/renderer/eve...
extensions/renderer/event_emitter.cc:173: v8::TryCatch
try_catch(context->GetIsolate());
On 2017/06/09 at 18:20:14, Devlin (sheriff Jun 9 - 12) wrote:
> On 2017/06/06 18:29:09, jbroman wrote:
> > nit: Lift the TryCatch outside the loop?
> 
> Done.  To confirm, can verbose try-catches handle multiple errors?

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

https://codereview.chromium.org/2921013002/diff/60001/extensions/renderer/eve...
File extensions/renderer/event_emitter.cc (right):

https://codereview.chromium.org/2921013002/diff/60001/extensions/renderer/eve...
extensions/renderer/event_emitter.cc:155: ->CreateDataProperty(context, i,
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:16]

The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run

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

https://codereview.chromium.org/2921013002/diff/60001/extensions/renderer/eve...
File extensions/renderer/event_emitter.cc (right):

https://codereview.chromium.org/2921013002/diff/60001/extensions/renderer/eve...
extensions/renderer/event_emitter.cc:155: ->CreateDataProperty(context, i,
On 2017/06/09 19:56:36, jbroman wrote:
> 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.)

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

[commit-bot: I haz the power, 2017-06-09 20:26:32]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-09 22:06:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-09 22:06:34]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jbroman, 2017-06-12 15:17:03]

lgtm stands

[lazyboy, 2017-06-12 22:19:19]

lgtm

(the returned results array not having 1:1 correspondence with the # of
listeners seems very confusing indeed).

[Devlin, 2017-06-12 22:40:31]

The CQ bit was checked by rdevlin.cronin@chromium.org

[commit-bot: I haz the power, 2017-06-12 22:41:10]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-13 00:19:20]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1497307231835030,
"parent_rev": "c3360ef672239afe5cd159b893a20fdf22106d8b", "commit_rev":
"56182de7cc577abbf7cfaca0f40a5e97c778d4cc"}

[commit-bot: I haz the power, 2017-06-13 00:19:34]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1497307231835030,
"parent_rev": "a4ab2b3a0adc017cfe2d0ab19ad2092bf3beabb7", "commit_rev":
"a3c4e75c6feb70b9af379067bf2daa7c9ef92b62"}

[commit-bot: I haz the power, 2017-06-13 00:19:47]

Description was changed from

==========
[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
==========

to

==========
[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}
Committed:
https://chromium.googlesource.com/chromium/src/+/a3c4e75c6feb70b9af379067bf2d...
==========

[commit-bot: I haz the power, 2017-06-13 00:19:50]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/a3c4e75c6feb70b9af379067bf2d...