Multiprocess test client: Android child process launcher rework.

Multiprocess test client: Android child process launcher rework.

Changing the way test start child processese on Android.
Forking natively should not be used on Andoid, instead child processes
should be started through the Android framework, which requires going
to Java and using Android services.

For that we introduce the MultiprocessTestClientLauncher java class
which starts a service in a new process. We provide several
MultiprocessTestClientService service classes to ensure we can have
multiple child processes.
File descriptor are passed from the parent to the child process as
Parcelable, and we use the GlobalDescriptors class to register them.

TEST=Run the mojo_system_unittests
BUG=666356
Review-Url: https://codereview.chromium.org/2549363004
Cr-Commit-Position: refs/heads/master@{#441799}
Committed: https://chromium.googlesource.com/chromium/src/+/5513e4622febbc838dfebddb51347ab797747836

[Jay Civelli, 2016-12-07 19:08:57]

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

[commit-bot: I haz the power, 2016-12-07 19:09:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Jay Civelli, 2016-12-07 19:19:38]

Description was changed from

==========
Multiprocess test client: Android child process launcher rework.

Changing the way test start child processese on Android.
Forking natively should not be used on Andoid, instead we should start
the child process through the Android framework, which requires going
through Java and using services.

BUG=666356
==========

to

==========
Multiprocess test client: Android child process launcher rework.

Changing the way test start child processese on Android.
Forking natively should not be used on Andoid, instead child processes
should be started through the Android framework, which requires going
to Java and using Android services.

For that we introduce the MultiprocessTestClientLauncher java class
which starts a service in a new process. We provide several
MultiprocessTestClientService service classes to ensure we can have
multiple child processes.
File descriptor are passed from the parent to the child process as
Parcelable, and we use the GlobalDescriptors class to register them.

TEST=Run the mojo_system_unittests
BUG=666356
==========

[Jay Civelli, 2016-12-07 19:21:50]

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

[commit-bot: I haz the power, 2016-12-07 19:25:45]

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

[commit-bot: I haz the power, 2016-12-07 19:25:46]

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

[Jay Civelli, 2016-12-07 19:33:11]

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

[commit-bot: I haz the power, 2016-12-07 19:33:46]

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, 2016-12-07 20:19:32]

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

[commit-bot: I haz the power, 2016-12-07 20:19:33]

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

[Jay Civelli, 2016-12-09 22:35:22]

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

[commit-bot: I haz the power, 2016-12-09 22:36:18]

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, 2016-12-09 22:45:36]

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

[commit-bot: I haz the power, 2016-12-09 22:45:37]

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

[Jay Civelli, 2016-12-09 22:51:31]

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

[commit-bot: I haz the power, 2016-12-09 22:52: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, 2016-12-09 23:49:42]

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

[commit-bot: I haz the power, 2016-12-09 23:49:43]

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

[Jay Civelli, 2016-12-12 20:14:59]

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

[Jay Civelli, 2016-12-12 20:15:40]

Last failing tests should be fixed.
Ready for review.

[commit-bot: I haz the power, 2016-12-12 20:16:34]

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, 2016-12-12 21:21:47]

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

[commit-bot: I haz the power, 2016-12-12 21:21:48]

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

[Ken Rockot(use gerrit already), 2016-12-12 22:23:08]

Looks great, but we'll need to do something different for the child process
entry point.

https://codereview.chromium.org/2549363004/diff/140001/base/test/android/java...
File
base/test/android/java/src/org/chromium/base/MultiprocessTestClientLauncher.java
(right):

https://codereview.chromium.org/2549363004/diff/140001/base/test/android/java...
base/test/android/java/src/org/chromium/base/MultiprocessTestClientLauncher.java:33:
// Not supposed to be instanciated.
nit: instantiated

https://codereview.chromium.org/2549363004/diff/140001/mojo/edk/embedder/plat...
File mojo/edk/embedder/platform_channel_pair_android.cc (right):

https://codereview.chromium.org/2549363004/diff/140001/mojo/edk/embedder/plat...
mojo/edk/embedder/platform_channel_pair_android.cc:1: // Copyright 2016 The
Chromium Authors. All rights reserved.
nit: I think it might be less confusing to just lump this code in the _posix.cc
source when !defined(OS_ANDROID)

https://codereview.chromium.org/2549363004/diff/140001/mojo/edk/test/run_all_...
File mojo/edk/test/run_all_unittests.cc (right):

https://codereview.chromium.org/2549363004/diff/140001/mojo/edk/test/run_all_...
mojo/edk/test/run_all_unittests.cc:26: base::android::AndroidMainFunctionThunk
main_thunk(&main);
Unfortunately I'm reminded that use of non-constexpr static initializers is
explicitly forbidden by the style guide, and there's no way for this constructor
to be constexpr. Maybe someone has a more clever idea for injecting an entry
point without forcing an extra link-time requirement on all dependents. e.g. it
would be lame if we declared something like base::android::AndroidTestClientMain
and required all targets to define it.

https://codereview.chromium.org/2549363004/diff/140001/testing/android/native...
File testing/android/native_test/native_test_launcher.cc (right):

https://codereview.chromium.org/2549363004/diff/140001/testing/android/native...
testing/android/native_test/native_test_launcher.cc:128: main(argc, &(argv[0]));
nit: unnecessary

[Jay Civelli, 2016-12-13 18:03:14]

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

[commit-bot: I haz the power, 2016-12-13 18:03:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Jay Civelli, 2016-12-13 18:23:26]

https://codereview.chromium.org/2549363004/diff/140001/base/test/android/java...
File
base/test/android/java/src/org/chromium/base/MultiprocessTestClientLauncher.java
(right):

https://codereview.chromium.org/2549363004/diff/140001/base/test/android/java...
base/test/android/java/src/org/chromium/base/MultiprocessTestClientLauncher.java:33:
// Not supposed to be instanciated.
On 2016/12/12 22:23:08, Ken Rockot wrote:
> nit: instantiated

Done.

https://codereview.chromium.org/2549363004/diff/140001/mojo/edk/embedder/plat...
File mojo/edk/embedder/platform_channel_pair_android.cc (right):

https://codereview.chromium.org/2549363004/diff/140001/mojo/edk/embedder/plat...
mojo/edk/embedder/platform_channel_pair_android.cc:1: // Copyright 2016 The
Chromium Authors. All rights reserved.
On 2016/12/12 22:23:08, Ken Rockot wrote:
> nit: I think it might be less confusing to just lump this code in the
_posix.cc
> source when !defined(OS_ANDROID)

Done.

https://codereview.chromium.org/2549363004/diff/140001/mojo/edk/test/run_all_...
File mojo/edk/test/run_all_unittests.cc (right):

https://codereview.chromium.org/2549363004/diff/140001/mojo/edk/test/run_all_...
mojo/edk/test/run_all_unittests.cc:26: base::android::AndroidMainFunctionThunk
main_thunk(&main);
On 2016/12/12 22:23:08, Ken Rockot wrote:
> Unfortunately I'm reminded that use of non-constexpr static initializers is
> explicitly forbidden by the style guide, and there's no way for this
constructor
> to be constexpr. Maybe someone has a more clever idea for injecting an entry
> point without forcing an extra link-time requirement on all dependents. e.g.
it
> would be lame if we declared something like
base::android::AndroidTestClientMain
> and required all targets to define it.

Per our discussion, I created a main_stub and made the few targets that depend
on base:test_support but don't have a main depend on it.

https://codereview.chromium.org/2549363004/diff/140001/testing/android/native...
File testing/android/native_test/native_test_launcher.cc (right):

https://codereview.chromium.org/2549363004/diff/140001/testing/android/native...
testing/android/native_test/native_test_launcher.cc:128: main(argc, &(argv[0]));
On 2016/12/12 22:23:08, Ken Rockot wrote:
> nit: unnecessary

Done.

[Ken Rockot(use gerrit already), 2016-12-13 18:36:17]

LGTM. I'll leave it up to a base owner to decide whether this main stub thing is
better or worse than the style-guide-violating static initializer approach from
before. I would personally prefer the latter, but, ya know... style. :P

https://codereview.chromium.org/2549363004/diff/180001/base/test/main_stub.cc
File base/test/main_stub.cc (right):

https://codereview.chromium.org/2549363004/diff/180001/base/test/main_stub.cc...
base/test/main_stub.cc:6: return 1;
nit: Maybe this should also NOTREACHED()

https://codereview.chromium.org/2549363004/diff/180001/mojo/edk/embedder/plat...
File mojo/edk/embedder/platform_channel_pair.h (right):

https://codereview.chromium.org/2549363004/diff/180001/mojo/edk/embedder/plat...
mojo/edk/embedder/platform_channel_pair.h:72: // TODO(b/670106): remove the
command_line param.
nit: Please use chromium TODO syntax, i.e. TODO(jcivelli): Blah.
http://crbug.com/670106. You can also TODO(jcivelli/rockot) if you like.

[Ken Rockot(use gerrit already), 2016-12-13 18:36:32]

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

[commit-bot: I haz the power, 2016-12-13 18:37:13]

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, 2016-12-13 19:55:16]

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

[commit-bot: I haz the power, 2016-12-13 19:55:17]

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

[nyquist, 2016-12-13 20:40:32]

https://codereview.chromium.org/2549363004/diff/180001/base/test/android/java...
File base/test/android/java/src/org/chromium/base/FileDescriptorInfo.java
(right):

https://codereview.chromium.org/2549363004/diff/180001/base/test/android/java...
base/test/android/java/src/org/chromium/base/FileDescriptorInfo.java:19: public
final int mId;
Nit: Could you describe what the ID is for?

https://codereview.chromium.org/2549363004/diff/180001/base/test/android/java...
base/test/android/java/src/org/chromium/base/FileDescriptorInfo.java:20: public
final ParcelFileDescriptor mFd;
Nit: Public fields should not have the m-prefix I believe.

( See:
https://source.android.com/source/code-style.html#follow-field-naming-convent...
)

https://codereview.chromium.org/2549363004/diff/180001/base/test/android/java...
File base/test/android/java/src/org/chromium/base/ITestClient.aidl (right):

https://codereview.chromium.org/2549363004/diff/180001/base/test/android/java...
base/test/android/java/src/org/chromium/base/ITestClient.aidl:14: int launch(in
String[] args, in FileDescriptorInfo[] fdsToMap);
Could you explain what the return value is here? I thought at first it was some
error-code, but then it turns out it's the PID.

I guess at that point it might look weird not to document the other methods as
well.

https://codereview.chromium.org/2549363004/diff/180001/base/test/android/java...
base/test/android/java/src/org/chromium/base/ITestClient.aidl:16: boolean
forceStopSynchronous(int exitCode);
What does the return value signify here? It seems like we're always returning
true. Will this change going forward?

https://codereview.chromium.org/2549363004/diff/180001/base/test/android/java...
File base/test/android/java/src/org/chromium/base/MainReturnCodeResult.java
(right):

https://codereview.chromium.org/2549363004/diff/180001/base/test/android/java...
base/test/android/java/src/org/chromium/base/MainReturnCodeResult.java:16: 
Nit: remove empty line

https://codereview.chromium.org/2549363004/diff/180001/base/test/android/java...
File
base/test/android/java/src/org/chromium/base/MultiprocessTestClientLauncher.java
(right):

https://codereview.chromium.org/2549363004/diff/180001/base/test/android/java...
base/test/android/java/src/org/chromium/base/MultiprocessTestClientLauncher.java:28:
public class MultiprocessTestClientLauncher {
Make class final since it's not supposed to be instantiated, and only contains
static methods.

https://codereview.chromium.org/2549363004/diff/180001/base/test/android/java...
base/test/android/java/src/org/chromium/base/MultiprocessTestClientLauncher.java:29:
private static final String TAG = "MultiprocTCLauncher";
Nit: cr_ prefix

https://codereview.chromium.org/2549363004/diff/180001/base/test/android/java...
base/test/android/java/src/org/chromium/base/MultiprocessTestClientLauncher.java:46:
// You must hold mLock to access any member variable below:
@GuardedBy("mLock") annotation for both?

https://codereview.chromium.org/2549363004/diff/180001/base/test/android/java...
base/test/android/java/src/org/chromium/base/MultiprocessTestClientLauncher.java:63:
mLock.wait();
This does not support spurious wake-ups. Do we want to support that maybe?

https://codereview.chromium.org/2549363004/diff/180001/base/test/android/java...
base/test/android/java/src/org/chromium/base/MultiprocessTestClientLauncher.java:102:
private boolean mConnected;
@GuardedBy("mConnectedLock") ?

https://codereview.chromium.org/2549363004/diff/180001/base/test/android/java...
base/test/android/java/src/org/chromium/base/MultiprocessTestClientLauncher.java:103:
private final Object mConnectedLock = new Object();
Nit: Could you order the finals before the non-finals? Makes it simpler to see
what will change over time.

https://codereview.chromium.org/2549363004/diff/180001/base/test/android/java...
base/test/android/java/src/org/chromium/base/MultiprocessTestClientLauncher.java:117:
mConnectedLock.wait();
Will this be waiting basically until the test itself times out, in case
something's wrong?

https://codereview.chromium.org/2549363004/diff/180001/base/test/android/java...
base/test/android/java/src/org/chromium/base/MultiprocessTestClientLauncher.java:152:
public String getServiceClassName() {
This deserves a comment, referring to the different files.

https://codereview.chromium.org/2549363004/diff/180001/base/test/android/java...
base/test/android/java/src/org/chromium/base/MultiprocessTestClientLauncher.java:159:
public int getSlot() {
Nit: empty line spacing between methods

https://codereview.chromium.org/2549363004/diff/180001/base/test/android/java...
base/test/android/java/src/org/chromium/base/MultiprocessTestClientLauncher.java:244:
connection.getService().forceStopSynchronous(exitCode);
This returns a boolean, and I think this might be the only caller. Should it not
be a boolean, or should the return value be stored? Also, should you just return
the result here, and return true in the else-statement?

https://codereview.chromium.org/2549363004/diff/180001/base/test/android/java...
base/test/android/java/src/org/chromium/base/MultiprocessTestClientLauncher.java:250:
// System.exits.
Nit: "System.exit()"

https://codereview.chromium.org/2549363004/diff/180001/base/test/android/java...
File
base/test/android/java/src/org/chromium/base/MultiprocessTestClientService.java
(right):

https://codereview.chromium.org/2549363004/diff/180001/base/test/android/java...
base/test/android/java/src/org/chromium/base/MultiprocessTestClientService.java:23:
private static final String TAG = "TestClient";
Nit: cr_ prefix these days I think.

https://codereview.chromium.org/2549363004/diff/180001/base/test/android/java...
base/test/android/java/src/org/chromium/base/MultiprocessTestClientService.java:28:
private MainReturnCodeResult mResult;
@GuardedBy("mResultLock") ?

https://codereview.chromium.org/2549363004/diff/180001/base/test/android/java...
base/test/android/java/src/org/chromium/base/MultiprocessTestClientService.java:56:
mResultLock.wait(timeoutMs);
This doesn't handle spurious wake-ups. Do you want to add support for that?

https://codereview.chromium.org/2549363004/diff/180001/base/test/android/java...
base/test/android/java/src/org/chromium/base/MultiprocessTestClientService.java:80:
static boolean sAlreadyInitialized = false;
Nit: Could this static be at the top instead? 
Maybe an order on along the lines of:
1) static final
2) static
3) final
4) No qualifier

https://codereview.chromium.org/2549363004/diff/180001/base/test/android/java...
base/test/android/java/src/org/chromium/base/MultiprocessTestClientService.java:82:
@SuppressFBWarnings({"ST_WRITE_TO_STATIC_FROM_INSTANCE_METHOD", "DM_EXIT"})
Do you really need to suppress the write-to-static? Could you just extract a
static method that does the writing instead and call it from there?

https://codereview.chromium.org/2549363004/diff/180001/base/test/android/java...
base/test/android/java/src/org/chromium/base/MultiprocessTestClientService.java:94:
System.exit(0);
I think we usually add an extra return-call after this.

https://codereview.chromium.org/2549363004/diff/180001/base/test/android/java...
base/test/android/java/src/org/chromium/base/MultiprocessTestClientService.java:104:
Log.e(TAG, "onCreate");
Is this necessary as an error? (same below with onDestroy)
It's fine with me to keep it, but maybe not error? Feels more like verbose or
debug to me. Also looks like one of those "forgot to delete before
upload"-trickses.

https://codereview.chromium.org/2549363004/diff/180001/base/test/android/java...
base/test/android/java/src/org/chromium/base/MultiprocessTestClientService.java:109:
Log.e(TAG, "onDestroy");
If the log is removed, this just calls super, so I guess if you remove the log,
also remove this method.

https://codereview.chromium.org/2549363004/diff/180001/base/test/android/java...
base/test/android/java/src/org/chromium/base/MultiprocessTestClientService.java:116:
stopSelf();
From what I remember from services, they can be started or bound to (or both, in
rare cases). It seems that the pattern here is to use binding. Why are we then
stopping the service, if there were no calls to startService()?

https://codereview.chromium.org/2549363004/diff/180001/base/test/android/java...
base/test/android/java/src/org/chromium/base/MultiprocessTestClientService.java:121:
private void loadLibraries() {
What's the reason for this not using the LibraryLoader in //base? Could we maybe
clarify why as a comment?

https://codereview.chromium.org/2549363004/diff/180001/base/test/android/java...
base/test/android/java/src/org/chromium/base/MultiprocessTestClientService.java:123:
Log.i(TAG, "loading: %s", library);
Do we need this? If so, uppercase L?

https://codereview.chromium.org/2549363004/diff/180001/base/test/android/mult...
File base/test/android/multiprocess_test_client_service.cc (right):

https://codereview.chromium.org/2549363004/diff/180001/base/test/android/mult...
base/test/android/multiprocess_test_client_service.cc:43: DCHECK(fd_ids.size()
== fd_fds.size());
CHECK? You only use fd_ids.size() below, so if this DCHECK fails, you might go
out of bounds below.

https://codereview.chromium.org/2549363004/diff/180001/base/test/multiprocess...
File base/test/multiprocess_test_android.cc (right):

https://codereview.chromium.org/2549363004/diff/180001/base/test/multiprocess...
base/test/multiprocess_test_android.cc:27: // NOTE: This MUST NOT run on the
main thread of the NativeTest application.
Could this be tested in code as well?

https://codereview.chromium.org/2549363004/diff/180001/base/test/multiprocess...
base/test/multiprocess_test_android.cc:74: if (exit_code)
Nit: any reason why the new code is not consistent in using curlies or not?
Like "if (!command_line.HasSwitch(switches::kTestChildProcess))" above.

https://codereview.chromium.org/2549363004/diff/180001/base/test/test_support...
File base/test/test_support_android.cc (left):

https://codereview.chromium.org/2549363004/diff/180001/base/test/test_support...
base/test/test_support_android.cc:163: if (base::AndroidIsChildProcess())
Are there other places where we can remove checks like this now?

https://codereview.chromium.org/2549363004/diff/180001/testing/android/native...
File testing/android/native_test/java/AndroidManifest.xml.jinja2 (right):

https://codereview.chromium.org/2549363004/diff/180001/testing/android/native...
testing/android/native_test/java/AndroidManifest.xml.jinja2:51: {% set
num_test_client_services = 5 %}
Could you refer to the class names here to ensure if people change this, they'd
also add a new file?

[Jay Civelli, 2016-12-14 02:20:12]

https://codereview.chromium.org/2549363004/diff/180001/base/test/android/java...
File base/test/android/java/src/org/chromium/base/FileDescriptorInfo.java
(right):

https://codereview.chromium.org/2549363004/diff/180001/base/test/android/java...
base/test/android/java/src/org/chromium/base/FileDescriptorInfo.java:19: public
final int mId;
On 2016/12/13 20:40:30, nyquist wrote:
> Nit: Could you describe what the ID is for?

Done.

https://codereview.chromium.org/2549363004/diff/180001/base/test/android/java...
base/test/android/java/src/org/chromium/base/FileDescriptorInfo.java:20: public
final ParcelFileDescriptor mFd;
On 2016/12/13 20:40:30, nyquist wrote:
> Nit: Public fields should not have the m-prefix I believe.
> 
> ( See:
>
https://source.android.com/source/code-style.html#follow-field-naming-convent...
> )

Done.

https://codereview.chromium.org/2549363004/diff/180001/base/test/android/java...
File base/test/android/java/src/org/chromium/base/ITestClient.aidl (right):

https://codereview.chromium.org/2549363004/diff/180001/base/test/android/java...
base/test/android/java/src/org/chromium/base/ITestClient.aidl:14: int launch(in
String[] args, in FileDescriptorInfo[] fdsToMap);
On 2016/12/13 20:40:30, nyquist wrote:
> Could you explain what the return value is here? I thought at first it was
some
> error-code, but then it turns out it's the PID.
> 
> I guess at that point it might look weird not to document the other methods as
> well.

Sorry, I had forgotten to comment that file entirely. Should be better now.

https://codereview.chromium.org/2549363004/diff/180001/base/test/android/java...
base/test/android/java/src/org/chromium/base/ITestClient.aidl:16: boolean
forceStopSynchronous(int exitCode);
On 2016/12/13 20:40:30, nyquist wrote:
> What does the return value signify here? It seems like we're always returning
> true. Will this change going forward?

I explained in the comment.

https://codereview.chromium.org/2549363004/diff/180001/base/test/android/java...
File base/test/android/java/src/org/chromium/base/MainReturnCodeResult.java
(right):

https://codereview.chromium.org/2549363004/diff/180001/base/test/android/java...
base/test/android/java/src/org/chromium/base/MainReturnCodeResult.java:16: 
On 2016/12/13 20:40:30, nyquist wrote:
> Nit: remove empty line

Done.

https://codereview.chromium.org/2549363004/diff/180001/base/test/android/java...
File
base/test/android/java/src/org/chromium/base/MultiprocessTestClientLauncher.java
(right):

https://codereview.chromium.org/2549363004/diff/180001/base/test/android/java...
base/test/android/java/src/org/chromium/base/MultiprocessTestClientLauncher.java:28:
public class MultiprocessTestClientLauncher {
On 2016/12/13 20:40:30, nyquist wrote:
> Make class final since it's not supposed to be instantiated, and only contains
> static methods.

Done.

https://codereview.chromium.org/2549363004/diff/180001/base/test/android/java...
base/test/android/java/src/org/chromium/base/MultiprocessTestClientLauncher.java:29:
private static final String TAG = "MultiprocTCLauncher";
On 2016/12/13 20:40:30, nyquist wrote:
> Nit: cr_ prefix

Is that an official rule? I can't seem to find it in the style guide and for 513
tags defined in the source tree only 86 have the cr prefix.
(I am asking because these tags are limited to 20 chars and that takes 3 out of
them)

https://codereview.chromium.org/2549363004/diff/180001/base/test/android/java...
base/test/android/java/src/org/chromium/base/MultiprocessTestClientLauncher.java:46:
// You must hold mLock to access any member variable below:
On 2016/12/13 20:40:30, nyquist wrote:
> @GuardedBy("mLock") annotation for both?

Ah, awesome! I was looking for that at some point.

https://codereview.chromium.org/2549363004/diff/180001/base/test/android/java...
base/test/android/java/src/org/chromium/base/MultiprocessTestClientLauncher.java:63:
mLock.wait();
On 2016/12/13 20:40:30, nyquist wrote:
> This does not support spurious wake-ups. Do we want to support that maybe?

Remove the return so now it should.

https://codereview.chromium.org/2549363004/diff/180001/base/test/android/java...
base/test/android/java/src/org/chromium/base/MultiprocessTestClientLauncher.java:102:
private boolean mConnected;
On 2016/12/13 20:40:31, nyquist wrote:
> @GuardedBy("mConnectedLock") ?

Done.

https://codereview.chromium.org/2549363004/diff/180001/base/test/android/java...
base/test/android/java/src/org/chromium/base/MultiprocessTestClientLauncher.java:103:
private final Object mConnectedLock = new Object();
On 2016/12/13 20:40:30, nyquist wrote:
> Nit: Could you order the finals before the non-finals? Makes it simpler to see
> what will change over time.

Done.

https://codereview.chromium.org/2549363004/diff/180001/base/test/android/java...
base/test/android/java/src/org/chromium/base/MultiprocessTestClientLauncher.java:117:
mConnectedLock.wait();
On 2016/12/13 20:40:30, nyquist wrote:
> Will this be waiting basically until the test itself times out, in case
> something's wrong?

If we failed to bind, this won't be called. So the window is if the service
crashes before we get connected.
If that happen the framework would restart the service I believe. And if that
still failed, the test would indeed timeout.

https://codereview.chromium.org/2549363004/diff/180001/base/test/android/java...
base/test/android/java/src/org/chromium/base/MultiprocessTestClientLauncher.java:152:
public String getServiceClassName() {
On 2016/12/13 20:40:31, nyquist wrote:
> This deserves a comment, referring to the different files.

Done.

https://codereview.chromium.org/2549363004/diff/180001/base/test/android/java...
base/test/android/java/src/org/chromium/base/MultiprocessTestClientLauncher.java:159:
public int getSlot() {
On 2016/12/13 20:40:30, nyquist wrote:
> Nit: empty line spacing between methods

Done.

https://codereview.chromium.org/2549363004/diff/180001/base/test/android/java...
base/test/android/java/src/org/chromium/base/MultiprocessTestClientLauncher.java:244:
connection.getService().forceStopSynchronous(exitCode);
On 2016/12/13 20:40:30, nyquist wrote:
> This returns a boolean, and I think this might be the only caller. Should it
not
> be a boolean, or should the return value be stored? Also, should you just
return
> the result here, and return true in the else-statement?

Having a return value in forceStopSynchronous() is just a hack to force the
method to be synchronous.

https://codereview.chromium.org/2549363004/diff/180001/base/test/android/java...
base/test/android/java/src/org/chromium/base/MultiprocessTestClientLauncher.java:250:
// System.exits.
On 2016/12/13 20:40:30, nyquist wrote:
> Nit: "System.exit()"

Done.

https://codereview.chromium.org/2549363004/diff/180001/base/test/android/java...
File
base/test/android/java/src/org/chromium/base/MultiprocessTestClientService.java
(right):

https://codereview.chromium.org/2549363004/diff/180001/base/test/android/java...
base/test/android/java/src/org/chromium/base/MultiprocessTestClientService.java:23:
private static final String TAG = "TestClient";
On 2016/12/13 20:40:31, nyquist wrote:
> Nit: cr_ prefix these days I think.

OK, I'll do it...

https://codereview.chromium.org/2549363004/diff/180001/base/test/android/java...
base/test/android/java/src/org/chromium/base/MultiprocessTestClientService.java:28:
private MainReturnCodeResult mResult;
On 2016/12/13 20:40:31, nyquist wrote:
> @GuardedBy("mResultLock") ?

Done.

https://codereview.chromium.org/2549363004/diff/180001/base/test/android/java...
base/test/android/java/src/org/chromium/base/MultiprocessTestClientService.java:56:
mResultLock.wait(timeoutMs);
On 2016/12/13 20:40:31, nyquist wrote:
> This doesn't handle spurious wake-ups. Do you want to add support for that?

Good call.

https://codereview.chromium.org/2549363004/diff/180001/base/test/android/java...
base/test/android/java/src/org/chromium/base/MultiprocessTestClientService.java:80:
static boolean sAlreadyInitialized = false;
On 2016/12/13 20:40:31, nyquist wrote:
> Nit: Could this static be at the top instead? 
> Maybe an order on along the lines of:
> 1) static final
> 2) static
> 3) final
> 4) No qualifier

Done.

https://codereview.chromium.org/2549363004/diff/180001/base/test/android/java...
base/test/android/java/src/org/chromium/base/MultiprocessTestClientService.java:82:
@SuppressFBWarnings({"ST_WRITE_TO_STATIC_FROM_INSTANCE_METHOD", "DM_EXIT"})
On 2016/12/13 20:40:31, nyquist wrote:
> Do you really need to suppress the write-to-static? Could you just extract a
> static method that does the writing instead and call it from there?

Done.

https://codereview.chromium.org/2549363004/diff/180001/base/test/android/java...
base/test/android/java/src/org/chromium/base/MultiprocessTestClientService.java:94:
System.exit(0);
On 2016/12/13 20:40:31, nyquist wrote:
> I think we usually add an extra return-call after this.

Done.

https://codereview.chromium.org/2549363004/diff/180001/base/test/android/java...
base/test/android/java/src/org/chromium/base/MultiprocessTestClientService.java:104:
Log.e(TAG, "onCreate");
On 2016/12/13 20:40:31, nyquist wrote:
> Is this necessary as an error? (same below with onDestroy)
> It's fine with me to keep it, but maybe not error? Feels more like verbose or
> debug to me. Also looks like one of those "forgot to delete before
> upload"-trickses.

Yeah, removed, debug stuff that ends up polluting everyone's logcat.

https://codereview.chromium.org/2549363004/diff/180001/base/test/android/java...
base/test/android/java/src/org/chromium/base/MultiprocessTestClientService.java:109:
Log.e(TAG, "onDestroy");
On 2016/12/13 20:40:31, nyquist wrote:
> If the log is removed, this just calls super, so I guess if you remove the
log,
> also remove this method.

Done.

https://codereview.chromium.org/2549363004/diff/180001/base/test/android/java...
base/test/android/java/src/org/chromium/base/MultiprocessTestClientService.java:116:
stopSelf();
On 2016/12/13 20:40:31, nyquist wrote:
> From what I remember from services, they can be started or bound to (or both,
in
> rare cases). It seems that the pattern here is to use binding. Why are we then
> stopping the service, if there were no calls to startService()?

You are right, I don't think this is needed.

https://codereview.chromium.org/2549363004/diff/180001/base/test/android/java...
base/test/android/java/src/org/chromium/base/MultiprocessTestClientService.java:121:
private void loadLibraries() {
On 2016/12/13 20:40:31, nyquist wrote:
> What's the reason for this not using the LibraryLoader in //base? Could we
maybe
> clarify why as a comment?

None, now doing so.

https://codereview.chromium.org/2549363004/diff/180001/base/test/android/java...
base/test/android/java/src/org/chromium/base/MultiprocessTestClientService.java:123:
Log.i(TAG, "loading: %s", library);
On 2016/12/13 20:40:31, nyquist wrote:
> Do we need this? If so, uppercase L?

This is now gone.

https://codereview.chromium.org/2549363004/diff/180001/base/test/android/mult...
File base/test/android/multiprocess_test_client_service.cc (right):

https://codereview.chromium.org/2549363004/diff/180001/base/test/android/mult...
base/test/android/multiprocess_test_client_service.cc:43: DCHECK(fd_ids.size()
== fd_fds.size());
On 2016/12/13 20:40:31, nyquist wrote:
> CHECK? You only use fd_ids.size() below, so if this DCHECK fails, you might go
> out of bounds below.

Done.

https://codereview.chromium.org/2549363004/diff/180001/base/test/main_stub.cc
File base/test/main_stub.cc (right):

https://codereview.chromium.org/2549363004/diff/180001/base/test/main_stub.cc...
base/test/main_stub.cc:6: return 1;
On 2016/12/13 18:36:17, Ken Rockot wrote:
> nit: Maybe this should also NOTREACHED()

Good point, done.

https://codereview.chromium.org/2549363004/diff/180001/base/test/multiprocess...
File base/test/multiprocess_test_android.cc (right):

https://codereview.chromium.org/2549363004/diff/180001/base/test/multiprocess...
base/test/multiprocess_test_android.cc:27: // NOTE: This MUST NOT run on the
main thread of the NativeTest application.
On 2016/12/13 20:40:31, nyquist wrote:
> Could this be tested in code as well?
MultiprocessTestClientLauncher.launchClient() does throw a RuntimeException if
called on the main thread.

https://codereview.chromium.org/2549363004/diff/180001/base/test/multiprocess...
base/test/multiprocess_test_android.cc:74: if (exit_code)
On 2016/12/13 20:40:31, nyquist wrote:
> Nit: any reason why the new code is not consistent in using curlies or not?
> Like "if (!command_line.HasSwitch(switches::kTestChildProcess))" above.

Done.

https://codereview.chromium.org/2549363004/diff/180001/base/test/test_support...
File base/test/test_support_android.cc (left):

https://codereview.chromium.org/2549363004/diff/180001/base/test/test_support...
base/test/test_support_android.cc:163: if (base::AndroidIsChildProcess())
On 2016/12/13 20:40:31, nyquist wrote:
> Are there other places where we can remove checks like this now?

base::AndroidIsChildProcess() is gone, so the compiler would tell us.

https://codereview.chromium.org/2549363004/diff/180001/mojo/edk/embedder/plat...
File mojo/edk/embedder/platform_channel_pair.h (right):

https://codereview.chromium.org/2549363004/diff/180001/mojo/edk/embedder/plat...
mojo/edk/embedder/platform_channel_pair.h:72: // TODO(b/670106): remove the
command_line param.
On 2016/12/13 18:36:17, Ken Rockot wrote:
> nit: Please use chromium TODO syntax, i.e. TODO(jcivelli): Blah.
> http://crbug.com/670106. You can also TODO(jcivelli/rockot) if you like.

Done.

https://codereview.chromium.org/2549363004/diff/180001/testing/android/native...
File testing/android/native_test/java/AndroidManifest.xml.jinja2 (right):

https://codereview.chromium.org/2549363004/diff/180001/testing/android/native...
testing/android/native_test/java/AndroidManifest.xml.jinja2:51: {% set
num_test_client_services = 5 %}
On 2016/12/13 20:40:31, nyquist wrote:
> Could you refer to the class names here to ensure if people change this,
they'd
> also add a new file?

Done.

[Jay Civelli, 2016-12-14 02:20:22]

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

[commit-bot: I haz the power, 2016-12-14 02:21:03]

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, 2016-12-14 02:28:19]

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

[commit-bot: I haz the power, 2016-12-14 02:28:20]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[Jay Civelli, 2016-12-14 06:08:02]

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

[commit-bot: I haz the power, 2016-12-14 06:08:26]

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, 2016-12-14 07:44:46]

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

[commit-bot: I haz the power, 2016-12-14 07:44:47]

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

[Jay Civelli, 2016-12-14 16:45:08]

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

[commit-bot: I haz the power, 2016-12-14 16:45:38]

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, 2016-12-14 19:04:06]

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

[commit-bot: I haz the power, 2016-12-14 19:04:07]

Dry run: This issue passed the CQ dry run.

[nyquist, 2016-12-14 23:26:18]

lgtm

https://codereview.chromium.org/2549363004/diff/180001/base/test/android/java...
File
base/test/android/java/src/org/chromium/base/MultiprocessTestClientLauncher.java
(right):

https://codereview.chromium.org/2549363004/diff/180001/base/test/android/java...
base/test/android/java/src/org/chromium/base/MultiprocessTestClientLauncher.java:29:
private static final String TAG = "MultiprocTCLauncher";
On 2016/12/14 02:20:10, Jay Civelli wrote:
> On 2016/12/13 20:40:30, nyquist wrote:
> > Nit: cr_ prefix
> 
> Is that an official rule? I can't seem to find it in the style guide and for
513
> tags defined in the source tree only 86 have the cr prefix.
> (I am asking because these tags are limited to 20 chars and that takes 3 out
of
> them)

You can read the Android Logging documentation here:
https://chromium.googlesource.com/chromium/src/+/master/docs/android_logging.md

If you down add the cr_ prefix yourself, it is done automatically by String
concatenation on-the-fly instead, which seems unnecessary. About the String
length, I believe that cropped after the concatenation, so it doesn't matter
whether you do it in code or here I think.

The code for this in Log.java is here:
https://cs.chromium.org/chromium/src/base/android/java/src/org/chromium/base/...

https://codereview.chromium.org/2549363004/diff/180001/base/test/android/java...
base/test/android/java/src/org/chromium/base/MultiprocessTestClientLauncher.java:46:
// You must hold mLock to access any member variable below:
On 2016/12/14 02:20:10, Jay Civelli wrote:
> On 2016/12/13 20:40:30, nyquist wrote:
> > @GuardedBy("mLock") annotation for both?
> 
> Ah, awesome! I was looking for that at some point.

And would you look at that... It found a bug :-)

https://codereview.chromium.org/2549363004/diff/240001/base/test/android/java...
File
base/test/android/java/src/org/chromium/base/MultiprocessTestClientLauncher.java
(right):

https://codereview.chromium.org/2549363004/diff/240001/base/test/android/java...
base/test/android/java/src/org/chromium/base/MultiprocessTestClientLauncher.java:143:
public void onServiceDisconnected(ComponentName className) {
Should this clear mService? Or are we assuming since that we free ourselves, it
doesn't matter what we are holding on to?

https://codereview.chromium.org/2549363004/diff/240001/base/test/android/java...
File
base/test/android/java/src/org/chromium/base/MultiprocessTestClientService.java
(right):

https://codereview.chromium.org/2549363004/diff/240001/base/test/android/java...
base/test/android/java/src/org/chromium/base/MultiprocessTestClientService.java:45:
fdFds[i] = fdsToMap[i].fd.detachFd();
Do we want to explicitly mention that this takes ownership of the native file
descriptor here? Or do we not want to comment since it originated from our own
native code?

https://codereview.chromium.org/2549363004/diff/240001/mojo/edk/embedder/plat...
File mojo/edk/embedder/platform_channel_pair.h (right):

https://codereview.chromium.org/2549363004/diff/240001/mojo/edk/embedder/plat...
mojo/edk/embedder/platform_channel_pair.h:72: // TODO(jcivelli): remove the
command_line param. http://b/670106
Are you sure this URL points to the right place? Could it be that you meant
crbug.com/670106?

[Jay Civelli, 2016-12-15 01:28:14]

https://codereview.chromium.org/2549363004/diff/240001/base/test/android/java...
File
base/test/android/java/src/org/chromium/base/MultiprocessTestClientLauncher.java
(right):

https://codereview.chromium.org/2549363004/diff/240001/base/test/android/java...
base/test/android/java/src/org/chromium/base/MultiprocessTestClientLauncher.java:143:
public void onServiceDisconnected(ComponentName className) {
On 2016/12/14 23:26:18, nyquist (OOO back 1-3) wrote:
> Should this clear mService? Or are we assuming since that we free ourselves,
it
> doesn't matter what we are holding on to?

Yes, once freed this instance should be going away, with everything it holds.

https://codereview.chromium.org/2549363004/diff/240001/base/test/android/java...
File
base/test/android/java/src/org/chromium/base/MultiprocessTestClientService.java
(right):

https://codereview.chromium.org/2549363004/diff/240001/base/test/android/java...
base/test/android/java/src/org/chromium/base/MultiprocessTestClientService.java:45:
fdFds[i] = fdsToMap[i].fd.detachFd();
On 2016/12/14 23:26:18, nyquist (OOO back 1-3) wrote:
> Do we want to explicitly mention that this takes ownership of the native file
> descriptor here? Or do we not want to comment since it originated from our own
> native code?

Done.

https://codereview.chromium.org/2549363004/diff/240001/mojo/edk/embedder/plat...
File mojo/edk/embedder/platform_channel_pair.h (right):

https://codereview.chromium.org/2549363004/diff/240001/mojo/edk/embedder/plat...
mojo/edk/embedder/platform_channel_pair.h:72: // TODO(jcivelli): remove the
command_line param. http://b/670106
On 2016/12/14 23:26:18, nyquist (OOO back 1-3) wrote:
> Are you sure this URL points to the right place? Could it be that you meant
> crbug.com/670106?

Good catch, done.

[Jay Civelli, 2016-12-15 01:36:16]

jcivelli@chromium.org changed reviewers:
+ boliu@chromium.org, dpranke@chromium.org, sdefresne@chromium.org,
thakis@chromium.org

[Jay Civelli, 2016-12-15 01:36:17]

Adding OWNER reviewers:
boliu@ could you look at android_webview/test/BUILD.gn
thakis@ could you look at base/ (base/test has already been OWNER reviewed by
nyquist@)
dpranke@ could you look at testing/ and
	build/android/pylib/gtest/gtest_test_instance.py
sdefresne@ could you look at components/test_runner/BUILD.gn

[boliu, 2016-12-15 01:43:02]

https://codereview.chromium.org/2549363004/diff/260001/android_webview/test/B...
File android_webview/test/BUILD.gn (left):

https://codereview.chromium.org/2549363004/diff/260001/android_webview/test/B...
android_webview/test/BUILD.gn:267: "//base:base_java_test_support",
this change seems totally unrelated to this CL? pull it out into a separate one?

[Nico, 2016-12-15 01:46:00]

I won't get to this until Friday.

[sdefresne, 2016-12-15 11:50:50]

components/test_runner/BUILD.gn LG but can we see a try run on android?

[Jay Civelli, 2016-12-16 16:57:27]

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

[commit-bot: I haz the power, 2016-12-16 16:57:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Jay Civelli, 2016-12-16 16:59:52]

https://codereview.chromium.org/2549363004/diff/260001/android_webview/test/B...
File android_webview/test/BUILD.gn (left):

https://codereview.chromium.org/2549363004/diff/260001/android_webview/test/B...
android_webview/test/BUILD.gn:267: "//base:base_java_test_support",
On 2016/12/15 01:43:02, boliu wrote:
> this change seems totally unrelated to this CL? pull it out into a separate
one?

It is somehow related. I changed testing/test.gni to add base_java_test_support
to the deps of all test target on Android.
As a result I looked for all occurrences of it and removed it where it was not
needed.

[boliu, 2016-12-16 17:13:41]

https://codereview.chromium.org/2549363004/diff/260001/android_webview/test/B...
File android_webview/test/BUILD.gn (left):

https://codereview.chromium.org/2549363004/diff/260001/android_webview/test/B...
android_webview/test/BUILD.gn:267: "//base:base_java_test_support",
On 2016/12/16 16:59:52, Jay Civelli wrote:
> On 2016/12/15 01:43:02, boliu wrote:
> > this change seems totally unrelated to this CL? pull it out into a separate
> one?
> 
> It is somehow related. I changed testing/test.gni to add
base_java_test_support
> to the deps of all test target on Android.
> As a result I looked for all occurrences of it and removed it where it was not
> needed.

Sure.. lgtm

[commit-bot: I haz the power, 2016-12-16 21:27:05]

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

[commit-bot: I haz the power, 2016-12-16 21:27:06]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Jay Civelli, 2016-12-20 18:50:03]

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

[Jay Civelli, 2016-12-20 18:50:03]

Adding dcheng@ for base OWNER review (base/test has already been OWNER reviewed
by
nyquist@)

[Jay Civelli, 2016-12-20 18:50:57]

On 2016/12/15 11:50:50, sdefresne wrote:
> components/test_runner/BUILD.gn LG but can we see a try run on android?

Android try runs have now been run.

[Dirk Pranke, 2016-12-20 20:26:23]

testing/test.gni lgtm .

let me know if there's anything else you wanted me to review.

[Robert Sesek, 2016-12-20 20:44:33]

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

[Robert Sesek, 2016-12-20 20:44:35]

dcheng@ pointed me at this. A few comments below.

https://codereview.chromium.org/2549363004/diff/260001/base/posix/global_desc...
File base/posix/global_descriptors.h (right):

https://codereview.chromium.org/2549363004/diff/260001/base/posix/global_desc...
base/posix/global_descriptors.h:76: // Generates a random key and uses it to set
the descriptor.
This seems like it could be confusing unless Register is always the last of the
Set operations that is performed, especially since Set does not return whether
or not a replacement happened. Random may very well return 4, 5, or 6 some
times, which could break things that use kBaseDescriptor + value. I also think
that it could make debugging hard if it ever did occur, since it would be
non-deterministic.

Is there a reason why this needs to be done this way, as opposed to just
choosing a kBaseDescriptor+N like the other users of this class?

https://codereview.chromium.org/2549363004/diff/260001/base/process/process.h
File base/process/process.h (right):

https://codereview.chromium.org/2549363004/diff/260001/base/process/process.h...
base/process/process.h:111: bool WaitForExit(int* exit_code) const;
Is this const-ification related? Seems like it could be done separately.

https://codereview.chromium.org/2549363004/diff/260001/base/test/android/java...
File base/test/android/java/src/org/chromium/base/FileDescriptorInfo.java
(right):

https://codereview.chromium.org/2549363004/diff/260001/base/test/android/java...
base/test/android/java/src/org/chromium/base/FileDescriptorInfo.java:20: public
final int id;
Since the base C++ calls this "key" -- why not do the same? That'd help a little
bit with "fd" and "id" having only 1/2 characters difference.

https://codereview.chromium.org/2549363004/diff/260001/base/test/android/java...
File
base/test/android/java/src/org/chromium/base/MultiprocessTestClientLauncher.java
(right):

https://codereview.chromium.org/2549363004/diff/260001/base/test/android/java...
base/test/android/java/src/org/chromium/base/MultiprocessTestClientLauncher.java:270:
private static FileDescriptorInfo[] makeFdInfoArray(int[] ids, int[] fds) {
If you go with the rename of id -> key, then carry it through here too.

https://codereview.chromium.org/2549363004/diff/260001/base/test/android/java...
File
base/test/android/java/src/org/chromium/base/MultiprocessTestClientService.java
(right):

https://codereview.chromium.org/2549363004/diff/260001/base/test/android/java...
base/test/android/java/src/org/chromium/base/MultiprocessTestClientService.java:36:
private MainReturnCodeResult mResult;
Could you use a Future<MainResultCodeResult> instead of this and the lock?

https://codereview.chromium.org/2549363004/diff/260001/base/test/android/java...
base/test/android/java/src/org/chromium/base/MultiprocessTestClientService.java:140:
private static void markInitialized() {
This is called exactly once, so inline it?

https://codereview.chromium.org/2549363004/diff/260001/base/test/android/mult...
File base/test/android/multiprocess_test_client_service.cc (right):

https://codereview.chromium.org/2549363004/diff/260001/base/test/android/mult...
base/test/android/multiprocess_test_client_service.cc:47:
base::GlobalDescriptors::GetInstance()->Set(fd_ids[i], fd_fds[i]);
… and rename in here, too. This line is what made me suggest the id -> key name,
since |fd_ids| and |fd_fds| was hard to mentally parse as different variables.

https://codereview.chromium.org/2549363004/diff/260001/base/test/main_stub.cc
File base/test/main_stub.cc (right):

https://codereview.chromium.org/2549363004/diff/260001/base/test/main_stub.cc...
base/test/main_stub.cc:9: // don't have a min function.
nit: main

https://codereview.chromium.org/2549363004/diff/260001/mojo/edk/embedder/plat...
File mojo/edk/embedder/platform_channel_pair_posix.cc (right):

https://codereview.chromium.org/2549363004/diff/260001/mojo/edk/embedder/plat...
mojo/edk/embedder/platform_channel_pair_posix.cc:134: 
nit: extra blank line

https://codereview.chromium.org/2549363004/diff/260001/testing/android/native...
File testing/android/native_test/native_test_jni_onload.cc (left):

https://codereview.chromium.org/2549363004/diff/260001/testing/android/native...
testing/android/native_test/native_test_jni_onload.cc:23: 
Drop this file from the CL?

[Jay Civelli, 2016-12-20 23:07:27]

https://codereview.chromium.org/2549363004/diff/260001/base/posix/global_desc...
File base/posix/global_descriptors.h (right):

https://codereview.chromium.org/2549363004/diff/260001/base/posix/global_desc...
base/posix/global_descriptors.h:76: // Generates a random key and uses it to set
the descriptor.
On 2016/12/20 20:44:34, Robert Sesek wrote:
> This seems like it could be confusing unless Register is always the last of
the
> Set operations that is performed, especially since Set does not return whether
> or not a replacement happened. Random may very well return 4, 5, or 6 some
> times, which could break things that use kBaseDescriptor + value. I also think
> that it could make debugging hard if it ever did occur, since it would be
> non-deterministic.
> 
> Is there a reason why this needs to be done this way, as opposed to just
> choosing a kBaseDescriptor+N like the other users of this class?

What I don't like about the kBaseDescriptor+N approach is that it becomes hard
to maintain with several modules, which then need to all agree on their base
descriptor. (and with servicification we'll have to address it some generic way)
Here I am doing some of what is suggested in the comment at the top of the class
by choosing randomly. But I agree this can cause problems with collision with
calls to Set().
Would it work for you if I gave the generated keys a high range and check if set
is called with anything in that range?

https://codereview.chromium.org/2549363004/diff/260001/base/process/process.h
File base/process/process.h (right):

https://codereview.chromium.org/2549363004/diff/260001/base/process/process.h...
base/process/process.h:111: bool WaitForExit(int* exit_code) const;
On 2016/12/20 20:44:34, Robert Sesek wrote:
> Is this const-ification related? Seems like it could be done separately.

It is needed as in base/test/multiprocess_test.cc we now call these 2 methods on
a const base::Process&.

https://codereview.chromium.org/2549363004/diff/260001/base/test/android/java...
File base/test/android/java/src/org/chromium/base/FileDescriptorInfo.java
(right):

https://codereview.chromium.org/2549363004/diff/260001/base/test/android/java...
base/test/android/java/src/org/chromium/base/FileDescriptorInfo.java:20: public
final int id;
On 2016/12/20 20:44:34, Robert Sesek wrote:
> Since the base C++ calls this "key" -- why not do the same? That'd help a
little
> bit with "fd" and "id" having only 1/2 characters difference.

Good idea, done.

https://codereview.chromium.org/2549363004/diff/260001/base/test/android/java...
File
base/test/android/java/src/org/chromium/base/MultiprocessTestClientLauncher.java
(right):

https://codereview.chromium.org/2549363004/diff/260001/base/test/android/java...
base/test/android/java/src/org/chromium/base/MultiprocessTestClientLauncher.java:270:
private static FileDescriptorInfo[] makeFdInfoArray(int[] ids, int[] fds) {
On 2016/12/20 20:44:34, Robert Sesek wrote:
> If you go with the rename of id -> key, then carry it through here too.

Done.

https://codereview.chromium.org/2549363004/diff/260001/base/test/android/java...
File
base/test/android/java/src/org/chromium/base/MultiprocessTestClientService.java
(right):

https://codereview.chromium.org/2549363004/diff/260001/base/test/android/java...
base/test/android/java/src/org/chromium/base/MultiprocessTestClientService.java:36:
private MainReturnCodeResult mResult;
On 2016/12/20 20:44:34, Robert Sesek wrote:
> Could you use a Future<MainResultCodeResult> instead of this and the lock?

Do you mean having a FutureTask<MainResultCodeResult> that calls main and having
waitForMainToReturn() calling get on it?
I am not sure, as I don't know if we have guarantees on the ordering of AIDL
calls.
So we would have to protect the FutureTask with a lock to deal with the case
where waitForMainToReturn() ends up being called before launch().

https://codereview.chromium.org/2549363004/diff/260001/base/test/android/java...
base/test/android/java/src/org/chromium/base/MultiprocessTestClientService.java:140:
private static void markInitialized() {
On 2016/12/20 20:44:34, Robert Sesek wrote:
> This is called exactly once, so inline it?

Is to avoid FindBugs complaining that a static field is being set from a
non-static method.
I added a comment.

https://codereview.chromium.org/2549363004/diff/260001/base/test/android/mult...
File base/test/android/multiprocess_test_client_service.cc (right):

https://codereview.chromium.org/2549363004/diff/260001/base/test/android/mult...
base/test/android/multiprocess_test_client_service.cc:47:
base::GlobalDescriptors::GetInstance()->Set(fd_ids[i], fd_fds[i]);
On 2016/12/20 20:44:34, Robert Sesek wrote:
> … and rename in here, too. This line is what made me suggest the id -> key
name,
> since |fd_ids| and |fd_fds| was hard to mentally parse as different variables.

Done.

https://codereview.chromium.org/2549363004/diff/260001/base/test/main_stub.cc
File base/test/main_stub.cc (right):

https://codereview.chromium.org/2549363004/diff/260001/base/test/main_stub.cc...
base/test/main_stub.cc:9: // don't have a min function.
On 2016/12/20 20:44:34, Robert Sesek wrote:
> nit: main

Done.

https://codereview.chromium.org/2549363004/diff/260001/mojo/edk/embedder/plat...
File mojo/edk/embedder/platform_channel_pair_posix.cc (right):

https://codereview.chromium.org/2549363004/diff/260001/mojo/edk/embedder/plat...
mojo/edk/embedder/platform_channel_pair_posix.cc:134: 
On 2016/12/20 20:44:34, Robert Sesek wrote:
> nit: extra blank line

Done.

https://codereview.chromium.org/2549363004/diff/260001/testing/android/native...
File testing/android/native_test/native_test_jni_onload.cc (left):

https://codereview.chromium.org/2549363004/diff/260001/testing/android/native...
testing/android/native_test/native_test_jni_onload.cc:23: 
On 2016/12/20 20:44:34, Robert Sesek wrote:
> Drop this file from the CL?

Done.

[Robert Sesek, 2016-12-21 19:26:07]

https://codereview.chromium.org/2549363004/diff/260001/base/posix/global_desc...
File base/posix/global_descriptors.h (right):

https://codereview.chromium.org/2549363004/diff/260001/base/posix/global_desc...
base/posix/global_descriptors.h:76: // Generates a random key and uses it to set
the descriptor.
On 2016/12/20 23:07:26, Jay Civelli wrote:
> On 2016/12/20 20:44:34, Robert Sesek wrote:
> > This seems like it could be confusing unless Register is always the last of
> the
> > Set operations that is performed, especially since Set does not return
whether
> > or not a replacement happened. Random may very well return 4, 5, or 6 some
> > times, which could break things that use kBaseDescriptor + value. I also
think
> > that it could make debugging hard if it ever did occur, since it would be
> > non-deterministic.
> > 
> > Is there a reason why this needs to be done this way, as opposed to just
> > choosing a kBaseDescriptor+N like the other users of this class?
> 
> What I don't like about the kBaseDescriptor+N approach is that it becomes hard
> to maintain with several modules, which then need to all agree on their base
> descriptor. (and with servicification we'll have to address it some generic
way)

I agree that this could start to become a problem with servicification, but I
think tackling that problem is probably best done on its own, rather than ad-hoc
in this CL. For example, it may be possible, with a little refactoring, to have
GlobalDescriptors assign the keys and return them from Set, rather than letting
the clients specify.

> Here I am doing some of what is suggested in the comment at the top of the
class
> by choosing randomly. But I agree this can cause problems with collision with
> calls to Set().
> Would it work for you if I gave the generated keys a high range and check if
set
> is called with anything in that range?

I think what the comment is getting at something like this (a static random
value as opposed to dynamically choosing one:
https://cs.chromium.org/chromium/src/sandbox/linux/seccomp-bpf/sandbox_bpf.cc...).
I'd prefer to not add this new method and to continue using GlobalDescriptors
the way it is used elsewhere.

https://codereview.chromium.org/2549363004/diff/260001/base/test/android/java...
File
base/test/android/java/src/org/chromium/base/MultiprocessTestClientService.java
(right):

https://codereview.chromium.org/2549363004/diff/260001/base/test/android/java...
base/test/android/java/src/org/chromium/base/MultiprocessTestClientService.java:36:
private MainReturnCodeResult mResult;
On 2016/12/20 23:07:26, Jay Civelli wrote:
> On 2016/12/20 20:44:34, Robert Sesek wrote:
> > Could you use a Future<MainResultCodeResult> instead of this and the lock?
> 
> Do you mean having a FutureTask<MainResultCodeResult> that calls main and
having
> waitForMainToReturn() calling get on it?
> I am not sure, as I don't know if we have guarantees on the ordering of AIDL
> calls.
> So we would have to protect the FutureTask with a lock to deal with the case
> where waitForMainToReturn() ends up being called before launch().

Ah, I see your point. I was actually thinking of CompletableFuture<T> (but
that's only in API 24) or SettableFuture <T> (but that's only in Guava), but
since neither of those are options and given your comment, just ignore this.

[Jay Civelli, 2016-12-21 22:26:24]

https://codereview.chromium.org/2549363004/diff/260001/base/posix/global_desc...
File base/posix/global_descriptors.h (right):

https://codereview.chromium.org/2549363004/diff/260001/base/posix/global_desc...
base/posix/global_descriptors.h:76: // Generates a random key and uses it to set
the descriptor.
On 2016/12/21 19:26:07, Robert Sesek wrote:
> On 2016/12/20 23:07:26, Jay Civelli wrote:
> > On 2016/12/20 20:44:34, Robert Sesek wrote:
> > > This seems like it could be confusing unless Register is always the last
of
> > the
> > > Set operations that is performed, especially since Set does not return
> whether
> > > or not a replacement happened. Random may very well return 4, 5, or 6 some
> > > times, which could break things that use kBaseDescriptor + value. I also
> think
> > > that it could make debugging hard if it ever did occur, since it would be
> > > non-deterministic.
> > > 
> > > Is there a reason why this needs to be done this way, as opposed to just
> > > choosing a kBaseDescriptor+N like the other users of this class?
> > 
> > What I don't like about the kBaseDescriptor+N approach is that it becomes
hard
> > to maintain with several modules, which then need to all agree on their base
> > descriptor. (and with servicification we'll have to address it some generic
> way)
> 
> I agree that this could start to become a problem with servicification, but I
> think tackling that problem is probably best done on its own, rather than
ad-hoc
> in this CL. For example, it may be possible, with a little refactoring, to
have
> GlobalDescriptors assign the keys and return them from Set, rather than
letting
> the clients specify.
> 
> > Here I am doing some of what is suggested in the comment at the top of the
> class
> > by choosing randomly. But I agree this can cause problems with collision
with
> > calls to Set().
> > Would it work for you if I gave the generated keys a high range and check if
> set
> > is called with anything in that range?
> 
> I think what the comment is getting at something like this (a static random
> value as opposed to dynamically choosing one:
>
https://cs.chromium.org/chromium/src/sandbox/linux/seccomp-bpf/sandbox_bpf.cc...).
> I'd prefer to not add this new method and to continue using GlobalDescriptors
> the way it is used elsewhere.

Sounds good, now using a fixed key.
I will try to look at what you suggested (having Set() return the key) in a
future CL.

[Jay Civelli, 2016-12-21 22:27:39]

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

[commit-bot: I haz the power, 2016-12-21 22:28:03]

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, 2016-12-22 01:37:36]

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

[commit-bot: I haz the power, 2016-12-22 01:37:37]

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

[Robert Sesek, 2016-12-22 23:21:54]

LGTM w/ a few nits

https://codereview.chromium.org/2549363004/diff/340001/base/test/android/java...
File
base/test/android/java/src/org/chromium/base/MultiprocessTestClientService.java
(right):

https://codereview.chromium.org/2549363004/diff/340001/base/test/android/java...
base/test/android/java/src/org/chromium/base/MultiprocessTestClientService.java:84:
return true;
Any reason why this returns a boolean?

https://codereview.chromium.org/2549363004/diff/340001/base/test/android/mult...
File base/test/android/multiprocess_test_client_service.cc (right):

https://codereview.chromium.org/2549363004/diff/340001/base/test/android/mult...
base/test/android/multiprocess_test_client_service.cc:43: CHECK(keys.size() ==
fds.size());
nit: CHECK_EQ

https://codereview.chromium.org/2549363004/diff/340001/base/test/android/mult...
base/test/android/multiprocess_test_client_service.cc:45:
std::vector<base::ScopedFD> fd_scoped_fds;
Unused variable.

https://codereview.chromium.org/2549363004/diff/340001/base/test/multiprocess...
File base/test/multiprocess_test_android.cc (right):

https://codereview.chromium.org/2549363004/diff/340001/base/test/multiprocess...
base/test/multiprocess_test_android.cc:34: std::vector<int> fd_ids;
nit: ids -> keys

[Jay Civelli, 2016-12-22 23:44:41]

https://codereview.chromium.org/2549363004/diff/340001/base/test/android/java...
File
base/test/android/java/src/org/chromium/base/MultiprocessTestClientService.java
(right):

https://codereview.chromium.org/2549363004/diff/340001/base/test/android/java...
base/test/android/java/src/org/chromium/base/MultiprocessTestClientService.java:84:
return true;
On 2016/12/22 23:21:53, Robert Sesek wrote:
> Any reason why this returns a boolean?

This is to force the method to be synchronous.
By making this IPC method return something, we guarantee the caller is going to
be blocked until the remote method gets executed.
(there is a comment in the AIDL file)

https://codereview.chromium.org/2549363004/diff/340001/base/test/android/mult...
File base/test/android/multiprocess_test_client_service.cc (right):

https://codereview.chromium.org/2549363004/diff/340001/base/test/android/mult...
base/test/android/multiprocess_test_client_service.cc:43: CHECK(keys.size() ==
fds.size());
On 2016/12/22 23:21:54, Robert Sesek wrote:
> nit: CHECK_EQ

Done.

https://codereview.chromium.org/2549363004/diff/340001/base/test/android/mult...
base/test/android/multiprocess_test_client_service.cc:45:
std::vector<base::ScopedFD> fd_scoped_fds;
On 2016/12/22 23:21:54, Robert Sesek wrote:
> Unused variable.

Done.

https://codereview.chromium.org/2549363004/diff/340001/base/test/multiprocess...
File base/test/multiprocess_test_android.cc (right):

https://codereview.chromium.org/2549363004/diff/340001/base/test/multiprocess...
base/test/multiprocess_test_android.cc:34: std::vector<int> fd_ids;
On 2016/12/22 23:21:54, Robert Sesek wrote:
> nit: ids -> keys

Done.

[Jay Civelli, 2016-12-22 23:44:49]

The CQ bit was checked by jcivelli@chromium.org

[Jay Civelli, 2016-12-22 23:44:50]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
nyquist@chromium.org, dpranke@chromium.org, boliu@chromium.org,
rsesek@chromium.org
Link to the patchset: https://codereview.chromium.org/2549363004/#ps360001
(title: "Addressed rsesek@'s comments")

[commit-bot: I haz the power, 2016-12-22 23:45:22]

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, 2016-12-22 23:54:42]

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

[commit-bot: I haz the power, 2016-12-22 23:54:44]

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

[dcheng, 2016-12-22 23:55:56]

On 2016/12/22 23:54:44, commit-bot: I haz the power wrote:
> 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...)

RS LGTM based on rsesek's review

[dcheng, 2016-12-22 23:57:34]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2016-12-22 23:58:21]

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, 2016-12-23 00:06:31]

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

[commit-bot: I haz the power, 2016-12-23 00:06:33]

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, 2016-12-23 00:07:52]

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

[Jay Civelli, 2016-12-23 00:08:15]

Adding eroman@ for /net OWNERS review.

[eroman, 2016-12-23 00:21:48]

net LGTM

[Jay Civelli, 2016-12-23 00:31:59]

The CQ bit was checked by jcivelli@chromium.org

[commit-bot: I haz the power, 2016-12-23 00:32:44]

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, 2016-12-23 02:55:37]

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

[commit-bot: I haz the power, 2016-12-23 02:55:39]

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

[Jay Civelli, 2016-12-29 21:07:35]

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

[commit-bot: I haz the power, 2016-12-29 21:07:47]

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, 2016-12-29 22:46:20]

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

[commit-bot: I haz the power, 2016-12-29 22:46:22]

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

[Jay Civelli, 2017-01-04 20:57:35]

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

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

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-04 21:48:19]

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

[commit-bot: I haz the power, 2017-01-04 21:48:21]

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

[Jay Civelli, 2017-01-04 23:39:07]

The CQ bit was checked by jcivelli@chromium.org

[Jay Civelli, 2017-01-04 23:39:07]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
dpranke@chromium.org, boliu@chromium.org, dcheng@chromium.org,
nyquist@chromium.org, eroman@chromium.org, rsesek@chromium.org
Link to the patchset: https://codereview.chromium.org/2549363004/#ps380001
(title: "Fixed content_unittests + synced")

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

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-05 00:56:59]

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

[commit-bot: I haz the power, 2017-01-05 00:57:00]

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

[Jay Civelli, 2017-01-05 18:04:46]

The CQ bit was checked by jcivelli@chromium.org

[Jay Civelli, 2017-01-05 18:04:47]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
dpranke@chromium.org, boliu@chromium.org, dcheng@chromium.org,
nyquist@chromium.org, eroman@chromium.org, rsesek@chromium.org
Link to the patchset: https://codereview.chromium.org/2549363004/#ps400001
(title: "content_unittests + sync")

[commit-bot: I haz the power, 2017-01-05 18:05: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-06 01:41:44]

CQ is committing da patch.

Bot data: {"patchset_id": 400001, "attempt_start_ts": 1483639486921230,
"parent_rev": "7b1ab28d89a2ff18515b3c699dfc02a15aae84ee", "commit_rev":
"5513e4622febbc838dfebddb51347ab797747836"}

[commit-bot: I haz the power, 2017-01-06 01:42:19]

Description was changed from

==========
Multiprocess test client: Android child process launcher rework.

Changing the way test start child processese on Android.
Forking natively should not be used on Andoid, instead child processes
should be started through the Android framework, which requires going
to Java and using Android services.

For that we introduce the MultiprocessTestClientLauncher java class
which starts a service in a new process. We provide several
MultiprocessTestClientService service classes to ensure we can have
multiple child processes.
File descriptor are passed from the parent to the child process as
Parcelable, and we use the GlobalDescriptors class to register them.

TEST=Run the mojo_system_unittests
BUG=666356
==========

to

==========
Multiprocess test client: Android child process launcher rework.

Changing the way test start child processese on Android.
Forking natively should not be used on Andoid, instead child processes
should be started through the Android framework, which requires going
to Java and using Android services.

For that we introduce the MultiprocessTestClientLauncher java class
which starts a service in a new process. We provide several
MultiprocessTestClientService service classes to ensure we can have
multiple child processes.
File descriptor are passed from the parent to the child process as
Parcelable, and we use the GlobalDescriptors class to register them.

TEST=Run the mojo_system_unittests
BUG=666356

Review-Url: https://codereview.chromium.org/2549363004
Cr-Commit-Position: refs/heads/master@{#441799}
Committed:
https://chromium.googlesource.com/chromium/src/+/5513e4622febbc838dfebddb5134...
==========

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

Committed patchset #21 (id:400001) as
https://chromium.googlesource.com/chromium/src/+/5513e4622febbc838dfebddb5134...

[Jay Civelli, 2017-01-06 02:15:43]

A revert of this CL (patchset #21 id:400001) has been created in
https://codereview.chromium.org/2613183002/ by jcivelli@chromium.org.

The reason for reverting is: Breaks build on Android Cronet ARM64 Builder (dbg).