From 5cb9f358a9650d158ffc8c165875292dc04b1dd0 Mon Sep 17 00:00:00 2001 From: Sudheer Shanka Date: Wed, 4 Jan 2023 22:28:03 -0800 Subject: [PATCH] Fix user cleanup in modern broadcast queue. When a user is removed, we iterate through all existing processes in that user and mark any pending receivers as "skipped" but the way we are currently iterating and removing the process queues at the same time could result in skipping over some process queues. Bug: 259529372 Bug: 260158381 Test: atest services/tests/mockingservicestests/src/com/android/server/am/BroadcastQueueModernImplTest.java Test: atest services/tests/mockingservicestests/src/com/android/server/am/BroadcastQueueTest.java Change-Id: I3487756ca4c230d3a67fa2f85751aefeb055a66e --- .../server/am/BroadcastQueueModernImpl.java | 4 +- .../am/BroadcastQueueModernImplTest.java | 37 ++++++++++++ .../android/server/am/BroadcastQueueTest.java | 57 ++++++++++++++++++- 3 files changed, 94 insertions(+), 4 deletions(-) diff --git a/services/core/java/com/android/server/am/BroadcastQueueModernImpl.java b/services/core/java/com/android/server/am/BroadcastQueueModernImpl.java index 8f241b2bbad5e..15dfd72e365ea 100644 --- a/services/core/java/com/android/server/am/BroadcastQueueModernImpl.java +++ b/services/core/java/com/android/server/am/BroadcastQueueModernImpl.java @@ -1341,7 +1341,7 @@ class BroadcastQueueModernImpl extends BroadcastQueue { @NonNull BroadcastPredicate broadcastPredicate, @NonNull BroadcastConsumer broadcastConsumer, boolean andRemove) { boolean didSomething = false; - for (int i = 0; i < mProcessQueues.size(); i++) { + for (int i = mProcessQueues.size() - 1; i >= 0; i--) { BroadcastProcessQueue leaf = mProcessQueues.valueAt(i); while (leaf != null) { if (queuePredicate.test(leaf)) { @@ -1363,7 +1363,7 @@ class BroadcastQueueModernImpl extends BroadcastQueue { private void forEachMatchingQueue( @NonNull Predicate queuePredicate, @NonNull Consumer queueConsumer) { - for (int i = 0; i < mProcessQueues.size(); i++) { + for (int i = mProcessQueues.size() - 1; i >= 0; i--) { BroadcastProcessQueue leaf = mProcessQueues.valueAt(i); while (leaf != null) { if (queuePredicate.test(leaf)) { 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 89077e324fb81..77127c536d6da 100644 --- a/services/tests/mockingservicestests/src/com/android/server/am/BroadcastQueueModernImplTest.java +++ b/services/tests/mockingservicestests/src/com/android/server/am/BroadcastQueueModernImplTest.java @@ -25,7 +25,10 @@ import static com.android.server.am.BroadcastProcessQueue.REASON_CONTAINS_PRIORI import static com.android.server.am.BroadcastProcessQueue.REASON_CONTAINS_RESULT_TO; import static com.android.server.am.BroadcastProcessQueue.insertIntoRunnableList; import static com.android.server.am.BroadcastProcessQueue.removeFromRunnableList; +import static com.android.server.am.BroadcastQueueTest.CLASS_BLUE; import static com.android.server.am.BroadcastQueueTest.CLASS_GREEN; +import static com.android.server.am.BroadcastQueueTest.CLASS_RED; +import static com.android.server.am.BroadcastQueueTest.CLASS_YELLOW; import static com.android.server.am.BroadcastQueueTest.PACKAGE_BLUE; import static com.android.server.am.BroadcastQueueTest.PACKAGE_GREEN; import static com.android.server.am.BroadcastQueueTest.PACKAGE_RED; @@ -981,6 +984,40 @@ public class BroadcastQueueModernImplTest { assertTrue(queue.isEmpty()); } + @Test + public void testCleanupDisabledPackageReceiversLocked() { + final Intent userPresent = new Intent(Intent.ACTION_USER_PRESENT); + final Intent timeTick = new Intent(Intent.ACTION_TIME_TICK); + + final BroadcastRecord record1 = makeBroadcastRecord(userPresent, List.of( + makeManifestReceiver(PACKAGE_GREEN, CLASS_GREEN), + makeManifestReceiver(PACKAGE_RED, CLASS_BLUE), + makeManifestReceiver(PACKAGE_YELLOW, CLASS_RED), + makeManifestReceiver(PACKAGE_BLUE, CLASS_GREEN) + )); + final BroadcastRecord record2 = makeBroadcastRecord(timeTick, List.of( + makeManifestReceiver(PACKAGE_GREEN, CLASS_GREEN), + makeManifestReceiver(PACKAGE_RED, CLASS_RED), + makeManifestReceiver(PACKAGE_YELLOW, CLASS_YELLOW) + )); + + // Halt all processing so that we get a consistent view + mHandlerThread.getLooper().getQueue().postSyncBarrier(); + mImpl.enqueueBroadcastLocked(record1); + mImpl.enqueueBroadcastLocked(record2); + + mImpl.cleanupDisabledPackageReceiversLocked(null, null, UserHandle.USER_SYSTEM); + + // Verify that all receivers have been marked as "skipped". + for (BroadcastRecord record : new BroadcastRecord[] {record1, record2}) { + for (int i = 0; i < record.receivers.size(); ++i) { + final String errMsg = "Unexpected delivery state for record:" + record + + "; receiver=" + record.receivers.get(i); + assertEquals(errMsg, BroadcastRecord.DELIVERY_SKIPPED, record.getDeliveryState(i)); + } + } + } + private Intent createPackageChangedIntent(int uid, List componentNameList) { final Intent packageChangedIntent = new Intent(Intent.ACTION_PACKAGE_CHANGED); packageChangedIntent.putExtra(Intent.EXTRA_UID, uid); 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 6800d5282806e..b6a53ac1846fa 100644 --- a/services/tests/mockingservicestests/src/com/android/server/am/BroadcastQueueTest.java +++ b/services/tests/mockingservicestests/src/com/android/server/am/BroadcastQueueTest.java @@ -25,6 +25,7 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotEquals; import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyBoolean; @@ -805,14 +806,27 @@ public class BroadcastQueueTest { null, null, null, null, null, null, userId, null)); } + private void verifyScheduleReceiver(VerificationMode mode, ProcessRecord app, + int userId) throws Exception { + verify(app.getThread(), mode).scheduleReceiverList( + manifestReceiver(null, + null, null, null, null, null, null, userId, null)); + } + private void verifyScheduleRegisteredReceiver(ProcessRecord app, - Intent intent) - throws Exception { + Intent intent) throws Exception { verify(app.getThread()).scheduleReceiverList( registeredReceiver(null, filterEqualsIgnoringComponent(intent), null, null, null, null, null, UserHandle.USER_SYSTEM, null)); } + private void verifyScheduleRegisteredReceiver(VerificationMode mode, ProcessRecord app, + int userId) throws Exception { + verify(app.getThread(), mode).scheduleReceiverList( + registeredReceiver(null, null, + null, null, null, null, null, userId, null)); + } + static final int USER_GUEST = 11; static final String PACKAGE_ANDROID = "android"; @@ -1222,6 +1236,45 @@ public class BroadcastQueueTest { new ComponentName(PACKAGE_GREEN, CLASS_BLUE)); } + @Test + public void testCleanup_userRemoved() throws Exception { + final ProcessRecord callerApp = makeActiveProcessRecord( + PACKAGE_RED, PACKAGE_RED, ProcessBehavior.NORMAL, + USER_GUEST); + + final Intent airplane = new Intent(Intent.ACTION_AIRPLANE_MODE_CHANGED); + final Intent timeZone = new Intent(Intent.ACTION_TIMEZONE_CHANGED); + try (SyncBarrier b = new SyncBarrier()) { + enqueueBroadcast(makeBroadcastRecord(airplane, callerApp, USER_GUEST, new ArrayList<>( + List.of(makeRegisteredReceiver(callerApp), + makeManifestReceiver(PACKAGE_GREEN, CLASS_RED, USER_GUEST), + makeManifestReceiver(PACKAGE_BLUE, CLASS_GREEN, USER_GUEST), + makeManifestReceiver(PACKAGE_YELLOW, CLASS_BLUE, USER_GUEST))))); + enqueueBroadcast(makeBroadcastRecord(timeZone, callerApp, USER_GUEST, new ArrayList<>( + List.of(makeRegisteredReceiver(callerApp), + makeManifestReceiver(PACKAGE_GREEN, CLASS_RED, USER_GUEST), + makeManifestReceiver(PACKAGE_BLUE, CLASS_GREEN, USER_GUEST), + makeManifestReceiver(PACKAGE_YELLOW, CLASS_BLUE, USER_GUEST))))); + + synchronized (mAms) { + mQueue.cleanupDisabledPackageReceiversLocked(null, null, USER_GUEST); + } + } + + waitForIdle(); + // Legacy stack does not remove registered receivers as part of + // cleanUpDisabledPackageReceiversLocked() call, so verify this only on modern queue. + if (mImpl == Impl.MODERN) { + verifyScheduleReceiver(never(), callerApp, USER_GUEST); + verifyScheduleRegisteredReceiver(never(), callerApp, USER_GUEST); + } + for (String pkg : new String[] { + PACKAGE_GREEN, PACKAGE_BLUE, PACKAGE_YELLOW + }) { + assertNull(mAms.getProcessRecordLocked(pkg, getUidForPackage(pkg, USER_GUEST))); + } + } + /** * Verify that killing a running process skips registered receivers. */