Android: centralize renderer binding management in ChildProcessLauncher.

Android: centralize renderer binding management in ChildProcessLauncher.

This patch centralizes renderer binding management in ChildProcessLauncher,
simplifying ChildProcessConnection and encapsulating the logic in a nested
class.

BindingManager becomes responsible for all tasks related to adding, removing
(possibly in a delayed way) and inspecting the state of renderer out-of-memory
bindings.

BUG=290351
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=223391

[ppi, 2013-09-12 19:40:08]

What do you guys think?

[klobag.chromium, 2013-09-13 05:46:25]

https://codereview.chromium.org/23532058/diff/3001/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/ChildProcessConnection.java
(left):

https://codereview.chromium.org/23532058/diff/3001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/ChildProcessConnection.java:180:
}
This used to catch a lot of cases, especially from stop() where it unbind all
the bindings. Now it all depends on ContentViewCore.

e.g. if the current tab's renderer crashed, detachActive won't get called, so
oomBindingCount won't be reset as it did here.

https://codereview.chromium.org/23532058/diff/3001/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/ChildProcessConnection.java
(right):

https://codereview.chromium.org/23532058/diff/3001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/ChildProcessConnection.java:388:
return mInitialBinding.isBound();
within synchronized(mLock)

https://codereview.chromium.org/23532058/diff/3001/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java
(right):

https://codereview.chromium.org/23532058/diff/3001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:233:
// Every new connection is bound with initial oom binding.
Should this be within mCountLock?

https://codereview.chromium.org/23532058/diff/3001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:255:
decrementOomCount(pid);
Should this be called even when it is unbound? e.g. the connection is stopped
before the delay timeout, we should still clear the count here, right?

https://codereview.chromium.org/23532058/diff/3001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:295:
decrementOomCount(pid);
ditto

[ppi, 2013-09-13 16:59:16]

Thanks, Grace! Could you please have a look at my replies below?

https://codereview.chromium.org/23532058/diff/3001/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/ChildProcessConnection.java
(left):

https://codereview.chromium.org/23532058/diff/3001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/ChildProcessConnection.java:180:
}
On 2013/09/13 05:46:25, klobag.chromium wrote:
> This used to catch a lot of cases, especially from stop() where it unbind all
> the bindings. Now it all depends on ContentViewCore.
> 
> e.g. if the current tab's renderer crashed, detachActive won't get called, so
> oomBindingCount won't be reset as it did here.

If the current tab's renderer crashes we specifically don't want to reset the
count. The reason we count the bindings is to be able to read the count after
the process dies to decide if it was a crash or oom kill.

Please note that in this case the logic does not change with this patch - the
removed check had "!mServiceDisconnected" so that ChildProcessLauncher was not
changing the count after a crash.

https://codereview.chromium.org/23532058/diff/3001/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/ChildProcessConnection.java
(right):

https://codereview.chromium.org/23532058/diff/3001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/ChildProcessConnection.java:388:
return mInitialBinding.isBound();
On 2013/09/13 05:46:25, klobag.chromium wrote:
> within synchronized(mLock)

Done.

https://codereview.chromium.org/23532058/diff/3001/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java
(right):

https://codereview.chromium.org/23532058/diff/3001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:233:
// Every new connection is bound with initial oom binding.
On 2013/09/13 05:46:25, klobag.chromium wrote:
> Should this be within mCountLock?

Done.

https://codereview.chromium.org/23532058/diff/3001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:255:
decrementOomCount(pid);
On 2013/09/13 05:46:25, klobag.chromium wrote:
> Should this be called even when it is unbound? e.g. the connection is stopped
> before the delay timeout, we should still clear the count here, right?

We intentionally clear the count on starting new process, not on stopping it (or
crashing), so that the number of oom bindings can be read after the process
crashes. I clarified this in the comment for isOomProtected().

https://codereview.chromium.org/23532058/diff/3001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:295:
decrementOomCount(pid);
On 2013/09/13 05:46:25, klobag.chromium wrote:
> ditto

Please see above.

[Yaron, 2013-09-13 18:31:07]

https://codereview.chromium.org/23532058/diff/15001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java
(right):

https://codereview.chromium.org/23532058/diff/15001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:202:
// mOomBindingCount.
This looks to be inconsistent. See incrementOomCount and isOomProtected. The
latter, at least, is called in an unguarded location.

https://codereview.chromium.org/23532058/diff/15001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:429:
sBindingManager.addNewConnection(pid);
This is a slight change in behaviour, right? Now this is guaranteed to be 1
whereas before a warmed up connection could have 2 oom bindings? Oh actually,
it's fine because when we get the connection, we know it has to be first the
binding.

[ppi, 2013-09-13 19:21:41]

Thanks, Yaron!

I have addressed your comments in patch set 3 and inline. Could you have another
look?

https://codereview.chromium.org/23532058/diff/15001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java
(right):

https://codereview.chromium.org/23532058/diff/15001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:202:
// mOomBindingCount.
On 2013/09/13 18:31:07, Yaron wrote:
> This looks to be inconsistent. See incrementOomCount and isOomProtected. The
> latter, at least, is called in an unguarded location.

incrementOomCount() is in one "synchronized" block together with the call that
adds the binding. I assume not synchronizing within the method is fine, as all
of that locking logic is private and the class consumers won't run into trouble
because of it. Please let me know if you disagree.

As to isOomProtected(), good catch, done.

https://codereview.chromium.org/23532058/diff/15001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:429:
sBindingManager.addNewConnection(pid);
On 2013/09/13 18:31:07, Yaron wrote:
> This is a slight change in behaviour, right? Now this is guaranteed to be 1
> whereas before a warmed up connection could have 2 oom bindings? Oh actually,
> it's fine because when we get the connection, we know it has to be first the
> binding. 

Exactly - the idea is that ChildProcessConnection binds only the initialBinding
on its own, so we set the initial count to 1. Then all calls to add and remove
strong bindings are routed through BindingManager so we can keep track of them.

[klobag.chromium, 2013-09-13 21:33:04]

https://codereview.chromium.org/23532058/diff/3001/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/ChildProcessConnection.java
(left):

https://codereview.chromium.org/23532058/diff/3001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/ChildProcessConnection.java:180:
}
I see. The logic assumes Android will not reuse the pid for the renderer process
during browser process's life time. The old pid stays in the array until browser
dies. This may be OK assumption. Please add it in the comment.


On 2013/09/13 16:59:16, ppi wrote:
> On 2013/09/13 05:46:25, klobag.chromium wrote:
> > This used to catch a lot of cases, especially from stop() where it unbind
all
> > the bindings. Now it all depends on ContentViewCore.
> > 
> > e.g. if the current tab's renderer crashed, detachActive won't get called,
so
> > oomBindingCount won't be reset as it did here.
> 
> If the current tab's renderer crashes we specifically don't want to reset the
> count. The reason we count the bindings is to be able to read the count after
> the process dies to decide if it was a crash or oom kill.
> 
> Please note that in this case the logic does not change with this patch - the
> removed check had "!mServiceDisconnected" so that ChildProcessLauncher was not
> changing the count after a crash.

[Yaron, 2013-09-13 21:52:17]

lgtm

[ppi, 2013-09-16 17:22:35]

Thanks, Grace and Yaron!

https://codereview.chromium.org/23532058/diff/3001/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/ChildProcessConnection.java
(left):

https://codereview.chromium.org/23532058/diff/3001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/ChildProcessConnection.java:180:
}
On 2013/09/13 21:33:05, klobag.chromium wrote:
> I see. The logic assumes Android will not reuse the pid for the renderer
process
> during browser process's life time. The old pid stays in the array until
browser
> dies. This may be OK assumption. Please add it in the comment.
> 
> 
> On 2013/09/13 16:59:16, ppi wrote:
> > On 2013/09/13 05:46:25, klobag.chromium wrote:
> > > This used to catch a lot of cases, especially from stop() where it unbind
> all
> > > the bindings. Now it all depends on ContentViewCore.
> > > 
> > > e.g. if the current tab's renderer crashed, detachActive won't get called,
> so
> > > oomBindingCount won't be reset as it did here.
> > 
> > If the current tab's renderer crashes we specifically don't want to reset
the
> > count. The reason we count the bindings is to be able to read the count
after
> > the process dies to decide if it was a crash or oom kill.
> > 
> > Please note that in this case the logic does not change with this patch -
the
> > removed check had "!mServiceDisconnected" so that ChildProcessLauncher was
not
> > changing the count after a crash.
> 

Actually, we don't even assume that, because the entry for a pid is cleared when
a new connection for the pid is created. It's just that isOomProtected() will
always refer to the most recent renderer process of given pid - I clarified that
in patch set 5.

[commit-bot: I haz the power, 2013-09-16 17:23:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ppi@chromium.org/23532058/36001

[commit-bot: I haz the power, 2013-09-16 20:07:49]

Change committed as 223391