Proper way to ensure process dies before restart it

The previous fixes would give up the global AM lock which could
result races. Now keep polling to make sure the process goes away
before restart it.

Bug: 141857656
Test: manual
Change-Id: I661f9d23ea579874632399585ad375b0892c6da7

2 files changed, 92 insertions, 14 deletions

diff --git a/services/core/java/com/android/server/am/ActivityManagerService.java b/services/core/java/com/android/server/am/ActivityManagerService.java
index 53a5fc6c203a..57a355d5bad0 100644
--- a/services/core/java/com/android/server/am/ActivityManagerService.java
+++ b/services/core/java/com/android/server/am/ActivityManagerService.java
@@ -6694,7 +6694,7 @@ public class ActivityManagerService extends IActivityManager.Stub
 
     private final long[] mProcessStateStatsLongs = new long[1];
 
-    private boolean isProcessAliveLocked(ProcessRecord proc) {
+    boolean isProcessAliveLocked(ProcessRecord proc) {
         if (proc.pid <= 0) {
             if (DEBUG_OOM_ADJ) Slog.d(TAG, "Process hasn't started yet: " + proc);
             return false;
@@ -6711,7 +6711,10 @@ public class ActivityManagerService extends IActivityManager.Stub
         final long state = mProcessStateStatsLongs[0];
         if (DEBUG_OOM_ADJ) Slog.d(TAG, "RETRIEVED STATE FOR " + proc.procStatFile + ": "
                 + (char)state);
-        return state != 'Z' && state != 'X' && state != 'x' && state != 'K';
+        if (state != 'Z' && state != 'X' && state != 'x' && state != 'K') {
+            return Process.getUidForPid(proc.pid) == proc.uid;
+        }
+        return false;
     }
 
     private String checkContentProviderAssociation(ProcessRecord callingApp, int callingUid,
@@ -6784,14 +6787,14 @@ public class ActivityManagerService extends IActivityManager.Stub
                 // (See the commit message on I2c4ba1e87c2d47f2013befff10c49b3dc337a9a7 to see
                 // how to test this case.)
                 if (cpr.proc.killed && cpr.proc.killedByAm) {
-                    checkTime(startTime, "getContentProviderImpl: before appDied (killedByAm)");
                     final long iden = Binder.clearCallingIdentity();
                     try {
-                        appDiedLocked(cpr.proc);
+                        mProcessList.killProcAndWaitIfNecessaryLocked(cpr.proc, false,
+                                cpr.uid == cpr.proc.uid || cpr.proc.isolated,
+                                "getContentProviderImpl: %s (killedByAm)", startTime);
                     } finally {
                         Binder.restoreCallingIdentity(iden);
                     }
-                    checkTime(startTime, "getContentProviderImpl: after appDied (killedByAm)");
                 }
             }
 
@@ -6895,9 +6898,8 @@ public class ActivityManagerService extends IActivityManager.Stub
                     Slog.i(TAG, "Existing provider " + cpr.name.flattenToShortString()
                             + " is crashing; detaching " + r);
                     boolean lastRef = decProviderCountLocked(conn, cpr, token, stable);
-                    checkTime(startTime, "getContentProviderImpl: before appDied");
-                    appDiedLocked(cpr.proc);
-                    checkTime(startTime, "getContentProviderImpl: after appDied");
+                    mProcessList.killProcAndWaitIfNecessaryLocked(cpr.proc,
+                            false, true, "getContentProviderImpl: %s", startTime);
                     if (!lastRef) {
                         // This wasn't the last ref our process had on
                         // the provider...  we have now been killed, bail.
diff --git a/services/core/java/com/android/server/am/ProcessList.java b/services/core/java/com/android/server/am/ProcessList.java
index 2bb703545cad..32975d7792f5 100644
--- a/services/core/java/com/android/server/am/ProcessList.java
+++ b/services/core/java/com/android/server/am/ProcessList.java
@@ -78,6 +78,9 @@ import android.os.Trace;
 import android.os.UserHandle;
 import android.os.storage.StorageManager;
 import android.os.storage.StorageManagerInternal;
+import android.system.ErrnoException;
+import android.system.Os;
+import android.system.OsConstants;
 import android.text.TextUtils;
 import android.util.ArrayMap;
 import android.util.EventLog;
@@ -283,6 +286,16 @@ public final class ProcessList {
     // lmkd reconnect delay in msecs
     private final static long LMDK_RECONNECT_DELAY_MS = 1000;
 
+    /**
+     * How long between a process kill and we actually receive its death recipient
+     */
+    private static final long PROC_KILL_TIMEOUT = 2000; // 2 seconds;
+
+    /**
+     * How long between polls to check if the given process is dead or not.
+     */
+    private static final long PROC_DEATH_POLL_INTERVAL = 100;
+
     ActivityManagerService mService = null;
 
     // To kill process groups asynchronously
@@ -1421,7 +1434,7 @@ public final class ProcessList {
         if (app.pendingStart) {
             return true;
         }
-        long startTime = SystemClock.elapsedRealtime();
+        long startTime = SystemClock.uptimeMillis();
         if (app.pid > 0 && app.pid != ActivityManagerService.MY_PID) {
             checkSlow(startTime, "startProcess: removing from pids map");
             mService.mPidsSelfLocked.remove(app);
@@ -1856,7 +1869,7 @@ public final class ProcessList {
             boolean knownToBeDead, int intentFlags, HostingRecord hostingRecord,
             boolean allowWhileBooting, boolean isolated, int isolatedUid, boolean keepIfLarge,
             String abiOverride, String entryPoint, String[] entryPointArgs, Runnable crashHandler) {
-        long startTime = SystemClock.elapsedRealtime();
+        long startTime = SystemClock.uptimeMillis();
         ProcessRecord app;
         if (!isolated) {
             app = getProcessRecordLocked(processName, info.uid, keepIfLarge);
@@ -1917,10 +1930,9 @@ public final class ProcessList {
             // An application record is attached to a previous process,
             // clean it up now.
             if (DEBUG_PROCESSES) Slog.v(TAG_PROCESSES, "App died: " + app);
-            checkSlow(startTime, "startProcess: bad proc running, killing");
-            ProcessList.killProcessGroup(app.uid, app.pid);
-            mService.handleAppDiedLocked(app, true, true);
-            checkSlow(startTime, "startProcess: done killing old proc");
+            // do the killing
+            killProcAndWaitIfNecessaryLocked(app, true, app.uid == info.uid || app.isolated,
+                    "startProcess: bad proc running, killing: %s", startTime);
         }
 
         if (app == null) {
@@ -1961,6 +1973,70 @@ public final class ProcessList {
         return success ? app : null;
     }
 
+    /**
+     * A lite version of checking if a process is alive or not, by using kill(2) with signal 0.
+     *
+     * <p>
+     * Note that, zombie processes are stil "alive" in this case, use the {@link
+     * ActivityManagerService#isProcessAliveLocked} if zombie processes need to be excluded.
+     * </p>
+     */
+    @GuardedBy("mService")
+    private boolean isProcessAliveLiteLocked(ProcessRecord app) {
+        try {
+            Os.kill(app.pid, 0);
+        } catch (ErrnoException e) {
+            return e.errno != OsConstants.ESRCH;
+        }
+        return true;
+    }
+
+    /**
+     * Kill (if asked to) and wait for the given process died if necessary
+     * @param app - The process record to kill
+     * @param doKill - Kill the given process record
+     * @param wait - Wait for the death of the given process
+     * @param formatString - The log message for slow operation
+     * @param startTime - The start timestamp of the operation
+     */
+    @GuardedBy("mService")
+    void killProcAndWaitIfNecessaryLocked(final ProcessRecord app, final boolean doKill,
+            final boolean wait, final String formatString, final long startTime) {
+
+        checkSlow(startTime, String.format(formatString, "before appDied"));
+
+        if (doKill) {
+            // do the killing
+            ProcessList.killProcessGroup(app.uid, app.pid);
+        }
+
+        // wait for the death
+        if (wait) {
+            boolean isAlive = true;
+            // ideally we should use pidfd_open(2) but it's available on kernel 5.3 or later
+
+            final long timeout = SystemClock.uptimeMillis() + PROC_KILL_TIMEOUT;
+            isAlive = isProcessAliveLiteLocked(app);
+            while (timeout > SystemClock.uptimeMillis() && isAlive) {
+                try {
+                    Thread.sleep(PROC_DEATH_POLL_INTERVAL);
+                } catch (InterruptedException e) {
+                }
+                isAlive = isProcessAliveLiteLocked(app);
+            }
+
+            if (isAlive) {
+                // Maybe the process goes into zombie, use an expensive API to check again.
+                if (mService.isProcessAliveLocked(app)) {
+                    Slog.w(TAG, String.format(formatString,
+                              "waiting for app killing timed out"));
+                }
+            }
+        }
+
+        checkSlow(startTime, String.format(formatString, "after appDied"));
+    }
+
     @GuardedBy("mService")
     private String isProcStartValidLocked(ProcessRecord app, long expectedStartSeq) {
         StringBuilder sb = null;