More robust updating of "runnable" list.
Whenever BroadcastProcessQueue.getRunnableAt() changes, we need to
check its position in the "runnable" list to ensure it stays sorted.
We've been returning boolean values to indicate when a caller should
invoke updateRunnableList(), so we expand this to more methods that
can have side-effects, and use the "@CheckResult" annotation to
ensure callers don't forget their duty.
It's okay to err on the side of calling updateRunnableList() when
unsure, since it has a fast-path that returns quickly if a queue is
already sorted correctly.
Bug: 274681945
Test: atest FrameworksMockingServicesTests:BroadcastQueueTest
Test: atest FrameworksMockingServicesTests:BroadcastQueueModernImplTest
Test: atest FrameworksMockingServicesTests:BroadcastRecordTest
Change-Id: Ief7d5f8540e5cbe3adc1f49c7703d41ae19003b8
diff --git a/services/core/java/com/android/server/am/BroadcastProcessQueue.java b/services/core/java/com/android/server/am/BroadcastProcessQueue.java
index 5140f90..23eaae2 100644
--- a/services/core/java/com/android/server/am/BroadcastProcessQueue.java
+++ b/services/core/java/com/android/server/am/BroadcastProcessQueue.java
@@ -21,6 +21,7 @@
import static com.android.server.am.BroadcastRecord.isDeliveryStateTerminal;
import static com.android.server.am.BroadcastRecord.isReceiverEquals;
+import android.annotation.CheckResult;
import android.annotation.IntDef;
import android.annotation.NonNull;
import android.annotation.Nullable;
@@ -341,7 +342,12 @@
* Predicates that choose to remove a broadcast <em>must</em> finish
* delivery of the matched broadcast, to ensure that situations like ordered
* broadcasts are handled consistently.
+ *
+ * @return if this operation may have changed internal state, indicating
+ * that the caller is responsible for invoking
+ * {@link BroadcastQueueModernImpl#updateRunnableList}
*/
+ @CheckResult
public boolean forEachMatchingBroadcast(@NonNull BroadcastPredicate predicate,
@NonNull BroadcastConsumer consumer, boolean andRemove) {
boolean didSomething = false;
@@ -354,6 +360,7 @@
return didSomething;
}
+ @CheckResult
private boolean forEachMatchingBroadcastInQueue(@NonNull ArrayDeque<SomeArgs> queue,
@NonNull BroadcastPredicate predicate, @NonNull BroadcastConsumer consumer,
boolean andRemove) {
@@ -370,6 +377,10 @@
args.recycle();
it.remove();
onBroadcastDequeued(record, recordIndex, recordWouldBeSkipped);
+ } else {
+ // Even if we're leaving broadcast in queue, it may have
+ // been mutated in such a way to change our runnable time
+ invalidateRunnableAt();
}
didSomething = true;
}
@@ -381,30 +392,43 @@
/**
* Update the actively running "warm" process for this process.
+ *
+ * @return if this operation may have changed internal state, indicating
+ * that the caller is responsible for invoking
+ * {@link BroadcastQueueModernImpl#updateRunnableList}
*/
- public void setProcess(@Nullable ProcessRecord app) {
+ @CheckResult
+ public boolean setProcessAndUidFrozen(@Nullable ProcessRecord app, boolean uidFrozen) {
this.app = app;
- if (app != null) {
- setProcessInstrumented(app.getActiveInstrumentation() != null);
- setProcessPersistent(app.isPersistent());
- } else {
- setProcessInstrumented(false);
- setProcessPersistent(false);
- }
// Since we may have just changed our PID, invalidate cached strings
mCachedToString = null;
mCachedToShortString = null;
+
+ boolean didSomething = false;
+ if (app != null) {
+ didSomething |= setProcessInstrumented(app.getActiveInstrumentation() != null);
+ didSomething |= setProcessPersistent(app.isPersistent());
+ didSomething |= setUidFrozen(uidFrozen);
+ } else {
+ didSomething |= setProcessInstrumented(false);
+ didSomething |= setProcessPersistent(false);
+ didSomething |= setUidFrozen(uidFrozen);
+ }
+ return didSomething;
}
/**
* Update if this UID is in the "frozen" state, typically signaling that
* broadcast dispatch should be paused or delayed.
*/
- public void setUidFrozen(boolean frozen) {
+ private boolean setUidFrozen(boolean frozen) {
if (mUidFrozen != frozen) {
mUidFrozen = frozen;
invalidateRunnableAt();
+ return true;
+ } else {
+ return false;
}
}
@@ -413,10 +437,13 @@
* signaling that broadcast dispatch should bypass all pauses or delays, to
* avoid holding up test suites.
*/
- public void setProcessInstrumented(boolean instrumented) {
+ private boolean setProcessInstrumented(boolean instrumented) {
if (mProcessInstrumented != instrumented) {
mProcessInstrumented = instrumented;
invalidateRunnableAt();
+ return true;
+ } else {
+ return false;
}
}
@@ -424,10 +451,13 @@
* Update if this process is in the "persistent" state, which signals broadcast dispatch should
* bypass all pauses or delays to prevent the system from becoming out of sync with itself.
*/
- public void setProcessPersistent(boolean persistent) {
+ private boolean setProcessPersistent(boolean persistent) {
if (mProcessPersistent != persistent) {
mProcessPersistent = persistent;
invalidateRunnableAt();
+ return true;
+ } else {
+ return false;
}
}
@@ -647,8 +677,20 @@
return mActive != null;
}
- void forceDelayBroadcastDelivery(long delayedDurationMs) {
- mForcedDelayedDurationMs = delayedDurationMs;
+ /**
+ * @return if this operation may have changed internal state, indicating
+ * that the caller is responsible for invoking
+ * {@link BroadcastQueueModernImpl#updateRunnableList}
+ */
+ @CheckResult
+ boolean forceDelayBroadcastDelivery(long delayedDurationMs) {
+ if (mForcedDelayedDurationMs != delayedDurationMs) {
+ mForcedDelayedDurationMs = delayedDurationMs;
+ invalidateRunnableAt();
+ return true;
+ } else {
+ return false;
+ }
}
/**
@@ -720,10 +762,21 @@
* broadcasts would be prioritized for dispatching, even if there are urgent broadcasts
* waiting. This is typically used in case there are callers waiting for "barrier" to be
* reached.
+ *
+ * @return if this operation may have changed internal state, indicating
+ * that the caller is responsible for invoking
+ * {@link BroadcastQueueModernImpl#updateRunnableList}
*/
+ @CheckResult
@VisibleForTesting
- void setPrioritizeEarliest(boolean prioritizeEarliest) {
- mPrioritizeEarliest = prioritizeEarliest;
+ boolean setPrioritizeEarliest(boolean prioritizeEarliest) {
+ if (mPrioritizeEarliest != prioritizeEarliest) {
+ mPrioritizeEarliest = prioritizeEarliest;
+ invalidateRunnableAt();
+ return true;
+ } else {
+ return false;
+ }
}
/**
diff --git a/services/core/java/com/android/server/am/BroadcastQueueModernImpl.java b/services/core/java/com/android/server/am/BroadcastQueueModernImpl.java
index e574b49..e35f822 100644
--- a/services/core/java/com/android/server/am/BroadcastQueueModernImpl.java
+++ b/services/core/java/com/android/server/am/BroadcastQueueModernImpl.java
@@ -360,8 +360,6 @@
// If app isn't running, and there's nothing in the queue, clean up
if (queue.isEmpty() && !queue.isActive() && !queue.isProcessWarm()) {
removeProcessQueue(queue.processName, queue.uid);
- } else {
- updateQueueDeferred(queue);
}
}
@@ -496,8 +494,7 @@
// relevant per-process queue
final BroadcastProcessQueue queue = getProcessQueue(app);
if (queue != null) {
- queue.setProcess(app);
- queue.setUidFrozen(mUidFrozen.get(queue.uid, false));
+ setQueueProcess(queue, app);
}
boolean didSomething = false;
@@ -538,8 +535,7 @@
// relevant per-process queue
final BroadcastProcessQueue queue = getProcessQueue(app);
if (queue != null) {
- queue.setProcess(null);
- queue.setUidFrozen(mUidFrozen.get(queue.uid, false));
+ setQueueProcess(queue, null);
}
if ((mRunningColdStart != null) && (mRunningColdStart == queue)) {
@@ -566,6 +562,7 @@
return (r.receivers.get(i) instanceof BroadcastFilter);
}, mBroadcastConsumerSkip, true);
if (didSomething || queue.isEmpty()) {
+ updateQueueDeferred(queue);
updateRunnableList(queue);
enqueueUpdateRunningList();
}
@@ -631,6 +628,7 @@
setDeliveryState(queue, null, r, i, receiver, BroadcastRecord.DELIVERY_DEFERRED,
"deferred at enqueue time");
}
+ updateQueueDeferred(queue);
updateRunnableList(queue);
enqueueUpdateRunningList();
}
@@ -1079,6 +1077,7 @@
final int queueIndex = getRunningIndexOf(queue);
mRunning[queueIndex] = null;
+ updateQueueDeferred(queue);
updateRunnableList(queue);
enqueueUpdateRunningList();
@@ -1161,6 +1160,7 @@
getReceiverUid(otherReceiver));
if (otherQueue != null) {
otherQueue.invalidateRunnableAt();
+ updateQueueDeferred(otherQueue);
updateRunnableList(otherQueue);
}
}
@@ -1291,6 +1291,7 @@
if (queuePredicate.test(leaf)) {
if (leaf.forEachMatchingBroadcast(broadcastPredicate,
broadcastConsumer, andRemove)) {
+ updateQueueDeferred(leaf);
updateRunnableList(leaf);
didSomething = true;
}
@@ -1304,29 +1305,40 @@
return didSomething;
}
- private void forEachMatchingQueue(
+ private boolean forEachMatchingQueue(
@NonNull Predicate<BroadcastProcessQueue> queuePredicate,
@NonNull Consumer<BroadcastProcessQueue> queueConsumer) {
+ boolean didSomething = false;
for (int i = mProcessQueues.size() - 1; i >= 0; i--) {
BroadcastProcessQueue leaf = mProcessQueues.valueAt(i);
while (leaf != null) {
if (queuePredicate.test(leaf)) {
queueConsumer.accept(leaf);
+ updateQueueDeferred(leaf);
updateRunnableList(leaf);
+ didSomething = true;
}
leaf = leaf.processNameNext;
}
}
+ if (didSomething) {
+ enqueueUpdateRunningList();
+ }
+ return didSomething;
}
- private void updateQueueDeferred(
- @NonNull BroadcastProcessQueue leaf) {
+ @SuppressWarnings("CheckResult")
+ private void updateQueueDeferred(@NonNull BroadcastProcessQueue leaf) {
if (leaf.isDeferredUntilActive()) {
+ // We ignore the returned value here since callers are invoking us
+ // just before updateRunnableList()
leaf.forEachMatchingBroadcast((r, i) -> {
return r.deferUntilActive && (r.getDeliveryState(i)
== BroadcastRecord.DELIVERY_PENDING);
}, mBroadcastConsumerDefer, false);
} else if (leaf.hasDeferredBroadcasts()) {
+ // We ignore the returned value here since callers are invoking us
+ // just before updateRunnableList()
leaf.forEachMatchingBroadcast((r, i) -> {
return r.deferUntilActive && (r.getDeliveryState(i)
== BroadcastRecord.DELIVERY_DEFERRED);
@@ -1356,10 +1368,7 @@
while (leaf != null) {
// Update internal state by refreshing values previously
// read from any known running process
- leaf.setProcess(leaf.app);
- leaf.setUidFrozen(frozen);
- updateQueueDeferred(leaf);
- updateRunnableList(leaf);
+ setQueueProcess(leaf, leaf.app);
leaf = leaf.processNameNext;
}
enqueueUpdateRunningList();
@@ -1520,8 +1529,20 @@
private void updateWarmProcess(@NonNull BroadcastProcessQueue queue) {
if (!queue.isProcessWarm()) {
- queue.setProcess(mService.getProcessRecordLocked(queue.processName, queue.uid));
- queue.setUidFrozen(mUidFrozen.get(queue.uid, false));
+ setQueueProcess(queue, mService.getProcessRecordLocked(queue.processName, queue.uid));
+ }
+ }
+
+ /**
+ * Update the {@link ProcessRecord} associated with the given
+ * {@link BroadcastProcessQueue}. Also updates any runnable status that
+ * might have changed as a side-effect.
+ */
+ private void setQueueProcess(@NonNull BroadcastProcessQueue queue,
+ @Nullable ProcessRecord app) {
+ if (queue.setProcessAndUidFrozen(app, mUidFrozen.get(queue.uid, false))) {
+ updateQueueDeferred(queue);
+ updateRunnableList(queue);
}
}
@@ -1715,8 +1736,7 @@
}
BroadcastProcessQueue created = new BroadcastProcessQueue(mConstants, processName, uid);
- created.setProcess(mService.getProcessRecordLocked(processName, uid));
- created.setUidFrozen(mUidFrozen.get(uid, false));
+ setQueueProcess(created, mService.getProcessRecordLocked(processName, uid));
if (leaf == null) {
mProcessQueues.put(uid, created);
diff --git a/services/tests/mockingservicestests/src/com/android/server/am/BroadcastQueueModernImplTest.java b/services/tests/mockingservicestests/src/com/android/server/am/BroadcastQueueModernImplTest.java
index c68db4f..74b68e7 100644
--- a/services/tests/mockingservicestests/src/com/android/server/am/BroadcastQueueModernImplTest.java
+++ b/services/tests/mockingservicestests/src/com/android/server/am/BroadcastQueueModernImplTest.java
@@ -371,9 +371,9 @@
List.of(makeMockRegisteredReceiver()), false);
queue.enqueueOrReplaceBroadcast(airplaneRecord, 0, false);
- queue.setUidFrozen(false);
+ queue.setProcessAndUidFrozen(null, false);
final long notCachedRunnableAt = queue.getRunnableAt();
- queue.setUidFrozen(true);
+ queue.setProcessAndUidFrozen(null, true);
final long cachedRunnableAt = queue.getRunnableAt();
assertThat(cachedRunnableAt).isGreaterThan(notCachedRunnableAt);
assertFalse(queue.isRunnable());
@@ -398,9 +398,9 @@
List.of(makeMockRegisteredReceiver()), false);
queue.enqueueOrReplaceBroadcast(airplaneRecord, 0, false);
- queue.setUidFrozen(false);
+ queue.setProcessAndUidFrozen(null, false);
final long notCachedRunnableAt = queue.getRunnableAt();
- queue.setUidFrozen(true);
+ queue.setProcessAndUidFrozen(null, true);
final long cachedRunnableAt = queue.getRunnableAt();
assertThat(cachedRunnableAt).isGreaterThan(notCachedRunnableAt);
assertTrue(queue.isRunnable());
@@ -430,13 +430,13 @@
// verify that:
// (a) the queue is immediately runnable by existence of a fg-priority broadcast
// (b) the next one up is the fg-priority broadcast despite its later enqueue time
- queue.setUidFrozen(false);
+ queue.setProcessAndUidFrozen(null, false);
assertTrue(queue.isRunnable());
assertThat(queue.getRunnableAt()).isAtMost(airplaneRecord.enqueueClockTime);
assertEquals(ProcessList.SCHED_GROUP_DEFAULT, queue.getPreferredSchedulingGroupLocked());
assertEquals(queue.peekNextBroadcastRecord(), airplaneRecord);
- queue.setUidFrozen(true);
+ queue.setProcessAndUidFrozen(null, true);
assertTrue(queue.isRunnable());
assertThat(queue.getRunnableAt()).isAtMost(airplaneRecord.enqueueClockTime);
assertEquals(ProcessList.SCHED_GROUP_DEFAULT, queue.getPreferredSchedulingGroupLocked());
@@ -499,7 +499,7 @@
private void doRunnableAt_Frozen(BroadcastRecord testRecord, int testRunnableAtReason) {
final BroadcastProcessQueue queue = new BroadcastProcessQueue(mConstants,
PACKAGE_GREEN, getUidForPackage(PACKAGE_GREEN));
- queue.setUidFrozen(true);
+ queue.setProcessAndUidFrozen(null, true);
final BroadcastRecord lazyRecord = makeBroadcastRecord(
new Intent(Intent.ACTION_AIRPLANE_MODE_CHANGED),