From 7529b207666881c747c2685e4279a4cea6e0a581 Mon Sep 17 00:00:00 2001 From: Jeff Sharkey Date: Tue, 28 Mar 2023 10:26:28 -0600 Subject: [PATCH] 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 --- .../server/am/BroadcastProcessQueue.java | 83 +++++++++++++++---- .../server/am/BroadcastQueueModernImpl.java | 54 ++++++++---- .../am/BroadcastQueueModernImplTest.java | 14 ++-- 3 files changed, 112 insertions(+), 39 deletions(-) diff --git a/services/core/java/com/android/server/am/BroadcastProcessQueue.java b/services/core/java/com/android/server/am/BroadcastProcessQueue.java index 5140f9056e119..23eaae23e99f8 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.deliveryStateToString; 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 @@ class BroadcastProcessQueue { * Predicates that choose to remove a broadcast must 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 @@ class BroadcastProcessQueue { return didSomething; } + @CheckResult private boolean forEachMatchingBroadcastInQueue(@NonNull ArrayDeque queue, @NonNull BroadcastPredicate predicate, @NonNull BroadcastConsumer consumer, boolean andRemove) { @@ -370,6 +377,10 @@ class BroadcastProcessQueue { 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 @@ class BroadcastProcessQueue { /** * 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 @@ class BroadcastProcessQueue { * 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 @@ class BroadcastProcessQueue { * 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 @@ class BroadcastProcessQueue { 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 @@ class BroadcastProcessQueue { * 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 e574b490f3fd0..e35f8226d3b1a 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 @@ class BroadcastQueueModernImpl extends BroadcastQueue { // 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 @@ class BroadcastQueueModernImpl extends BroadcastQueue { // 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 @@ class BroadcastQueueModernImpl extends BroadcastQueue { // 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 @@ class BroadcastQueueModernImpl extends BroadcastQueue { return (r.receivers.get(i) instanceof BroadcastFilter); }, mBroadcastConsumerSkip, true); if (didSomething || queue.isEmpty()) { + updateQueueDeferred(queue); updateRunnableList(queue); enqueueUpdateRunningList(); } @@ -631,6 +628,7 @@ class BroadcastQueueModernImpl extends BroadcastQueue { setDeliveryState(queue, null, r, i, receiver, BroadcastRecord.DELIVERY_DEFERRED, "deferred at enqueue time"); } + updateQueueDeferred(queue); updateRunnableList(queue); enqueueUpdateRunningList(); } @@ -1079,6 +1077,7 @@ class BroadcastQueueModernImpl extends BroadcastQueue { final int queueIndex = getRunningIndexOf(queue); mRunning[queueIndex] = null; + updateQueueDeferred(queue); updateRunnableList(queue); enqueueUpdateRunningList(); @@ -1161,6 +1160,7 @@ class BroadcastQueueModernImpl extends BroadcastQueue { getReceiverUid(otherReceiver)); if (otherQueue != null) { otherQueue.invalidateRunnableAt(); + updateQueueDeferred(otherQueue); updateRunnableList(otherQueue); } } @@ -1291,6 +1291,7 @@ class BroadcastQueueModernImpl extends BroadcastQueue { if (queuePredicate.test(leaf)) { if (leaf.forEachMatchingBroadcast(broadcastPredicate, broadcastConsumer, andRemove)) { + updateQueueDeferred(leaf); updateRunnableList(leaf); didSomething = true; } @@ -1304,29 +1305,40 @@ class BroadcastQueueModernImpl extends BroadcastQueue { return didSomething; } - private void forEachMatchingQueue( + private boolean forEachMatchingQueue( @NonNull Predicate queuePredicate, @NonNull Consumer 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 @@ class BroadcastQueueModernImpl extends BroadcastQueue { 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 @@ class BroadcastQueueModernImpl extends BroadcastQueue { 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 @@ class BroadcastQueueModernImpl extends BroadcastQueue { } 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 c68db4f673372..74b68e7f3e0fd 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 @@ public final class BroadcastQueueModernImplTest { 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 @@ public final class BroadcastQueueModernImplTest { 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 @@ public final class BroadcastQueueModernImplTest { // 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 @@ public final class BroadcastQueueModernImplTest { 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),