Moving some CPL logic to CPLH.

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

Index: content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncherHelper.java
diff --git a/content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncherHelper.java b/content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncherHelper.java
index d810afd8769e06992db363bbacc94e45eb8ab9f5..d15747aca7cd7801398636bea7911beeefe69471 100644
--- a/content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncherHelper.java
+++ b/content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncherHelper.java
@@ -5,14 +5,21 @@
 package org.chromium.content.browser;
 
 import android.content.Context;
+import android.os.Bundle;
+import android.os.IBinder;
 import android.os.ParcelFileDescriptor;
 
 import org.chromium.base.ContextUtils;
+import org.chromium.base.CpuFeatures;
 import org.chromium.base.Log;
+import org.chromium.base.VisibleForTesting;
 import org.chromium.base.annotations.CalledByNative;
 import org.chromium.base.annotations.JNINamespace;
+import org.chromium.base.library_loader.Linker;
 import org.chromium.base.process_launcher.ChildProcessCreationParams;
 import org.chromium.base.process_launcher.FileDescriptorInfo;
+import org.chromium.content.app.ChromiumLinkerParams;
+import org.chromium.content.common.ContentSwitches;
 
 import java.io.IOException;
 
@@ -22,14 +29,19 @@ import java.io.IOException;
  * Each public or jni methods should have explicit documentation on what threads they are called.
  */
 @JNINamespace("content::internal")
-class ChildProcessLauncherHelper {
+public class ChildProcessLauncherHelper {
     private static final String TAG = "ChildProcLH";
 
     // Represents an invalid process handle; same as base/process/process.h kNullProcessHandle.
     private static final int NULL_PROCESS_HANDLE = 0;
 
+    // The IBinder provided to the created service.
+    private final IBinder mIBinderCallback;
+
     // Note native pointer is only guaranteed live until nativeOnChildProcessStarted.
     private long mNativeChildProcessLauncherHelper;
+
+    // The actual service connection. Set once we have connected to the service.
     private ChildProcessConnection mChildProcessConnection;
 
     @CalledByNative
@@ -51,20 +63,86 @@ class ChildProcessLauncherHelper {
         return new FileDescriptorInfo(id, pFd, offset, size);
     }
 
+    @VisibleForTesting
     @CalledByNative
-    private static ChildProcessLauncherHelper create(long nativePointer, int paramId,
+    public static ChildProcessLauncherHelper createAndStart(long nativePointer, int paramId,
             final String[] commandLine, FileDescriptorInfo[] filesToBeMapped) {
         assert LauncherThread.runningOnLauncherThread();
-        return new ChildProcessLauncherHelper(nativePointer, paramId, commandLine, filesToBeMapped);
+        String processType =
+                ContentSwitches.getSwitchValue(commandLine, ContentSwitches.SWITCH_PROCESS_TYPE);
+
+        ChildProcessCreationParams params = ChildProcessCreationParams.get(paramId);
+        if (paramId != ChildProcessCreationParams.DEFAULT_ID && params == null) {
+            throw new RuntimeException("CreationParams id " + paramId + " not found");
+        }
+
+        Context context = ContextUtils.getApplicationContext();
+        boolean sandboxed = true;
+        boolean alwaysInForeground = false;
+        if (!ContentSwitches.SWITCH_RENDERER_PROCESS.equals(processType)) {
+            if (params != null && !params.getPackageName().equals(context.getPackageName())) {
+                // WebViews and WebAPKs have renderer processes running in their applications.
+                // When launching these renderer processes, {@link ManagedChildProcessConnection}
+                // requires the package name of the application which holds the renderer process.
+                // Therefore, the package name in ChildProcessCreationParams could be the package
+                // name of WebViews, WebAPKs, or Chrome, depending on the host application.
+                // Except renderer process, all other child processes should use Chrome's package
+                // name. In WebAPK, ChildProcessCreationParams are initialized with WebAPK's
+                // package name. Make a copy of the WebAPK's params, but replace the package with
+                // Chrome's package to use when initializing a non-renderer processes.
+                // TODO(boliu): Should fold into |paramId|. Investigate why this is needed.
+                params = new ChildProcessCreationParams(context.getPackageName(),
+                        params.getIsExternalService(), params.getLibraryProcessType(),
+                        params.getBindToCallerCheck());
+            }
+            if (ContentSwitches.SWITCH_GPU_PROCESS.equals(processType)) {
+                sandboxed = false;
+                alwaysInForeground = true;
+            } else {
+                // We only support sandboxed utility processes now.
+                assert ContentSwitches.SWITCH_UTILITY_PROCESS.equals(processType);
+            }
+        }
+
+        ChildProcessLauncherHelper process_launcher =
+                new ChildProcessLauncherHelper(nativePointer, processType);
+        process_launcher.start(
+                context, commandLine, filesToBeMapped, params, sandboxed, alwaysInForeground);
+        return process_launcher;
     }
 
-    private ChildProcessLauncherHelper(long nativePointer, int paramId, final String[] commandLine,
-            FileDescriptorInfo[] filesToBeMapped) {
+    @VisibleForTesting
+    public static ChildProcessLauncherHelper createAndStartForTesting(long nativePointer,

[boliu, 2017/05/15 21:38:22]

nit: put test-only methods at the very end

[Jay Civelli, 2017/05/17 16:42:50]

Done.

+            String[] commandLine, FileDescriptorInfo[] filesToBeMapped,
+            ChildProcessCreationParams creationParams, boolean sandboxed,
+            boolean alwaysInForeground) {
+        String processType =
+                ContentSwitches.getSwitchValue(commandLine, ContentSwitches.SWITCH_PROCESS_TYPE);
+        ChildProcessLauncherHelper launcherHelper =
+                new ChildProcessLauncherHelper(nativePointer, processType);
+        launcherHelper.start(ContextUtils.getApplicationContext(), commandLine, filesToBeMapped,
+                creationParams, sandboxed, alwaysInForeground);
+        return launcherHelper;
+    }
+
+    private ChildProcessLauncherHelper(long nativePointer, String processType) {
         assert LauncherThread.runningOnLauncherThread();
         mNativeChildProcessLauncherHelper = nativePointer;
+        mIBinderCallback = ContentSwitches.SWITCH_GPU_PROCESS.equals(processType)
+                ? new GpuProcessCallback()
+                : null;
+    }
+
+    private void start(Context context, String[] commandLine,
+            final FileDescriptorInfo[] filesToBeMapped, ChildProcessCreationParams params,
+            boolean sandboxed, boolean alwaysInForeground) {
+        boolean bindToCallerCheck = params == null ? false : params.getBindToCallerCheck();
+        Bundle serviceBundle = createServiceBundle(bindToCallerCheck);
+        onBeforeConnectionAllocated(serviceBundle);
 
-        ChildProcessLauncher.start(ContextUtils.getApplicationContext(), paramId, commandLine,
-                filesToBeMapped, new ChildProcessLauncher.LaunchCallback() {
+        Bundle connectionBundle = createConnectionBundle(commandLine, filesToBeMapped);
+        ChildProcessLauncher.start(context, serviceBundle,
+                connectionBundle, new ChildProcessLauncher.LaunchCallback() {
                     @Override
                     public void onChildProcessStarted(ChildProcessConnection connection) {
                         mChildProcessConnection = connection;
@@ -73,8 +151,16 @@ class ChildProcessLauncherHelper {
                                     mNativeChildProcessLauncherHelper, getPid());
                         }
                         mNativeChildProcessLauncherHelper = 0;
+                        // Proactively close the FDs rather than waiting for the GC to do it.
+                        try {

[boliu, 2017/05/15 21:38:22]

hmm, I guess this is the earliest in this class. But I'd still say move it to
above the native call

[Jay Civelli, 2017/05/17 16:42:50]

Done.

+                            for (FileDescriptorInfo fileInfo : filesToBeMapped) {
+                                fileInfo.fd.close();
+                            }
+                        } catch (IOException ioe) {
+                            Log.w(TAG, "Failed to close FD.", ioe);
+                        }
                     }
-                });
+                }, getIBinderCallback(), sandboxed, alwaysInForeground, params);
     }
 
     private int getPid() {
@@ -130,4 +216,79 @@ class ChildProcessLauncherHelper {
     // Can be called on a number of threads, including launcher, and binder.
     private static native void nativeOnChildProcessStarted(
             long nativeChildProcessLauncherHelper, int pid);
+
+    private static boolean sLinkerInitialized;
+    private static long sLinkerLoadAddress;
+
+    private static ChromiumLinkerParams getLinkerParamsForNewConnection() {

[boliu, 2017/05/15 21:38:22]

static method, can we have a thread assert?

[Jay Civelli, 2017/05/17 16:42:50]

Done.

+        if (!sLinkerInitialized) {
+            if (Linker.isUsed()) {
+                sLinkerLoadAddress = Linker.getInstance().getBaseLoadAddress();
+                if (sLinkerLoadAddress == 0) {
+                    Log.i(TAG, "Shared RELRO support disabled!");
+                }
+            }
+            sLinkerInitialized = true;
+        }
+
+        if (sLinkerLoadAddress == 0) return null;
+
+        // Always wait for the shared RELROs in service processes.
+        final boolean waitForSharedRelros = true;
+        if (Linker.areTestsEnabled()) {
+            Linker linker = Linker.getInstance();
+            return new ChromiumLinkerParams(sLinkerLoadAddress, waitForSharedRelros,
+                    linker.getTestRunnerClassNameForTesting(),
+                    linker.getImplementationForTesting());
+        } else {
+            return new ChromiumLinkerParams(sLinkerLoadAddress, waitForSharedRelros);
+        }
+    }
+
+    /**
+     * Creates the common bundle to be passed to child processes through the service binding intent.
+     * If the service gets recreated by the framework the intent will be reused, so these parameters
+     * should be common to all processes of that type.
+     *
+     * @param commandLine Command line params to be passed to the service.
+     * @param linkerParams Linker params to start the service.
+     */
+    // TODO(jcivelli): make private once warmup connection code is move from ChildProcessLauncher to
+    // this class.
+    static Bundle createServiceBundle(boolean bindToCallerCheck) {
+        Bundle bundle = new Bundle();
+        bundle.putBoolean(ChildProcessConstants.EXTRA_BIND_TO_CALLER, bindToCallerCheck);
+        return bundle;
+    }
+
+    @VisibleForTesting
+    public static Bundle createConnectionBundle(
+            String[] commandLine, FileDescriptorInfo[] filesToBeMapped) {
+        Bundle bundle = new Bundle();
+        bundle.putStringArray(ChildProcessConstants.EXTRA_COMMAND_LINE, commandLine);
+        bundle.putParcelableArray(ChildProcessConstants.EXTRA_FILES, filesToBeMapped);
+        // content specific parameters.
+        bundle.putInt(ChildProcessConstants.EXTRA_CPU_COUNT, CpuFeatures.getCount());
+        bundle.putLong(ChildProcessConstants.EXTRA_CPU_FEATURES, CpuFeatures.getMask());
+        bundle.putBundle(Linker.EXTRA_LINKER_SHARED_RELROS, Linker.getInstance().getSharedRelros());

[boliu, 2017/05/15 21:38:22]

I don't know this code, but does this need to be ordered with
getLinkerParamsForNewConnection? should we assert sLinkerInitialized. Previous
code constructed bundles in steps, and the getLinkerParamsForNewConnection step
always happens before setup connnection. But now this is no longer clear.

[Jay Civelli, 2017/05/17 16:42:50]

Good point. Now initializing the Linker in the constructor to make the
initialization order clearer and added asserts. 
(createConnectionBundle is only used from tests outside of this class, so we
should be safe)

+        return bundle;
+    }
+
+    // Below are methods that will eventually be moved to a content delegate class.
+
+    private void onBeforeConnectionAllocated(Bundle commonParameters) {
+        // Add content specific parameters.
+        commonParameters.putParcelable(
+                ChildProcessConstants.EXTRA_LINKER_PARAMS, getLinkerParamsForNewConnection());
+    }
+
+    private IBinder getIBinderCallback() {
+        return mIBinderCallback;
+    }
+
+    // Testing only related methods.
+    @VisibleForTesting
+    public ChildProcessConnection getChildProcessConnection() {
+        return mChildProcessConnection;
+    }
 }