Merging Managed and Important connection.

Merging Managed and Important connection together again.

As part of the effort to move ChildProcessConnection to base, merging
ImportantChildProcessConnection and ManagedChildProcessConnection into
a unique class again.
The ChildProcessLauncher adds a strong binding to privileged processes
and add sanboxed processes to the BindingManager.
Also changing the OOM protected state to waived bound only and making
the ChildProcessLauncherHelper policy clearer.

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

[Jay Civelli, 2017-05-10 01:43:27]

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

[commit-bot: I haz the power, 2017-05-10 01:44:36]

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-10 03:47:14]

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

[commit-bot: I haz the power, 2017-05-10 03:47:15]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet;
builder either lacks capacity or does not exist (misspelled?))
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does
not exist (misspelled?))
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, build
has not started yet; builder either lacks capacity or does not exist
(misspelled?))
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
build has not started yet; builder either lacks capacity or does not exist
(misspelled?))
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does
not exist (misspelled?))
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does
not exist (misspelled?))
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build
has not started yet; builder either lacks capacity or does not exist
(misspelled?))

[Jay Civelli, 2017-05-10 16:27:22]

Description was changed from

==========
Merging Managed and Important connection together again.

BUG=
==========

to

==========
Merging Managed and Important connection together again.

As part of the effort to move ChildProcessConnection to base, making it a
unique class.

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

[Jay Civelli, 2017-05-10 16:27:22]

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

[Jay Civelli, 2017-05-10 17:02:27]

Description was changed from

==========
Merging Managed and Important connection together again.

As part of the effort to move ChildProcessConnection to base, making it a
unique class.

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

to

==========
Merging Managed and Important connection together again.

As part of the effort to move ChildProcessConnection to base, merging
ImportantChildProcessConnection and ManagedChildProcessConnection into
a unique class again.
The ChildProcessLauncher adds a strong binding to privileged processes
and add sanboxed processes to the BindingManager.
Also changing the OOM protected state to waived bound only and making
the ChildProcessLauncherHelper policy clearer.

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

[Jay Civelli, 2017-05-10 17:02:41]

Patchset #1 (id:1) has been deleted

[Jay Civelli, 2017-05-10 17:02:49]

Patchset #1 (id:20001) has been deleted

[Jay Civelli, 2017-05-10 17:03:03]

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

[commit-bot: I haz the power, 2017-05-10 17:04:58]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[boliu, 2017-05-10 18:34:01]

https://codereview.chromium.org/2874643002/diff/40001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ChildConnectionAllocator.java
(left):

https://codereview.chromium.org/2874643002/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ChildConnectionAllocator.java:109:
BaseChildProcessConnection.Factory connectionFactory, String packageName,
well this throws a wrench for my CL :p

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

https://codereview.chromium.org/2874643002/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:567:
connection.addStrongBinding();
adding strong binding after the fact is still changing behavior in ways I don't
want to really think about. previous behavior is that "always in foreground" was
bound with strong binding to start with

https://codereview.chromium.org/2874643002/diff/40001/content/public/android/...
File
content/public/android/junit/src/org/chromium/content/browser/BindingManagerImplTest.java
(right):

https://codereview.chromium.org/2874643002/diff/40001/content/public/android/...
content/public/android/junit/src/org/chromium/content/browser/BindingManagerImplTest.java:107:
private static ChildProcessConnection mockChildProcessConnection(
nit: createTestChildProcessConnection? mock being the verb sounds odd..

[commit-bot: I haz the power, 2017-05-10 18:42:44]

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

[commit-bot: I haz the power, 2017-05-10 18:42:45]

Dry run: This issue passed the CQ dry run.

[Jay Civelli, 2017-05-10 20:57:59]

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

[commit-bot: I haz the power, 2017-05-10 20:59:04]

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-10 21:31:14]

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

[commit-bot: I haz the power, 2017-05-10 21:31:15]

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

[Jay Civelli, 2017-05-11 21:43:12]

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

[Jay Civelli, 2017-05-11 21:43:29]

https://codereview.chromium.org/2874643002/diff/40001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ChildConnectionAllocator.java
(left):

https://codereview.chromium.org/2874643002/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ChildConnectionAllocator.java:109:
BaseChildProcessConnection.Factory connectionFactory, String packageName,
On 2017/05/10 18:34:00, boliu wrote:
> well this throws a wrench for my CL :p

Land yours first, I'll make mine work.

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

https://codereview.chromium.org/2874643002/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:567:
connection.addStrongBinding();
On 2017/05/10 18:34:00, boliu wrote:
> adding strong binding after the fact is still changing behavior in ways I
don't
> want to really think about. previous behavior is that "always in foreground"
was
> bound with strong binding to start with

Added a way to start the connection with a strong binding.

https://codereview.chromium.org/2874643002/diff/40001/content/public/android/...
File
content/public/android/junit/src/org/chromium/content/browser/BindingManagerImplTest.java
(right):

https://codereview.chromium.org/2874643002/diff/40001/content/public/android/...
content/public/android/junit/src/org/chromium/content/browser/BindingManagerImplTest.java:107:
private static ChildProcessConnection mockChildProcessConnection(
On 2017/05/10 18:34:01, boliu wrote:
> nit: createTestChildProcessConnection? mock being the verb sounds odd..

Done.

[commit-bot: I haz the power, 2017-05-11 21:44:17]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Jay Civelli, 2017-05-11 21:50:28]

On 2017/05/11 21:43:29, Jay Civelli wrote:
>
https://codereview.chromium.org/2874643002/diff/40001/content/public/android/...
> File
>
content/public/android/java/src/org/chromium/content/browser/ChildConnectionAllocator.java
> (left):
> 
>
https://codereview.chromium.org/2874643002/diff/40001/content/public/android/...
>
content/public/android/java/src/org/chromium/content/browser/ChildConnectionAllocator.java:109:
> BaseChildProcessConnection.Factory connectionFactory, String packageName,
> On 2017/05/10 18:34:00, boliu wrote:
> > well this throws a wrench for my CL :p
> 
> Land yours first, I'll make mine work.
Old comment, yours has already landed.
I synced, addressed conflicts and made the new test work.

> 
>
https://codereview.chromium.org/2874643002/diff/40001/content/public/android/...
> File
>
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java
> (right):
> 
>
https://codereview.chromium.org/2874643002/diff/40001/content/public/android/...
>
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:567:
> connection.addStrongBinding();
> On 2017/05/10 18:34:00, boliu wrote:
> > adding strong binding after the fact is still changing behavior in ways I
> don't
> > want to really think about. previous behavior is that "always in foreground"
> was
> > bound with strong binding to start with
> 
> Added a way to start the connection with a strong binding.
> 
>
https://codereview.chromium.org/2874643002/diff/40001/content/public/android/...
> File
>
content/public/android/junit/src/org/chromium/content/browser/BindingManagerImplTest.java
> (right):
> 
>
https://codereview.chromium.org/2874643002/diff/40001/content/public/android/...
>
content/public/android/junit/src/org/chromium/content/browser/BindingManagerImplTest.java:107:
> private static ChildProcessConnection mockChildProcessConnection(
> On 2017/05/10 18:34:01, boliu wrote:
> > nit: createTestChildProcessConnection? mock being the verb sounds odd..
> 
> Done.

[boliu, 2017-05-11 23:54:49]

lgtm % comments

https://codereview.chromium.org/2874643002/diff/80001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java
(right):

https://codereview.chromium.org/2874643002/diff/80001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java:182:
* connection goes away, but ManagedConnection itself is kept (until overwritten
by a new entry
is this true?

https://codereview.chromium.org/2874643002/diff/80001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ChildProcessConnection.java
(right):

https://codereview.chromium.org/2874643002/diff/80001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ChildProcessConnection.java:487:
if ((useStrongBinding && !mStrongBinding.bind())
nit: might be easier to read:

boolean success = useStrongBinding ? mStrongBinding.bind() :
mInitialBinding.bind();
if (!success) return false;

https://codereview.chromium.org/2874643002/diff/80001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ChildProcessConnection.java:640:
private void updateWaivedBoundOnlyState() {
nit: put this above the test methods, actually, probably next to
isWaivedBoundOnlyOrWasWhenDied

[Jay Civelli, 2017-05-12 00:26:10]

https://codereview.chromium.org/2874643002/diff/80001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java
(right):

https://codereview.chromium.org/2874643002/diff/80001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java:182:
* connection goes away, but ManagedConnection itself is kept (until overwritten
by a new entry
On 2017/05/11 23:54:48, boliu wrote:
> is this true?

Brought it back somehow probably during the conflict resolution with one of my
other patch.
Removed.

https://codereview.chromium.org/2874643002/diff/80001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ChildProcessConnection.java
(right):

https://codereview.chromium.org/2874643002/diff/80001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ChildProcessConnection.java:487:
if ((useStrongBinding && !mStrongBinding.bind())
On 2017/05/11 23:54:48, boliu wrote:
> nit: might be easier to read:
> 
> boolean success = useStrongBinding ? mStrongBinding.bind() :
> mInitialBinding.bind();
> if (!success) return false;

Done.

https://codereview.chromium.org/2874643002/diff/80001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ChildProcessConnection.java:640:
private void updateWaivedBoundOnlyState() {
On 2017/05/11 23:54:48, boliu wrote:
> nit: put this above the test methods, actually, probably next to
> isWaivedBoundOnlyOrWasWhenDied

Done.

[Jay Civelli, 2017-05-12 00:26:17]

The CQ bit was checked by jcivelli@chromium.org

[Jay Civelli, 2017-05-12 00:26:18]

The patchset sent to the CQ was uploaded after l-g-t-m from boliu@chromium.org
Link to the patchset: https://codereview.chromium.org/2874643002/#ps100001
(title: "Address boliu@'s comments + sync")

[commit-bot: I haz the power, 2017-05-12 00:27:25]

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-12 00:51:25]

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

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

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Jay Civelli, 2017-05-12 00:53:22]

jcivelli@chromium.org changed reviewers:
+ nyquist@chromium.org

[Jay Civelli, 2017-05-12 00:53:22]

Adding nyquist@ for OWNERS review of
chrome/android/javatests/src/org/chromium/chrome/browser/BindingManagerIntegrationTest.java

[nyquist, 2017-05-12 21:20:00]

chrome/android/javatests/src/org/chromium/chrome/browser/BindingManagerIntegrationTest.java
lgtm

[Jay Civelli, 2017-05-15 16:13:04]

The CQ bit was checked by jcivelli@chromium.org

[Jay Civelli, 2017-05-15 16:13:05]

The patchset sent to the CQ was uploaded after l-g-t-m from
nyquist@chromium.org, boliu@chromium.org
Link to the patchset: https://codereview.chromium.org/2874643002/#ps120001
(title: "Fix test + sync")

[commit-bot: I haz the power, 2017-05-15 16:13:48]

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-15 17:38:28]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1494864784867890,
"parent_rev": "ea05207e94618c286f12bbf97f5f65ecc552d663", "commit_rev":
"e8aabd539bd139049d3890562a45339428e7ece1"}

[commit-bot: I haz the power, 2017-05-15 17:38:44]

Description was changed from

==========
Merging Managed and Important connection together again.

As part of the effort to move ChildProcessConnection to base, merging
ImportantChildProcessConnection and ManagedChildProcessConnection into
a unique class again.
The ChildProcessLauncher adds a strong binding to privileged processes
and add sanboxed processes to the BindingManager.
Also changing the OOM protected state to waived bound only and making
the ChildProcessLauncherHelper policy clearer.

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

to

==========
Merging Managed and Important connection together again.

As part of the effort to move ChildProcessConnection to base, merging
ImportantChildProcessConnection and ManagedChildProcessConnection into
a unique class again.
The ChildProcessLauncher adds a strong binding to privileged processes
and add sanboxed processes to the BindingManager.
Also changing the OOM protected state to waived bound only and making
the ChildProcessLauncherHelper policy clearer.

BUG=689758

Review-Url: https://codereview.chromium.org/2874643002
Cr-Commit-Position: refs/heads/master@{#471819}
Committed:
https://chromium.googlesource.com/chromium/src/+/e8aabd539bd139049d3890562a45...
==========

[commit-bot: I haz the power, 2017-05-15 17:38:47]

Committed patchset #5 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/e8aabd539bd139049d3890562a45...