android: Post onServiceConnected to launcher thread

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

 // Copyright 2012 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.browser;
 
 import android.content.Context;
 import android.os.Bundle;
 import android.os.IBinder;
 import android.os.RemoteException;
@@ skipping 118 matching lines @@
                 sBindingManager.releaseAllModerateBindings();
             }
         }
         return connection;
     }
 
     private static final long FREE_CONNECTION_DELAY_MILLIS = 1;
 
     private static void freeConnection(ChildProcessConnection connection) {
         assert LauncherThread.runningOnLauncherThread();
-        synchronized (sSpareConnectionLock) {
-            if (connection.equals(sSpareSandboxedConnection)) sSpareSandboxedConnection = null;
-        }
+        if (connection == sSpareSandboxedConnection) clearSpareConnection();
 
         // Freeing a service should be delayed. This is so that we avoid immediately reusing the
         // freed service (see http://crbug.com/164069): the framework might keep a service process
         // alive when it's been unbound for a short time. If a new connection to the same service
         // is bound at that point, the process is reused and bad things happen (mostly static
         // variables are set when we don't expect them to).
         final ChildProcessConnection conn = connection;
         LauncherThread.postDelayed(new Runnable() {
             @Override
             public void run() {
@@ skipping 25 matching lines @@
         }, FREE_CONNECTION_DELAY_MILLIS);
     }
 
     // Represents an invalid process handle; same as base/process/process.h kNullProcessHandle.
     private static final int NULL_PROCESS_HANDLE = 0;
 
     // Map from pid to ChildService connection.
     private static Map<Integer, ChildProcessConnection> sServiceMap =
             new ConcurrentHashMap<Integer, ChildProcessConnection>();
 
-    // Lock and monitor for these members {{{
-    private static final Object sSpareConnectionLock = new Object();
-    // A pre-allocated and pre-bound connection ready for connection setup, or null.
+    // These variables are used for the warm up sandboxed connection.
+    // |sSpareSandboxedConnection| is non-null when there is a pending connection. Note it's cleared
+    // to null again after the connection is used for a real child process.
+    // |sSpareConnectionStarting| is true if ChildProcessConnection.StartCallback has not fired.
+    // This is used for a child process allocation to determine if StartCallback should be chained.
+    // |sSpareConnectionStartCallback| is the chained StartCallback. This is also used to determine
+    // if there is already a child process launch that's used this this connection.
     private static ChildProcessConnection sSpareSandboxedConnection;
-    // If sSpareSandboxedConnection is not null, this indicates whether the service is
-    // ready for connection setup. Wait on the monitor lock to be notified when this
-    // state changes. sSpareSandboxedConnection may be null after waiting, if starting
-    // the service failed.
     private static boolean sSpareConnectionStarting;
-    // }}}
+    private static ChildProcessConnection.StartCallback sSpareConnectionStartCallback;
 
     // Manages oom bindings used to bind chind services.
     private static BindingManager sBindingManager = BindingManagerImpl.createBindingManager();
 
     // Whether the main application is currently brought to the foreground.
     private static boolean sApplicationInForeground = true;
 
     // TODO(boliu): This should be internal to content.
     public static BindingManager getBindingManager() {
         return sBindingManager;
@@ skipping 54 matching lines @@
     /**
      * Should be called early in startup so the work needed to spawn the child process can be done
      * in parallel to other startup work. Spare connection is created in sandboxed child process.
      * @param context the application context used for the connection.
      */
     public static void warmUp(final Context context) {
         assert ThreadUtils.runningOnUiThread();
         LauncherThread.post(new Runnable() {
             @Override
             public void run() {
-                synchronized (sSpareConnectionLock) {
-                    if (sSpareSandboxedConnection == null) {
-                        ChildProcessCreationParams params = ChildProcessCreationParams.getDefault();
-                        sSpareConnectionStarting = true;
+                if (sSpareSandboxedConnection != null) return;
+                ChildProcessCreationParams params = ChildProcessCreationParams.getDefault();
 
-                        ChildProcessConnection.StartCallback startCallback =
-                                new ChildProcessConnection.StartCallback() {
-                                    @Override
-                                    public void onChildStarted() {
-                                        synchronized (sSpareConnectionLock) {
-                                            sSpareConnectionStarting = false;
-                                            sSpareConnectionLock.notify();
-                                        }
-                                    }
+                ChildProcessConnection.StartCallback startCallback =
+                        new ChildProcessConnection.StartCallback() {
+                            @Override
+                            public void onChildStarted() {
+                                assert LauncherThread.runningOnLauncherThread();
+                                sSpareConnectionStarting = false;
+                                if (sSpareConnectionStartCallback != null) {
+                                    sSpareConnectionStartCallback.onChildStarted();
+                                    clearSpareConnection();

[Robert Sesek, 2017/04/17 18:29:16]

Why is this only done in the condition?

[boliu, 2017/04/17 18:34:18]

If there is no chained callback, then it means onChildStarted for the warm up
happened before anything tried to use the warm up. Clearing it in that case
would be like warming up a connection and then immediately throwing it away.

[Robert Sesek, 2017/04/17 18:37:04]

Acknowledged. Might be worth leaving a note since that's a little unobvious.

[boliu, 2017/04/17 18:57:13]

Added a comment.

+                                }
+                            }
 
-                                    @Override
-                                    public void onChildStartFailed() {
-                                        Log.e(TAG, "Failed to warm up the spare sandbox service");
-                                        synchronized (sSpareConnectionLock) {
-                                            sSpareSandboxedConnection = null;
-                                            sSpareConnectionStarting = false;
-                                            sSpareConnectionLock.notify();
-                                        }
-                                    }
-                                };
-                        ChildSpawnData spawnData = new ChildSpawnData(context,
-                                null /* commandLine */, -1 /* child process id */,
-                                null /* filesToBeMapped */, null /* launchCallback */,
-                                null /* child process callback */, true /* inSandbox */,
-                                SPARE_CONNECTION_ALWAYS_IN_FOREGROUND, params);
-                        sSpareSandboxedConnection = allocateBoundConnection(
-                                spawnData, startCallback, true /* forWarmUp */);
-                    }
-                }
+                            @Override
+                            public void onChildStartFailed() {
+                                assert LauncherThread.runningOnLauncherThread();
+                                Log.e(TAG, "Failed to warm up the spare sandbox service");
+                                if (sSpareConnectionStartCallback != null) {
+                                    sSpareConnectionStartCallback.onChildStartFailed();
+                                }
+                                clearSpareConnection();
+                            }
+                        };
+                ChildSpawnData spawnData = new ChildSpawnData(context, null /* commandLine */,
+                        -1 /* child process id */, null /* filesToBeMapped */,
+                        null /* launchCallback */, null /* child process callback */,
+                        true /* inSandbox */, SPARE_CONNECTION_ALWAYS_IN_FOREGROUND, params);
+                sSpareSandboxedConnection =
+                        allocateBoundConnection(spawnData, startCallback, true /* forWarmUp */);
+                sSpareConnectionStarting = sSpareSandboxedConnection != null;
             }
         });
     }
 
+    private static void clearSpareConnection() {
+        assert LauncherThread.runningOnLauncherThread();
+        sSpareSandboxedConnection = null;
+        sSpareConnectionStarting = false;
+        sSpareConnectionStartCallback = null;
+    }
+
     /**
      * Spawns and connects to a child process. May be called on any thread. It will not block, but
      * will instead callback to {@link #nativeOnChildProcessStarted} when the connection is
      * established. Note this callback will not necessarily be from the same thread (currently it
      * always comes from the main thread).
      *
      * @param context Context used to obtain the application context.
      * @param paramId Key used to retrieve ChildProcessCreationParams.
      * @param commandLine The child process command line argv.
      * @param filesToBeMapped File IDs, FDs, offsets, and lengths to pass through.
@@ skipping 46 matching lines @@
             final FileDescriptorInfo[] filesToBeMapped, final LaunchCallback launchCallback,
             final IBinder childProcessCallback, final boolean inSandbox,
             final boolean alwaysInForeground, final ChildProcessCreationParams creationParams) {
         assert LauncherThread.runningOnLauncherThread();
         try {
             TraceEvent.begin("ChildProcessLauncher.startInternal");
 
             ChildProcessConnection allocatedConnection = null;
             String packageName = creationParams != null ? creationParams.getPackageName()
                     : context.getPackageName();
-            synchronized (sSpareConnectionLock) {
-                if (inSandbox && sSpareSandboxedConnection != null
-                        && SPARE_CONNECTION_ALWAYS_IN_FOREGROUND == alwaysInForeground
-                        && sSpareSandboxedConnection.getPackageName().equals(packageName)
-                        // Object identity check for getDefault should be enough. The default is
-                        // not supposed to change once set.
-                        && creationParams == ChildProcessCreationParams.getDefault()) {
-                    while (sSpareConnectionStarting) {
-                        try {
-                            sSpareConnectionLock.wait();
-                        } catch (InterruptedException ex) {
+            ChildProcessConnection.StartCallback startCallback =
+                    new ChildProcessConnection.StartCallback() {
+                        @Override
+                        public void onChildStarted() {}
+
+                        @Override
+                        public void onChildStartFailed() {
+                            assert LauncherThread.runningOnLauncherThread();
+                            Log.e(TAG, "ChildProcessConnection.start failed, trying again");
+                            LauncherThread.post(new Runnable() {
+                                @Override
+                                public void run() {
+                                    // The child process may already be bound to another client
+                                    // (this can happen if multi-process WebView is used in more
+                                    // than one process), so try starting the process again.
+                                    // This connection that failed to start has not been freed,
+                                    // so a new bound connection will be allocated.
+                                    startInternal(context, commandLine, childProcessId,
+                                            filesToBeMapped, launchCallback, childProcessCallback,
+                                            inSandbox, alwaysInForeground, creationParams);
+                                }
+                            });
                         }
-                    }
-                    allocatedConnection = sSpareSandboxedConnection;
-                    sSpareSandboxedConnection = null;
+                    };
+
+            if (inSandbox && sSpareSandboxedConnection != null
+                    && sSpareConnectionStartCallback == null
+                    && SPARE_CONNECTION_ALWAYS_IN_FOREGROUND == alwaysInForeground
+                    && sSpareSandboxedConnection.getPackageName().equals(packageName)
+                    // Object identity check for getDefault should be enough. The default is
+                    // not supposed to change once set.
+                    && creationParams == ChildProcessCreationParams.getDefault()) {
+                allocatedConnection = sSpareSandboxedConnection;
+                if (sSpareConnectionStarting) {
+                    sSpareConnectionStartCallback = startCallback;
+                } else {
+                    clearSpareConnection();
                 }
             }
             if (allocatedConnection == null) {
-                ChildProcessConnection.StartCallback startCallback =
-                        new ChildProcessConnection.StartCallback() {
-                            @Override
-                            public void onChildStarted() {}
-
-                            @Override
-                            public void onChildStartFailed() {
-                                Log.e(TAG, "ChildProcessConnection.start failed, trying again");
-                                LauncherThread.post(new Runnable() {
-                                    @Override
-                                    public void run() {
-                                        // The child process may already be bound to another client
-                                        // (this can happen if multi-process WebView is used in more
-                                        // than one process), so try starting the process again.
-                                        // This connection that failed to start has not been freed,
-                                        // so a new bound connection will be allocated.
-                                        startInternal(context, commandLine, childProcessId,
-                                                filesToBeMapped, launchCallback,
-                                                childProcessCallback, inSandbox, alwaysInForeground,
-                                                creationParams);
-                                    }
-                                });
-                            }
-                        };
 
                 ChildSpawnData spawnData = new ChildSpawnData(context, commandLine, childProcessId,
                         filesToBeMapped, launchCallback, childProcessCallback, inSandbox,
                         alwaysInForeground, creationParams);
                 allocatedConnection =
                         allocateBoundConnection(spawnData, startCallback, false /* forWarmUp */);
                 if (allocatedConnection == null) {
                     return null;
                 }
             }
 
-            Log.d(TAG, "Setting up connection to process: slot=%d",
-                    allocatedConnection.getServiceNumber());
             triggerConnectionSetup(allocatedConnection, commandLine, childProcessId,
                     filesToBeMapped, childProcessCallback, launchCallback);
             return allocatedConnection;
         } finally {
             TraceEvent.end("ChildProcessLauncher.startInternal");
         }
     }
 
     /**
      * Create the common bundle to be passed to child processes.
@@ skipping 10 matching lines @@
         bundle.putLong(ChildProcessConstants.EXTRA_CPU_FEATURES, CpuFeatures.getMask());
         bundle.putBundle(Linker.EXTRA_LINKER_SHARED_RELROS, Linker.getInstance().getSharedRelros());
         return bundle;
     }
 
     @VisibleForTesting
     static void triggerConnectionSetup(final ChildProcessConnection connection,
             String[] commandLine, int childProcessId, FileDescriptorInfo[] filesToBeMapped,
             final IBinder childProcessCallback, final LaunchCallback launchCallback) {
         assert LauncherThread.runningOnLauncherThread();
+        Log.d(TAG, "Setting up connection to process: slot=%d", connection.getServiceNumber());
         ChildProcessConnection.ConnectionCallback connectionCallback =
                 new ChildProcessConnection.ConnectionCallback() {
                     @Override
                     public void onConnected(int pid) {
                         Log.d(TAG, "on connect callback, pid=%d", pid);
                         if (pid != NULL_PROCESS_HANDLE) {
                             sBindingManager.addNewConnection(pid, connection);
                             sServiceMap.put(pid, connection);
                         }
                         // If the connection fails and pid == 0, the Java-side cleanup was already
@@ skipping 43 matching lines @@
 
         try {
             ((ChildProcessConnectionImpl) sServiceMap.get(pid)).crashServiceForTesting();
         } catch (RemoteException ex) {
             return false;
         }
 
         return true;
     }
 }