Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 464c84ab authored by Jing Ji's avatar Jing Ji
Browse files

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
parent 1325fdd0
Loading
Loading
Loading
Loading
+22 −32
Original line number Diff line number Diff line
@@ -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);
            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()) {
+106 −9
Original line number Diff line number Diff line
@@ -687,6 +687,12 @@ public final class ProcessList {
    @GuardedBy("mService")
    final ArrayList<ProcessRecord> mRemovedProcesses = new ArrayList<ProcessRecord>();

    /**
     * Processes that are killed by us and being waiting for the death notification.
     */
    @GuardedBy("mService")
    final ProcessMap<ProcessRecord> mDyingProcesses = new ProcessMap<>();

    // Self locked with the inner lock within the RemoteCallbackList
    private final RemoteCallbackList<IProcessObserver> 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);
    }

+26 −2
Original line number Diff line number Diff line
@@ -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);
        if (unlinkDeath) {
            unlinkDeathRecipient();
        }
        makeInactive(processStats);
        setWaitingToKill(null);