Multiprocess test client: Android child process launcher rework.

base/test/android/java/src/org/chromium/base/MultiprocessTestClientService.java

+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+package org.chromium.base;
+
+import android.app.Service;
+import android.content.Intent;
+import android.os.Handler;
+import android.os.IBinder;
+import android.os.Process;
+
+import org.chromium.base.annotations.CalledByNative;
+import org.chromium.base.annotations.JNINamespace;
+import org.chromium.base.annotations.SuppressFBWarnings;
+import org.chromium.base.library_loader.NativeLibraries;
+
+/**
+ * The service implementation used to host all multiprocess test client code.
+ */
+@JNINamespace("base::android")
+public class MultiprocessTestClientService extends Service {
+    private static final String TAG = "TestClient";

[nyquist, 2016/12/13 20:40:31]

Nit: cr_ prefix these days I think.

[Jay Civelli, 2016/12/14 02:20:11]

OK, I'll do it...

+
+    private final Handler mHandler = new Handler();
+
+    private final Object mResultLock = new Object();
+    private MainReturnCodeResult mResult;

[nyquist, 2016/12/13 20:40:31]

@GuardedBy("mResultLock") ?

[Jay Civelli, 2016/12/14 02:20:11]

Done.

+
+    private final ITestClient.Stub mBinder = new ITestClient.Stub() {
+        @Override
+        public int launch(final String[] commandLine, FileDescriptorInfo[] fdsToMap) {
+            final int[] fdIds = new int[fdsToMap.length];
+            final int[] fdFds = new int[fdsToMap.length];
+            for (int i = 0; i < fdsToMap.length; i++) {
+                fdIds[i] = fdsToMap[i].mId;
+                fdFds[i] = fdsToMap[i].mFd.detachFd();
+            }
+            // Don't run main directly, it would block and the response would not be returned.
+            // We post to the main thread as this thread does not have a Looper.
+            mHandler.post(new Runnable() {
+                @Override
+                public void run() {
+                    nativeRunMain(commandLine, fdIds, fdFds);
+                }
+            });
+            return Process.myPid();
+        }
+
+        @SuppressFBWarnings("RCN_REDUNDANT_NULLCHECK_OF_NULL_VALUE")
+        @Override
+        public MainReturnCodeResult waitForMainToReturn(int timeoutMs) {
+            synchronized (mResultLock) {
+                if (mResult != null) return mResult;
+                try {
+                    mResultLock.wait(timeoutMs);

[nyquist, 2016/12/13 20:40:31]

This doesn't handle spurious wake-ups. Do you want to add support for that?

[Jay Civelli, 2016/12/14 02:20:11]

Good call.

+                } catch (InterruptedException ie) {
+                    Log.e(TAG, "Failed to wait for main return value.");
+                    return new MainReturnCodeResult(0, true /* timed-out */);
+                }
+                assert mResult != null;
+                return mResult;
+            }
+        }
+
+        @SuppressFBWarnings("DM_EXIT")
+        @Override
+        public boolean forceStopSynchronous(int exitCode) {
+            System.exit(exitCode);
+            return true;
+        }
+
+        @SuppressFBWarnings("DM_EXIT")
+        @Override
+        public void forceStop(int exitCode) {
+            System.exit(exitCode);
+        }
+    };
+
+    static boolean sAlreadyInitialized = false;

[nyquist, 2016/12/13 20:40:31]

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

[Jay Civelli, 2016/12/14 02:20:11]

Done.

+
+    @SuppressFBWarnings({"ST_WRITE_TO_STATIC_FROM_INSTANCE_METHOD", "DM_EXIT"})

[nyquist, 2016/12/13 20:40:31]

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?

[Jay Civelli, 2016/12/14 02:20:11]

Done.

+    @Override
+    public void onCreate() {
+        super.onCreate();
+
+        if (sAlreadyInitialized) {
+            // The framework controls how services are reused and even though nothing is bound to a
+            // service it might be kept around. Since we really want to fork a new process when we
+            // bind, we'll kill the process early when a service is reused, forcing the framework to
+            // recreate the service in a new process.
+            // This is not ideal, but there are no clear alternatives at this point.
+            Log.e(TAG, "Service being reused, forcing stoppage.");
+            System.exit(0);

[nyquist, 2016/12/13 20:40:31]

I think we usually add an extra return-call after this.

[Jay Civelli, 2016/12/14 02:20:11]

Done.

+        }
+        sAlreadyInitialized = true;
+
+        ContextUtils.initApplicationContext(getApplicationContext());
+
+        PathUtils.setPrivateDataDirectorySuffix("chrome_multiprocess_test_client_service");
+
+        loadLibraries();
+
+        Log.e(TAG, "onCreate");

[nyquist, 2016/12/13 20:40:31]

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.

[Jay Civelli, 2016/12/14 02:20:11]

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

+    }
+
+    @Override
+    public void onDestroy() {
+        Log.e(TAG, "onDestroy");

[nyquist, 2016/12/13 20:40:31]

If the log is removed, this just calls super, so I guess if you remove the log,
also remove this method.

[Jay Civelli, 2016/12/14 02:20:11]

Done.

+        super.onDestroy();
+    }
+
+    @Override
+    public IBinder onBind(Intent intent) {
+        // Only bind the first incoming request.
+        stopSelf();

[nyquist, 2016/12/13 20:40:31]

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()?

[Jay Civelli, 2016/12/14 02:20:11]

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

+
+        return mBinder;
+    }
+
+    private void loadLibraries() {

[nyquist, 2016/12/13 20:40:31]

What's the reason for this not using the LibraryLoader in //base? Could we maybe
clarify why as a comment?

[Jay Civelli, 2016/12/14 02:20:11]

None, now doing so.

+        for (String library : NativeLibraries.LIBRARIES) {
+            Log.i(TAG, "loading: %s", library);

[nyquist, 2016/12/13 20:40:31]

Do we need this? If so, uppercase L?

[Jay Civelli, 2016/12/14 02:20:11]

This is now gone.

+            System.loadLibrary(library);
+            Log.i(TAG, "loaded: %s", library);
+        }
+        ContextUtils.initApplicationContextForNative();
+    }
+
+    @CalledByNative
+    private void setMainReturnValue(int result) {
+        synchronized (mResultLock) {
+            mResult = new MainReturnCodeResult(result, false /* timed-out */);
+            mResultLock.notifyAll();
+        }
+    }
+
+    private native void nativeRunMain(String[] commandLine, int[] fdsToMapIds, int[] fdsToMapFds);
+}