BroadcastQueue: Handle successor ProcessRecords.
In rare cases, a request to startProcessLocked() returns a different
ProcessRecord from what is eventually attached. This change fixes
the modern stack to not be confused in these situations, by always
looking up the relevant BroadcastProcessQueue, instead of trying to
do a raw ProcessRecord equality check.
Add health check to verify that cold starts aren't abandoned. Add
debugging details for each "blocked" broadcast.
Bug: 253617038
Test: atest FrameworksMockingServicesTests:BroadcastRecordTest
Test: atest FrameworksMockingServicesTests:BroadcastQueueTest
Test: atest FrameworksMockingServicesTests:BroadcastQueueModernImplTest
Change-Id: Iab95f80ee554589f66eefe351370d37e44210295
diff --git a/services/core/java/com/android/server/am/BroadcastProcessQueue.java b/services/core/java/com/android/server/am/BroadcastProcessQueue.java
index 97635b5..0d6ac1d 100644
--- a/services/core/java/com/android/server/am/BroadcastProcessQueue.java
+++ b/services/core/java/com/android/server/am/BroadcastProcessQueue.java
@@ -114,6 +114,14 @@
private int mActiveIndex;
/**
+ * When defined, the receiver actively being dispatched into this process
+ * was considered "blocked" until at least the given count of other
+ * receivers have reached a terminal state; typically used for ordered
+ * broadcasts and priority traunches.
+ */
+ private int mActiveBlockedUntilTerminalCount;
+
+ /**
* Count of {@link #mActive} broadcasts that have been dispatched since this
* queue was last idle.
*/
@@ -304,6 +312,7 @@
final SomeArgs next = mPending.removeFirst();
mActive = (BroadcastRecord) next.arg1;
mActiveIndex = next.argi1;
+ mActiveBlockedUntilTerminalCount = next.argi2;
mActiveCountSinceIdle++;
mActiveViaColdStart = false;
next.recycle();
@@ -316,6 +325,7 @@
public void makeActiveIdle() {
mActive = null;
mActiveIndex = 0;
+ mActiveBlockedUntilTerminalCount = -1;
mActiveCountSinceIdle = 0;
mActiveViaColdStart = false;
invalidateRunnableAt();
@@ -664,27 +674,14 @@
}
pw.print(" because ");
pw.print(reasonToString(mRunnableAtReason));
- if (mRunnableAtReason == REASON_BLOCKED) {
- final SomeArgs next = mPending.peekFirst();
- if (next != null) {
- final BroadcastRecord r = (BroadcastRecord) next.arg1;
- final int blockedUntilTerminalCount = next.argi2;
- pw.print(" waiting for ");
- pw.print(blockedUntilTerminalCount);
- pw.print(" at ");
- pw.print(r.terminalCount);
- pw.print(" of ");
- pw.print(r.receivers.size());
- }
- }
pw.println();
pw.increaseIndent();
if (mActive != null) {
- dumpRecord(now, pw, mActive, mActiveIndex);
+ dumpRecord(now, pw, mActive, mActiveIndex, mActiveBlockedUntilTerminalCount);
}
for (SomeArgs args : mPending) {
final BroadcastRecord r = (BroadcastRecord) args.arg1;
- dumpRecord(now, pw, r, args.argi1);
+ dumpRecord(now, pw, r, args.argi1, args.argi2);
}
pw.decreaseIndent();
pw.println();
@@ -692,7 +689,7 @@
@NeverCompile
private void dumpRecord(@UptimeMillisLong long now, @NonNull IndentingPrintWriter pw,
- @NonNull BroadcastRecord record, int recordIndex) {
+ @NonNull BroadcastRecord record, int recordIndex, int blockedUntilTerminalCount) {
TimeUtils.formatDuration(record.enqueueTime, now, pw);
pw.print(' ');
pw.println(record.toShortString());
@@ -714,5 +711,13 @@
pw.print(info.activityInfo.name);
}
pw.println();
+ if (blockedUntilTerminalCount != -1) {
+ pw.print(" blocked until ");
+ pw.print(blockedUntilTerminalCount);
+ pw.print(", currently at ");
+ pw.print(record.terminalCount);
+ pw.print(" of ");
+ pw.println(record.receivers.size());
+ }
}
}
diff --git a/services/core/java/com/android/server/am/BroadcastQueueModernImpl.java b/services/core/java/com/android/server/am/BroadcastQueueModernImpl.java
index 25aa500..1e1ebeb 100644
--- a/services/core/java/com/android/server/am/BroadcastQueueModernImpl.java
+++ b/services/core/java/com/android/server/am/BroadcastQueueModernImpl.java
@@ -396,17 +396,12 @@
// Emit all trace events for this process into a consistent track
queue.traceTrackName = TAG + ".mRunning[" + queueIndex + "]";
- // If we're already warm, boost OOM adjust now; if cold we'll boost
- // it after the app has been started
- if (processWarm) {
- notifyStartedRunning(queue);
- }
-
// If we're already warm, schedule next pending broadcast now;
// otherwise we'll wait for the cold start to circle back around
queue.makeActiveNextPending();
if (processWarm) {
queue.traceProcessRunningBegin();
+ notifyStartedRunning(queue);
scheduleReceiverWarmLocked(queue);
} else {
queue.traceProcessStartingBegin();
@@ -441,15 +436,22 @@
@Override
public boolean onApplicationAttachedLocked(@NonNull ProcessRecord app) {
+ // Process records can be recycled, so always start by looking up the
+ // relevant per-process queue
+ final BroadcastProcessQueue queue = getProcessQueue(app);
+ if (queue != null) {
+ queue.app = app;
+ }
+
boolean didSomething = false;
- if ((mRunningColdStart != null) && (mRunningColdStart.app == app)) {
+ if ((mRunningColdStart != null) && (mRunningColdStart == queue)) {
// We've been waiting for this app to cold start, and it's ready
// now; dispatch its next broadcast and clear the slot
- final BroadcastProcessQueue queue = mRunningColdStart;
mRunningColdStart = null;
queue.traceProcessEnd();
queue.traceProcessRunningBegin();
+ notifyStartedRunning(queue);
scheduleReceiverWarmLocked(queue);
// We might be willing to kick off another cold start
@@ -471,19 +473,25 @@
@Override
public void onApplicationCleanupLocked(@NonNull ProcessRecord app) {
- if ((mRunningColdStart != null) && (mRunningColdStart.app == app)) {
+ // Process records can be recycled, so always start by looking up the
+ // relevant per-process queue
+ final BroadcastProcessQueue queue = getProcessQueue(app);
+ if (queue != null) {
+ queue.app = null;
+ }
+
+ if ((mRunningColdStart != null) && (mRunningColdStart == queue)) {
// We've been waiting for this app to cold start, and it had
// trouble; clear the slot and fail delivery below
mRunningColdStart = null;
+ queue.traceProcessEnd();
+
// We might be willing to kick off another cold start
enqueueUpdateRunningList();
}
- final BroadcastProcessQueue queue = getProcessQueue(app);
if (queue != null) {
- queue.app = null;
-
// If queue was running a broadcast, fail it
if (queue.isActive()) {
finishReceiverLocked(queue, BroadcastRecord.DELIVERY_FAILURE);
@@ -567,7 +575,7 @@
}
} else {
// Otherwise we don't need to block at all
- blockedUntilTerminalCount = 0;
+ blockedUntilTerminalCount = -1;
}
queue.enqueueOrReplaceBroadcast(r, i, blockedUntilTerminalCount);
@@ -619,9 +627,7 @@
if (DEBUG_BROADCAST) logv("Scheduling " + r + " to cold " + queue);
queue.app = mService.startProcessLocked(queue.processName, info, true, intentFlags,
hostingRecord, zygotePolicyFlags, allowWhileBooting, false);
- if (queue.app != null) {
- notifyStartedRunning(queue);
- } else {
+ if (queue.app == null) {
mRunningColdStart = null;
finishReceiverLocked(queue, BroadcastRecord.DELIVERY_FAILURE);
return;
@@ -1159,6 +1165,12 @@
}
}
+ // Verify that pending cold start hasn't been orphaned
+ if (mRunningColdStart != null) {
+ checkState(getRunningIndexOf(mRunningColdStart) >= 0,
+ "isOrphaned " + mRunningColdStart);
+ }
+
// Verify health of all known process queues
for (int i = 0; i < mProcessQueues.size(); i++) {
BroadcastProcessQueue leaf = mProcessQueues.valueAt(i);
diff --git a/services/tests/mockingservicestests/src/com/android/server/am/BroadcastQueueTest.java b/services/tests/mockingservicestests/src/com/android/server/am/BroadcastQueueTest.java
index 7ad89250..c125448 100644
--- a/services/tests/mockingservicestests/src/com/android/server/am/BroadcastQueueTest.java
+++ b/services/tests/mockingservicestests/src/com/android/server/am/BroadcastQueueTest.java
@@ -113,6 +113,7 @@
import java.util.Objects;
import java.util.Set;
import java.util.concurrent.atomic.AtomicInteger;
+import java.util.concurrent.atomic.AtomicReference;
import java.util.function.UnaryOperator;
/**
@@ -154,10 +155,11 @@
private BroadcastQueue mQueue;
/**
- * When enabled {@link ActivityManagerService#startProcessLocked} will fail
- * by returning {@code null}; otherwise it will spawn a new mock process.
+ * Desired behavior of the next
+ * {@link ActivityManagerService#startProcessLocked} call.
*/
- private boolean mFailStartProcess;
+ private AtomicReference<ProcessStartBehavior> mNextProcessStartBehavior = new AtomicReference<>(
+ ProcessStartBehavior.SUCCESS);
/**
* Map from PID to registered registered runtime receivers.
@@ -216,16 +218,46 @@
doAnswer((invocation) -> {
Log.v(TAG, "Intercepting startProcessLocked() for "
+ Arrays.toString(invocation.getArguments()));
- if (mFailStartProcess) {
+ final ProcessStartBehavior behavior = mNextProcessStartBehavior
+ .getAndSet(ProcessStartBehavior.SUCCESS);
+ if (behavior == ProcessStartBehavior.FAIL_NULL) {
return null;
}
final String processName = invocation.getArgument(0);
final ApplicationInfo ai = invocation.getArgument(1);
final ProcessRecord res = makeActiveProcessRecord(ai, processName,
ProcessBehavior.NORMAL, UnaryOperator.identity());
+ final ProcessRecord deliverRes;
+ switch (behavior) {
+ case SUCCESS_PREDECESSOR:
+ case FAIL_TIMEOUT_PREDECESSOR:
+ // Create a different process that will be linked to the
+ // returned process via a predecessor/successor relationship
+ mActiveProcesses.remove(res);
+ deliverRes = makeActiveProcessRecord(ai, processName,
+ ProcessBehavior.NORMAL, UnaryOperator.identity());
+ deliverRes.mPredecessor = res;
+ res.mSuccessor = deliverRes;
+ break;
+ default:
+ deliverRes = res;
+ break;
+ }
mHandlerThread.getThreadHandler().post(() -> {
synchronized (mAms) {
- mQueue.onApplicationAttachedLocked(res);
+ switch (behavior) {
+ case SUCCESS:
+ case SUCCESS_PREDECESSOR:
+ mQueue.onApplicationAttachedLocked(deliverRes);
+ break;
+ case FAIL_TIMEOUT:
+ case FAIL_TIMEOUT_PREDECESSOR:
+ mActiveProcesses.remove(deliverRes);
+ mQueue.onApplicationTimeoutLocked(deliverRes);
+ break;
+ default:
+ throw new UnsupportedOperationException();
+ }
}
});
return res;
@@ -281,9 +313,10 @@
// Verify that all processes have finished handling broadcasts
for (ProcessRecord app : mActiveProcesses) {
- assertTrue(app.toShortString(), app.mReceivers.numberOfCurReceivers() == 0);
- assertTrue(app.toShortString(), mQueue.getPreferredSchedulingGroupLocked(app)
- == ProcessList.SCHED_GROUP_UNDEFINED);
+ assertEquals(app.toShortString(), 0,
+ app.mReceivers.numberOfCurReceivers());
+ assertEquals(app.toShortString(), ProcessList.SCHED_GROUP_UNDEFINED,
+ mQueue.getPreferredSchedulingGroupLocked(app));
}
}
@@ -325,6 +358,19 @@
}
}
+ private enum ProcessStartBehavior {
+ /** Process starts successfully */
+ SUCCESS,
+ /** Process starts successfully via predecessor */
+ SUCCESS_PREDECESSOR,
+ /** Process fails by reporting timeout */
+ FAIL_TIMEOUT,
+ /** Process fails by reporting timeout via predecessor */
+ FAIL_TIMEOUT_PREDECESSOR,
+ /** Process fails by immediately returning null */
+ FAIL_NULL,
+ }
+
private enum ProcessBehavior {
/** Process broadcasts normally */
NORMAL,
@@ -956,18 +1002,16 @@
final ProcessRecord callerApp = makeActiveProcessRecord(PACKAGE_RED);
// Send broadcast while process starts are failing
- mFailStartProcess = true;
+ mNextProcessStartBehavior.set(ProcessStartBehavior.FAIL_NULL);
final Intent airplane = new Intent(Intent.ACTION_AIRPLANE_MODE_CHANGED);
enqueueBroadcast(makeBroadcastRecord(airplane, callerApp,
- List.of(makeManifestReceiver(PACKAGE_GREEN, CLASS_GREEN),
- makeManifestReceiver(PACKAGE_YELLOW, CLASS_YELLOW))));
+ List.of(makeManifestReceiver(PACKAGE_GREEN, CLASS_GREEN))));
// Confirm that queue goes idle, with no processes
waitForIdle();
assertEquals(1, mActiveProcesses.size());
// Send more broadcasts with working process starts
- mFailStartProcess = false;
final Intent timezone = new Intent(Intent.ACTION_TIMEZONE_CHANGED);
enqueueBroadcast(makeBroadcastRecord(timezone, callerApp,
List.of(makeManifestReceiver(PACKAGE_GREEN, CLASS_GREEN),
@@ -981,7 +1025,6 @@
final ProcessRecord receiverYellowApp = mAms.getProcessRecordLocked(PACKAGE_YELLOW,
getUidForPackage(PACKAGE_YELLOW));
verifyScheduleReceiver(never(), receiverGreenApp, airplane);
- verifyScheduleReceiver(never(), receiverYellowApp, airplane);
verifyScheduleReceiver(times(1), receiverGreenApp, timezone);
verifyScheduleReceiver(times(1), receiverYellowApp, timezone);
}
@@ -1071,6 +1114,52 @@
new ComponentName(PACKAGE_GREEN, CLASS_GREEN));
}
+ @Test
+ public void testCold_Success() throws Exception {
+ doCold(ProcessStartBehavior.SUCCESS);
+ }
+
+ @Test
+ public void testCold_Success_Predecessor() throws Exception {
+ doCold(ProcessStartBehavior.SUCCESS_PREDECESSOR);
+ }
+
+ @Test
+ public void testCold_Fail_Null() throws Exception {
+ doCold(ProcessStartBehavior.FAIL_NULL);
+ }
+
+ @Test
+ public void testCold_Fail_Timeout() throws Exception {
+ doCold(ProcessStartBehavior.FAIL_TIMEOUT);
+ }
+
+ @Test
+ public void testCold_Fail_Timeout_Predecessor() throws Exception {
+ doCold(ProcessStartBehavior.FAIL_TIMEOUT_PREDECESSOR);
+ }
+
+ private void doCold(ProcessStartBehavior behavior) throws Exception {
+ final ProcessRecord callerApp = makeActiveProcessRecord(PACKAGE_RED);
+
+ mNextProcessStartBehavior.set(behavior);
+ final Intent airplane = new Intent(Intent.ACTION_AIRPLANE_MODE_CHANGED);
+ enqueueBroadcast(makeBroadcastRecord(airplane, callerApp,
+ List.of(makeManifestReceiver(PACKAGE_GREEN, CLASS_GREEN))));
+ waitForIdle();
+
+ // Regardless of success/failure of above, we should always be able to
+ // recover and begin sending future broadcasts
+ final Intent timezone = new Intent(Intent.ACTION_TIMEZONE_CHANGED);
+ enqueueBroadcast(makeBroadcastRecord(timezone, callerApp,
+ List.of(makeManifestReceiver(PACKAGE_GREEN, CLASS_GREEN))));
+ waitForIdle();
+
+ final ProcessRecord receiverApp = mAms.getProcessRecordLocked(PACKAGE_GREEN,
+ getUidForPackage(PACKAGE_GREEN));
+ verifyScheduleReceiver(receiverApp, timezone);
+ }
+
/**
* Verify that we skip broadcasts to an app being backed up.
*/
