Android: remove the initial binding when the renderer commits.

Android: remove the initial binding when the renderer commits.

This patch removes the initial binding when the renderer commits a
navigation, rather than after a fixed delay from starting the renderer.

We have to wait until the first commit before relying on the  visibility
signal, because a renderer for the foreground tab is considered
background until it commits.

BUG=421041
Committed: https://crrev.com/5bab673bb8b4ddfbcc7ef996688d813afbd02ca4
Cr-Commit-Position: refs/heads/master@{#302392}

[ppi, 2014-10-23 17:26:34]

Patchset #1 (id:1) has been deleted

[ppi, 2014-10-23 17:29:46]

ppi@chromium.org changed reviewers:
+ tedchoc@chromium.org, yfriedmanc@chromium.org

[ppi, 2014-10-23 17:33:50]

ppi@chromium.org changed reviewers:
+ yfriedman@chromium.org
- yfriedmanc@chromium.org

[ppi, 2014-10-23 17:33:50]

Yaron, Ted - ptal.

Adding Ted as it would probably make sense to have another reviewer familiar
with those bits for the time Yaron is out-of-chromium.

I'll add a test to BindingManagerIntegrationTest in the clank-side CL.

https://codereview.chromium.org/673573003/diff/20001/chrome/android/java/src/...
File chrome/android/java/src/org/chromium/chrome/browser/Tab.java (right):

https://codereview.chromium.org/673573003/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/Tab.java:281:
ChildProcessLauncher.determinedVisibility(
This whole bit makes me sad, but we need to somehow map from the tab to its
renderer to lift the initial binding at commit...

Alternatively we would thread the call from RenderFrameHostManager which knows
the renderer for the frame, but this would be arguably even worse, polluting key
cross-platform logic with Android-only bits.

[Ted C, 2014-10-25 00:38:52]

overall this seems reasonable to me.  Granted, I don't know the code all that
well, so please forgive me if these comments are completely ridiculous.

https://codereview.chromium.org/673573003/diff/20001/chrome/android/java/src/...
File chrome/android/java/src/org/chromium/chrome/browser/Tab.java (right):

https://codereview.chromium.org/673573003/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/Tab.java:281:
ChildProcessLauncher.determinedVisibility(
On 2014/10/23 17:33:50, ppi wrote:
> This whole bit makes me sad, but we need to somehow map from the tab to its
> renderer to lift the initial binding at commit...
> 
> Alternatively we would thread the call from RenderFrameHostManager which knows
> the renderer for the frame, but this would be arguably even worse, polluting
key
> cross-platform logic with Android-only bits.

Should hang off of ContentViewCore instead of Tab?  If we are adding the strong
initial binding for all consumers of ContentViewCore, I think we should put this
closer to the source so they get this behavior for free as well (ContentShell
for example).

Now are there any failure events where we should be doing the same thing here? 
Are there failure cases where the navigation is not committed but marked as
failed where we would be keeping these strong bindings (I suspect you know more
than me or maybe we should include someone like creis@).

https://codereview.chromium.org/673573003/diff/20001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java
(right):

https://codereview.chromium.org/673573003/diff/20001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java:244:
managedConnection.determinedVisibility();
I assume the BindingManagerImpl is a 1:1 with the browser process/application?

I was thinking that maybe we could be overly cautious and call
determinedVisibility on mLastInForeground when inside of setInForeground of a
new process to catch issues where we're not clearing it correctly.

But that I guess is covering up some programming flaw we have somewhere.

And I was also thinking that might not work in multi-window where there can be
two visible foreground renderers and mLastInForeground could still be visible
and we wouldn't want to clear that bit.

https://codereview.chromium.org/673573003/diff/20001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java
(right):

https://codereview.chromium.org/673573003/diff/20001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:310:
* Called when the renderer committs a navigation. This signals a time at which
it is safe to
s/committs/commits

[ppi, 2014-10-29 20:13:02]

Patchset #3 (id:60001) has been deleted

[ppi, 2014-10-29 20:13:49]

Thanks, Ted! Ptal.

https://codereview.chromium.org/673573003/diff/20001/chrome/android/java/src/...
File chrome/android/java/src/org/chromium/chrome/browser/Tab.java (right):

https://codereview.chromium.org/673573003/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/Tab.java:281:
ChildProcessLauncher.determinedVisibility(
On 2014/10/25 00:38:51, Ted C wrote:
> On 2014/10/23 17:33:50, ppi wrote:
> > This whole bit makes me sad, but we need to somehow map from the tab to its
> > renderer to lift the initial binding at commit...
> > 
> > Alternatively we would thread the call from RenderFrameHostManager which
knows
> > the renderer for the frame, but this would be arguably even worse, polluting
> key
> > cross-platform logic with Android-only bits.
> 
> Should hang off of ContentViewCore instead of Tab?  If we are adding the
strong
> initial binding for all consumers of ContentViewCore, I think we should put
this
> closer to the source so they get this behavior for free as well (ContentShell
> for example).
> 
> Now are there any failure events where we should be doing the same thing here?

> Are there failure cases where the navigation is not committed but marked as
> failed where we would be keeping these strong bindings (I suspect you know
more
> than me or maybe we should include someone like creis@).

Done for moving to CVC and done for the failure event - we now remove the
binding either when the provisional load commits or fails.

A renderer that failed the provisional load can be killed at any point afair -
we'll just recreate it upon the next navigation attempt.

https://codereview.chromium.org/673573003/diff/20001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java
(right):

https://codereview.chromium.org/673573003/diff/20001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java:244:
managedConnection.determinedVisibility();
On 2014/10/25 00:38:52, Ted C wrote:
> I assume the BindingManagerImpl is a 1:1 with the browser process/application?
> 
> I was thinking that maybe we could be overly cautious and call
> determinedVisibility on mLastInForeground when inside of setInForeground of a
> new process to catch issues where we're not clearing it correctly.
> 
> But that I guess is covering up some programming flaw we have somewhere.
> 
> And I was also thinking that might not work in multi-window where there can be
> two visible foreground renderers and mLastInForeground could still be visible
> and we wouldn't want to clear that bit.

Yup, we have one BindingManagerImpl (effectively a singleton) for the browser
process / application.

I agree that calling determinedVisibility on mLastInForeground could cover flaws
that we probably would prefer to surface / discover and fix properly.

Also, this entire "mLastInForeground" thing should not be really needed - I'd
prefer to rely on the OS for prioritizing the most recently bound of our
renderers above the others on the killing list. We introduced this bit for L
thinking that it's needed, but I'm not sure if it's really the case. I'd be
happy to see it going away one day.

https://codereview.chromium.org/673573003/diff/20001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java
(right):

https://codereview.chromium.org/673573003/diff/20001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:310:
* Called when the renderer committs a navigation. This signals a time at which
it is safe to
On 2014/10/25 00:38:52, Ted C wrote:
> s/committs/commits

Done.

[Yaron, 2014-10-29 20:52:28]

So in general this seems ok. I do have one concern though because I'm not sure
when NavigationEntryCommitted happens. Is it once the renderer performs a
navigation and starts receiving data or when it attempts/requests the
navigation?

Say I'm on a flaky wireless. I click a link on an app, it's supposed to load in
chrome but I don't get an initial response from the website. I then background
chrome and it's still maintaining the strong initial binding to the renderer
right? Given that we don't kill the sockets or change the binding in any way,
until it times out, we keep the renderer(s) around.

I'd say it's fine if either:
1) The timeout leading to a failed load isn't very long (on the order of seconds
to low minutes)
2) We'd get the NavigationEntryCommitted when the renderer attempts the
navigation as opposed to start receiving data.

wdyt? Maybe ask jam/creis about my original Q if it's not clear?

[ppi, 2014-10-30 17:29:14]

Thanks, Yaron!

Pending navigation is committed when we receive first bytes of the http response
from the server.

The case of a slow-to-commit navigations + backgrounding Chrome is interesting,
but I think we're covered there - the priority of bound services is tied to the
visibility of the application. Just as our strong binding (which we hold in
background) doesn't raise the priority of our renderer above "cch" (when we are
in background), neither does the initial binding.

To make sure I made Chrome never remove the initial binding. Process list with
Chrome in fg shows:

    Proc # 4: fore  F/ /T  trm: 0
4544:com.google.android.apps.chrome:sandboxed_process2/u0a120i30 (service)
    Proc # 0: fore  F/A/T  trm: 0 4308:com.google.android.apps.chrome/u0a120
(top-activity)
    Proc # 6: vis   F/ /T  trm: 0
4475:com.google.android.apps.chrome:sandboxed_process1/u0a120i29 (service)
    Proc # 5: vis   F/ /T  trm: 0
4334:com.google.android.apps.chrome:sandboxed_process0/u0a120i28 (service)

, with Chrome in bg everything is cached:

    Proc # 8: cch   B/ /Ca trm: 0
4475:com.google.android.apps.chrome:sandboxed_process1/u0a120i29
(cch-client-act)
    Proc # 7: cch   B/ /Ca trm: 0
4334:com.google.android.apps.chrome:sandboxed_process0/u0a120i28
(cch-client-act)
    Proc # 6: cch   B/ /Ca trm: 0
4544:com.google.android.apps.chrome:sandboxed_process2/u0a120i30
(cch-client-act)
    Proc # 4: cch   B/ /CA trm: 0 4308:com.google.android.apps.chrome/u0a120
(cch-act)

[Yaron, 2014-10-30 23:56:37]

lgtm - thanks for the explanation

[Yaron, 2014-10-31 00:03:10]

On 2014/10/30 23:56:37, Yaron (OOO - On Leave) wrote:
> lgtm - thanks for the explanation

David raises an interesting point where you could spawn a few tabs in short
order (maybe from some other app) and if none are receiving data, all of those
tabs would have strong bindings. That could lead to considerable memory usage
but it's probably edgey

[ppi, 2014-11-01 17:52:51]

Good point, although we already have this problem to some extent, as we keep the
initial binding attached for one second.

I think in the long term we should probably consider having some throttling
similar to what Desktop does on tab restore and say never load more than N tabs
simultaneously.

Thanks for the review!

[ppi, 2014-11-01 18:54:24]

The CQ bit was checked by ppi@chromium.org

[commit-bot: I haz the power, 2014-11-01 18:55:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/673573003/120001

[commit-bot: I haz the power, 2014-11-01 19:44:11]

Committed patchset #5 (id:120001)

[commit-bot: I haz the power, 2014-11-01 19:44:47]

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