android: Don't wait for warm up connection

android: Add test for child connection warmup

BUG=689758
Review-Url: https://codereview.chromium.org/2693893006
Cr-Commit-Position: refs/heads/master@{#450527}
Committed: https://chromium.googlesource.com/chromium/src/+/687c39deed13217382e70376304e664ea0d124b9

[boliu, 2017-02-14 22:23:12]

boliu@chromium.org changed reviewers:
+ rsesek@chromium.org

[boliu, 2017-02-14 22:23:13]

ptal

[boliu, 2017-02-14 22:23:20]

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

[commit-bot: I haz the power, 2017-02-14 22:24:12]

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

[Robert Sesek, 2017-02-14 22:33:45]

I think this means that a WebView could end binding two renderers, and not in
the edge case (i.e. using WebView from multiple processes), if the warm up
doesn't complete before the renderer is requested.

https://codereview.chromium.org/2693893006/diff/1/content/public/android/java...
File
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java
(right):

https://codereview.chromium.org/2693893006/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:702:
&& !sSpareConnectionStarting) {
(maybe comment): sSpareConnectionStarting is true if warmUp() was called but the
StartCallback has not yet been invoked.

[boliu, 2017-02-14 22:39:28]

On 2017/02/14 22:33:45, Robert Sesek wrote:
> I think this means that a WebView could end binding two renderers, and not in
> the edge case (i.e. using WebView from multiple processes), if the warm up
> doesn't complete before the renderer is requested.

Hmm, good point..

If webview actually has 20 slots like chrome does, how is the maximum number of
connection enforced in webview?

> 
>
https://codereview.chromium.org/2693893006/diff/1/content/public/android/java...
> File
>
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java
> (right):
> 
>
https://codereview.chromium.org/2693893006/diff/1/content/public/android/java...
>
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:702:
> && !sSpareConnectionStarting) {
> (maybe comment): sSpareConnectionStarting is true if warmUp() was called but
the
> StartCallback has not yet been invoked.

[boliu, 2017-02-14 22:42:51]

On 2017/02/14 22:39:28, boliu wrote:
> On 2017/02/14 22:33:45, Robert Sesek wrote:
> > I think this means that a WebView could end binding two renderers, and not
in
> > the edge case (i.e. using WebView from multiple processes), if the warm up
> > doesn't complete before the renderer is requested.
> 
> Hmm, good point..
> 
> If webview actually has 20 slots like chrome does, how is the maximum number
of
> connection enforced in webview?

fwiw, even if there's only 1 slot in webview, the request will be queued up if
it comes before warm up is done, and never gets dequeued. hmm... I wonder if
that can actually happen in practice with 20 slots, that requires calling warmUp
after start up though, which I *think* doesn't happen in practice?

> 
> > 
> >
>
https://codereview.chromium.org/2693893006/diff/1/content/public/android/java...
> > File
> >
>
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/2693893006/diff/1/content/public/android/java...
> >
>
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:702:
> > && !sSpareConnectionStarting) {
> > (maybe comment): sSpareConnectionStarting is true if warmUp() was called but
> the
> > StartCallback has not yet been invoked.

[Robert Sesek, 2017-02-14 22:53:49]

rsesek@chromium.org changed reviewers:
+ torne@chromium.org

[Robert Sesek, 2017-02-14 22:53:49]

On 2017/02/14 22:42:51, boliu wrote:
> On 2017/02/14 22:39:28, boliu wrote:
> > On 2017/02/14 22:33:45, Robert Sesek wrote:
> > > I think this means that a WebView could end binding two renderers, and not
> in
> > > the edge case (i.e. using WebView from multiple processes), if the warm up
> > > doesn't complete before the renderer is requested.
> > 
> > Hmm, good point..
> > 
> > If webview actually has 20 slots like chrome does, how is the maximum number
> of
> > connection enforced in webview?

I don't actually know how this is done. Due to Monochrome, there should be 20
slots. +torne

> fwiw, even if there's only 1 slot in webview, the request will be queued up if
> it comes before warm up is done, and never gets dequeued. hmm... I wonder if
> that can actually happen in practice with 20 slots, that requires calling
warmUp
> after start up though, which I *think* doesn't happen in practice?

warmUp() should always happen before the first request is made, since it's done
as part of browser starting.

[boliu, 2017-02-14 23:06:12]

On 2017/02/14 22:53:49, Robert Sesek wrote:
> On 2017/02/14 22:42:51, boliu wrote:
> > On 2017/02/14 22:39:28, boliu wrote:
> > > On 2017/02/14 22:33:45, Robert Sesek wrote:
> > > > I think this means that a WebView could end binding two renderers, and
not
> > in
> > > > the edge case (i.e. using WebView from multiple processes), if the warm
up
> > > > doesn't complete before the renderer is requested.
> > > 
> > > Hmm, good point..
> > > 
> > > If webview actually has 20 slots like chrome does, how is the maximum
number
> > of
> > > connection enforced in webview?
> 
> I don't actually know how this is done. Due to Monochrome, there should be 20
> slots. +torne
> 
> > fwiw, even if there's only 1 slot in webview, the request will be queued up
if
> > it comes before warm up is done, and never gets dequeued. hmm... I wonder if
> > that can actually happen in practice with 20 slots, that requires calling
> warmUp
> > after start up though, which I *think* doesn't happen in practice?
> 
> warmUp() should always happen before the first request is made, since it's
done
> as part of browser starting.

Actually, cct can warmup after browser start up.

Ok, really can't do this then, unless I hook up onChildStarted to dequeuing from
the pending queue. Perhaps after some clean ups and refactoring to this code,
but right now that just sounds really horrifying.

I'll just add the test then..

[commit-bot: I haz the power, 2017-02-14 23:17:30]

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

[commit-bot: I haz the power, 2017-02-14 23:17:31]

Dry run: This issue passed the CQ dry run.

[boliu, 2017-02-14 23:31:41]

Description was changed from

==========
android: Don't wait for warm up connection

If warm up connection is not ready, just skip using it and create a new
one. Also add a dummy test for warm up.

BUG=691846
==========

to

==========
android: Add test for child connection warmup

BUG=689758
==========

[boliu, 2017-02-14 23:32:22]

Test only now. I'll get maria to review it

On 2017/02/14 23:06:12, boliu wrote:
> On 2017/02/14 22:53:49, Robert Sesek wrote:
> > On 2017/02/14 22:42:51, boliu wrote:
> > > On 2017/02/14 22:39:28, boliu wrote:
> > > > On 2017/02/14 22:33:45, Robert Sesek wrote:
> > > > > I think this means that a WebView could end binding two renderers, and
> not
> > > in
> > > > > the edge case (i.e. using WebView from multiple processes), if the
warm
> up
> > > > > doesn't complete before the renderer is requested.
> > > > 
> > > > Hmm, good point..
> > > > 
> > > > If webview actually has 20 slots like chrome does, how is the maximum
> number
> > > of
> > > > connection enforced in webview?
> > 
> > I don't actually know how this is done. Due to Monochrome, there should be
20
> > slots. +torne

It's kRendererProcessLimit=1, which is more of a soft limit.

More amusingly, on chrome on android,
RenderProcessHost::GetMaxRendererProcessCount is infinite, even though there are
only 20 slots. I guess back when that code was written, 20 was effectively
infinite, but that certainly isn't the case anymore..

[boliu, 2017-02-14 23:33:17]

boliu@chromium.org changed reviewers:
+ mariakhomenko@chromium.org
- rsesek@chromium.org, torne@chromium.org

[boliu, 2017-02-14 23:33:17]

maria: ptal

[Maria, 2017-02-14 23:37:08]

lgtm

[boliu, 2017-02-14 23:37:13]

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

[boliu, 2017-02-14 23:37:21]

The CQ bit was unchecked by boliu@chromium.org

[boliu, 2017-02-14 23:37:47]

The CQ bit was checked by boliu@chromium.org

[commit-bot: I haz the power, 2017-02-14 23:38:27]

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-02-15 00:35:37]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1487115467617910,
"parent_rev": "e5d12e077d791f66b56ce0a8a84c23e98da3fdee", "commit_rev":
"687c39deed13217382e70376304e664ea0d124b9"}

[commit-bot: I haz the power, 2017-02-15 00:36:15]

Description was changed from

==========
android: Add test for child connection warmup

BUG=689758
==========

to

==========
android: Add test for child connection warmup

BUG=689758

Review-Url: https://codereview.chromium.org/2693893006
Cr-Commit-Position: refs/heads/master@{#450527}
Committed:
https://chromium.googlesource.com/chromium/src/+/687c39deed13217382e70376304e...
==========

[commit-bot: I haz the power, 2017-02-15 00:36:16]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/687c39deed13217382e70376304e...