From 464c84abd7286b7092be22ca1f9a4bfd9bb0c5e7 Mon Sep 17 00:00:00 2001 From: Jing Ji Date: Wed, 24 Mar 2021 19:04:52 -0700 Subject: [PATCH] Handle a race condition between process killing and restarting If the app is killed by system_server itself, the related info could have been cleared before binderDied comes in, so when we restart it, we'd be unable to know if the previous instance is still alive or not. Now keep track of this type of process kills. Bug: 181777896 Test: Manual - Loop start camera, sleep 1, stop camera app Test: atest CtsAppTestCases Test: atest FrameworksServicesTests Test: atest FrameworksMockingServicesTests Change-Id: Icaa706b88fd40ad649defd792de1cdc05e4ae386 --- .../server/am/ActivityManagerService.java | 54 ++++---- .../com/android/server/am/ProcessList.java | 115 ++++++++++++++++-- .../com/android/server/am/ProcessRecord.java | 28 ++++- 3 files changed, 154 insertions(+), 43 deletions(-) diff --git a/services/core/java/com/android/server/am/ActivityManagerService.java b/services/core/java/com/android/server/am/ActivityManagerService.java index 492759f7c73d8..5220234013358 100644 --- a/services/core/java/com/android/server/am/ActivityManagerService.java +++ b/services/core/java/com/android/server/am/ActivityManagerService.java @@ -2971,9 +2971,9 @@ public class ActivityManagerService extends IActivityManager.Stub */ @GuardedBy("this") final void handleAppDiedLocked(ProcessRecord app, int pid, - boolean restarting, boolean allowRestart) { + boolean restarting, boolean allowRestart, boolean fromBinderDied) { boolean kept = cleanUpApplicationRecordLocked(app, pid, restarting, allowRestart, -1, - false /*replacingPid*/); + false /*replacingPid*/, fromBinderDied); if (!kept && !restarting) { removeLruProcessLocked(app); if (pid > 0) { @@ -3031,12 +3031,15 @@ public class ActivityManagerService extends IActivityManager.Stub final void appDiedLocked(ProcessRecord app, int pid, IApplicationThread thread, boolean fromBinderDied, String reason) { // First check if this ProcessRecord is actually active for the pid. + final ProcessRecord curProc; synchronized (mPidsSelfLocked) { - ProcessRecord curProc = mPidsSelfLocked.get(pid); - if (curProc != app) { + curProc = mPidsSelfLocked.get(pid); + } + if (curProc != app) { + if (!fromBinderDied || !mProcessList.handleDyingAppDeathLocked(app, pid)) { Slog.w(TAG, "Spurious death for " + app + ", curProc for " + pid + ": " + curProc); - return; } + return; } mBatteryStatsService.noteProcessDied(app.info.uid, pid); @@ -3076,7 +3079,7 @@ public class ActivityManagerService extends IActivityManager.Stub EventLogTags.writeAmProcDied(app.userId, pid, app.processName, setAdj, setProcState); if (DEBUG_CLEANUP) Slog.v(TAG_CLEANUP, "Dying app: " + app + ", pid: " + pid + ", thread: " + thread.asBinder()); - handleAppDiedLocked(app, pid, false, true); + handleAppDiedLocked(app, pid, false, true, fromBinderDied); if (doOomAdj) { updateOomAdjLocked(OomAdjuster.OOM_ADJ_REASON_PROCESS_END); @@ -4233,7 +4236,7 @@ public class ActivityManagerService extends IActivityManager.Stub EventLog.writeEvent(0x534e4554, "131105245", app.getStartUid(), msg); // If there is already an app occupying that pid that hasn't been cleaned up cleanUpApplicationRecordLocked(app, pid, false, false, -1, - true /*replacingPid*/); + true /*replacingPid*/, false /* fromBinderDied */); removePidLocked(pid, app); app = null; } @@ -4275,7 +4278,7 @@ public class ActivityManagerService extends IActivityManager.Stub // If this application record is still attached to a previous // process, clean it up now. if (app.getThread() != null) { - handleAppDiedLocked(app, pid, true, true); + handleAppDiedLocked(app, pid, true, true, false /* fromBinderDied */); } // Tell the process all about itself. @@ -4478,7 +4481,7 @@ public class ActivityManagerService extends IActivityManager.Stub app.unlinkDeathRecipient(); app.killLocked("error during bind", ApplicationExitInfo.REASON_INITIALIZATION_FAILURE, true); - handleAppDiedLocked(app, pid, false, true); + handleAppDiedLocked(app, pid, false, true, false /* fromBinderDied */); return false; } @@ -4543,7 +4546,7 @@ public class ActivityManagerService extends IActivityManager.Stub if (badApp) { app.killLocked("error during init", ApplicationExitInfo.REASON_INITIALIZATION_FAILURE, true); - handleAppDiedLocked(app, pid, false, true); + handleAppDiedLocked(app, pid, false, true, false /* fromBinderDied */); return false; } @@ -11485,7 +11488,8 @@ public class ActivityManagerService extends IActivityManager.Stub */ @GuardedBy("this") final boolean cleanUpApplicationRecordLocked(ProcessRecord app, int pid, - boolean restarting, boolean allowRestart, int index, boolean replacingPid) { + boolean restarting, boolean allowRestart, int index, boolean replacingPid, + boolean fromBinderDied) { boolean restart; synchronized (mProcLock) { if (index >= 0) { @@ -11493,7 +11497,10 @@ public class ActivityManagerService extends IActivityManager.Stub ProcessList.remove(pid); } - restart = app.onCleanupApplicationRecordLSP(mProcessStats, allowRestart); + // We don't want to unlinkDeathRecipient immediately, if it's not called from binder + // and it's not isolated, as we'd need the signal to bookkeeping the dying process list. + restart = app.onCleanupApplicationRecordLSP(mProcessStats, allowRestart, + fromBinderDied || app.isolated /* unlinkDeath */); } mAppProfiler.onCleanupApplicationRecordLocked(app); skipCurrentReceiverLocked(app); @@ -11523,25 +11530,7 @@ public class ActivityManagerService extends IActivityManager.Stub mProcessList.scheduleDispatchProcessDiedLocked(pid, app.info.uid); // If this is a preceding instance of another process instance - allowRestart = true; - synchronized (app) { - if (app.mSuccessor != null) { - // We don't allow restart with this ProcessRecord now, - // because we have created a new one already. - allowRestart = false; - // If it's persistent, add the successor to mPersistentStartingProcesses - if (app.isPersistent() && !app.isRemoved()) { - if (mPersistentStartingProcesses.indexOf(app.mSuccessor) < 0) { - mPersistentStartingProcesses.add(app.mSuccessor); - } - } - // clean up the field so the successor's proc starter could proceed. - app.mSuccessor.mPredecessor = null; - app.mSuccessor = null; - // Notify if anyone is waiting for it. - app.notifyAll(); - } - } + allowRestart = mProcessList.handlePrecedingAppDiedLocked(app); // If the caller is restarting this app, then leave it in its // current lists and let the caller take care of it. @@ -14637,7 +14626,8 @@ public class ActivityManagerService extends IActivityManager.Stub } } didSomething = true; - cleanUpApplicationRecordLocked(app, pid, false, true, -1, false /*replacingPid*/); + cleanUpApplicationRecordLocked(app, pid, false, true, -1, false /*replacingPid*/, + false /* fromBinderDied */); mProcessList.mRemovedProcesses.remove(i); if (app.isPersistent()) { diff --git a/services/core/java/com/android/server/am/ProcessList.java b/services/core/java/com/android/server/am/ProcessList.java index 51bcde8691898..110c58f427ae9 100644 --- a/services/core/java/com/android/server/am/ProcessList.java +++ b/services/core/java/com/android/server/am/ProcessList.java @@ -687,6 +687,12 @@ public final class ProcessList { @GuardedBy("mService") final ArrayList mRemovedProcesses = new ArrayList(); + /** + * Processes that are killed by us and being waiting for the death notification. + */ + @GuardedBy("mService") + final ProcessMap mDyingProcesses = new ProcessMap<>(); + // Self locked with the inner lock within the RemoteCallbackList private final RemoteCallbackList mProcessObservers = new RemoteCallbackList<>(); @@ -1790,6 +1796,9 @@ public final class ProcessList { app.setPid(0); app.setStartSeq(0); } + // Clear any residual death recipient link as the ProcessRecord could be reused. + app.unlinkDeathRecipient(); + app.setDyingPid(0); if (DEBUG_PROCESSES && mService.mProcessesOnHold.contains(app)) Slog.v( TAG_PROCESSES, @@ -2086,14 +2095,14 @@ public final class ProcessList { // If there is a preceding instance of the process, wait for its death with a timeout. // Use local reference since we are not using locks here final ProcessRecord predecessor = app.mPredecessor; - if (predecessor != null) { - final int pid = predecessor.getPid(); + int prevPid; + if (predecessor != null && (prevPid = predecessor.getDyingPid()) > 0) { long now = System.currentTimeMillis(); final long end = now + PROC_KILL_TIMEOUT; final int oldPolicy = StrictMode.getThreadPolicyMask(); try { StrictMode.setThreadPolicyMask(0); - Process.waitForProcessDeath(pid, PROC_KILL_TIMEOUT); + Process.waitForProcessDeath(prevPid, PROC_KILL_TIMEOUT); // It's killed successfully, but we'd make sure the cleanup work is done. synchronized (predecessor) { if (app.mPredecessor != null) { @@ -2103,17 +2112,22 @@ public final class ProcessList { predecessor.wait(end - now); } catch (InterruptedException e) { } + if (System.currentTimeMillis() >= end) { + Slog.w(TAG, predecessor + " " + prevPid + + " has died but its obituary delivery is slow."); + } } } - if (app.mPredecessor != null) { + if (app.mPredecessor != null && app.mPredecessor.getPid() > 0) { // The cleanup work hasn't be done yet, let's log it and continue. - Slog.w(TAG, predecessor + " has died, but its cleanup isn't done"); + Slog.w(TAG, predecessor + " " + prevPid + + " has died, but its cleanup isn't done"); } } } catch (Exception e) { // It's still alive... maybe blocked at uninterruptible sleep ? - Slog.wtf(TAG, predecessor.toString() + " refused to die, but we need to launch " - + app, e); + Slog.wtf(TAG, predecessor.toString() + " " + prevPid + + " refused to die, but we need to launch " + app, e); } finally { StrictMode.setThreadPolicyMask(oldPolicy); } @@ -2490,6 +2504,18 @@ public final class ProcessList { // routine of it yet, but we'd set it as the predecessor of the new process. predecessor = app; app = null; + } else if (!isolated) { + // This app may have been removed from process name maps, probably because we killed it + // and did the cleanup before the actual death notification. Check the dying processes. + predecessor = mDyingProcesses.get(processName, info.uid); + if (predecessor != null) { + if (app != null) { + app.mPredecessor = predecessor; + predecessor.mSuccessor = app; + } + Slog.w(TAG_PROCESSES, predecessor.toString() + " is attached to a previous process " + + predecessor.getDyingPid()); + } } if (app == null) { @@ -2658,7 +2684,7 @@ public final class ProcessList { + " belongs to another existing app:" + oldApp.processName + " startSeq:" + oldApp.getStartSeq()); mService.cleanUpApplicationRecordLocked(oldApp, pid, false, false, -1, - true /*replacingPid*/); + true /*replacingPid*/, false /* fromBinderDied */); } mService.addPidLocked(app); synchronized (mService.mPidsSelfLocked) { @@ -2872,7 +2898,8 @@ public final class ProcessList { } } app.killLocked(reason, reasonCode, subReason, true); - mService.handleAppDiedLocked(app, pid, willRestart, allowRestart); + mService.handleAppDiedLocked(app, pid, willRestart, allowRestart, + false /* fromBinderDied */); if (willRestart) { removeLruProcessLocked(app); mService.addAppLocked(app.info, null, false, null /* ABI override */, @@ -4902,6 +4929,55 @@ public final class ProcessList { return EVENT_INPUT; } + /** + * Handle the death notification if it's a dying app. + * + * @return {@code true} if it's a dying app that we were tracking. + */ + @GuardedBy("mService") + boolean handleDyingAppDeathLocked(ProcessRecord app, int pid) { + if (mProcessNames.get(app.processName, app.uid) != app + && mDyingProcesses.get(app.processName, app.uid) == app) { + // App has been removed already, meaning cleanup has done. + Slog.v(TAG, "Got obituary of " + pid + ":" + app.processName); + app.unlinkDeathRecipient(); + handlePrecedingAppDiedLocked(app); + // It's really gone now, let's remove from the dying process list. + mDyingProcesses.remove(app.processName, app.uid); + app.setDyingPid(0); + return true; + } + return false; + } + + /** + * Handle the case where the given app is a preceding instance of another process instance. + * + * @return {@code false} if this given app should not be allowed to restart. + */ + @GuardedBy("mService") + boolean handlePrecedingAppDiedLocked(ProcessRecord app) { + synchronized (app) { + if (app.mSuccessor != null) { + // We don't allow restart with this ProcessRecord now, + // because we have created a new one already. + // If it's persistent, add the successor to mPersistentStartingProcesses + if (app.isPersistent() && !app.isRemoved()) { + if (mService.mPersistentStartingProcesses.indexOf(app.mSuccessor) < 0) { + mService.mPersistentStartingProcesses.add(app.mSuccessor); + } + } + // clean up the field so the successor's proc starter could proceed. + app.mSuccessor.mPredecessor = null; + app.mSuccessor = null; + // Notify if anyone is waiting for it. + app.notifyAll(); + return false; + } + } + return true; + } + /** * Called by ActivityManagerService when a process died. */ @@ -4912,21 +4988,33 @@ public final class ProcessList { } Watchdog.getInstance().processDied(app.processName, app.getPid()); + if (app.getDeathRecipient() == null) { + // If we've done unlinkDeathRecipient before calling into this, remove from dying list. + mDyingProcesses.remove(app.processName, app.uid); + app.setDyingPid(0); + } mAppExitInfoTracker.scheduleNoteProcessDied(app); } /** * Called by ActivityManagerService when it decides to kill an application process. */ + @GuardedBy("mService") void noteAppKill(final ProcessRecord app, final @Reason int reason, final @SubReason int subReason, final String msg) { if (DEBUG_PROCESSES) { Slog.i(TAG, "note: " + app + " is being killed, reason: " + reason + ", sub-reason: " + subReason + ", message: " + msg); } + if (app.getPid() > 0 && !app.isolated && app.getDeathRecipient() != null) { + // We are killing it, put it into the dying process list. + mDyingProcesses.put(app.processName, app.uid, app); + app.setDyingPid(app.getPid()); + } mAppExitInfoTracker.scheduleNoteAppKill(app, reason, subReason, msg); } + @GuardedBy("mService") void noteAppKill(final int pid, final int uid, final @Reason int reason, final @SubReason int subReason, final String msg) { if (DEBUG_PROCESSES) { @@ -4934,6 +5022,15 @@ public final class ProcessList { + ", sub-reason: " + subReason + ", message: " + msg); } + final ProcessRecord app; + synchronized (mService.mPidsSelfLocked) { + app = mService.mPidsSelfLocked.get(pid); + } + if (app != null && app.uid == uid && !app.isolated && app.getDeathRecipient() != null) { + // We are killing it, put it into the dying process list. + mDyingProcesses.put(app.processName, uid, app); + app.setDyingPid(app.getPid()); + } mAppExitInfoTracker.scheduleNoteAppKill(pid, uid, reason, subReason, msg); } diff --git a/services/core/java/com/android/server/am/ProcessRecord.java b/services/core/java/com/android/server/am/ProcessRecord.java index 42e7ff4c57245..4dd191bcf77f7 100644 --- a/services/core/java/com/android/server/am/ProcessRecord.java +++ b/services/core/java/com/android/server/am/ProcessRecord.java @@ -106,6 +106,12 @@ class ProcessRecord implements WindowProcessListener { @CompositeRWLock({"mService", "mProcLock"}) int mPid; + /** + * The process ID which will be set when we're killing this process. + */ + @GuardedBy("mService") + private int mDyingPid; + /** * The gids this process was launched with. */ @@ -571,6 +577,16 @@ class ProcessRecord implements WindowProcessListener { mProfile.onProcessInactive(tracker); } + @GuardedBy("mService") + int getDyingPid() { + return mDyingPid; + } + + @GuardedBy("mService") + void setDyingPid(int dyingPid) { + mDyingPid = dyingPid; + } + @GuardedBy("mService") int[] getGids() { return mGids; @@ -732,6 +748,11 @@ class ProcessRecord implements WindowProcessListener { mDeathRecipient = deathRecipient; } + @GuardedBy("mService") + IBinder.DeathRecipient getDeathRecipient() { + return mDeathRecipient; + } + @GuardedBy({"mService", "mProcLock"}) void setActiveInstrumentation(ActiveInstrumentation instr) { mInstr = instr; @@ -896,11 +917,14 @@ class ProcessRecord implements WindowProcessListener { } @GuardedBy({"mService", "mProcLock"}) - boolean onCleanupApplicationRecordLSP(ProcessStatsService processStats, boolean allowRestart) { + boolean onCleanupApplicationRecordLSP(ProcessStatsService processStats, boolean allowRestart, + boolean unlinkDeath) { mErrorState.onCleanupApplicationRecordLSP(); resetPackageList(processStats); - unlinkDeathRecipient(); + if (unlinkDeath) { + unlinkDeathRecipient(); + } makeInactive(processStats); setWaitingToKill(null);