Bug 1465480 - Invalidate cached pid if child process dies r=jchen draft
authorJames Willcox <snorp@snorp.net>
Wed, 30 May 2018 12:58:57 -0500
changeset 804894 6b35cd1a9d4016b7f411abf3640361d3fa156ee3
parent 804893 f997e7c7168d674d0c009fde87512899ce3e605b
child 804895 aa6477eb011817a63687b5e03ab5f46bba6631a0
push id112488
push userbmo:snorp@snorp.net
push dateWed, 06 Jun 2018 17:59:13 +0000
reviewersjchen
bugs1465480
milestone62.0a1
Bug 1465480 - Invalidate cached pid if child process dies r=jchen This also cleans up a few unused methods. MozReview-Commit-ID: AMIcNzKp44N
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
@@ -10,11 +10,9 @@ import android.os.Bundle;
 import android.os.ParcelFileDescriptor;
 
 interface IChildProcess {
     int getPid();
     boolean start(in IProcessManager procMan, in String[] args, in Bundle extras, int flags,
                   in ParcelFileDescriptor prefsPfd, in ParcelFileDescriptor ipcPfd,
                   in ParcelFileDescriptor crashReporterPfd,
                   in ParcelFileDescriptor crashAnnotationPfd);
-
-    void crash();
 }
--- 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
@@ -5,24 +5,24 @@
 package org.mozilla.gecko.process;
 
 import org.mozilla.gecko.GeckoAppShell;
 import org.mozilla.gecko.GeckoThread;
 import org.mozilla.gecko.IGeckoEditableParent;
 import org.mozilla.gecko.annotation.WrapForJNI;
 import org.mozilla.gecko.util.ThreadUtils;
 
+import android.app.Service;
 import android.content.ComponentName;
 import android.content.Context;
 import android.content.Intent;
 import android.content.ServiceConnection;
 import android.os.Bundle;
 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.util.Log;
 
 import java.io.IOException;
 
 public final class GeckoProcessManager extends IProcessManager.Stub {
@@ -42,65 +42,83 @@ public final class GeckoProcessManager e
         return nativeGetEditableParent(contentId, tabId);
     }
 
     private static final class ChildConnection implements ServiceConnection,
                                                           IBinder.DeathRecipient {
         private static final int WAIT_TIMEOUT = 5000; // 5 seconds
 
         private final String mType;
+        private final Intent mIntent;
         private boolean mWaiting;
         private IChildProcess mChild;
         private int mPid;
 
         public ChildConnection(String type) {
             mType = type;
+
+            mIntent = new Intent();
+            mIntent.setClassName(GeckoAppShell.getApplicationContext(),
+                    GeckoServiceChildProcess.class.getName() + '$' + mType);
         }
 
         public synchronized int getPid() {
             if ((mPid == 0) && (mChild != null)) {
                 try {
                     mPid = mChild.getPid();
                 } catch (final RemoteException e) {
                     Log.e(LOGTAG, "Cannot get pid for " + mType, e);
                 }
             }
             return mPid;
         }
 
+        public synchronized void unbindAndStop() {
+            final Context context = GeckoAppShell.getApplicationContext();
+
+            if (mChild != null) {
+                context.unbindService(this);
+            }
+
+            context.stopService(mIntent);
+
+            mChild = null;
+            mPid = 0;
+            mWaiting = false;
+            notifyAll();
+        }
+
         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);
 
-            if (context.bindService(intent, this, Context.BIND_AUTO_CREATE)) {
+            // We explicitly use startService() here instead of bind() with BIND_AUTO_CREATE
+            // because otherwise Android will try to automatically restart the service in
+            // the case of a crash, but we want to control that ourselves.
+            if (context.startService(mIntent) == null) {
+                Log.e(LOGTAG, "Failed to start child service");
+                context.stopService(mIntent);
+                return null;
+            }
+
+            if (context.bindService(mIntent, this, 0)) {
                 waitForChildLocked();
                 if (mChild != null) {
                     return mChild;
                 }
             }
 
             Log.e(LOGTAG, "Cannot connect to process " + mType);
-            context.unbindService(this);
+            unbindAndStop();
             return null;
         }
 
-        public synchronized void unbind() {
-            final int pid = getPid();
-            if (pid != 0) {
-                Process.killProcess(pid);
-                waitForChildLocked();
-            }
-        }
-
         private void waitForChildLocked() {
             ThreadUtils.assertNotOnUiThread();
             mWaiting = true;
 
             final long startTime = SystemClock.uptimeMillis();
             while (mWaiting && SystemClock.uptimeMillis() - startTime < WAIT_TIMEOUT) {
                 try {
                     wait(WAIT_TIMEOUT);
@@ -120,80 +138,69 @@ public final class GeckoProcessManager e
 
             mChild = IChildProcess.Stub.asInterface(service);
             mWaiting = false;
             notifyAll();
         }
 
         @Override
         public synchronized void onServiceDisconnected(ComponentName name) {
-            mChild = null;
-            mPid = 0;
-            mWaiting = false;
-            notifyAll();
+            unbindAndStop();
         }
 
         @Override
         public synchronized void binderDied() {
-            Log.i(LOGTAG, "Binder died for " + mType);
-            if (mChild != null) {
-                mChild = null;
-                GeckoAppShell.getApplicationContext().unbindService(this);
-            }
+            Log.d(LOGTAG, "Binder died for " + mType);
+            unbindAndStop();
         }
     }
 
     private final SimpleArrayMap<String, ChildConnection> mConnections;
 
     private GeckoProcessManager() {
         mConnections = new SimpleArrayMap<String, ChildConnection>();
     }
 
     private ChildConnection getConnection(final String type) {
         synchronized (mConnections) {
             ChildConnection connection = mConnections.get(type);
-            if (connection == null) {
-                connection = new ChildConnection(type);
-                mConnections.put(type, connection);
+            if (connection != null) {
+                return connection;
             }
+
+            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);
             if (connection.bind() != null) {
                 connection.getPid();
             }
         }
     }
 
-    public void crashChild() {
-        try {
-            mConnections.get("tab").bind().crash();
-        } catch (RemoteException e) {
-        }
-    }
-
     @WrapForJNI
     private static int start(final String type, final String[] args,
                              final int prefsFd, final int ipcFd,
                              final int crashFd, final int crashAnnotationFd) {
-        return INSTANCE.start(type, args, prefsFd, ipcFd, crashFd, crashAnnotationFd, /* retry */ false);
+        return INSTANCE.startInternal(type, args, prefsFd, ipcFd, crashFd, crashAnnotationFd);
     }
 
     private int filterFlagsForChild(int flags) {
         return flags & (GeckoThread.FLAG_ENABLE_JAVA_CRASHREPORTER |
                 GeckoThread.FLAG_ENABLE_NATIVE_CRASHREPORTER);
     }
 
-    private int start(final String type, final String[] args, final int prefsFd,
-                      final int ipcFd, final int crashFd,
-                      final int crashAnnotationFd, final boolean retry) {
+    private int startInternal(final String type, final String[] args, final int prefsFd,
+                              final int ipcFd, final int crashFd,
+                              final int crashAnnotationFd) {
         final ChildConnection connection = getConnection(type);
         final IChildProcess child = connection.bind();
         if (child == null) {
             return 0;
         }
 
         final Bundle extras = GeckoThread.getActiveExtras();
         final ParcelFileDescriptor prefsPfd;
@@ -209,38 +216,35 @@ public final class GeckoProcessManager e
             Log.e(LOGTAG, "Cannot create fd for " + type, e);
             return 0;
         }
 
         final int flags = filterFlagsForChild(GeckoThread.getActiveFlags());
 
         boolean started = false;
         try {
+            Log.d(LOGTAG, String.format("Starting new child process of type '%s'", type));
             started = child.start(this, args, extras, flags, prefsPfd, ipcPfd, crashPfd,
                                   crashAnnotationPfd);
         } 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, prefsFd, ipcFd, crashFd, crashAnnotationFd, /* retry */ true);
+            Log.e(LOGTAG, "Failed to start child");
+            return 0;
         }
 
         try {
             if (crashAnnotationPfd != null) {
                 crashAnnotationPfd.close();
             }
             if (crashPfd != null) {
                 crashPfd.close();
             }
             ipcPfd.close();
         } catch (final IOException e) {
         }
 
+        Log.d(LOGTAG, "Started new child, pid = " + connection.getPid());
         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
@@ -86,32 +86,28 @@ public class GeckoServiceChildProcess ex
                     if (GeckoThread.initChildProcess(args, extras, flags, prefsFd, ipcFd, crashReporterFd,
                                                      crashAnnotationFd)) {
                         GeckoThread.launch();
                     }
                 }
             });
             return true;
         }
-
-        @Override
-        public void crash() {
-            GeckoThread.crash();
-        }
     };
 
     @Override
     public IBinder onBind(final Intent intent) {
         GeckoThread.launch(); // Preload Gecko.
         return mBinder;
     }
 
     @Override
     public boolean onUnbind(Intent intent) {
         Log.i(LOGTAG, "Service has been unbound. Stopping.");
+        stopSelf();
         Process.killProcess(Process.myPid());
         return false;
     }
 
     public static final class geckomediaplugin extends GeckoServiceChildProcess {}
 
     public static final class tab extends GeckoServiceChildProcess {}
 }