[Extensions] Make port creation synchronous in unload handlers

[Extensions] Make port creation synchronous in unload handlers

https://crrev.com/dbbe69e7a03b50940d5b7e615c04de531511cac3 made
extension message port initialization asynchronous in order to
address performance concerns around having a synchronous IPC.
Unfortunatley, this broke use cases where extensions use
chrome.runtime.sendMessage() from the unload handler for a page.

Fix this by using synchronous port creation in this case only. This is
essentially a partial revert of the above patch, but still leaves us in
a much better state than when all ports were created synchronously
(especially since we know no ports are created synchronously during page
load). In the long run, this is something we want to discourage, but
that should be done deliberately, and the original CL wasn't meant to
break anything.

Add a test to ensure we don't regress in the future.

Also add metrics for tracking the number of ports created during unload
handlers so that we can determine how common of an occurrence this is,
which can influence our decisions going forward.

BUG=660706
Committed: https://crrev.com/69424807aa1ee34516783d9dd206a95e9f034e33
Cr-Commit-Position: refs/heads/master@{#431925}

[Devlin, 2016-11-09 02:00:14]

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

[commit-bot: I haz the power, 2016-11-09 02:00:48]

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, 2016-11-09 06:48:33]

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

[commit-bot: I haz the power, 2016-11-09 06:48:33]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Devlin, 2016-11-09 22:19:17]

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

[commit-bot: I haz the power, 2016-11-09 22:20:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Devlin, 2016-11-10 00:51:07]

Description was changed from

==========
messaging
BUG=
==========

to

==========
[Extensions] Make port creation synchronous in unload handlers

https://crrev.com/dbbe69e7a03b50940d5b7e615c04de531511cac3 made
extension message port initialization asynchronous in order to
address performance concerns around having a synchronous IPC.
Unfortunatley, this broke use cases where extensions use
chrome.runtime.sendMessage() from the unload handler for a page.

Fix this by using synchronous port creation in this case only, which
still leaves us in a much better state than when all ports were
created synchronously. In the long run, this is something we want to
discourage, but that should be done deliberately, and this CL wasn't
meant to break anything.

Also add metrics for tracking the number of ports created during unload
handlers so that we can determine how common of an occurrence this is,
which can influence our decisions going forward.

BUG=660706
==========

[Devlin, 2016-11-10 00:51:38]

Description was changed from

==========
[Extensions] Make port creation synchronous in unload handlers

https://crrev.com/dbbe69e7a03b50940d5b7e615c04de531511cac3 made
extension message port initialization asynchronous in order to
address performance concerns around having a synchronous IPC.
Unfortunatley, this broke use cases where extensions use
chrome.runtime.sendMessage() from the unload handler for a page.

Fix this by using synchronous port creation in this case only, which
still leaves us in a much better state than when all ports were
created synchronously. In the long run, this is something we want to
discourage, but that should be done deliberately, and this CL wasn't
meant to break anything.

Also add metrics for tracking the number of ports created during unload
handlers so that we can determine how common of an occurrence this is,
which can influence our decisions going forward.

BUG=660706
==========

to

==========
[Extensions] Make port creation synchronous in unload handlers

https://crrev.com/dbbe69e7a03b50940d5b7e615c04de531511cac3 made
extension message port initialization asynchronous in order to
address performance concerns around having a synchronous IPC.
Unfortunatley, this broke use cases where extensions use
chrome.runtime.sendMessage() from the unload handler for a page.

Fix this by using synchronous port creation in this case only, which
still leaves us in a much better state than when all ports were
created synchronously. In the long run, this is something we want to
discourage, but that should be done deliberately, and this CL wasn't
meant to break anything.

Add a test to ensure we don't regress in the future.

Also add metrics for tracking the number of ports created during unload
handlers so that we can determine how common of an occurrence this is,
which can influence our decisions going forward.

BUG=660706
==========

[Devlin, 2016-11-10 00:58:35]

Description was changed from

==========
[Extensions] Make port creation synchronous in unload handlers

https://crrev.com/dbbe69e7a03b50940d5b7e615c04de531511cac3 made
extension message port initialization asynchronous in order to
address performance concerns around having a synchronous IPC.
Unfortunatley, this broke use cases where extensions use
chrome.runtime.sendMessage() from the unload handler for a page.

Fix this by using synchronous port creation in this case only, which
still leaves us in a much better state than when all ports were
created synchronously. In the long run, this is something we want to
discourage, but that should be done deliberately, and this CL wasn't
meant to break anything.

Add a test to ensure we don't regress in the future.

Also add metrics for tracking the number of ports created during unload
handlers so that we can determine how common of an occurrence this is,
which can influence our decisions going forward.

BUG=660706
==========

to

==========
[Extensions] Make port creation synchronous in unload handlers

https://crrev.com/dbbe69e7a03b50940d5b7e615c04de531511cac3 made
extension message port initialization asynchronous in order to
address performance concerns around having a synchronous IPC.
Unfortunatley, this broke use cases where extensions use
chrome.runtime.sendMessage() from the unload handler for a page.

Fix this by using synchronous port creation in this case only. This is 
essentially a partial revert of the above patch, but still leaves us in
a much better state than when all ports were created synchronously
(especially since we know no ports are created synchronously during page
load). In the long run, this is something we want to discourage, but
that should be done deliberately, and the original CL wasn't meant to
break anything.

Add a test to ensure we don't regress in the future.

Also add metrics for tracking the number of ports created during unload
handlers so that we can determine how common of an occurrence this is,
which can influence our decisions going forward.

BUG=660706
==========

[Devlin, 2016-11-10 01:02:27]

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

[Devlin, 2016-11-10 01:02:28]

dcheng@ and lazyboy@, mind taking a look?  This is basically a targeted revert
of https://crrev.com/dbbe69e7a03b50940d5b7e615c04de531511cac3 for very special
cases (the unload handlers), rather than reverting the entire thing.

Going forward, metrics can tell us how much we care about this in general. 
There are probably some fixes we could make, but I'd feel better with going back
to what we know worked first, since I'd like to merge it to m55 to avoid stable
breakage.

[lazyboy, 2016-11-11 01:39:17]

extensions lgtm /w nits.

https://codereview.chromium.org/2481413004/diff/40001/extensions/renderer/mes...
File extensions/renderer/messaging_bindings.h (right):

https://codereview.chromium.org/2481413004/diff/40001/extensions/renderer/mes...
extensions/renderer/messaging_bindings.h:121: int
ports_created_in_before_unload_ = 0;
since these are unsigned, can make them unsigned int or size_t.

https://codereview.chromium.org/2481413004/diff/40001/extensions/renderer/mes...
extensions/renderer/messaging_bindings.h:126: // The number of ports created
during the normal lifetime of the context.
nit: replace "normal lifetime" with something that explicitly states that this
is not during unload or beforeunload.

https://codereview.chromium.org/2481413004/diff/40001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2481413004/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:106738: +  <suffix name="Normal"
label="Created during the normal frame lifetime."/>
Same nit here, expand "normal" in the label.

[dcheng, 2016-11-11 08:27:46]

I really don't want to expose the unload event state outside of Blink. =/

Are there any viable alternatives to consider here? For example, could we switch
from async to sync IPC creation after document_idle? Obviously, it's preferable
to do this async...

(I guess there's no way to rearchitect this so it Just Works without involving
sync IPC? In my ideal universe, you could create a port async and start sending
(async) messages to it and it would just work, even if the thing is going away.)

[Devlin, 2016-11-11 15:33:44]

On 2016/11/11 08:27:46, dcheng wrote:
> I really don't want to expose the unload event state outside of Blink. =/
> 
> Are there any viable alternatives to consider here? For example, could we
switch
> from async to sync IPC creation after document_idle? Obviously, it's
preferable
> to do this async...

This is doable, but we'd lose a lot of the benefit of making the messages async
since significantly more of them would end up in the sync bucket.  I suppose
that it's most important during doc start/end, but still, I'd be sad.

> (I guess there's no way to rearchitect this so it Just Works without involving
> sync IPC? In my ideal universe, you could create a port async and start
sending
> (async) messages to it and it would just work, even if the thing is going
away.)

There are a few ways, but it's rather complicated, and not something I'd want to
merge back (whereas this likely *would* be merged back to avoid breakage in
stable).  I'm going to try and look into how to get rid of the sync IPC again
after this lands, but wouldn't merge that change back.  If/when I do that,
though, we could get rid of the unload state.

If you like, I could rename the method to something like
processingUnloadDoNotUse() to discourage future uses of it and add a TODO to
remove it?  Would that be a good compromise?

[dcheng, 2016-11-11 22:28:09]

On 2016/11/11 15:33:44, Devlin (slow) wrote:
> On 2016/11/11 08:27:46, dcheng wrote:
> > I really don't want to expose the unload event state outside of Blink. =/
> > 
> > Are there any viable alternatives to consider here? For example, could we
> switch
> > from async to sync IPC creation after document_idle? Obviously, it's
> preferable
> > to do this async...
> 
> This is doable, but we'd lose a lot of the benefit of making the messages
async
> since significantly more of them would end up in the sync bucket.  I suppose
> that it's most important during doc start/end, but still, I'd be sad.
> 
> > (I guess there's no way to rearchitect this so it Just Works without
involving
> > sync IPC? In my ideal universe, you could create a port async and start
> sending
> > (async) messages to it and it would just work, even if the thing is going
> away.)
> 
> There are a few ways, but it's rather complicated, and not something I'd want
to
> merge back (whereas this likely *would* be merged back to avoid breakage in
> stable).  I'm going to try and look into how to get rid of the sync IPC again
> after this lands, but wouldn't merge that change back.  If/when I do that,
> though, we could get rid of the unload state.

OK, I hope we can find a path forward here.

> 
> If you like, I could rename the method to something like
> processingUnloadDoNotUse() to discourage future uses of it and add a TODO to
> remove it?  Would that be a good compromise?

Yes, I think that would be better: comment that these methods are exposed to
allow fixing a stable blocker (with a link to the bug) and investigate how to
wholly remove the sync IPC.

[Devlin, 2016-11-11 23:37:22]

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

[commit-bot: I haz the power, 2016-11-11 23:37:43]

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, 2016-11-12 00:28:15]

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

[commit-bot: I haz the power, 2016-11-12 00:28:16]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[Devlin, 2016-11-12 01:24:30]

https://codereview.chromium.org/2481413004/diff/40001/extensions/renderer/mes...
File extensions/renderer/messaging_bindings.h (right):

https://codereview.chromium.org/2481413004/diff/40001/extensions/renderer/mes...
extensions/renderer/messaging_bindings.h:121: int
ports_created_in_before_unload_ = 0;
On 2016/11/11 01:39:17, lazyboy wrote:
> since these are unsigned, can make them unsigned int or size_t.

Done.

https://codereview.chromium.org/2481413004/diff/40001/extensions/renderer/mes...
extensions/renderer/messaging_bindings.h:126: // The number of ports created
during the normal lifetime of the context.
On 2016/11/11 01:39:17, lazyboy wrote:
> nit: replace "normal lifetime" with something that explicitly states that this
> is not during unload or beforeunload.

Done.

https://codereview.chromium.org/2481413004/diff/40001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2481413004/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:106738: +  <suffix name="Normal"
label="Created during the normal frame lifetime."/>
On 2016/11/11 01:39:17, lazyboy wrote:
> Same nit here, expand "normal" in the label.

Done.

[Devlin, 2016-11-12 01:24:45]

On 2016/11/11 22:28:09, dcheng wrote:
> On 2016/11/11 15:33:44, Devlin (slow) wrote:
> > On 2016/11/11 08:27:46, dcheng wrote:
> > > I really don't want to expose the unload event state outside of Blink. =/
> > > 
> > > Are there any viable alternatives to consider here? For example, could we
> > switch
> > > from async to sync IPC creation after document_idle? Obviously, it's
> > preferable
> > > to do this async...
> > 
> > This is doable, but we'd lose a lot of the benefit of making the messages
> async
> > since significantly more of them would end up in the sync bucket.  I suppose
> > that it's most important during doc start/end, but still, I'd be sad.
> > 
> > > (I guess there's no way to rearchitect this so it Just Works without
> involving
> > > sync IPC? In my ideal universe, you could create a port async and start
> > sending
> > > (async) messages to it and it would just work, even if the thing is going
> > away.)
> > 
> > There are a few ways, but it's rather complicated, and not something I'd
want
> to
> > merge back (whereas this likely *would* be merged back to avoid breakage in
> > stable).  I'm going to try and look into how to get rid of the sync IPC
again
> > after this lands, but wouldn't merge that change back.  If/when I do that,
> > though, we could get rid of the unload state.
> 
> OK, I hope we can find a path forward here.
> 
> > 
> > If you like, I could rename the method to something like
> > processingUnloadDoNotUse() to discourage future uses of it and add a TODO to
> > remove it?  Would that be a good compromise?
> 
> Yes, I think that would be better: comment that these methods are exposed to
> allow fixing a stable blocker (with a link to the bug) and investigate how to
> wholly remove the sync IPC.

SG, done.

[Devlin, 2016-11-12 02:45:24]

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

[Devlin, 2016-11-12 02:45:25]

Also +isherman@ for histograms.

[dcheng, 2016-11-12 03:33:13]

lgtm, thanks

[Ilya Sherman, 2016-11-14 18:00:28]

Metrics LGTM % comments:

https://codereview.chromium.org/2481413004/diff/60001/extensions/renderer/mes...
File extensions/renderer/messaging_bindings.cc (right):

https://codereview.chromium.org/2481413004/diff/60001/extensions/renderer/mes...
extensions/renderer/messaging_bindings.cc:273: ports_created_in_unload_ > 0) {
I don't really understand this check -- why do you record the metric iff at
least one of the counts is non-zero?

https://codereview.chromium.org/2481413004/diff/60001/extensions/renderer/mes...
extensions/renderer/messaging_bindings.cc:282: ports_created_normal_);
Optional: FWIW, it would probably be more memory efficient, and more or less
equally performant, to use UMA_HISTOGRAM_SPARSE_SLOWLY here, with some code to
forcibly cap the value that's reported.

[Devlin, 2016-11-14 18:49:58]

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

[Devlin, 2016-11-14 18:50:34]

https://codereview.chromium.org/2481413004/diff/60001/extensions/renderer/mes...
File extensions/renderer/messaging_bindings.cc (right):

https://codereview.chromium.org/2481413004/diff/60001/extensions/renderer/mes...
extensions/renderer/messaging_bindings.cc:273: ports_created_in_unload_ > 0) {
On 2016/11/14 18:00:28, Ilya Sherman wrote:
> I don't really understand this check -- why do you record the metric iff at
> least one of the counts is non-zero?

I think the vast majority of contexts will never create any message ports (all
web pages that don't talk to exxtensions or have an extension running on them,
in addition to any extension contexts that just don't happen to).  As such, I'm
more interested in the relationship between the creation times when ports are
used at all.  We could filter out all the noise of having 90% of the reported
values always be 0, but it seems more targeted to do it like this.  WDYT?

https://codereview.chromium.org/2481413004/diff/60001/extensions/renderer/mes...
extensions/renderer/messaging_bindings.cc:282: ports_created_normal_);
On 2016/11/14 18:00:28, Ilya Sherman wrote:
> Optional: FWIW, it would probably be more memory efficient, and more or less
> equally performant, to use UMA_HISTOGRAM_SPARSE_SLOWLY here, with some code to
> forcibly cap the value that's reported.

As discussed offline, actually going for UMA_HISTOGRAM_COUNTS_1000 here.

[commit-bot: I haz the power, 2016-11-14 18:51:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ilya Sherman, 2016-11-14 19:00:44]

(metrics still lgtm, thanks)

https://codereview.chromium.org/2481413004/diff/60001/extensions/renderer/mes...
File extensions/renderer/messaging_bindings.cc (right):

https://codereview.chromium.org/2481413004/diff/60001/extensions/renderer/mes...
extensions/renderer/messaging_bindings.cc:273: ports_created_in_unload_ > 0) {
On 2016/11/14 18:50:33, Devlin wrote:
> On 2016/11/14 18:00:28, Ilya Sherman wrote:
> > I don't really understand this check -- why do you record the metric iff at
> > least one of the counts is non-zero?
> 
> I think the vast majority of contexts will never create any message ports (all
> web pages that don't talk to exxtensions or have an extension running on them,
> in addition to any extension contexts that just don't happen to).  As such,
I'm
> more interested in the relationship between the creation times when ports are
> used at all.  We could filter out all the noise of having 90% of the reported
> values always be 0, but it seems more targeted to do it like this.  WDYT?

It just seems like you'll have a weird bias where 0 values might be confusing to
interpret.  But, if you're comfortable with this data format, and taking the
missing zeros into account when analyzing the data, then it seems fine.

[Devlin, 2016-11-14 19:54:27]

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

[Devlin, 2016-11-14 19:54:32]

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

[Devlin, 2016-11-14 19:54:33]

The patchset sent to the CQ was uploaded after l-g-t-m from
lazyboy@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2481413004/#ps80001
(title: "Ilya's")

[commit-bot: I haz the power, 2016-11-14 20:01:02]

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, 2016-11-14 22:42:33]

Description was changed from

==========
[Extensions] Make port creation synchronous in unload handlers

https://crrev.com/dbbe69e7a03b50940d5b7e615c04de531511cac3 made
extension message port initialization asynchronous in order to
address performance concerns around having a synchronous IPC.
Unfortunatley, this broke use cases where extensions use
chrome.runtime.sendMessage() from the unload handler for a page.

Fix this by using synchronous port creation in this case only. This is 
essentially a partial revert of the above patch, but still leaves us in
a much better state than when all ports were created synchronously
(especially since we know no ports are created synchronously during page
load). In the long run, this is something we want to discourage, but
that should be done deliberately, and the original CL wasn't meant to
break anything.

Add a test to ensure we don't regress in the future.

Also add metrics for tracking the number of ports created during unload
handlers so that we can determine how common of an occurrence this is,
which can influence our decisions going forward.

BUG=660706
==========

to

==========
[Extensions] Make port creation synchronous in unload handlers

https://crrev.com/dbbe69e7a03b50940d5b7e615c04de531511cac3 made
extension message port initialization asynchronous in order to
address performance concerns around having a synchronous IPC.
Unfortunatley, this broke use cases where extensions use
chrome.runtime.sendMessage() from the unload handler for a page.

Fix this by using synchronous port creation in this case only. This is 
essentially a partial revert of the above patch, but still leaves us in
a much better state than when all ports were created synchronously
(especially since we know no ports are created synchronously during page
load). In the long run, this is something we want to discourage, but
that should be done deliberately, and the original CL wasn't meant to
break anything.

Add a test to ensure we don't regress in the future.

Also add metrics for tracking the number of ports created during unload
handlers so that we can determine how common of an occurrence this is,
which can influence our decisions going forward.

BUG=660706
==========

[commit-bot: I haz the power, 2016-11-14 22:42:35]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-11-14 22:58:51]

Description was changed from

==========
[Extensions] Make port creation synchronous in unload handlers

https://crrev.com/dbbe69e7a03b50940d5b7e615c04de531511cac3 made
extension message port initialization asynchronous in order to
address performance concerns around having a synchronous IPC.
Unfortunatley, this broke use cases where extensions use
chrome.runtime.sendMessage() from the unload handler for a page.

Fix this by using synchronous port creation in this case only. This is 
essentially a partial revert of the above patch, but still leaves us in
a much better state than when all ports were created synchronously
(especially since we know no ports are created synchronously during page
load). In the long run, this is something we want to discourage, but
that should be done deliberately, and the original CL wasn't meant to
break anything.

Add a test to ensure we don't regress in the future.

Also add metrics for tracking the number of ports created during unload
handlers so that we can determine how common of an occurrence this is,
which can influence our decisions going forward.

BUG=660706
==========

to

==========
[Extensions] Make port creation synchronous in unload handlers

https://crrev.com/dbbe69e7a03b50940d5b7e615c04de531511cac3 made
extension message port initialization asynchronous in order to
address performance concerns around having a synchronous IPC.
Unfortunatley, this broke use cases where extensions use
chrome.runtime.sendMessage() from the unload handler for a page.

Fix this by using synchronous port creation in this case only. This is
essentially a partial revert of the above patch, but still leaves us in
a much better state than when all ports were created synchronously
(especially since we know no ports are created synchronously during page
load). In the long run, this is something we want to discourage, but
that should be done deliberately, and the original CL wasn't meant to
break anything.

Add a test to ensure we don't regress in the future.

Also add metrics for tracking the number of ports created during unload
handlers so that we can determine how common of an occurrence this is,
which can influence our decisions going forward.

BUG=660706

Committed: https://crrev.com/69424807aa1ee34516783d9dd206a95e9f034e33
Cr-Commit-Position: refs/heads/master@{#431925}
==========

[commit-bot: I haz the power, 2016-11-14 22:58:52]

Patchset 5 (id:??) landed as
https://crrev.com/69424807aa1ee34516783d9dd206a95e9f034e33
Cr-Commit-Position: refs/heads/master@{#431925}