Bug 1404144 - 1. Refactor child process code to support preloading; r=rbarker
authorJim Chen <nchen@mozilla.com>
Wed, 04 Oct 2017 22:28:43 -0400
changeset 384603 5467f3e1c9a8a0715b4ce2896b54c37a8caa11f1
parent 384602 34fa5d45bc114d7310c78de366f05ed7fbd92410
child 384604 b54db050153281803b8afc83d734e80f29f61d7e
push id32631
push userarchaeopteryx@coole-files.de
push dateThu, 05 Oct 2017 08:51:33 +0000
treeherdermozilla-central@66042a706980 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersrbarker
bugs1404144
milestone58.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1404144 - 1. Refactor child process code to support preloading; r=rbarker Refactor the code in GeckoProcessManager and GeckoServiceChildProcess so that, we can have a ChildConnection object that's bound but not started. This way we can bind the connection to preload Gecko child process, but hold off starting until told by Gecko main process. Some code is simplified. For example, `IChildProcess.stop` is removed in favor of killing the child process directly. MozReview-Commit-ID: 4XgmTuT0IAs
mobile/android/geckoview/src/main/aidl/org/mozilla/gecko/process/IChildProcess.aidl
mobile/android/geckoview/src/main/java/org/mozilla/gecko/process/GeckoProcessManager.java
mobile/android/geckoview/src/main/java/org/mozilla/gecko/process/GeckoServiceChildProcess.java
--- a/mobile/android/geckoview/src/main/aidl/org/mozilla/gecko/process/IChildProcess.aidl
+++ b/mobile/android/geckoview/src/main/aidl/org/mozilla/gecko/process/IChildProcess.aidl
@@ -4,12 +4,11 @@
 
 package org.mozilla.gecko.process;
 
 import org.mozilla.gecko.process.IProcessManager;
 
 import android.os.ParcelFileDescriptor;
 
 interface IChildProcess {
-    void stop();
     int getPid();
-    void start(in IProcessManager procMan, in String[] args, in ParcelFileDescriptor crashReporterPfd, in ParcelFileDescriptor ipcPfd);
+    boolean start(in IProcessManager procMan, in String[] args, in ParcelFileDescriptor crashReporterPfd, in ParcelFileDescriptor ipcPfd);
 }
--- a/mobile/android/geckoview/src/main/java/org/mozilla/gecko/process/GeckoProcessManager.java
+++ b/mobile/android/geckoview/src/main/java/org/mozilla/gecko/process/GeckoProcessManager.java
@@ -9,28 +9,25 @@ import org.mozilla.gecko.IGeckoEditableP
 import org.mozilla.gecko.annotation.WrapForJNI;
 import org.mozilla.gecko.mozglue.GeckoLoader;
 import org.mozilla.gecko.util.ThreadUtils;
 
 import android.content.ComponentName;
 import android.content.Context;
 import android.content.Intent;
 import android.content.ServiceConnection;
-import android.os.DeadObjectException;
 import android.os.IBinder;
 import android.os.ParcelFileDescriptor;
+import android.os.Process;
 import android.os.RemoteException;
+import android.os.SystemClock;
 import android.support.v4.util.SimpleArrayMap;
-import android.view.Surface;
 import android.util.Log;
 
 import java.io.IOException;
-import java.util.Collections;
-import java.util.concurrent.TimeUnit;
-import java.util.Map;
 
 public final class GeckoProcessManager extends IProcessManager.Stub {
     private static final String LOGTAG = "GeckoProcessManager";
     private static final GeckoProcessManager INSTANCE = new GeckoProcessManager();
 
     public static GeckoProcessManager getInstance() {
         return INSTANCE;
     }
@@ -39,144 +36,185 @@ public final class GeckoProcessManager e
     private static native IGeckoEditableParent nativeGetEditableParent(long contentId,
                                                                        long tabId);
 
     @Override // IProcessManager
     public IGeckoEditableParent getEditableParent(final long contentId, final long tabId) {
         return nativeGetEditableParent(contentId, tabId);
     }
 
-    private static final class ChildConnection implements ServiceConnection, IBinder.DeathRecipient {
-        public final String mType;
-        private boolean mWait = false;
-        public IChildProcess mChild = null;
-        public int mPid = 0;
+    private static final class ChildConnection implements ServiceConnection,
+                                                          IBinder.DeathRecipient {
+        private static final int WAIT_TIMEOUT = 5000; // 5 seconds
+
+        private final String mType;
+        private boolean mWaiting;
+        private IChildProcess mChild;
+        private int mPid;
+
         public ChildConnection(String type) {
             mType = type;
         }
 
-        void prepareToWait() {
-            mWait = true;
+        public synchronized int getPid() {
+            if (mPid == 0) {
+                try {
+                    mPid = mChild.getPid();
+                } catch (final RemoteException e) {
+                    Log.e(LOGTAG, "Cannot get pid for " + mType, e);
+                }
+            }
+            return mPid;
         }
 
-        void waitForChild() {
-            ThreadUtils.assertNotOnUiThread();
-            synchronized (this) {
-                if (mWait) {
-                    try {
-                        this.wait(5000); // 5 seconds
-                    } catch (final InterruptedException e) {
-                        Log.e(LOGTAG, "Interrupted while waiting for child service", e);
-                    }
+        public synchronized IChildProcess bind() {
+            if (mChild != null) {
+                return mChild;
+            }
+
+            final Context context = GeckoAppShell.getApplicationContext();
+            final Intent intent = new Intent();
+            intent.setClassName(context,
+                                GeckoServiceChildProcess.class.getName() + '$' + mType);
+            GeckoLoader.addEnvironmentToIntent(intent);
+
+            if (context.bindService(intent, this, Context.BIND_AUTO_CREATE)) {
+                waitForChildLocked();
+                if (mChild != null) {
+                    return mChild;
                 }
             }
+
+            Log.e(LOGTAG, "Cannot connect to process " + mType);
+            context.unbindService(this);
+            return null;
+        }
+
+        public synchronized void unbind() {
+            final int pid = getPid();
+            if (pid != 0) {
+                Process.killProcess(pid);
+                waitForChildLocked();
+            }
         }
 
-        void clearWait() {
-            mWait = false;
+        private void waitForChildLocked() {
+            ThreadUtils.assertNotOnUiThread();
+            mWaiting = true;
+
+            final long startTime = SystemClock.uptimeMillis();
+            while (mWaiting && SystemClock.uptimeMillis() - startTime < WAIT_TIMEOUT) {
+                try {
+                    wait(WAIT_TIMEOUT);
+                } catch (final InterruptedException e) {
+                }
+            }
+            mWaiting = false;
         }
 
         @Override
-        public void onServiceConnected(ComponentName name, IBinder service) {
+        public synchronized void onServiceConnected(ComponentName name, IBinder service) {
             try {
                 service.linkToDeath(this, 0);
             } catch (final RemoteException e) {
-                Log.e(LOGTAG, "Failed to link ChildConnection to death of service.", e);
+                Log.e(LOGTAG, "Cannot link to death for " + mType, e);
             }
+
             mChild = IChildProcess.Stub.asInterface(service);
-            try {
-                mPid = mChild.getPid();
-            } catch (final RemoteException e) {
-                Log.e(LOGTAG, "Failed to get child " + mType + " process PID. Process may have died.", e);
-            }
-            synchronized (this) {
-                if (mWait) {
-                    mWait = false;
-                    this.notifyAll();
-                }
-            }
+            mWaiting = false;
+            notifyAll();
         }
 
         @Override
-        public void onServiceDisconnected(ComponentName name) {
-            synchronized (INSTANCE.mConnections) {
-                INSTANCE.mConnections.remove(mType);
-            }
-            synchronized (this) {
-                if (mWait) {
-                    mWait = false;
-                    this.notifyAll();
-                }
-            }
+        public synchronized void onServiceDisconnected(ComponentName name) {
+            mChild = null;
+            mPid = 0;
+            mWaiting = false;
+            notifyAll();
         }
 
         @Override
-        public void binderDied() {
-            Log.e(LOGTAG, "Binder died. Attempt to unbind service: " + mType + " " + mPid);
-            try {
+        public synchronized void binderDied() {
+            Log.i(LOGTAG, "Binder died for " + mType);
+            if (mChild != null) {
+                mChild = null;
                 GeckoAppShell.getApplicationContext().unbindService(this);
-            } catch (final java.lang.IllegalArgumentException e) {
-                Log.e(LOGTAG, "Looks like connection was already unbound", e);
             }
         }
     }
 
-    SimpleArrayMap<String, ChildConnection> mConnections;
+    private final SimpleArrayMap<String, ChildConnection> mConnections;
 
     private GeckoProcessManager() {
         mConnections = new SimpleArrayMap<String, ChildConnection>();
     }
 
-    public int start(String type, String[] args, int crashFd, int ipcFd) {
-        ChildConnection connection = null;
+    private ChildConnection getConnection(final String type) {
         synchronized (mConnections) {
-            connection = mConnections.get(type);
+            ChildConnection connection = mConnections.get(type);
+            if (connection == null) {
+                connection = new ChildConnection(type);
+                mConnections.put(type, connection);
+            }
+            return connection;
+        }
+    }
+
+    public void preload(final String... types) {
+        for (final String type : types) {
+            final ChildConnection connection = getConnection(type);
+            connection.bind();
+            connection.getPid();
         }
-        if (connection != null) {
-            Log.w(LOGTAG, "Attempting to start a child process service that is already running. Attempting to kill existing process first");
-            connection.prepareToWait();
-            try {
-                connection.mChild.stop();
-                connection.waitForChild();
-            } catch (final RemoteException e) {
-                connection.clearWait();
+    }
+
+    @WrapForJNI
+    private static int start(final String type, final String[] args,
+                             final int crashFd, final int ipcFd) {
+        return INSTANCE.start(type, args, crashFd, ipcFd, /* retry */ false);
+    }
+
+    private int start(final String type, final String[] args, final int crashFd,
+                      final int ipcFd, final boolean retry) {
+        final ChildConnection connection = getConnection(type);
+        final IChildProcess child = connection.bind();
+        if (child == null) {
+            return 0;
+        }
+
+        final ParcelFileDescriptor crashPfd;
+        final ParcelFileDescriptor ipcPfd;
+        try {
+            crashPfd = (crashFd >= 0) ? ParcelFileDescriptor.fromFd(crashFd) : null;
+            ipcPfd = ParcelFileDescriptor.fromFd(ipcFd);
+        } catch (final IOException e) {
+            Log.e(LOGTAG, "Cannot create fd for " + type, e);
+            return 0;
+        }
+
+        boolean started = false;
+        try {
+            started = child.start(this, args, crashPfd, ipcPfd);
+        } catch (final RemoteException e) {
+        }
+
+        if (!started) {
+            if (retry) {
+                Log.e(LOGTAG, "Cannot restart child " + type);
+                return 0;
             }
+            Log.w(LOGTAG, "Attempting to kill running child " + type);
+            connection.unbind();
+            return start(type, args, crashFd, ipcFd, /* retry */ true);
         }
 
         try {
-            connection = new ChildConnection(type);
-            Intent intent = new Intent();
-            intent.setClassName(GeckoAppShell.getApplicationContext(),
-                                "org.mozilla.gecko.process.GeckoServiceChildProcess$" + type);
-            GeckoLoader.addEnvironmentToIntent(intent);
-            connection.prepareToWait();
-            GeckoAppShell.getApplicationContext().bindService(intent, connection, Context.BIND_AUTO_CREATE);
-            connection.waitForChild();
-            if (connection.mChild == null) {
-                // FAILED TO CONNECT.
-                Log.e(LOGTAG, "Failed to connect to child process of '" + type + "'");
-                GeckoAppShell.getApplicationContext().unbindService(connection);
-                return 0;
-            }
-            ParcelFileDescriptor crashPfd = null;
-            if (crashFd >= 0) {
-                crashPfd = ParcelFileDescriptor.fromFd(crashFd);
-            }
-            ParcelFileDescriptor ipcPfd = ParcelFileDescriptor.fromFd(ipcFd);
-            connection.mChild.start(this, args, crashPfd, ipcPfd);
             if (crashPfd != null) {
                 crashPfd.close();
             }
             ipcPfd.close();
-            synchronized (mConnections) {
-                mConnections.put(type, connection);
-            }
-            return connection.mPid;
-        } catch (final RemoteException e) {
-            Log.e(LOGTAG, "Unable to create child process for: '" + type + "'. Remote Exception:", e);
         } catch (final IOException e) {
-            Log.e(LOGTAG, "Unable to create child process for: '" + type + "'. Error creating ParcelFileDescriptor needed to create intent:", e);
         }
 
-        return 0;
+        return connection.getPid();
     }
 
 } // GeckoProcessManager
--- a/mobile/android/geckoview/src/main/java/org/mozilla/gecko/process/GeckoServiceChildProcess.java
+++ b/mobile/android/geckoview/src/main/java/org/mozilla/gecko/process/GeckoServiceChildProcess.java
@@ -1,16 +1,15 @@
 /* -*- Mode: Java; c-basic-offset: 4; tab-width: 4; indent-tabs-mode: nil; -*-
  * This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 package org.mozilla.gecko.process;
 
-import org.mozilla.gecko.annotation.JNITarget;
 import org.mozilla.gecko.annotation.WrapForJNI;
 import org.mozilla.gecko.GeckoAppShell;
 import org.mozilla.gecko.IGeckoEditableParent;
 import org.mozilla.gecko.mozglue.GeckoLoader;
 import org.mozilla.gecko.GeckoThread;
 import org.mozilla.gecko.mozglue.SafeIntent;
 import org.mozilla.gecko.util.ThreadUtils;
 
@@ -24,94 +23,85 @@ import android.os.RemoteException;
 import android.util.Log;
 
 public class GeckoServiceChildProcess extends Service {
 
     static private String LOGTAG = "GeckoServiceChildProcess";
 
     private static IProcessManager sProcessManager;
 
-    static private void stop() {
-        ThreadUtils.postToUiThread(new Runnable() {
-            @Override
-            public void run() {
-                Process.killProcess(Process.myPid());
-            }
-        });
-    }
-
     @WrapForJNI(calledFrom = "gecko")
     private static IGeckoEditableParent getEditableParent(final long contentId,
                                                           final long tabId) {
         try {
             return sProcessManager.getEditableParent(contentId, tabId);
         } catch (final RemoteException e) {
             Log.e(LOGTAG, "Cannot get editable", e);
             return null;
         }
     }
 
+    @Override
     public void onCreate() {
         super.onCreate();
-    }
 
-    public void onDestroy() {
-        super.onDestroy();
+        GeckoAppShell.ensureCrashHandling();
+        GeckoAppShell.setApplicationContext(getApplicationContext());
     }
 
+    @Override
     public int onStartCommand(final Intent intent, final int flags, final int startId) {
-        return Service.START_STICKY;
+        return Service.START_NOT_STICKY;
     }
 
-    private Binder mBinder = new IChildProcess.Stub() {
-        @Override
-        public void stop() {
-            GeckoServiceChildProcess.stop();
-        }
-
+    private final Binder mBinder = new IChildProcess.Stub() {
         @Override
         public int getPid() {
             return Process.myPid();
         }
 
         @Override
-        public void start(final IProcessManager procMan,
-                          final String[] args,
-                          final ParcelFileDescriptor crashReporterPfd,
-                          final ParcelFileDescriptor ipcPfd) {
-            if (sProcessManager != null) {
-                Log.e(LOGTAG, "Attempting to start a service that has already been started.");
-                return;
+        public boolean start(final IProcessManager procMan,
+                             final String[] args,
+                             final ParcelFileDescriptor crashReporterPfd,
+                             final ParcelFileDescriptor ipcPfd) {
+            synchronized (GeckoServiceChildProcess.class) {
+                if (sProcessManager != null) {
+                    Log.e(LOGTAG, "Child process already started");
+                    return false;
+                }
+                sProcessManager = procMan;
             }
-            sProcessManager = procMan;
 
-            final int crashReporterFd = crashReporterPfd != null ? crashReporterPfd.detachFd() : -1;
+            final int crashReporterFd = crashReporterPfd != null ?
+                                        crashReporterPfd.detachFd() : -1;
             final int ipcFd = ipcPfd != null ? ipcPfd.detachFd() : -1;
+
             ThreadUtils.postToUiThread(new Runnable() {
                 @Override
                 public void run() {
-                    GeckoAppShell.ensureCrashHandling();
-                    GeckoAppShell.setApplicationContext(getApplicationContext());
                     if (GeckoThread.initChildProcess(args, crashReporterFd, ipcFd)) {
                         GeckoThread.launch();
                     }
                 }
             });
+            return true;
         }
     };
 
+    @Override
     public IBinder onBind(final Intent intent) {
         GeckoLoader.setLastIntent(new SafeIntent(intent));
+        GeckoThread.launch(); // Preload Gecko.
         return mBinder;
     }
 
+    @Override
     public boolean onUnbind(Intent intent) {
         Log.i(LOGTAG, "Service has been unbound. Stopping.");
-        stop();
+        Process.killProcess(Process.myPid());
         return false;
     }
 
-    @JNITarget
-    static public final class geckomediaplugin extends GeckoServiceChildProcess {}
+    public static final class geckomediaplugin extends GeckoServiceChildProcess {}
 
-    @JNITarget
-    static public final class tab extends GeckoServiceChildProcess {}
+    public static final class tab extends GeckoServiceChildProcess {}
 }