android: Move PendingSpawnQueue into ChildConnectionAllocator

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 1a8996c81a8bbc549361a8b4ac3df2fdc6125d39..1a4546a210a084b357b4df283bcb81b40d243307 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
@@ -42,6 +42,10 @@ import java.util.concurrent.ConcurrentHashMap;
 
 /**
  * This class provides the method to start/stop ChildProcess called by native.
+ *
+ * Note about threading. The threading here is complicated and not well documented.
+ * Code can run on these threads: UI, Launcher, async thread pool, binder, and one-off
+ * background threads.
  */
 @JNINamespace("content")
 public class ChildProcessLauncher {
@@ -65,7 +69,7 @@ public class ChildProcessLauncher {
         private final boolean mInSandbox;
         // Each Allocator keeps a queue for the pending spawn data. Once a connection is free, we
         // dequeue the pending spawn data from the same allocator as the connection.
-        private final PendingSpawnQueue mPendingSpawnQueue = new PendingSpawnQueue();
+        private final Queue<SpawnData> mPendingSpawnQueue = new LinkedList<>();

[Maria, 2017/02/14 00:59:41]

Should this comment also mention that this uses mConnectionLock to synchronize?

[boliu, 2017/02/14 01:34:20]

Done.

 
         public ChildConnectionAllocator(boolean inSandbox, int numChildServices,
                 String serviceClassName) {
@@ -78,28 +82,32 @@ public class ChildProcessLauncher {
             mInSandbox = inSandbox;
         }
 
-        public ChildProcessConnection allocate(
-                Context context, ChildProcessConnection.DeathCallback deathCallback,
-                ChromiumLinkerParams chromiumLinkerParams,
-                boolean alwaysInForeground,
-                ChildProcessCreationParams creationParams) {
+        // Allocate or enqueue. If there are no free slots, return null and enqueue the spawn data.
+        public ChildProcessConnection allocate(SpawnData spawnData,
+                ChildProcessConnection.DeathCallback deathCallback,
+                ChromiumLinkerParams chromiumLinkerParams, boolean alwaysInForeground) {
+            assert spawnData.inSandbox() == mInSandbox;
             synchronized (mConnectionLock) {
                 if (mFreeConnectionIndices.isEmpty()) {
                     Log.d(TAG, "Ran out of services to allocate.");
+                    if (!spawnData.forWarmUp()) {
+                        mPendingSpawnQueue.add(spawnData);
+                    }
                     return null;
                 }
                 int slot = mFreeConnectionIndices.remove(0);
                 assert mChildProcessConnections[slot] == null;
-                mChildProcessConnections[slot] = new ChildProcessConnectionImpl(context, slot,
-                        mInSandbox, deathCallback, mChildClassName, chromiumLinkerParams,
-                        alwaysInForeground, creationParams);
+                mChildProcessConnections[slot] = new ChildProcessConnectionImpl(spawnData.context(),
+                        slot, mInSandbox, deathCallback, mChildClassName, chromiumLinkerParams,
+                        alwaysInForeground, spawnData.getCreationParams());
                 Log.d(TAG, "Allocator allocated a connection, sandbox: %b, slot: %d", mInSandbox,
                         slot);
                 return mChildProcessConnections[slot];
             }
         }
 
-        public void free(ChildProcessConnection connection) {
+        // Also return the first SpawnData in the pending queue, if any.
+        public SpawnData free(ChildProcessConnection connection) {
             synchronized (mConnectionLock) {
                 int slot = connection.getServiceNumber();
                 if (mChildProcessConnections[slot] != connection) {
@@ -115,6 +123,7 @@ public class ChildProcessLauncher {
                     Log.d(TAG, "Allocator freed a connection, sandbox: %b, slot: %d", mInSandbox,
                             slot);
                 }
+                return mPendingSpawnQueue.poll();
             }
         }
 
@@ -124,10 +133,6 @@ public class ChildProcessLauncher {
             }
         }
 
-        public PendingSpawnQueue getPendingSpawnQueue() {
-            return mPendingSpawnQueue;
-        }
-
         /** @return the count of connections managed by the allocator */
         @VisibleForTesting
         int allocatedConnectionsCountForTesting() {
@@ -138,9 +143,24 @@ public class ChildProcessLauncher {
         ChildProcessConnection[] connectionArrayForTesting() {
             return mChildProcessConnections;
         }
+
+        @VisibleForTesting
+        void enqueuePendingQueueForTesting(SpawnData spawnData) {
+            synchronized (mConnectionLock) {
+                mPendingSpawnQueue.add(spawnData);
+            }
+        }
+
+        @VisibleForTesting
+        int pendingSpawnsCountForTesting() {
+            synchronized (mConnectionLock) {
+                return mPendingSpawnQueue.size();
+            }
+        }
     }
 
-    private static class PendingSpawnData {
+    private static class SpawnData {
+        private final boolean mForWarmup; // Do not queue up if failed.
         private final Context mContext;
         private final String[] mCommandLine;
         private final int mChildProcessId;
@@ -150,15 +170,10 @@ public class ChildProcessLauncher {
         private final boolean mInSandbox;
         private final ChildProcessCreationParams mCreationParams;
 
-        private PendingSpawnData(
-                Context context,
-                String[] commandLine,
-                int childProcessId,
-                FileDescriptorInfo[] filesToBeMapped,
-                long clientContext,
-                int callbackType,
-                boolean inSandbox,
-                ChildProcessCreationParams creationParams) {
+        private SpawnData(boolean forWarmUp, Context context, String[] commandLine,
+                int childProcessId, FileDescriptorInfo[] filesToBeMapped, long clientContext,
+                int callbackType, boolean inSandbox, ChildProcessCreationParams creationParams) {
+            mForWarmup = forWarmUp;
             mContext = context;
             mCommandLine = commandLine;
             mChildProcessId = childProcessId;
@@ -169,6 +184,10 @@ public class ChildProcessLauncher {
             mCreationParams = creationParams;
         }
 
+        private boolean forWarmUp() {
+            return mForWarmup;
+        }
+
         private Context context() {
             return mContext;
         }
@@ -195,37 +214,6 @@ public class ChildProcessLauncher {
         }
     }
 
-    private static class PendingSpawnQueue {
-        // The list of pending process spawn requests and its lock.
-        // Operations on this queue must be atomical w.r.t. free connections updates.
-        private Queue<PendingSpawnData> mPendingSpawns = new LinkedList<PendingSpawnData>();
-        final Object mPendingSpawnsLock = new Object();
-
-        /**
-         * Queue up a spawn requests to be processed once a free service is available.
-         * Called when a spawn is requested while we are at the capacity.
-         */
-        public void enqueueLocked(final PendingSpawnData pendingSpawn) {
-            assert Thread.holdsLock(mPendingSpawnsLock);
-            mPendingSpawns.add(pendingSpawn);
-        }
-
-        /**
-         * Pop the next request from the queue. Called when a free service is available.
-         * @return the next spawn request waiting in the queue.
-         */
-        public PendingSpawnData dequeueLocked() {
-            assert Thread.holdsLock(mPendingSpawnsLock);
-            return mPendingSpawns.poll();
-        }
-
-        /** @return the count of pending spawns in the queue */
-        public int sizeLocked() {
-            assert Thread.holdsLock(mPendingSpawnsLock);
-            return mPendingSpawns.size();
-        }
-    }
-
     // Service class for child process.
     // Map from package name to ChildConnectionAllocator.
     private static Map<String, ChildConnectionAllocator> sSandboxedChildConnectionAllocatorMap;
@@ -356,18 +344,14 @@ public class ChildProcessLauncher {
         return sSandboxedChildConnectionAllocatorMap.get(packageName);
     }
 
-    /**
-     * Get the PendingSpawnQueue of the Allocator. Initialize the Allocator if needed.
-     */
-    private static PendingSpawnQueue getPendingSpawnQueue(Context context, String packageName,
-            boolean inSandbox) {
+    private static ChildConnectionAllocator getAllocatorForTesting(
+            Context context, String packageName, boolean inSandbox) {
         initConnectionAllocatorsIfNecessary(context, inSandbox, packageName);
-        return getConnectionAllocator(packageName, inSandbox).getPendingSpawnQueue();
+        return getConnectionAllocator(packageName, inSandbox);
     }
 
-    private static ChildProcessConnection allocateConnection(Context context, boolean inSandbox,
-            ChromiumLinkerParams chromiumLinkerParams, boolean alwaysInForeground,
-            ChildProcessCreationParams creationParams) {
+    private static ChildProcessConnection allocateConnection(SpawnData spawnData,
+            ChromiumLinkerParams chromiumLinkerParams, boolean alwaysInForeground) {
         ChildProcessConnection.DeathCallback deathCallback =
                 new ChildProcessConnection.DeathCallback() {
                     @Override
@@ -379,12 +363,14 @@ public class ChildProcessLauncher {
                         }
                     }
                 };
-        String packageName = creationParams != null ? creationParams.getPackageName()
-                : context.getPackageName();
+        final ChildProcessCreationParams creationParams = spawnData.getCreationParams();
+        final Context context = spawnData.context();
+        final boolean inSandbox = spawnData.inSandbox();
+        String packageName =
+                creationParams != null ? creationParams.getPackageName() : context.getPackageName();
         initConnectionAllocatorsIfNecessary(context, inSandbox, packageName);
         return getConnectionAllocator(packageName, inSandbox)
-                .allocate(context, deathCallback, chromiumLinkerParams, alwaysInForeground,
-                        creationParams);
+                .allocate(spawnData, deathCallback, chromiumLinkerParams, alwaysInForeground);
     }
 
     private static boolean sLinkerInitialized;
@@ -417,20 +403,23 @@ public class ChildProcessLauncher {
         }
     }
 
-    private static ChildProcessConnection allocateBoundConnection(Context context,
-            String[] commandLine, boolean inSandbox, boolean alwaysInForeground,
-            ChildProcessCreationParams creationParams,
-            ChildProcessConnection.StartCallback startCallback) {
+    private static ChildProcessConnection allocateBoundConnection(SpawnData spawnData,
+            boolean alwaysInForeground, ChildProcessConnection.StartCallback startCallback) {
+        final Context context = spawnData.context();
+        final String[] commandLine = spawnData.commandLine();
+        final boolean inSandbox = spawnData.inSandbox();
+        final ChildProcessCreationParams creationParams = spawnData.getCreationParams();
         ChromiumLinkerParams chromiumLinkerParams = getLinkerParamsForNewConnection();
-        ChildProcessConnection connection = allocateConnection(
-                context, inSandbox, chromiumLinkerParams, alwaysInForeground, creationParams);
+        ChildProcessConnection connection =
+                allocateConnection(spawnData, chromiumLinkerParams, alwaysInForeground);
         if (connection != null) {
             connection.start(commandLine, startCallback);
 
             String packageName = creationParams != null ? creationParams.getPackageName()
-                    : context.getPackageName();
-            if (inSandbox && !getConnectionAllocator(packageName, inSandbox)
-                    .isFreeConnectionAvailable()) {
+                                                        : context.getPackageName();
+            if (inSandbox
+                    && !getConnectionAllocator(packageName, inSandbox)
+                                .isFreeConnectionAvailable()) {
                 // Proactively releases all the moderate bindings once all the sandboxed services
                 // are allocated, which will be very likely to have some of them killed by OOM
                 // killer.
@@ -456,7 +445,7 @@ public class ChildProcessLauncher {
         ThreadUtils.postOnUiThreadDelayed(new Runnable() {
             @Override
             public void run() {
-                final PendingSpawnData pendingSpawn = freeConnectionAndDequeuePending(conn);
+                final SpawnData pendingSpawn = freeConnectionAndDequeuePending(conn);
                 if (pendingSpawn != null) {
                     new Thread(new Runnable() {
                         @Override
@@ -472,15 +461,11 @@ public class ChildProcessLauncher {
         }, FREE_CONNECTION_DELAY_MILLIS);
     }
 
-    private static PendingSpawnData freeConnectionAndDequeuePending(ChildProcessConnection conn) {
+    private static SpawnData freeConnectionAndDequeuePending(ChildProcessConnection conn) {
         ChildConnectionAllocator allocator = getConnectionAllocator(
                 conn.getPackageName(), conn.isInSandbox());
         assert allocator != null;
-        PendingSpawnQueue pendingSpawnQueue = allocator.getPendingSpawnQueue();
-        synchronized (pendingSpawnQueue.mPendingSpawnsLock) {
-            allocator.free(conn);
-            return pendingSpawnQueue.dequeueLocked();
-        }
+        return allocator.free(conn);
     }
 
     // Represents an invalid process handle; same as base/process/process.h kNullProcessHandle.
@@ -612,8 +597,12 @@ public class ChildProcessLauncher {
                                 }
                             }
                         };
-                sSpareSandboxedConnection = allocateBoundConnection(context, null, true, false,
-                        params, startCallback);
+                SpawnData spawnData = new SpawnData(true /* forWarmUp*/, context,
+                        null /* commandLine */, -1 /* child process id */,
+                        null /* filesToBeMapped */, 0 /* clientContext */,
+                        CALLBACK_FOR_RENDERER_PROCESS, true /* inSandbox */, params);

[Maria, 2017/02/14 00:59:41]

these new calls with a bunch of comments make me think we should consider
builder pattern. Not for this CL, but maybe later?

[boliu, 2017/02/14 01:34:20]

Hmm.. If we want to force caller to make a decision instead of taking default,
then builder doesn't help. Don't really know if that's important or not though.

Anyway, aesthetics is pretty low on my list of things to fix :p

+                sSpareSandboxedConnection = allocateBoundConnection(
+                        spawnData, false /* alwaysInForeground */, startCallback);
             }
         }
     }
@@ -722,8 +711,6 @@ public class ChildProcessLauncher {
             if (allocatedConnection == null) {
                 boolean alwaysInForeground = false;
                 if (callbackType == CALLBACK_FOR_GPU_PROCESS) alwaysInForeground = true;
-                PendingSpawnQueue pendingSpawnQueue = getPendingSpawnQueue(
-                        context, packageName, inSandbox);
                 ChildProcessConnection.StartCallback startCallback =
                         new ChildProcessConnection.StartCallback() {
                             @Override
@@ -747,17 +734,14 @@ public class ChildProcessLauncher {
                                 });
                             }
                         };
-                synchronized (pendingSpawnQueue.mPendingSpawnsLock) {
-                    allocatedConnection = allocateBoundConnection(
-                            context, commandLine, inSandbox, alwaysInForeground, creationParams,
-                            startCallback);
-                    if (allocatedConnection == null) {
-                        Log.d(TAG, "Allocation of new service failed. Queuing up pending spawn.");
-                        pendingSpawnQueue.enqueueLocked(new PendingSpawnData(context, commandLine,
-                                childProcessId, filesToBeMapped, clientContext,
-                                callbackType, inSandbox, creationParams));
-                        return null;
-                    }
+
+                SpawnData spawnData = new SpawnData(false /* forWarmUp */, context, commandLine,
+                        childProcessId, filesToBeMapped, clientContext, callbackType, inSandbox,
+                        creationParams);
+                allocatedConnection =
+                        allocateBoundConnection(spawnData, alwaysInForeground, startCallback);
+                if (allocatedConnection == null) {
+                    return null;
                 }
             }
 
@@ -893,14 +877,21 @@ public class ChildProcessLauncher {
     @VisibleForTesting
     static ChildProcessConnection allocateBoundConnectionForTesting(Context context,
             ChildProcessCreationParams creationParams) {
-        return allocateBoundConnection(context, null, true, false, creationParams, null);
+        return allocateBoundConnection(
+                new SpawnData(false /* forWarmUp */, context, null /* commandLine */,
+                        0 /* childProcessId */, null /* filesToBeMapped */, 0 /* clientContext */,
+                        CALLBACK_FOR_RENDERER_PROCESS, true /* inSandbox */, creationParams),
+                false /* alwaysInForeground */, null);
     }
 
     @VisibleForTesting
     static ChildProcessConnection allocateConnectionForTesting(Context context,
             ChildProcessCreationParams creationParams) {
         return allocateConnection(
-                context, true, getLinkerParamsForNewConnection(), false, creationParams);
+                new SpawnData(false /* forWarmUp */, context, null /* commandLine */,
+                        0 /* childProcessId */, null /* filesToBeMapped */, 0 /* clientContext */,
+                        CALLBACK_FOR_RENDERER_PROCESS, true /* inSandbox */, creationParams),
+                getLinkerParamsForNewConnection(), false);
     }
 
     /**
@@ -911,13 +902,11 @@ public class ChildProcessLauncher {
             ChildProcessCreationParams creationParams, boolean inSandbox) {
         String packageName = creationParams != null ? creationParams.getPackageName()
                 : context.getPackageName();
-        PendingSpawnQueue pendingSpawnQueue = getPendingSpawnQueue(context,
-                packageName, inSandbox);
-        synchronized (pendingSpawnQueue.mPendingSpawnsLock) {
-            pendingSpawnQueue.enqueueLocked(new PendingSpawnData(context, commandLine, 1,
-                    new FileDescriptorInfo[0], 0, CALLBACK_FOR_RENDERER_PROCESS, true,
-                    creationParams));
-        }
+        ChildConnectionAllocator allocator =
+                getAllocatorForTesting(context, packageName, inSandbox);
+        allocator.enqueuePendingQueueForTesting(new SpawnData(false /* forWarmUp*/, context,
+                commandLine, 1, new FileDescriptorInfo[0], 0, CALLBACK_FOR_RENDERER_PROCESS, true,
+                creationParams));
     }
 
     /**
@@ -935,8 +924,7 @@ public class ChildProcessLauncher {
      * @return gets the service connection array for a specific package name.
      */
     @VisibleForTesting
-    static ChildProcessConnection[] getSandboxedConnectionArrayForTesting(
-            String packageName) {
+    static ChildProcessConnection[] getSandboxedConnectionArrayForTesting(String packageName) {
         return sSandboxedChildConnectionAllocatorMap.get(packageName).connectionArrayForTesting();
     }
 
@@ -953,12 +941,11 @@ public class ChildProcessLauncher {
      * @return the count of pending spawns in the queue.
      */
     @VisibleForTesting
-    static int pendingSpawnsCountForTesting(Context context, String packageName,
-            boolean inSandbox) {
-        PendingSpawnQueue pendingSpawnQueue = getPendingSpawnQueue(context, packageName, inSandbox);
-        synchronized (pendingSpawnQueue.mPendingSpawnsLock) {
-            return pendingSpawnQueue.sizeLocked();
-        }
+    static int pendingSpawnsCountForTesting(
+            Context context, String packageName, boolean inSandbox) {
+        ChildConnectionAllocator allocator =
+                getAllocatorForTesting(context, packageName, inSandbox);
+        return allocator.pendingSpawnsCountForTesting();
     }
 
     /**