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}

content/public/android/java/src/org/chromium/content/browser/BaseChildProcessConnection.java

@@ -26,7 +26,6 @@ import org.chromium.base.process_launcher.IChildProcessService;
 import java.io.IOException;
 
 import javax.annotation.Nullable;
-import javax.annotation.concurrent.GuardedBy;
 
 /**
  * Manages a connection between the browser activity and a child service.
@@ -188,52 +187,39 @@ public abstract class BaseChildProcessConnection {
         }
     }
 
-    // Synchronization: While most internal flow occurs on the UI thread, the public API
-    // (specifically start and stop) may be called from any thread, hence all entry point methods
-    // into the class are synchronized on the lock to protect access to these members.
-    // TODO(jcivelli): crbug.com/714657 remove this lock.
-    private final Object mLock = new Object();
-
     // This is set in start() and is used in onServiceConnected().
-    @GuardedBy("mLock")
     private StartCallback mStartCallback;
 
     // This is set in setupConnection() and is later used in doConnectionSetupLocked(), after which
     // the variable is cleared. Therefore this is only valid while the connection is being set up.
-    @GuardedBy("mLock")
     private ConnectionParams mConnectionParams;
 
     // Callback provided in setupConnection() that will communicate the result to the caller. This
     // has to be called exactly once after setupConnection(), even if setup fails, so that the
     // caller can free up resources associated with the setup attempt. This is set to null after the
     // call.
-    @GuardedBy("mLock")
     private ConnectionCallback mConnectionCallback;
 
-    @GuardedBy("mLock")
     private IChildProcessService mService;
 
     // Set to true when the service connection callback runs. This differs from
     // mServiceConnectComplete, which tracks that the connection completed successfully.
-    @GuardedBy("mLock")
     private boolean mDidOnServiceConnected;
 
     // Set to true when the service connected successfully.
-    @GuardedBy("mLock")
     private boolean mServiceConnectComplete;
 
     // Set to true when the service disconnects, as opposed to being properly closed. This happens
     // when the process crashes or gets killed by the system out-of-memory killer.
-    @GuardedBy("mLock")
     private boolean mServiceDisconnected;
 
     // Process ID of the corresponding child process.
-    @GuardedBy("mLock")
     private int mPid;
 
     protected BaseChildProcessConnection(Context context, int number, boolean sandboxed,
             DeathCallback deathCallback, String serviceClassName,
             Bundle childProcessCommonParameters, ChildProcessCreationParams creationParams) {
+        assert LauncherThread.runningOnLauncherThread();
         mContext = context;
         mServiceNumber = number;
         mSandboxed = sandboxed;
@@ -246,33 +232,38 @@ public abstract class BaseChildProcessConnection {
     }
 
     public final Context getContext() {
+        assert LauncherThread.runningOnLauncherThread();
         return mContext;
     }
 
     public final int getServiceNumber() {
+        assert LauncherThread.runningOnLauncherThread();
         return mServiceNumber;
     }
 
     public final boolean isSandboxed() {
+        assert LauncherThread.runningOnLauncherThread();
         return mSandboxed;
     }
 
     public final String getPackageName() {
+        assert LauncherThread.runningOnLauncherThread();
         return mCreationParams != null ? mCreationParams.getPackageName()
                                        : mContext.getPackageName();
     }
 
     public final ChildProcessCreationParams getCreationParams() {
+        assert LauncherThread.runningOnLauncherThread();
         return mCreationParams;
     }
 
     public final IChildProcessService getService() {
-        synchronized (mLock) {
+        assert LauncherThread.runningOnLauncherThread();
         return mService;
     }
-    }
 
     public final ComponentName getServiceName() {
+        assert LauncherThread.runningOnLauncherThread();
         return mServiceName;
     }
 
@@ -280,10 +271,9 @@ public abstract class BaseChildProcessConnection {
      * @return the connection pid, or 0 if not yet connected
      */
     public int getPid() {
-        synchronized (mLock) {
+        assert LauncherThread.runningOnLauncherThread();
         return mPid;
     }
-    }
 
     /**
      * Starts a connection to an IChildProcessService. This must be followed by a call to
@@ -293,10 +283,10 @@ public abstract class BaseChildProcessConnection {
      * @param startCallback (optional) callback when the child process starts or fails to start.
      */
     public void start(StartCallback startCallback) {
+        assert LauncherThread.runningOnLauncherThread();
         try {
             TraceEvent.begin("BaseChildProcessConnection.start");
             assert LauncherThread.runningOnLauncherThread();
-            synchronized (mLock) {
             assert mConnectionParams
                     == null
                     : "setupConnection() called before start() in BaseChildProcessConnection.";
@@ -309,7 +299,6 @@ public abstract class BaseChildProcessConnection {
                 // TODO(ppi): Can we hard-fail here?
                 mDeathCallback.onChildProcessDied(BaseChildProcessConnection.this);
             }
-            }
         } finally {
             TraceEvent.end("BaseChildProcessConnection.start");
         }
@@ -327,7 +316,6 @@ public abstract class BaseChildProcessConnection {
     public void setupConnection(String[] commandLine, FileDescriptorInfo[] filesToBeMapped,
             @Nullable IBinder callback, ConnectionCallback connectionCallback) {
         assert LauncherThread.runningOnLauncherThread();
-        synchronized (mLock) {
         assert mConnectionParams == null;
         if (mServiceDisconnected) {
             Log.w(TAG, "Tried to setup a connection that already disconnected.");
@@ -347,23 +335,20 @@ public abstract class BaseChildProcessConnection {
             TraceEvent.end("BaseChildProcessConnection.setupConnection");
         }
     }
-    }
 
     /**
      * Terminates the connection to IChildProcessService, closing all bindings. It is safe to call
      * this multiple times.
      */
     public void stop() {
-        synchronized (mLock) {
+        assert LauncherThread.runningOnLauncherThread();
         unbind();
         mService = null;
         mConnectionParams = null;
     }
-    }
 
     private void onServiceConnectedOnLauncherThread(IBinder service) {
         assert LauncherThread.runningOnLauncherThread();
-        synchronized (mLock) {
         // A flag from the parent class ensures we run the post-connection logic only once
         // (instead of once per each ChildServiceConnection).
         if (mDidOnServiceConnected) {
@@ -410,15 +395,12 @@ public abstract class BaseChildProcessConnection {
                 doConnectionSetupLocked();
             }
         } finally {
-                TraceEvent.end(
-                        "BaseChildProcessConnection.ChildServiceConnection.onServiceConnected");
-            }
+            TraceEvent.end("BaseChildProcessConnection.ChildServiceConnection.onServiceConnected");
         }
     }
 
     private void onServiceDisconnectedOnLauncherThread() {
         assert LauncherThread.runningOnLauncherThread();
-        synchronized (mLock) {
         // Ensure that the disconnection logic runs only once (instead of once per each
         // ChildServiceConnection).
         if (mServiceDisconnected) {
@@ -435,10 +417,8 @@ public abstract class BaseChildProcessConnection {
         }
         mConnectionCallback = null;
     }
-    }
 
     private void onSetupConnectionResult(int pid) {
-        synchronized (mLock) {
         mPid = pid;
         assert mPid != 0 : "Child service claims to be run by a process of pid=0.";
 
@@ -447,14 +427,12 @@ public abstract class BaseChildProcessConnection {
         }
         mConnectionCallback = null;
     }
-    }
 
     /**
      * Called after the connection parameters have been set (in setupConnection()) *and* a
      * connection has been established (as signaled by onServiceConnected()). These two events can
      * happen in any order. Has to be called with mLock.
      */
-    @GuardedBy("mLock")
     private void doConnectionSetupLocked() {
         try {
             TraceEvent.begin("BaseChildProcessConnection.doConnectionSetupLocked");
@@ -498,10 +476,12 @@ public abstract class BaseChildProcessConnection {
     protected abstract void unbind();
 
     protected ChildServiceConnection createServiceConnection(int bindFlags) {
+        assert LauncherThread.runningOnLauncherThread();
         return new ChildServiceConnectionImpl(bindFlags);
     }
 
     protected boolean shouldBindAsExportedService() {
+        assert LauncherThread.runningOnLauncherThread();
         return Build.VERSION.SDK_INT >= Build.VERSION_CODES.N && getCreationParams() != null
                 && getCreationParams().getIsExternalService()
                 && isExportedService(isSandboxed(), getContext(), getServiceName());
@@ -529,15 +509,11 @@ public abstract class BaseChildProcessConnection {
 
     @VisibleForTesting
     public void crashServiceForTesting() throws RemoteException {
-        synchronized (mLock) {
         mService.crashIntentionallyForTesting();
     }
-    }
 
     @VisibleForTesting
-    public boolean isConnected() {
-        synchronized (mLock) {
+    boolean isConnected() {
         return mService != null;
     }
-    }
 }

content/shell/android/BUILD.gn

@@ -96,6 +96,7 @@ android_library("content_shell_apk_java") {
 
   java_files = [
     "shell_apk/src/org/chromium/content_shell_apk/ChildProcessLauncherTestHelperService.java",
+    "shell_apk/src/org/chromium/content_shell_apk/ChildProcessLauncherTestUtils.java",
     "shell_apk/src/org/chromium/content_shell_apk/ContentShellActivity.java",
     "shell_apk/src/org/chromium/content_shell_apk/ContentShellApplication.java",
   ]

content/shell/android/shell_apk/src/org/chromium/content_shell_apk/ChildProcessLauncherTestHelperService.java

@@ -5,7 +5,6 @@
 package org.chromium.content_shell_apk;
 
 import android.app.Service;
-import android.content.Context;
 import android.content.Intent;
 import android.os.Handler;
 import android.os.HandlerThread;
@@ -22,12 +21,6 @@ import org.chromium.base.library_loader.ProcessInitException;
 import org.chromium.base.process_launcher.ChildProcessCreationParams;
 import org.chromium.base.process_launcher.FileDescriptorInfo;
 import org.chromium.content.browser.BaseChildProcessConnection;
-import org.chromium.content.browser.ChildProcessLauncher;
-import org.chromium.content.browser.LauncherThread;
-
-import java.util.concurrent.Callable;
-import java.util.concurrent.FutureTask;
-import java.util.concurrent.Semaphore;
 
 /**
  * A Service that assists the ChildProcessLauncherTest that responds to one message, which
@@ -53,53 +46,6 @@ public class ChildProcessLauncherTestHelperService extends Service {
 
     private final HandlerThread mHandlerThread = new HandlerThread("Helper Service Handler");
 
-    public static void runOnLauncherThreadBlocking(final Runnable runnable) {
-        if (LauncherThread.runningOnLauncherThread()) {
-            runnable.run();
-            return;
-        }
-        final Semaphore done = new Semaphore(0);
-        LauncherThread.post(new Runnable() {
-            @Override
-            public void run() {
-                runnable.run();
-                done.release();
-            }
-        });
-        done.acquireUninterruptibly();
-    }
-
-    public static <R> R runOnLauncherAndGetResult(Callable<R> callable) {
-        if (LauncherThread.runningOnLauncherThread()) {
-            try {
-                return callable.call();
-            } catch (Exception e) {
-                throw new RuntimeException(e);
-            }
-        }
-        try {
-            FutureTask<R> task = new FutureTask<R>(callable);
-            LauncherThread.post(task);
-            return task.get();
-        } catch (Exception e) {
-            throw new RuntimeException(e);
-        }
-    }
-
-    public static BaseChildProcessConnection startInternalForTesting(final Context context,
-            final String[] commandLine, final FileDescriptorInfo[] filesToMap,
-            final ChildProcessCreationParams params) {
-        return runOnLauncherAndGetResult(new Callable<BaseChildProcessConnection>() {
-            @Override
-            public BaseChildProcessConnection call() {
-                return ChildProcessLauncher.startInternal(context, commandLine,
-                        0 /* childProcessId */, filesToMap, null /* launchCallback */,
-                        null /* childProcessCallback */, true /* inSandbox */,
-                        false /* alwaysInForeground */, params);
-            }
-        });
-    }
-
     @Override
     public void onCreate() {
         CommandLine.init(null);
@@ -125,7 +71,8 @@ public class ChildProcessLauncherTestHelperService extends Service {
         ChildProcessCreationParams params = new ChildProcessCreationParams(
                 getPackageName(), false, LibraryProcessType.PROCESS_CHILD, bindToCaller);
         final BaseChildProcessConnection conn =
-                startInternalForTesting(this, commandLine, new FileDescriptorInfo[0], params);
+                ChildProcessLauncherTestUtils.startInternalForTesting(
+                        this, commandLine, new FileDescriptorInfo[0], params);
 
         // Poll the connection until it is set up. The main test in ChildProcessLauncherTest, which
         // has bound the connection to this service, manages the timeout via the lifetime of this
@@ -136,10 +83,11 @@ public class ChildProcessLauncherTestHelperService extends Service {
 
             @Override
             public void run() {
-                if (conn.getPid() != 0) {
+                int pid = ChildProcessLauncherTestUtils.getConnectionPid(conn);
+                if (pid != 0) {
                     try {
-                        mReplyTo.send(Message.obtain(null, MSG_BIND_SERVICE_REPLY, conn.getPid(),
-                                    conn.getServiceNumber()));
+                        mReplyTo.send(Message.obtain(null, MSG_BIND_SERVICE_REPLY, pid,
+                                ChildProcessLauncherTestUtils.getConnectionServiceNumber(conn)));
                     } catch (RemoteException ex) {
                         throw new RuntimeException(ex);
                     }

content/shell/android/shell_apk/src/org/chromium/content_shell_apk/ChildProcessLauncherTestUtils.java

+// Copyright 2017 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.content_shell_apk;
+
+import android.content.Context;
+
+import org.chromium.base.process_launcher.ChildProcessCreationParams;
+import org.chromium.base.process_launcher.FileDescriptorInfo;
+import org.chromium.base.process_launcher.IChildProcessService;
+import org.chromium.content.browser.BaseChildProcessConnection;
+import org.chromium.content.browser.ChildProcessLauncher;
+import org.chromium.content.browser.LauncherThread;
+
+import java.util.concurrent.Callable;
+import java.util.concurrent.FutureTask;
+import java.util.concurrent.Semaphore;
+
+/** An assortment of static methods used in tests that deal with launching child processes. */
+public final class ChildProcessLauncherTestUtils {
+    // Do not instanciate, use static methods instead.
+    private ChildProcessLauncherTestUtils() {}
+
+    public static void runOnLauncherThreadBlocking(final Runnable runnable) {
+        if (LauncherThread.runningOnLauncherThread()) {
+            runnable.run();
+            return;
+        }
+        final Semaphore done = new Semaphore(0);
+        LauncherThread.post(new Runnable() {
+            @Override
+            public void run() {
+                runnable.run();
+                done.release();
+            }
+        });
+        done.acquireUninterruptibly();
+    }
+
+    public static <R> R runOnLauncherAndGetResult(Callable<R> callable) {
+        if (LauncherThread.runningOnLauncherThread()) {
+            try {
+                return callable.call();
+            } catch (Exception e) {
+                throw new RuntimeException(e);
+            }
+        }
+        try {
+            FutureTask<R> task = new FutureTask<R>(callable);
+            LauncherThread.post(task);
+            return task.get();
+        } catch (Exception e) {
+            throw new RuntimeException(e);
+        }
+    }
+
+    public static BaseChildProcessConnection startInternalForTesting(final Context context,
+            final String[] commandLine, final FileDescriptorInfo[] filesToMap,
+            final ChildProcessCreationParams params) {
+        return runOnLauncherAndGetResult(new Callable<BaseChildProcessConnection>() {
+            @Override
+            public BaseChildProcessConnection call() {
+                return ChildProcessLauncher.startInternal(context, commandLine,
+                        0 /* childProcessId */, filesToMap, null /* launchCallback */,
+                        null /* childProcessCallback */, true /* inSandbox */,
+                        false /* alwaysInForeground */, params);
+            }
+        });
+    }
+
+    // Retrieves the PID of the passed in connection on the launcher thread as to not assert.
+    public static int getConnectionPid(final BaseChildProcessConnection connection) {
+        return runOnLauncherAndGetResult(new Callable<Integer>() {
+            @Override
+            public Integer call() {
+                return connection.getPid();
+            }
+        });
+    }
+
+    // Retrieves the service number of the passed in connection on the launcher thread as to not
+    // assert.
+    public static int getConnectionServiceNumber(final BaseChildProcessConnection connection) {
+        return runOnLauncherAndGetResult(new Callable<Integer>() {
+            @Override
+            public Integer call() {
+                return connection.getServiceNumber();
+            }
+        });
+    }
+
+    // Retrieves the service of the passed in connection on the launcher thread as to not assert.
+    public static IChildProcessService getConnectionService(
+            final BaseChildProcessConnection connection) {
+        return runOnLauncherAndGetResult(new Callable<IChildProcessService>() {
+            @Override
+            public IChildProcessService call() {
+                return connection.getService();
+            }
+        });
+    }
+}