[Extensions Bindings] Request JS execution from messaging bindings

[Extensions Bindings] Request JS execution from messaging bindings

Instead of synchronously forcing JS execution in messaging bindings, use
safer alternatives to request JS execution. Since this modifies the
ability of the called function to return a result (synchronously, at
least), change messaging bindings to query for listeners natively rather
than in JS (which is also less likely to be incorrect).

This involves introducing a way to query if a given context has a
listener for an event. Provide this functionality for both native and
JS bindings. For native bindings, simply expose a method on
APIEventHandler. For JS bindings, make the NullAttachmentStrategy
register unmanaged events with the event bindings.

Also expose a way to get the Dispatcher statically from the messaging
bindings in order to get at the ExtensionBindingsSystem, and restructure
ownership of the dispatcher in extensions/shell.

BUG=629431
Review-Url: https://codereview.chromium.org/2909673003
Cr-Commit-Position: refs/heads/master@{#478492}
Committed: https://chromium.googlesource.com/chromium/src/+/978464a97d85014ca4414366789702be3b35fa15

[Devlin, 2017-05-26 17:54:39]

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

[commit-bot: I haz the power, 2017-05-26 17:54:59]

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-05-26 18:11:25]

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

[commit-bot: I haz the power, 2017-05-26 18:11:26]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Devlin, 2017-05-26 20:38:34]

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

[commit-bot: I haz the power, 2017-05-26 20:39:15]

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-05-26 22:09:31]

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

[commit-bot: I haz the power, 2017-05-26 22:09:32]

Dry run: This issue passed the CQ dry run.

[Devlin, 2017-05-26 22:54:49]

Description was changed from

==========
Maybe fix
BUG=
==========

to

==========
[Extensions Bindings] Request JS execution from messaging bindings

Instead of synchronously forcing JS execution in messaging bindings, use
safer alternatives to request JS execution. Since this modifies the
ability of the called function to return a result (synchronously, at
least), change messaging bindings to query for listeners natively rather
than in JS (which is also less likely to be incorrect).

This involves introducing a way to query if a given context has a
listener for an event. Provide this functionality for both native and
JS bindings. For native bindings, simply expose a method on
APIEventHandler. For JS bindings, make the NullAttachmentStrategy
register unmanaged events with the event bindings.

Also expose a way to get the Dispatcher statically from the messaging
bindings in order to get at the ExtensionBindingsSystem, and restructure
ownership of the dispatcher in extensions/shell.

BUG=629431
==========

[Devlin, 2017-05-27 00:51:04]

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

[commit-bot: I haz the power, 2017-05-27 00:51:23]

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-05-27 01:26:58]

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

[commit-bot: I haz the power, 2017-05-27 01:26:59]

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

[Devlin, 2017-05-27 01:29:12]

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

[Devlin, 2017-05-27 01:29:12]

lazyboy@, mind taking a look when you get back?

(The bots are red on the latest patch set, but seems unrelated - they're green
on #2)

[Devlin, 2017-06-02 22:22:24]

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:23:00]

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:15:45]

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

[commit-bot: I haz the power, 2017-06-02 23:15:46]

Dry run: This issue passed the CQ dry run.

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

lazyboy@, friendly ping? :)

[lazyboy, 2017-06-05 18:18:33]

Very sorry for slipping this one, I had drafts saved hence didn't see it in my
queue.

https://codereview.chromium.org/2909673003/diff/40001/extensions/renderer/api...
File extensions/renderer/api_event_handler.cc (right):

https://codereview.chromium.org/2909673003/diff/40001/extensions/renderer/api...
extensions/renderer/api_event_handler.cc:271: return emitter->GetNumListeners()
> 0;
Given that we only care about EventEmitter::HasListeners from non-test code
outside of EventEmitter, consider making HasListeners public in EventEmitter and
make GetNumListeners->GetNumListenersForTesting.

https://codereview.chromium.org/2909673003/diff/40001/extensions/renderer/api...
File extensions/renderer/api_event_handler.h (right):

https://codereview.chromium.org/2909673003/diff/40001/extensions/renderer/api...
extensions/renderer/api_event_handler.h:89: // Returns the EventListeners for a
given |event_name| and |context|.
the number of EventListeners

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

https://codereview.chromium.org/2909673003/diff/40001/extensions/renderer/eve...
extensions/renderer/event_bindings.cc:197: auto& unmanaged_listeners =
g_unmanaged_listeners.Get();
const auto& here and in line 203?

https://codereview.chromium.org/2909673003/diff/40001/extensions/renderer/mes...
File extensions/renderer/messaging_bindings.cc (left):

https://codereview.chromium.org/2909673003/diff/40001/extensions/renderer/mes...
extensions/renderer/messaging_bindings.cc:92:
base::Bind(&MessagingBindings::RemovePortWithJsId, bindings->GetWeakPtr(),
RemovePortWithJsId is unused now.

https://codereview.chromium.org/2909673003/diff/40001/extensions/renderer/res...
File extensions/renderer/resources/messaging.js (right):

https://codereview.chromium.org/2909673003/diff/40001/extensions/renderer/res...
extensions/renderer/resources/messaging.js:255: if (!usedPort) {
nit: isPortUsed (usedPort var might be misleading to mean that it is the used
port).

https://codereview.chromium.org/2909673003/diff/40001/extensions/renderer/res...
extensions/renderer/resources/messaging.js:261:
messagingNatives.CloseChannel(portId, false);
nit: false /* force_close */

[Devlin, 2017-06-09 18:38:59]

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

[commit-bot: I haz the power, 2017-06-09 18:39:28]

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:38:10]

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

[commit-bot: I haz the power, 2017-06-09 19:38:11]

Dry run: This issue passed the CQ dry run.

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

https://codereview.chromium.org/2909673003/diff/40001/extensions/renderer/api...
File extensions/renderer/api_event_handler.cc (right):

https://codereview.chromium.org/2909673003/diff/40001/extensions/renderer/api...
extensions/renderer/api_event_handler.cc:271: return emitter->GetNumListeners()
> 0;
On 2017/06/05 18:18:32, lazyboy wrote:
> Given that we only care about EventEmitter::HasListeners from non-test code
> outside of EventEmitter, consider making HasListeners public in EventEmitter
and
> make GetNumListeners->GetNumListenersForTesting.

Good thought.  Since this patch is already a bit large and that's somewhat
tangential, I've added a TODO for now.

https://codereview.chromium.org/2909673003/diff/40001/extensions/renderer/api...
File extensions/renderer/api_event_handler.h (right):

https://codereview.chromium.org/2909673003/diff/40001/extensions/renderer/api...
extensions/renderer/api_event_handler.h:89: // Returns the EventListeners for a
given |event_name| and |context|.
On 2017/06/05 18:18:32, lazyboy wrote:
> the number of EventListeners

Done.

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

https://codereview.chromium.org/2909673003/diff/40001/extensions/renderer/eve...
extensions/renderer/event_bindings.cc:197: auto& unmanaged_listeners =
g_unmanaged_listeners.Get();
On 2017/06/05 18:18:32, lazyboy wrote:
> const auto& here and in line 203?

Done.

https://codereview.chromium.org/2909673003/diff/40001/extensions/renderer/mes...
File extensions/renderer/messaging_bindings.cc (left):

https://codereview.chromium.org/2909673003/diff/40001/extensions/renderer/mes...
extensions/renderer/messaging_bindings.cc:92:
base::Bind(&MessagingBindings::RemovePortWithJsId, bindings->GetWeakPtr(),
On 2017/06/05 18:18:32, lazyboy wrote:
> RemovePortWithJsId is unused now.

Good catch!  Done.

https://codereview.chromium.org/2909673003/diff/40001/extensions/renderer/res...
File extensions/renderer/resources/messaging.js (right):

https://codereview.chromium.org/2909673003/diff/40001/extensions/renderer/res...
extensions/renderer/resources/messaging.js:255: if (!usedPort) {
On 2017/06/05 18:18:33, lazyboy wrote:
> nit: isPortUsed (usedPort var might be misleading to mean that it is the used
> port).

Good point; changed to wasPortUsed.

https://codereview.chromium.org/2909673003/diff/40001/extensions/renderer/res...
extensions/renderer/resources/messaging.js:261:
messagingNatives.CloseChannel(portId, false);
On 2017/06/05 18:18:33, lazyboy wrote:
> nit: false /* force_close */

Done.

[lazyboy, 2017-06-10 00:09:05]

lgtm

https://codereview.chromium.org/2909673003/diff/40001/extensions/renderer/api...
File extensions/renderer/api_event_handler.cc (right):

https://codereview.chromium.org/2909673003/diff/40001/extensions/renderer/api...
extensions/renderer/api_event_handler.cc:271: return emitter->GetNumListeners()
> 0;
On 2017/06/09 20:18:01, Devlin (sheriff Jun 9 - 12) wrote:
> On 2017/06/05 18:18:32, lazyboy wrote:
> > Given that we only care about EventEmitter::HasListeners from non-test code
> > outside of EventEmitter, consider making HasListeners public in EventEmitter
> and
> > make GetNumListeners->GetNumListenersForTesting.
> 
> Good thought.  Since this patch is already a bit large and that's somewhat
> tangential, I've added a TODO for now.

Acknowledged.

[Devlin, 2017-06-10 01:25:52]

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

[commit-bot: I haz the power, 2017-06-10 01:26:19]

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-10 01:30:08]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1497057952431590,
"parent_rev": "3ad400079c49cfc57cb008cfb23dcd3a99139dab", "commit_rev":
"978464a97d85014ca4414366789702be3b35fa15"}

[commit-bot: I haz the power, 2017-06-10 01:30:21]

Description was changed from

==========
[Extensions Bindings] Request JS execution from messaging bindings

Instead of synchronously forcing JS execution in messaging bindings, use
safer alternatives to request JS execution. Since this modifies the
ability of the called function to return a result (synchronously, at
least), change messaging bindings to query for listeners natively rather
than in JS (which is also less likely to be incorrect).

This involves introducing a way to query if a given context has a
listener for an event. Provide this functionality for both native and
JS bindings. For native bindings, simply expose a method on
APIEventHandler. For JS bindings, make the NullAttachmentStrategy
register unmanaged events with the event bindings.

Also expose a way to get the Dispatcher statically from the messaging
bindings in order to get at the ExtensionBindingsSystem, and restructure
ownership of the dispatcher in extensions/shell.

BUG=629431
==========

to

==========
[Extensions Bindings] Request JS execution from messaging bindings

Instead of synchronously forcing JS execution in messaging bindings, use
safer alternatives to request JS execution. Since this modifies the
ability of the called function to return a result (synchronously, at
least), change messaging bindings to query for listeners natively rather
than in JS (which is also less likely to be incorrect).

This involves introducing a way to query if a given context has a
listener for an event. Provide this functionality for both native and
JS bindings. For native bindings, simply expose a method on
APIEventHandler. For JS bindings, make the NullAttachmentStrategy
register unmanaged events with the event bindings.

Also expose a way to get the Dispatcher statically from the messaging
bindings in order to get at the ExtensionBindingsSystem, and restructure
ownership of the dispatcher in extensions/shell.

BUG=629431

Review-Url: https://codereview.chromium.org/2909673003
Cr-Commit-Position: refs/heads/master@{#478492}
Committed:
https://chromium.googlesource.com/chromium/src/+/978464a97d85014ca44143667897...
==========

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

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