Execute operations of ChildProcessLauncher on launcher thread

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

Index: content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java
diff --git a/content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java b/content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java
index 7f06d4cb72d110fc20dff887cd2714464b0fe5b3..e34c04efee6bea17464c96853456eb4227db747f 100644
--- a/content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java
+++ b/content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java
@@ -218,6 +218,8 @@ public class ChildProcessLauncher {
     private static ChildConnectionAllocator sSandboxedChildConnectionAllocator;
     private static ChildConnectionAllocator sPrivilegedChildConnectionAllocator;
 
+    private static boolean sOnTesting;
+
     private static final String NUM_SANDBOXED_SERVICES_KEY =
             "org.chromium.content.browser.NUM_SANDBOXED_SERVICES";
     private static final String NUM_PRIVILEGED_SERVICES_KEY =
@@ -279,7 +281,17 @@ public class ChildProcessLauncher {
         ChildProcessConnection.DeathCallback deathCallback =
                 new ChildProcessConnection.DeathCallback() {
                     @Override
-                    public void onChildProcessDied(ChildProcessConnection connection) {
+                    public void onChildProcessDied(final ChildProcessConnection connection) {
+                        boolean ret = runOnLauncherThread(new Runnable() {
+                            @Override
+                            public void run() {
+                                childProcessDied(connection);
+                            }
+                        });
+                        assert ret;
+                    }
+
+                    void childProcessDied(ChildProcessConnection connection) {
                         if (connection.getPid() != 0) {
                             stop(connection.getPid());
                         } else {
@@ -360,7 +372,7 @@ public class ChildProcessLauncher {
 
                 final PendingSpawnData pendingSpawn = sPendingSpawnQueue.dequeue();
                 if (pendingSpawn != null) {
-                    new Thread(new Runnable() {
+                    boolean ret = runOnLauncherThread(new Runnable() {
                         @Override
                         public void run() {
                             startInternal(pendingSpawn.context(), pendingSpawn.commandLine(),
@@ -368,7 +380,8 @@ public class ChildProcessLauncher {
                                     pendingSpawn.clientContext(), pendingSpawn.callbackType(),
                                     pendingSpawn.inSandbox());
                         }
-                    }).start();
+                    });
+                    assert ret;
                 }
             }
         }, FREE_CONNECTION_DELAY_MILLIS);
@@ -385,7 +398,14 @@ public class ChildProcessLauncher {
     private static ChildProcessConnection sSpareSandboxedConnection = null;
 
     // Manages oom bindings used to bind chind services.
-    private static BindingManager sBindingManager = BindingManagerImpl.createBindingManager();
+    private static BindingManager sBindingManager = BindingManagerImpl.createBindingManager(
+            new BindingManagerImpl.LauncherThreadExecutor() {
+                @Override
+                public void execute(Runnable runnable) {
+                    boolean ret = runOnLauncherThread(runnable);
+                    assert ret;
+                }
+            });
 
     // Map from surface id to Surface.
     private static Map<Integer, Surface> sViewSurfaceMap =
@@ -472,16 +492,28 @@ public class ChildProcessLauncher {
      * Called when the renderer commits a navigation. This signals a time at which it is safe to
      * rely on renderer visibility signalled through setInForeground. See http://crbug.com/421041.
      */
-    public static void determinedVisibility(int pid) {
-        sBindingManager.determinedVisibility(pid);
+    public static void determinedVisibility(final int pid) {
+        boolean ret = runOnLauncherThread(new Runnable() {
+            @Override
+            public void run() {
+                sBindingManager.determinedVisibility(pid);
+            }
+        });
+        assert ret;
     }
 
     /**
      * Called when the embedding application is sent to background.
      */
     public static void onSentToBackground() {
-        sApplicationInForeground = false;
-        sBindingManager.onSentToBackground();
+        Runnable task = new Runnable() {
+            @Override
+            public void run() {
+                sApplicationInForeground = false;
+                sBindingManager.onSentToBackground();
+            }
+        };
+        if (!runOnLauncherThread(task)) sApplicationInForeground = false;
     }
 
     /**
@@ -497,8 +529,14 @@ public class ChildProcessLauncher {
      * Called when the embedding application is brought to foreground.
      */
     public static void onBroughtToForeground() {
-        sApplicationInForeground = true;
-        sBindingManager.onBroughtToForeground();
+        Runnable task = new Runnable() {
+            @Override
+            public void run() {
+                sApplicationInForeground = true;
+                sBindingManager.onBroughtToForeground();
+            }
+        };
+        if (!runOnLauncherThread(task)) sApplicationInForeground = true;
     }
 
     /**
@@ -514,13 +552,21 @@ public class ChildProcessLauncher {
      * created in sandboxed child process.
      * @param context the application context used for the connection.
      */
-    public static void warmUp(Context context) {
-        synchronized (ChildProcessLauncher.class) {
-            assert !ThreadUtils.runningOnUiThread();
-            if (sSpareSandboxedConnection == null) {
-                sSpareSandboxedConnection = allocateBoundConnection(context, null, true, false);
+    public static void warmUp(final Context context) {
+        assert !ThreadUtils.runningOnUiThread();

[Yaron, 2015/09/04 16:04:04]

Hmm. Why bother asserting if you're just posting to another thread? This doesn't
do anything interesting anymore so I think it's fine to update callers to
execute on UI thread (they're all currently using AsyncTasks).

Oh now I see the comment below. So in the even that the process launcher thread
isn't available, we execute synchronously? Maybe the best bet is to put that in
an AsyncTask?

[Jaekyun Seok (inactive), 2015/09/08 09:45:12]

Done.

+        Runnable allocator = new Runnable() {
+            @Override
+            public void run() {
+                synchronized (ChildProcessLauncher.class) {
+                    if (sSpareSandboxedConnection == null) {
+                        sSpareSandboxedConnection =
+                                allocateBoundConnection(context, null, true, false);
+                    }
+                }
             }
-        }
+        };
+        // LauncherThread might not be ready. In that case, we use the current thread.
+        if (!runOnLauncherThread(allocator)) allocator.run();
     }
 
     private static String getSwitchValue(final String[] commandLine, String switchKey) {
@@ -642,7 +688,17 @@ public class ChildProcessLauncher {
         ChildProcessConnection.ConnectionCallback connectionCallback =
                 new ChildProcessConnection.ConnectionCallback() {
                     @Override
-                    public void onConnected(int pid) {
+                    public void onConnected(final int pid) {
+                        boolean ret = runOnLauncherThread(new Runnable() {
+                            @Override
+                            public void run() {
+                                connect(pid);
+                            }
+                        });
+                        assert ret;

[Yaron, 2015/09/04 16:04:04]

erm, asserts kinda suck. They don't even work on art (there's a bug filed for
this) but can you make this throw a RuntimeException instead (same throughout
your CL)

[Jaekyun Seok (inactive), 2015/09/08 09:45:12]

Done.

+                    }
+
+                    void connect(int pid) {
                         Log.d(TAG, "on connect callback, pid=%d context=%d callbackType=%d",
                                 pid, clientContext, callbackType);
                         if (pid != NULL_PROCESS_HANDLE) {
@@ -819,8 +875,27 @@ public class ChildProcessLauncher {
         return true;
     }
 
+    @CalledByNative
+    static void executeOnLauncherThread(Runnable runnable) {
+        runnable.run();
+    }
+
+    @VisibleForTesting
+    public static void setOnTesting(boolean onTesting) {
+        sOnTesting = onTesting;

[Yaron, 2015/09/04 16:04:04]

I'm not thrilled with this because it's no longer testing how things normally
work. Could try starting up the native PROCESS_LAUNCHER thread:
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/tes...
?

[Jaekyun Seok (inactive), 2015/09/08 09:45:12]

This is only for ChildProcessLauncherTest which is actually unittests for
ChildProcessLauncher; it doesn't test how things normally work anyway.
So I want keep this.

+    }
+
+    private static boolean runOnLauncherThread(Runnable runnable) {
+        if (sOnTesting) {
+            runnable.run();
+            return true;
+        }
+        return nativeRunOnLauncherThread(runnable);
+    }
+
     private static native void nativeOnChildProcessStarted(long clientContext, int pid);
     private static native void nativeEstablishSurfacePeer(
             int pid, Surface surface, int primaryID, int secondaryID);
     private static native boolean nativeIsSingleProcess();
+    private static native boolean nativeRunOnLauncherThread(Runnable runnable);
 }