android: Async setupConnection binder call

android: Async setupConnection binder call

Make setupConnection oneway/async. Add a ICallbackInt interface to
return the PID of the child process. The reply is then posted back to
launcher thread. Connection setup is already mostly async so only
involves moving related code to the reply callback.

One caveat is that getCallingPid no longer works and just returns 0
for oneway calls. The pid check is not strictly necessary, so dropped
the from setupConnection instead. 

BUG=689758, 690118
Review-Url: https://codereview.chromium.org/2810433002
Cr-Commit-Position: refs/heads/master@{#463332}
Committed: https://chromium.googlesource.com/chromium/src/+/39c7bd71dc2f9c9f692dd346e4e4e525af25ede1

Reland contains fix for flaky test. The pid is received asynchronously
now, so wait for it after allocating a connection.

Review-Url: https://codereview.chromium.org/2810433002
Cr-Commit-Position: refs/heads/master@{#463419}
Committed: https://chromium.googlesource.com/chromium/src/+/03ee275e219a1dd9ab281452507aeb7357c6f4c7

[boliu, 2017-04-09 22:26:07]

Description was changed from

==========
android: Async setupConnection binder call

WIP still has failing tests

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

to

==========
android: Async setupConnection binder call

Make setupConnection oneway/async. Add a ICallbackInt interface to
return the PID of the child process. The reply is then posted back to
launcher thread. Connection setup is already mostly async so only
involves moving related code to the reply callback.

One caveat is that getCallingPid no longer works and just returns 0
for oneway calls. The pid check is not strictly necessary, so dropped
the from setupConnection instead. 

BUG=689758, 690118
==========

[boliu, 2017-04-09 22:26:16]

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

[commit-bot: I haz the power, 2017-04-09 22:26:19]

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-04-09 23:36:56]

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

[commit-bot: I haz the power, 2017-04-09 23:36:56]

Dry run: This issue passed the CQ dry run.

[boliu, 2017-04-10 13:40:18]

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

[boliu, 2017-04-10 13:40:19]

ptal

rsesek: everything
jcivelli: should I update ITestClient in the same CL?

I'll get a base owner to stamp the gn change after

[Jay Civelli, 2017-04-10 16:12:44]

On 2017/04/10 13:40:19, boliu wrote:
> ptal
> 
> rsesek: everything
> jcivelli: should I update ITestClient in the same CL?
We can do that as a follow up, or when I merge both at some later point.
 
> 
> I'll get a base owner to stamp the gn change after

[Robert Sesek, 2017-04-10 16:17:58]

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

https://codereview.chromium.org/2810433002/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java:136:
Log.e(TAG, "Client pid %d does not match the bound pid %d", callingPid,
This is removing the bindToCaller check?

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

https://codereview.chromium.org/2810433002/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ChildProcessConnectionImpl.java:411:
// We proactively close the FDs rather than wait for GC & finalizer.
Closing the FDs should still be possible after calling setupConnection.

[boliu, 2017-04-10 16:23:05]

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

https://codereview.chromium.org/2810433002/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java:136:
Log.e(TAG, "Client pid %d does not match the bound pid %d", callingPid,
On 2017/04/10 16:17:58, Robert Sesek wrote:
> This is removing the bindToCaller check?

Yeah I put this in the description. getCallingPid returns 0 for a oneway call.
And this appears to be undocumented behavior, yay android.

So can have browser pass its pid her, or just don't check it, since it's not
strictly necessary. If bindToCaller fails first, browser should not call
setupConnection.

[Robert Sesek, 2017-04-10 16:42:44]

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

https://codereview.chromium.org/2810433002/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java:136:
Log.e(TAG, "Client pid %d does not match the bound pid %d", callingPid,
On 2017/04/10 16:23:05, boliu wrote:
> On 2017/04/10 16:17:58, Robert Sesek wrote:
> > This is removing the bindToCaller check?
> 
> Yeah I put this in the description. getCallingPid returns 0 for a oneway call.
> And this appears to be undocumented behavior, yay android.
> 
> So can have browser pass its pid her, or just don't check it, since it's not
> strictly necessary. If bindToCaller fails first, browser should not call
> setupConnection.

Ah, sorry, yes missed that. It seems that the kernel driver indeed doesn't put
the "from" task into the transaction data for ONE_WAY, but the calling EUID does
get put. (Probably because there's no guarantee that the calling PID doesn't go
away during the async transaction?)

Agree it should be fine to drop this, then.

[boliu, 2017-04-10 16:56:29]

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

[boliu, 2017-04-10 16:56:30]

ptal

+torne for base (to stamp the gn change more or less)

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

https://codereview.chromium.org/2810433002/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ChildProcessConnectionImpl.java:411:
// We proactively close the FDs rather than wait for GC & finalizer.
On 2017/04/10 16:17:58, Robert Sesek wrote:
> Closing the FDs should still be possible after calling setupConnection.

Done.

[Torne, 2017-04-10 17:05:12]

lgtm

[Robert Sesek, 2017-04-10 17:05:55]

lgtm

[boliu, 2017-04-10 17:10:03]

The CQ bit was checked by boliu@chromium.org

[commit-bot: I haz the power, 2017-04-10 17:10: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-04-10 18:23:09]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1491844203098520,
"parent_rev": "b2726a53946fc14984cd8d3f73ec5b2dfc1656c3", "commit_rev":
"39c7bd71dc2f9c9f692dd346e4e4e525af25ede1"}

[commit-bot: I haz the power, 2017-04-10 18:23:56]

Description was changed from

==========
android: Async setupConnection binder call

Make setupConnection oneway/async. Add a ICallbackInt interface to
return the PID of the child process. The reply is then posted back to
launcher thread. Connection setup is already mostly async so only
involves moving related code to the reply callback.

One caveat is that getCallingPid no longer works and just returns 0
for oneway calls. The pid check is not strictly necessary, so dropped
the from setupConnection instead. 

BUG=689758, 690118
==========

to

==========
android: Async setupConnection binder call

Make setupConnection oneway/async. Add a ICallbackInt interface to
return the PID of the child process. The reply is then posted back to
launcher thread. Connection setup is already mostly async so only
involves moving related code to the reply callback.

One caveat is that getCallingPid no longer works and just returns 0
for oneway calls. The pid check is not strictly necessary, so dropped
the from setupConnection instead. 

BUG=689758, 690118

Review-Url: https://codereview.chromium.org/2810433002
Cr-Commit-Position: refs/heads/master@{#463332}
Committed:
https://chromium.googlesource.com/chromium/src/+/39c7bd71dc2f9c9f692dd346e4e4...
==========

[commit-bot: I haz the power, 2017-04-10 18:23:59]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/39c7bd71dc2f9c9f692dd346e4e4...

[please use gerrit instead, 2017-04-10 20:51:15]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/2806343003/ by rouslan@chromium.org.

The reason for reverting is: Appears to have broken
ChildProcessLauncherTest_testBindServiceFromMultipleProcesses/0 on
https://build.chromium.org/p/chromium.linux/builders/Android%20Tests%20%28dbg...

junit.framework.AssertionFailedError
	at
org.chromium.content.browser.ChildProcessLauncherTest.testBindServiceFromMultipleProcesses(ChildProcessLauncherTest.java:448)
	at java.lang.reflect.Method.invokeNative(Native Method)
	at
android.test.InstrumentationTestCase.runMethod(InstrumentationTestCase.java:214)
	at
android.test.InstrumentationTestCase.runTest(InstrumentationTestCase.java:199)
	at org.chromium.base.test.BaseTestResult.run(BaseTestResult.java:129)
	at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:191)
	at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:176)
	at
android.test.InstrumentationTestRunner.onStart(InstrumentationTestRunner.java:554)
	at
android.app.Instrumentation$InstrumentationThread.run(Instrumentation.java:1701).

[boliu, 2017-04-10 21:05:16]

Description was changed from

==========
android: Async setupConnection binder call

Make setupConnection oneway/async. Add a ICallbackInt interface to
return the PID of the child process. The reply is then posted back to
launcher thread. Connection setup is already mostly async so only
involves moving related code to the reply callback.

One caveat is that getCallingPid no longer works and just returns 0
for oneway calls. The pid check is not strictly necessary, so dropped
the from setupConnection instead. 

BUG=689758, 690118

Review-Url: https://codereview.chromium.org/2810433002
Cr-Commit-Position: refs/heads/master@{#463332}
Committed:
https://chromium.googlesource.com/chromium/src/+/39c7bd71dc2f9c9f692dd346e4e4...
==========

to

==========
android: Async setupConnection binder call

Make setupConnection oneway/async. Add a ICallbackInt interface to
return the PID of the child process. The reply is then posted back to
launcher thread. Connection setup is already mostly async so only
involves moving related code to the reply callback.

One caveat is that getCallingPid no longer works and just returns 0
for oneway calls. The pid check is not strictly necessary, so dropped
the from setupConnection instead. 

BUG=689758, 690118

Review-Url: https://codereview.chromium.org/2810433002
Cr-Commit-Position: refs/heads/master@{#463332}
Committed:
https://chromium.googlesource.com/chromium/src/+/39c7bd71dc2f9c9f692dd346e4e4...
==========

[boliu, 2017-04-10 21:07:31]

Description was changed from

==========
android: Async setupConnection binder call

Make setupConnection oneway/async. Add a ICallbackInt interface to
return the PID of the child process. The reply is then posted back to
launcher thread. Connection setup is already mostly async so only
involves moving related code to the reply callback.

One caveat is that getCallingPid no longer works and just returns 0
for oneway calls. The pid check is not strictly necessary, so dropped
the from setupConnection instead. 

BUG=689758, 690118

Review-Url: https://codereview.chromium.org/2810433002
Cr-Commit-Position: refs/heads/master@{#463332}
Committed:
https://chromium.googlesource.com/chromium/src/+/39c7bd71dc2f9c9f692dd346e4e4...
==========

to

==========
android: Async setupConnection binder call

Make setupConnection oneway/async. Add a ICallbackInt interface to
return the PID of the child process. The reply is then posted back to
launcher thread. Connection setup is already mostly async so only
involves moving related code to the reply callback.

One caveat is that getCallingPid no longer works and just returns 0
for oneway calls. The pid check is not strictly necessary, so dropped
the from setupConnection instead. 

BUG=689758, 690118

Review-Url: https://codereview.chromium.org/2810433002
Cr-Commit-Position: refs/heads/master@{#463332}
Committed:
https://chromium.googlesource.com/chromium/src/+/39c7bd71dc2f9c9f692dd346e4e4...

Reland contains fix for flaky test. The pid is received asynchronously
now, so wait for it after allocating a connection.
==========

[boliu, 2017-04-10 21:09:07]

fwiw, I already noticed that flake, and wrote the flake fix here:
https://codereview.chromium.org/2810933002/

just gonna pull that change in and ride the approvals here

[boliu, 2017-04-10 21:09:32]

The CQ bit was checked by boliu@chromium.org

[boliu, 2017-04-10 21:09:33]

The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org,
torne@chromium.org
Link to the patchset: https://codereview.chromium.org/2810433002/#ps80001
(title: "deflake test")

[commit-bot: I haz the power, 2017-04-10 21:10:23]

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-04-10 22:21:36]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1491858572774110,
"parent_rev": "d3a2a83fbff70f8beb2f1bbdf691fc2cbb27ea0d", "commit_rev":
"03ee275e219a1dd9ab281452507aeb7357c6f4c7"}

[commit-bot: I haz the power, 2017-04-10 22:22:30]

Description was changed from

==========
android: Async setupConnection binder call

Make setupConnection oneway/async. Add a ICallbackInt interface to
return the PID of the child process. The reply is then posted back to
launcher thread. Connection setup is already mostly async so only
involves moving related code to the reply callback.

One caveat is that getCallingPid no longer works and just returns 0
for oneway calls. The pid check is not strictly necessary, so dropped
the from setupConnection instead. 

BUG=689758, 690118

Review-Url: https://codereview.chromium.org/2810433002
Cr-Commit-Position: refs/heads/master@{#463332}
Committed:
https://chromium.googlesource.com/chromium/src/+/39c7bd71dc2f9c9f692dd346e4e4...

Reland contains fix for flaky test. The pid is received asynchronously
now, so wait for it after allocating a connection.
==========

to

==========
android: Async setupConnection binder call

Make setupConnection oneway/async. Add a ICallbackInt interface to
return the PID of the child process. The reply is then posted back to
launcher thread. Connection setup is already mostly async so only
involves moving related code to the reply callback.

One caveat is that getCallingPid no longer works and just returns 0
for oneway calls. The pid check is not strictly necessary, so dropped
the from setupConnection instead. 

BUG=689758, 690118

Review-Url: https://codereview.chromium.org/2810433002
Cr-Commit-Position: refs/heads/master@{#463332}
Committed:
https://chromium.googlesource.com/chromium/src/+/39c7bd71dc2f9c9f692dd346e4e4...

Reland contains fix for flaky test. The pid is received asynchronously
now, so wait for it after allocating a connection.

Review-Url: https://codereview.chromium.org/2810433002
Cr-Commit-Position: refs/heads/master@{#463419}
Committed:
https://chromium.googlesource.com/chromium/src/+/03ee275e219a1dd9ab281452507a...
==========

[commit-bot: I haz the power, 2017-04-10 22:22:31]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/03ee275e219a1dd9ab281452507a...