Bind Android ChildProcessServices to a specific client PID.

Bind Android ChildProcessServices to a specific client PID.

A ChildProcessService may only be setup once, but nothing enforced this limit,
and a second client of the service would receive no indication of failure. This
situation could occur with Android WebView in multi-process mode, if two
android:processes in the same package tried to use WebView.

To solve this issue, introduce an initial handshake message in
IChildProcessService that must be issued before setupConnection. This method
binds the service to the calling PID. If that binding fails, the
ChildProcessLauncher can then re-try creating a service process using the
next available slot.

BUG=558377, 683133
Review-Url: https://codereview.chromium.org/2626413004
Cr-Commit-Position: refs/heads/master@{#445779}
Committed: https://chromium.googlesource.com/chromium/src/+/abd3ee77b9cfd7b3d8cf5f95c511e328645f8fde

[Robert Sesek, 2017-01-14 01:21:46]

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

[commit-bot: I haz the power, 2017-01-14 01:22:05]

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-01-14 01:58:53]

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

[commit-bot: I haz the power, 2017-01-14 01:58:54]

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

[Robert Sesek, 2017-01-14 20:57:54]

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

[commit-bot: I haz the power, 2017-01-14 20:58:02]

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-01-14 21:37:14]

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

[commit-bot: I haz the power, 2017-01-14 21:37:15]

Dry run: This issue passed the CQ dry run.

[Robert Sesek, 2017-01-14 21:46:16]

Patchset #2 (id:20001) has been deleted

[Robert Sesek, 2017-01-14 21:46:22]

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

[Robert Sesek, 2017-01-14 21:46:24]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2017-01-14 21:46:25]

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-01-14 22:02:45]

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

[commit-bot: I haz the power, 2017-01-14 22:20:15]

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

[commit-bot: I haz the power, 2017-01-14 22:20:15]

Dry run: This issue passed the CQ dry run.

[Tobias Sargeant, 2017-01-14 23:24:58]

https://codereview.chromium.org/2626413004/diff/40001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java
(right):

https://codereview.chromium.org/2626413004/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java:118:
Log.e(TAG, "Client pid %d does not match the bound pid %d", callingPid,
Maybe log a different message if mBoundCallingPid is zero, so that it's clear
that bindToCaller wasn't called?

Do accesses to mBoundCallingPid need a lock?

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

https://codereview.chromium.org/2626413004/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:587:

Presumably the fact that warmUp doesn't retry means that in second and
subsequent browser processes, warmUp will be non-functional?

https://codereview.chromium.org/2626413004/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:757:
triggerConnectionSetup(allocatedConnection, commandLine, childProcessId,
I'm not sure that I understand this code, but in the case where we have to
retry, won't each call to startInternal reach here and call
triggerConnectionSetup with whatever allocatedConnection is (whether or not it
was claimed)?

[Torne, 2017-01-16 12:44:43]

https://codereview.chromium.org/2626413004/diff/40001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java
(right):

https://codereview.chromium.org/2626413004/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java:118:
Log.e(TAG, "Client pid %d does not match the bound pid %d", callingPid,
On 2017/01/14 23:24:57, Tobias Sargeant wrote:
> Do accesses to mBoundCallingPid need a lock?

Yeah, this isn't threadsafe right now I think (there's more than one binder
thread). It either needs a lock, or to be an atomic we can test-and-set.

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

https://codereview.chromium.org/2626413004/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ChildProcessConnectionImpl.java:179:
} finally {
Unless there's another exception type that can happen here that we want to
propagate, there's no need for this to be in a finally block, it can just be
after the try-catch?

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

https://codereview.chromium.org/2626413004/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:730:
new AsyncTask<Void, Void, Void>() {
If the asynctask only has a doInBackground and is void, you can just pass a
Runnable to the execute() method of the executor, which looks a bit neater. The
AsyncTask machinery is only useful for the case where you pass a result back to
the UI thread.

https://codereview.chromium.org/2626413004/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:757:
triggerConnectionSetup(allocatedConnection, commandLine, childProcessId,
On 2017/01/14 23:24:57, Tobias Sargeant wrote:
> I'm not sure that I understand this code, but in the case where we have to
> retry, won't each call to startInternal reach here and call
> triggerConnectionSetup with whatever allocatedConnection is (whether or not it
> was claimed)?

I think it will have early returned on line 750 above because
allocatedConnection is still null? Not sure..

[Robert Sesek, 2017-01-17 15:49:05]

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

[commit-bot: I haz the power, 2017-01-17 15:49:14]

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-01-17 16:55:55]

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

[commit-bot: I haz the power, 2017-01-17 16:55:56]

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...)

[Robert Sesek, 2017-01-17 19:12:27]

Patchset #2 (id:60001) has been deleted

[Robert Sesek, 2017-01-17 19:12:37]

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

[commit-bot: I haz the power, 2017-01-17 19:13:20]

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-01-17 19:50:09]

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

[commit-bot: I haz the power, 2017-01-17 19:50:10]

Dry run: This issue passed the CQ dry run.

[Robert Sesek, 2017-01-17 19:51:22]

https://codereview.chromium.org/2626413004/diff/40001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java
(right):

https://codereview.chromium.org/2626413004/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java:118:
Log.e(TAG, "Client pid %d does not match the bound pid %d", callingPid,
On 2017/01/16 12:44:43, Torne wrote:
> On 2017/01/14 23:24:57, Tobias Sargeant wrote:
> > Do accesses to mBoundCallingPid need a lock?
> 
> Yeah, this isn't threadsafe right now I think (there's more than one binder
> thread). It either needs a lock, or to be an atomic we can test-and-set.

Added a lock. Also mCallback was already not-thread-safe, so protected that too.

https://codereview.chromium.org/2626413004/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java:118:
Log.e(TAG, "Client pid %d does not match the bound pid %d", callingPid,
On 2017/01/14 23:24:57, Tobias Sargeant wrote:
> Maybe log a different message if mBoundCallingPid is zero, so that it's clear
> that bindToCaller wasn't called?

Done.

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

https://codereview.chromium.org/2626413004/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ChildProcessConnectionImpl.java:179:
} finally {
On 2017/01/16 12:44:43, Torne wrote:
> Unless there's another exception type that can happen here that we want to
> propagate, there's no need for this to be in a finally block, it can just be
> after the try-catch?

Done.

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

https://codereview.chromium.org/2626413004/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:587:

On 2017/01/14 23:24:58, Tobias Sargeant wrote:
> Presumably the fact that warmUp doesn't retry means that in second and
> subsequent browser processes, warmUp will be non-functional?

Since warmUp is only used for the first connection, there's no point in
re-trying it if it fails.

https://codereview.chromium.org/2626413004/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:730:
new AsyncTask<Void, Void, Void>() {
On 2017/01/16 12:44:43, Torne wrote:
> If the asynctask only has a doInBackground and is void, you can just pass a
> Runnable to the execute() method of the executor, which looks a bit neater.
The
> AsyncTask machinery is only useful for the case where you pass a result back
to
> the UI thread.

Done.

https://codereview.chromium.org/2626413004/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:757:
triggerConnectionSetup(allocatedConnection, commandLine, childProcessId,
On 2017/01/16 12:44:43, Torne wrote:
> On 2017/01/14 23:24:57, Tobias Sargeant wrote:
> > I'm not sure that I understand this code, but in the case where we have to
> > retry, won't each call to startInternal reach here and call
> > triggerConnectionSetup with whatever allocatedConnection is (whether or not
it
> > was claimed)?
> 
> I think it will have early returned on line 750 above because
> allocatedConnection is still null? Not sure..

Right. allocatedConnection will be null so it will allocateBoundConnection() a
new one.

[Torne, 2017-01-18 13:55:13]

LGTM

[Tobias Sargeant, 2017-01-19 10:42:49]

On 2017/01/18 13:55:13, Torne wrote:
> LGTM

LGTM too.

[Robert Sesek, 2017-01-19 16:31:21]

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

[Robert Sesek, 2017-01-19 16:31:22]

+mariakhomenko for OWNERS

[Maria, 2017-01-19 18:09:09]

Please add tests for the new functionality.

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

https://codereview.chromium.org/2626413004/diff/70007/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:728:
public void onChildStartFailed() {
Not sure I understand this -- won't this create an infinite loop in case of
failure?

[Robert Sesek, 2017-01-19 19:10:07]

On 2017/01/19 18:09:09, Maria wrote:
> Please add tests for the new functionality.

I looked pretty extensively at adding a test but didn't seen a totally clean
option. To properly test this, we need another, non-isolated process in the same
package as the test to bind to a sandboxed process, so that it can conflict with
the test trying to bind to the same service. I think this means adding something
like a ChildProcessLauncherTestHelperService to ContentShell.apk. Does that
sound reasonable?

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

https://codereview.chromium.org/2626413004/diff/70007/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:728:
public void onChildStartFailed() {
On 2017/01/19 18:09:09, Maria wrote:
> Not sure I understand this -- won't this create an infinite loop in case of
> failure?

You're not the only one who mentioned that, so I'll add a comment. This doesn't
create an infinite loop. In the case of failure, we don't mark the connection as
freed, so the most-recent slot doesn't get picked. Instead, the next slot is
allocated and used instead.

[Maria, 2017-01-19 21:31:54]

On 2017/01/19 19:10:07, Robert Sesek wrote:
> On 2017/01/19 18:09:09, Maria wrote:
> > Please add tests for the new functionality.
> 
> I looked pretty extensively at adding a test but didn't seen a totally clean
> option. To properly test this, we need another, non-isolated process in the
same
> package as the test to bind to a sandboxed process, so that it can conflict
with
> the test trying to bind to the same service. I think this means adding
something
> like a ChildProcessLauncherTestHelperService to ContentShell.apk. Does that
> sound reasonable?

Hmm, ideally we would add it to ContentShellTest.apk, but I guess that won't
work because both processes have to be under the same apk? If so, I think adding
a helper service to ContentShell is fine. We have some test providers and
activities in chrome/android/javatests/AndroidManifest.xml for example of what's
been done before.

> 
>
https://codereview.chromium.org/2626413004/diff/70007/content/public/android/...
> File
>
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java
> (right):
> 
>
https://codereview.chromium.org/2626413004/diff/70007/content/public/android/...
>
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:728:
> public void onChildStartFailed() {
> On 2017/01/19 18:09:09, Maria wrote:
> > Not sure I understand this -- won't this create an infinite loop in case of
> > failure?
> 
> You're not the only one who mentioned that, so I'll add a comment. This
doesn't
> create an infinite loop. In the case of failure, we don't mark the connection
as
> freed, so the most-recent slot doesn't get picked. Instead, the next slot is
> allocated and used instead.

[Torne, 2017-01-20 12:06:42]

On 2017/01/19 19:10:07, Robert Sesek wrote:
> On 2017/01/19 18:09:09, Maria wrote:
> > Please add tests for the new functionality.
> 
> I looked pretty extensively at adding a test but didn't seen a totally clean
> option. To properly test this, we need another, non-isolated process in the
same
> package as the test to bind to a sandboxed process, so that it can conflict
with
> the test trying to bind to the same service. I think this means adding
something
> like a ChildProcessLauncherTestHelperService to ContentShell.apk. Does that
> sound reasonable?
> 
>
https://codereview.chromium.org/2626413004/diff/70007/content/public/android/...
> File
>
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java
> (right):
> 
>
https://codereview.chromium.org/2626413004/diff/70007/content/public/android/...
>
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:728:
> public void onChildStartFailed() {
> On 2017/01/19 18:09:09, Maria wrote:
> > Not sure I understand this -- won't this create an infinite loop in case of
> > failure?
> 
> You're not the only one who mentioned that, so I'll add a comment. This
doesn't
> create an infinite loop. In the case of failure, we don't mark the connection
as
> freed, so the most-recent slot doesn't get picked. Instead, the next slot is
> allocated and used instead.

Does that mean the current process will never try that slot ever again, even in
a future attempt to bind a child process? This seems like we could eventually
run out of slots if the two processes create enough renderers with the wrong
timing. Am I misunderstanding?

[Tobias Sargeant, 2017-01-20 13:44:41]

Torne asked that this and the other concurrent CL to add service entries to
webview not pollute the above bug, so I filed crbug.com/683133. Could you please
update the bug number to reference that?

I've tested this manually locally, by the way, and it fixes multiprocess webview
for apps that we knew were broken, while not affecting Chrome. Are the tests
going to be as complex as I expect to write? Given the urgent need to widely
test multiprocess, would it be possible to consider landing this first, and
following up with a CL that adds tests?

[Robert Sesek, 2017-01-23 20:17:12]

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

[commit-bot: I haz the power, 2017-01-23 20:17:32]

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-01-23 20:27:12]

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

[commit-bot: I haz the power, 2017-01-23 20:27:14]

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

[Robert Sesek, 2017-01-23 21:26:52]

Patchset #3 (id:90001) has been deleted

[Robert Sesek, 2017-01-23 21:27:18]

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

[commit-bot: I haz the power, 2017-01-23 21:28:08]

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-01-23 22:16:04]

PTAL, added a test.

[Maria, 2017-01-23 22:28:42]

lgtm

Thank you for adding the test. Very much appreciated.

https://codereview.chromium.org/2626413004/diff/110001/content/shell/android/...
File
content/shell/android/shell_apk/src/org/chromium/content_shell_apk/ChildProcessLauncherTestHelperService.java
(right):

https://codereview.chromium.org/2626413004/diff/110001/content/shell/android/...
content/shell/android/shell_apk/src/org/chromium/content_shell_apk/ChildProcessLauncherTestHelperService.java:28:
* starts a sandboxed service process via the ChildProcessLauncher. This is
requierd to test
nit: typo "required"

[commit-bot: I haz the power, 2017-01-23 23:00:04]

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

[commit-bot: I haz the power, 2017-01-23 23:00:06]

Dry run: This issue passed the CQ dry run.

[Robert Sesek, 2017-01-23 23:19:39]

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

[commit-bot: I haz the power, 2017-01-23 23:20:41]

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-01-24 00:58:05]

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

[commit-bot: I haz the power, 2017-01-24 00:58:06]

Dry run: This issue passed the CQ dry run.

[Torne, 2017-01-24 12:39:34]

still LGTM, thanks for the test.

[Robert Sesek, 2017-01-24 16:26:51]

https://codereview.chromium.org/2626413004/diff/110001/content/shell/android/...
File
content/shell/android/shell_apk/src/org/chromium/content_shell_apk/ChildProcessLauncherTestHelperService.java
(right):

https://codereview.chromium.org/2626413004/diff/110001/content/shell/android/...
content/shell/android/shell_apk/src/org/chromium/content_shell_apk/ChildProcessLauncherTestHelperService.java:28:
* starts a sandboxed service process via the ChildProcessLauncher. This is
requierd to test
On 2017/01/23 22:28:41, Maria wrote:
> nit: typo "required"

Done.

[Robert Sesek, 2017-01-24 16:26:55]

The CQ bit was checked by rsesek@chromium.org

[Robert Sesek, 2017-01-24 16:26:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
tobiasjs@chromium.org, mariakhomenko@chromium.org
Link to the patchset: https://codereview.chromium.org/2626413004/#ps130001
(title: "typo")

[commit-bot: I haz the power, 2017-01-24 16:27:03]

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-01-24 16:36:02]

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

[commit-bot: I haz the power, 2017-01-24 16:36:03]

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...)

[Robert Sesek, 2017-01-24 16:38:37]

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

[commit-bot: I haz the power, 2017-01-24 16:38:42]

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-01-24 16:39:02]

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

[Robert Sesek, 2017-01-24 16:39:03]

+boliu for content OWNERS

[commit-bot: I haz the power, 2017-01-24 16:42:22]

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

[commit-bot: I haz the power, 2017-01-24 16:42:23]

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...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[boliu, 2017-01-24 17:37:34]

all minor things that can be fixed later, so lgtm

one other potential optimization is maybe we should randomize
mFreeConnectionIndices on start up

https://codereview.chromium.org/2626413004/diff/150001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/ChildProcessConnectionImpl.java
(right):

https://codereview.chromium.org/2626413004/diff/150001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ChildProcessConnectionImpl.java:181:
if (mStartCallback != null) {
nit: could do this check first, so it's not repeated and easier to read

https://codereview.chromium.org/2626413004/diff/150001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ChildProcessConnectionImpl.java:187:
}
can set mStartCallback to null after? the closure seem to have a lot of state..

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

https://codereview.chromium.org/2626413004/diff/150001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:730:
AsyncTask.THREAD_POOL_EXECUTOR.execute(new Runnable() {
can we just use the launcher thread (through native code I guess..) AsyncTask
thread pool is shared with the app, so can be backed up, and launching child
process is pretty time sensitive?

or maybe not, this is only webview when app is using this from multiple
processes, so they are doing something already

https://codereview.chromium.org/2626413004/diff/150001/content/public/android...
File
content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherTest.java
(right):

https://codereview.chromium.org/2626413004/diff/150001/content/public/android...
content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherTest.java:406:
ChildProcessLauncher.getServiceMapForTesting();
I guess ideally, this should wait for the actual nativeOnChildProcessStarted
call. But mocking that might be hard.

[Robert Sesek, 2017-01-24 18:03:47]

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

[commit-bot: I haz the power, 2017-01-24 18:04:04]

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-01-24 18:52:49]

On 2017/01/24 17:37:34, boliu wrote:
> all minor things that can be fixed later, so lgtm

Thanks. I'll send you a follow-up CL today or tomorrow, since Alex needs this
for the build candidate tonight.

[Robert Sesek, 2017-01-24 18:54:19]

Description was changed from

==========
Bind Android ChildProcessServices to a specific client PID.

A ChildProcessService may only be setup once, but nothing enforced this limit,
and a second client of the service would receive no indication of failure. This
situation could occur with Android WebView in multi-process mode, if two
android:processes in the same package tried to use WebView.

To solve this issue, introduce an initial handshake message in
IChildProcessService that must be issued before setupConnection. This method
binds the service to the calling PID. If that binding fails, the
ChildProcessLauncher can then re-try creating a service process using the
next available slot.

BUG=558377
==========

to

==========
Bind Android ChildProcessServices to a specific client PID.

A ChildProcessService may only be setup once, but nothing enforced this limit,
and a second client of the service would receive no indication of failure. This
situation could occur with Android WebView in multi-process mode, if two
android:processes in the same package tried to use WebView.

To solve this issue, introduce an initial handshake message in
IChildProcessService that must be issued before setupConnection. This method
binds the service to the calling PID. If that binding fails, the
ChildProcessLauncher can then re-try creating a service process using the
next available slot.

BUG=558377,683133
==========

[Robert Sesek, 2017-01-24 19:28:22]

The CQ bit was checked by rsesek@chromium.org

[Robert Sesek, 2017-01-24 19:28:23]

The patchset sent to the CQ was uploaded after l-g-t-m from torne@chromium.org,
tobiasjs@chromium.org, mariakhomenko@chromium.org
Link to the patchset: https://codereview.chromium.org/2626413004/#ps150001
(title: "Add OWNERS file")

[commit-bot: I haz the power, 2017-01-24 19:29:17]

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-01-24 19:38:16]

CQ is committing da patch.

Bot data: {"patchset_id": 150001, "attempt_start_ts": 1485286102896360,
"parent_rev": "9686be68e53348bab6f4ce846bf07d494f1575a0", "commit_rev":
"abd3ee77b9cfd7b3d8cf5f95c511e328645f8fde"}

[commit-bot: I haz the power, 2017-01-24 19:38:48]

Description was changed from

==========
Bind Android ChildProcessServices to a specific client PID.

A ChildProcessService may only be setup once, but nothing enforced this limit,
and a second client of the service would receive no indication of failure. This
situation could occur with Android WebView in multi-process mode, if two
android:processes in the same package tried to use WebView.

To solve this issue, introduce an initial handshake message in
IChildProcessService that must be issued before setupConnection. This method
binds the service to the calling PID. If that binding fails, the
ChildProcessLauncher can then re-try creating a service process using the
next available slot.

BUG=558377,683133
==========

to

==========
Bind Android ChildProcessServices to a specific client PID.

A ChildProcessService may only be setup once, but nothing enforced this limit,
and a second client of the service would receive no indication of failure. This
situation could occur with Android WebView in multi-process mode, if two
android:processes in the same package tried to use WebView.

To solve this issue, introduce an initial handshake message in
IChildProcessService that must be issued before setupConnection. This method
binds the service to the calling PID. If that binding fails, the
ChildProcessLauncher can then re-try creating a service process using the
next available slot.

BUG=558377,683133

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

[commit-bot: I haz the power, 2017-01-24 19:38:50]

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

[Robert Sesek, 2017-01-25 16:14:03]

Comments addressed on this CL: https://codereview.chromium.org/2652293002/

https://codereview.chromium.org/2626413004/diff/150001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/ChildProcessConnectionImpl.java
(right):

https://codereview.chromium.org/2626413004/diff/150001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ChildProcessConnectionImpl.java:181:
if (mStartCallback != null) {
On 2017/01/24 17:37:34, boliu wrote:
> nit: could do this check first, so it's not repeated and easier to read

Done.

https://codereview.chromium.org/2626413004/diff/150001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ChildProcessConnectionImpl.java:187:
}
On 2017/01/24 17:37:34, boliu wrote:
> can set mStartCallback to null after? the closure seem to have a lot of
state..

Done.

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

https://codereview.chromium.org/2626413004/diff/150001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:730:
AsyncTask.THREAD_POOL_EXECUTOR.execute(new Runnable() {
On 2017/01/24 17:37:34, boliu wrote:
> can we just use the launcher thread (through native code I guess..) AsyncTask
> thread pool is shared with the app, so can be backed up, and launching child
> process is pretty time sensitive?
> 
> or maybe not, this is only webview when app is using this from multiple
> processes, so they are doing something already

I would prefer to do this on the ProcessLauncher thread, but there's no Java
handle to it. Is it worth round-tripping through JNI to avoid this? (We're
already operating in an edge condition here).

https://codereview.chromium.org/2626413004/diff/150001/content/public/android...
File
content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherTest.java
(right):

https://codereview.chromium.org/2626413004/diff/150001/content/public/android...
content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherTest.java:406:
ChildProcessLauncher.getServiceMapForTesting();
On 2017/01/24 17:37:34, boliu wrote:
> I guess ideally, this should wait for the actual nativeOnChildProcessStarted
> call. But mocking that might be hard.

Right, we're interested in the native callback getting run, but there is no
plumbing for testing that.