Making ChildProcessConnection only accessed from the launcher thread.

Making ChildProcessConnection only accessed from the launcher thread.

As a result, removing locks and synchronizations.
Also changing the way we store and report the OOM protected state.
We now update the OOM protected state every time it changes as long as we
are bound (that happens only on the launcher thread).
When retrieving that OOM protected state (which happens on the IO
thread), we return that state directly without the need of a lock.

BUG=714657
Review-Url: https://codereview.chromium.org/2840303002
Cr-Commit-Position: refs/heads/master@{#467892}
Committed: https://chromium.googlesource.com/chromium/src/+/93637728a91bf34e608f0c281febaf06af44fe30

[Jay Civelli, 2017-04-26 20:56:03]

Description was changed from

==========
Making ChildProcessConnection only accessed from the launcher thread.

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

to

==========
Making ChildProcessConnection only accessed from the launcher thread.

BUG=714657
==========

[Jay Civelli, 2017-04-26 20:56:04]

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

[Jay Civelli, 2017-04-26 23:57:29]

Description was changed from

==========
Making ChildProcessConnection only accessed from the launcher thread.

BUG=714657
==========

to

==========
Making ChildProcessConnection only accessed from the launcher thread.

As a result, removing locks and synchronizations.
Also changing the way we store and report the OOM protected state.
We now update the OOM protected state every time it changes as long as we
are bound (that happens only on the launcher thread).
When retrieving the state (which happens on the IO thread), we return
that state directly without the need of a lock.

BUG=714657
==========

[Jay Civelli, 2017-04-26 23:57:39]

Patchset #1 (id:1) has been deleted

[Jay Civelli, 2017-04-26 23:57:44]

Patchset #1 (id:20001) has been deleted

[Jay Civelli, 2017-04-26 23:57:49]

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

[commit-bot: I haz the power, 2017-04-26 23:58:56]

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-04-27 00:04:27]

Description was changed from

==========
Making ChildProcessConnection only accessed from the launcher thread.

As a result, removing locks and synchronizations.
Also changing the way we store and report the OOM protected state.
We now update the OOM protected state every time it changes as long as we
are bound (that happens only on the launcher thread).
When retrieving the state (which happens on the IO thread), we return
that state directly without the need of a lock.

BUG=714657
==========

to

==========
Making ChildProcessConnection only accessed from the launcher thread.

As a result, removing locks and synchronizations.
Also changing the way we store and report the OOM protected state.
We now update the OOM protected state every time it changes as long as we
are bound (that happens only on the launcher thread).
When retrieving that OOM protected state (which happens on the IO
thread), we return that state directly without the need of a lock.

BUG=714657
==========

[commit-bot: I haz the power, 2017-04-27 01:34:55]

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

[commit-bot: I haz the power, 2017-04-27 01:34:56]

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

[boliu, 2017-04-27 04:14:55]

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

https://codereview.chromium.org/2840303002/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ManagedChildProcessConnection.java:102:
if (!isBound()) {
this check is not needed anymore?

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

https://codereview.chromium.org/2840303002/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ManagedChildProcessConnection.java:124:
// Does not need to be synchronized since this is a boolean.
Do note this is racy but it's not really preventable without blocking

[Jay Civelli, 2017-04-27 18:48:21]

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

[Jay Civelli, 2017-04-27 18:48:32]

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

https://codereview.chromium.org/2840303002/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ManagedChildProcessConnection.java:102:
if (!isBound()) {
On 2017/04/27 04:14:55, boliu wrote:
> this check is not needed anymore?

No. I added it in my previous CL as this would be called multiple times and
mWasOoomProtectedOnUnbind would end up being overwritten to false causing tests
to fail.
Now that we are not storing a state on unbind, it can be called multiple times
with no side effect.

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

https://codereview.chromium.org/2840303002/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ManagedChildProcessConnection.java:124:
// Does not need to be synchronized since this is a boolean.
On 2017/04/27 04:14:55, boliu wrote:
> Do note this is racy but it's not really preventable without blocking

Changed comment.

[commit-bot: I haz the power, 2017-04-27 18:49:05]

Dry run: CQ is trying da patch.

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

[boliu, 2017-04-27 18:58:06]

https://codereview.chromium.org/2840303002/diff/60001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/BaseChildProcessConnection.java
(right):

https://codereview.chromium.org/2840303002/diff/60001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/BaseChildProcessConnection.java:245:
public final int getServiceNumberNoAssert() {
tests can use ChildProcessLauncherTestHelperService.runOnLauncherAndGetResult,
instead of complicating production code

[commit-bot: I haz the power, 2017-04-27 19:43:55]

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

[commit-bot: I haz the power, 2017-04-27 19:43:56]

Dry run: This issue passed the CQ dry run.

[Jay Civelli, 2017-04-27 20:19:12]

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

[Jay Civelli, 2017-04-27 20:19:20]

https://codereview.chromium.org/2840303002/diff/60001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/BaseChildProcessConnection.java
(right):

https://codereview.chromium.org/2840303002/diff/60001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/BaseChildProcessConnection.java:245:
public final int getServiceNumberNoAssert() {
On 2017/04/27 18:58:05, boliu wrote:
> tests can use ChildProcessLauncherTestHelperService.runOnLauncherAndGetResult,
> instead of complicating production code

Oh right. Added some connection specific method and moved them to a new utility
class.

[commit-bot: I haz the power, 2017-04-27 20:19:28]

Dry run: CQ is trying da patch.

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

[boliu, 2017-04-27 20:25:34]

lgtm

https://codereview.chromium.org/2840303002/diff/80001/content/shell/android/s...
File
content/shell/android/shell_apk/src/org/chromium/content_shell_apk/ChildProcessLauncherTestUtils.java
(right):

https://codereview.chromium.org/2840303002/diff/80001/content/shell/android/s...
content/shell/android/shell_apk/src/org/chromium/content_shell_apk/ChildProcessLauncherTestUtils.java:74:
return ChildProcessLauncherTestUtils.runOnLauncherAndGetResult(new
Callable<Integer>() {
no need for this qualification here and below

[Jay Civelli, 2017-04-27 20:31:46]

https://codereview.chromium.org/2840303002/diff/80001/content/shell/android/s...
File
content/shell/android/shell_apk/src/org/chromium/content_shell_apk/ChildProcessLauncherTestUtils.java
(right):

https://codereview.chromium.org/2840303002/diff/80001/content/shell/android/s...
content/shell/android/shell_apk/src/org/chromium/content_shell_apk/ChildProcessLauncherTestUtils.java:74:
return ChildProcessLauncherTestUtils.runOnLauncherAndGetResult(new
Callable<Integer>() {
On 2017/04/27 20:25:34, boliu wrote:
> no need for this qualification here and below

Done.

[Jay Civelli, 2017-04-27 20:31:50]

The CQ bit was checked by jcivelli@chromium.org

[Jay Civelli, 2017-04-27 20:31:51]

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/2840303002/#ps100001
(title: "Clean-up + sync")

[commit-bot: I haz the power, 2017-04-27 20:32:40]

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-27 21:21:50]

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

[commit-bot: I haz the power, 2017-04-27 21:21:51]

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

[Jay Civelli, 2017-04-28 04:23:41]

The CQ bit was checked by jcivelli@chromium.org

[commit-bot: I haz the power, 2017-04-28 04:24:19]

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-28 05:18:37]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1493353421253590,
"parent_rev": "d97cc73459881aeedfb39ed40ff5e0f9aefea9aa", "commit_rev":
"93637728a91bf34e608f0c281febaf06af44fe30"}

[commit-bot: I haz the power, 2017-04-28 05:18:44]

Description was changed from

==========
Making ChildProcessConnection only accessed from the launcher thread.

As a result, removing locks and synchronizations.
Also changing the way we store and report the OOM protected state.
We now update the OOM protected state every time it changes as long as we
are bound (that happens only on the launcher thread).
When retrieving that OOM protected state (which happens on the IO
thread), we return that state directly without the need of a lock.

BUG=714657
==========

to

==========
Making ChildProcessConnection only accessed from the launcher thread.

As a result, removing locks and synchronizations.
Also changing the way we store and report the OOM protected state.
We now update the OOM protected state every time it changes as long as we
are bound (that happens only on the launcher thread).
When retrieving that OOM protected state (which happens on the IO
thread), we return that state directly without the need of a lock.

BUG=714657

Review-Url: https://codereview.chromium.org/2840303002
Cr-Commit-Position: refs/heads/master@{#467892}
Committed:
https://chromium.googlesource.com/chromium/src/+/93637728a91bf34e608f0c281feb...
==========

[commit-bot: I haz the power, 2017-04-28 05:18:45]

Committed patchset #4 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/93637728a91bf34e608f0c281feb...