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

Commit 971da4f3 authored by Jing Ji's avatar Jing Ji
Browse files

Wait for the death of precede instance of the launching process

...from a context without global AM lock held.

Previous CL was waiting for it with global AM lock held, which
would result sluggish of the system. Now moving the waiting prior
to start the new instance of the process.

In the case of precedence hasn't died yet, don't reuse the old
instance of ContentProviderRecord, otherwise when we clean up
the old ProcessRecord, the client of new ContentProvider instance
could get killed unexpectedly.

Bug: 141857656
Test: Manual (See the bug comment #49 for the procedure)
Change-Id: I957e2f443b3f9688ca449cfccc68aba22aff7f05
parent be1592ef
Loading
Loading
Loading
Loading
+53 −16
Original line number Diff line number Diff line
@@ -6860,6 +6860,7 @@ public class ActivityManagerService extends IActivityManager.Stub
                }
            }
            ProcessRecord dyingProc = null;
            if (cpr != null && cpr.proc != null) {
                providerRunning = !cpr.proc.killed;
@@ -6869,14 +6870,9 @@ 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) {
                    final long iden = Binder.clearCallingIdentity();
                    try {
                        mProcessList.killProcAndWaitIfNecessaryLocked(cpr.proc, false,
                                cpr.uid == cpr.proc.uid || cpr.proc.isolated,
                                "getContentProviderImpl: %s (killedByAm)", startTime);
                    } finally {
                        Binder.restoreCallingIdentity(iden);
                    }
                    Slog.wtf(TAG, cpr.proc.toString() + " was killed by AM but isn't really dead");
                    // Now we are going to wait for the death before starting the new process.
                    dyingProc = cpr.proc;
                }
            }
@@ -6977,18 +6973,18 @@ public class ActivityManagerService extends IActivityManager.Stub
                    // has been killed on us.  We need to wait for a new
                    // process to be started, and make sure its death
                    // doesn't kill our process.
                    Slog.i(TAG, "Existing provider " + cpr.name.flattenToShortString()
                    Slog.wtf(TAG, "Existing provider " + cpr.name.flattenToShortString()
                            + " is crashing; detaching " + r);
                    boolean lastRef = decProviderCountLocked(conn, cpr, token, stable);
                    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.
                        // the provider...  we will be killed during cleaning up, bail.
                        return null;
                    }
                    // We'll just start a new process to host the content provider
                    providerRunning = false;
                    conn = null;
                    dyingProc = cpr.proc;
                } else {
                    cpr.proc.verifiedAdj = cpr.proc.setAdj;
                }
@@ -7064,7 +7060,7 @@ public class ActivityManagerService extends IActivityManager.Stub
                checkTime(startTime, "getContentProviderImpl: before getProviderByClass");
                cpr = mProviderMap.getProviderByClass(comp, userId);
                checkTime(startTime, "getContentProviderImpl: after getProviderByClass");
                final boolean firstClass = cpr == null;
                boolean firstClass = cpr == null;
                if (firstClass) {
                    final long ident = Binder.clearCallingIdentity();
@@ -7095,6 +7091,13 @@ public class ActivityManagerService extends IActivityManager.Stub
                    } finally {
                        Binder.restoreCallingIdentity(ident);
                    }
                } else if (dyingProc == cpr.proc) {
                    // The old stable connection's client should be killed during proc cleaning up,
                    // so do not re-use the old ContentProviderRecord, otherwise the new clients
                    // could get killed unexpectedly.
                    cpr = new ContentProviderRecord(cpr);
                    // This is sort of "firstClass"
                    firstClass = true;
                }
                checkTime(startTime, "getContentProviderImpl: now have ContentProviderRecord");
@@ -14208,10 +14211,19 @@ public class ActivityManagerService extends IActivityManager.Stub
                cpr.launchingApp = null;
                cpr.notifyAll();
            }
            mProviderMap.removeProviderByClass(cpr.name, UserHandle.getUserId(cpr.uid));
            final int userId = UserHandle.getUserId(cpr.uid);
            // Don't remove from provider map if it doesn't match
            // could be a new content provider is starting
            if (mProviderMap.getProviderByClass(cpr.name, userId) == cpr) {
                mProviderMap.removeProviderByClass(cpr.name, userId);
            }
            String names[] = cpr.info.authority.split(";");
            for (int j = 0; j < names.length; j++) {
                mProviderMap.removeProviderByName(names[j], UserHandle.getUserId(cpr.uid));
                // Don't remove from provider map if it doesn't match
                // could be a new content provider is starting
                if (mProviderMap.getProviderByName(names[j], userId) == cpr) {
                    mProviderMap.removeProviderByName(names[j], userId);
                }
            }
        }
@@ -14306,6 +14318,10 @@ public class ActivityManagerService extends IActivityManager.Stub
        // Remove published content providers.
        for (int i = app.pubProviders.size() - 1; i >= 0; i--) {
            ContentProviderRecord cpr = app.pubProviders.valueAt(i);
            if (cpr.proc != app) {
                // If the hosting process record isn't really us, bail out
                continue;
            }
            final boolean alwaysRemove = app.bad || !allowRestart;
            final boolean inLaunching = removeDyingProviderLocked(app, cpr, alwaysRemove);
            if (!alwaysRemove && inLaunching && cpr.hasConnectionOrHandle()) {
@@ -14391,6 +14407,27 @@ public class ActivityManagerService extends IActivityManager.Stub
        mUiHandler.obtainMessage(DISPATCH_PROCESS_DIED_UI_MSG, app.pid, app.info.uid,
                null).sendToTarget();
        // If this is a precede 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.removed) {
                    if (mPersistentStartingProcesses.indexOf(app.mSuccessor) < 0) {
                        mPersistentStartingProcesses.add(app.mSuccessor);
                    }
                }
                // clean up the field so the successor's proc starter could proceed.
                app.mSuccessor.mPrecedence = null;
                app.mSuccessor = null;
                // Notify if anyone is waiting for it.
                app.notifyAll();
            }
        }
        // If the caller is restarting this app, then leave it in its
        // current lists and let the caller take care of it.
        if (restarting) {
@@ -14420,7 +14457,7 @@ public class ActivityManagerService extends IActivityManager.Stub
        mAtmInternal.onCleanUpApplicationRecord(app.getWindowProcessController());
        mProcessList.noteProcessDiedLocked(app);
        if (restart && !app.isolated) {
        if (restart && allowRestart && !app.isolated) {
            // We have components that still need to be running in the
            // process, so re-launch it.
            if (index < 0) {
+77 −61
Original line number Diff line number Diff line
@@ -1842,26 +1842,9 @@ public final class ProcessList {
        if (mService.mConstants.FLAG_PROCESS_START_ASYNC) {
            if (DEBUG_PROCESSES) Slog.i(TAG_PROCESSES,
                    "Posting procStart msg for " + app.toShortString());
            mService.mProcStartHandler.post(() -> {
                try {
                    final Process.ProcessStartResult startResult = startProcess(app.hostingRecord,
                            entryPoint, app, app.startUid, gids, runtimeFlags, mountExternal,
                            app.seInfo, requiredAbi, instructionSet, invokeWith, app.startTime);
                    synchronized (mService) {
                        handleProcessStartedLocked(app, startResult, startSeq);
                    }
                } catch (RuntimeException e) {
                    synchronized (mService) {
                        Slog.e(ActivityManagerService.TAG, "Failure starting process "
                                + app.processName, e);
                        mPendingStarts.remove(startSeq);
                        app.pendingStart = false;
                        mService.forceStopPackageLocked(app.info.packageName,
                                UserHandle.getAppId(app.uid),
                                false, false, true, false, false, app.userId, "start failure");
                    }
                }
            });
            mService.mProcStartHandler.post(() -> handleProcessStart(
                    app, entryPoint, gids, runtimeFlags, mountExternal, requiredAbi,
                    instructionSet, invokeWith, startSeq));
            return true;
        } else {
            try {
@@ -1882,6 +1865,66 @@ public final class ProcessList {
        }
    }

    /**
     * Main handler routine to start the given process from the ProcStartHandler.
     *
     * <p>Note: this function doesn't hold the global AM lock intentionally.</p>
     */
    private void handleProcessStart(final ProcessRecord app, final String entryPoint,
            final int[] gids, final int runtimeFlags, final int mountExternal,
            final String requiredAbi, final String instructionSet,
            final String invokeWith, final long startSeq) {
        // If there is a precede instance of the process, wait for its death with a timeout.
        // Use local reference since we are not using locks here
        final ProcessRecord precedence = app.mPrecedence;
        if (precedence != null) {
            final int pid = precedence.pid;
            long now = System.currentTimeMillis();
            final long end = now + PROC_KILL_TIMEOUT;
            try {
                Process.waitForProcessDeath(pid, PROC_KILL_TIMEOUT);
                // It's killed successfully, but we'd make sure the cleanup work is done.
                synchronized (precedence) {
                    if (app.mPrecedence != null) {
                        now = System.currentTimeMillis();
                        if (now < end) {
                            try {
                                precedence.wait(end - now);
                            } catch (InterruptedException e) {
                            }
                        }
                    }
                    if (app.mPrecedence != null) {
                        // The cleanup work hasn't be done yet, let's log it and continue.
                        Slog.w(TAG, precedence + " has died, but its cleanup isn't done");
                    }
                }
            } catch (Exception e) {
                // It's still alive...
                Slog.wtf(TAG, precedence.toString() + " refused to die, but we need to launch "
                        + app);
            }
        }
        try {
            final Process.ProcessStartResult startResult = startProcess(app.hostingRecord,
                    entryPoint, app, app.startUid, gids, runtimeFlags, mountExternal,
                    app.seInfo, requiredAbi, instructionSet, invokeWith, app.startTime);
            synchronized (mService) {
                handleProcessStartedLocked(app, startResult, startSeq);
            }
        } catch (RuntimeException e) {
            synchronized (mService) {
                Slog.e(ActivityManagerService.TAG, "Failure starting process "
                        + app.processName, e);
                mPendingStarts.remove(startSeq);
                app.pendingStart = false;
                mService.forceStopPackageLocked(app.info.packageName,
                        UserHandle.getAppId(app.uid),
                        false, false, true, false, false, app.userId, "start failure");
            }
        }
    }

    @GuardedBy("mService")
    public void killAppZygoteIfNeededLocked(AppZygote appZygote, boolean force) {
        final ApplicationInfo appInfo = appZygote.getAppInfo();
@@ -2136,6 +2179,7 @@ public final class ProcessList {
                + " app=" + app + " knownToBeDead=" + knownToBeDead
                + " thread=" + (app != null ? app.thread : null)
                + " pid=" + (app != null ? app.pid : -1));
        ProcessRecord precedence = null;
        if (app != null && app.pid > 0) {
            if ((!knownToBeDead && !app.killed) || app.thread == null) {
                // We already have the app running, or are waiting for it to
@@ -2150,9 +2194,15 @@ 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);
            // do the killing
            killProcAndWaitIfNecessaryLocked(app, true, app.uid == info.uid || app.isolated,
                    "startProcess: bad proc running, killing: %s", startTime);
            checkSlow(startTime, "startProcess: bad proc running, killing");
            ProcessList.killProcessGroup(app.uid, app.pid);
            checkSlow(startTime, "startProcess: done killing old proc");

            Slog.wtf(TAG_PROCESSES, app.toString() + " is attached to a previous process");
            // We are not going to re-use the ProcessRecord, as we haven't dealt with the cleanup
            // routine of it yet, but we'd set it as the precedence of the new process.
            precedence = app;
            app = null;
        }

        if (app == null) {
@@ -2166,6 +2216,10 @@ public final class ProcessList {
            app.crashHandler = crashHandler;
            app.isolatedEntryPoint = entryPoint;
            app.isolatedEntryPointArgs = entryPointArgs;
            if (precedence != null) {
                app.mPrecedence = precedence;
                precedence.mSuccessor = app;
            }
            checkSlow(startTime, "startProcess: done creating new process record");
        } else {
            // If this is a new package in the process, add the package to the list
@@ -2193,44 +2247,6 @@ public final class ProcessList {
        return success ? app : null;
    }

    /**
     * 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);
            noteAppKill(app, ApplicationExitInfo.REASON_OTHER,
                    ApplicationExitInfo.SUBREASON_UNKNOWN,
                    String.format(formatString, ""));
        }

        // wait for the death
        if (wait) {
            try {
                Process.waitForProcessDeath(app.pid, PROC_KILL_TIMEOUT);
            } catch (Exception e) {
                // 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;
+8 −0
Original line number Diff line number Diff line
@@ -320,6 +320,14 @@ class ProcessRecord implements WindowProcessListener {
    // set of disabled compat changes for the process (all others are enabled)
    long[] mDisabledCompatChanges;

    // The precede instance of the process, which would exist when the previous process is killed
    // but not fully dead yet; in this case, the new instance of the process should be held until
    // this precede instance is fully dead.
    volatile ProcessRecord mPrecedence;
    // The succeeding instance of the process, which is going to be started after this process
    // is killed successfully.
    volatile ProcessRecord mSuccessor;

    // Cached task info for OomAdjuster
    private static final int VALUE_INVALID = -1;
    private static final int VALUE_FALSE = 0;