From 07e307877408f3b64274e791c9a1861c2674ae36 Mon Sep 17 00:00:00 2001 From: Sudheer Shanka Date: Tue, 25 Apr 2023 01:59:54 +0000 Subject: [PATCH] Skip enqueueing receivers that are going to be skipped by policy. The current logic assumes that only receivers in apps that are not already running will be skipped but this is not the case. Since we are going to skip the broadcast anyway, don't enqueue it in the broadcast process queue. Bug: 277815320 Test: atest FrameworksMockingServicesTests:BroadcastQueueTest Test: atest FrameworksMockingServicesTests:BroadcastQueueModernImplTest Change-Id: I27cb4a0959c20e2dbc656f9650dea4d0643bcba5 --- .../server/am/BroadcastProcessQueue.java | 41 ++++------- .../server/am/BroadcastQueueModernImpl.java | 24 +++---- .../am/BroadcastQueueModernImplTest.java | 71 +++++++++++++------ 3 files changed, 71 insertions(+), 65 deletions(-) diff --git a/services/core/java/com/android/server/am/BroadcastProcessQueue.java b/services/core/java/com/android/server/am/BroadcastProcessQueue.java index 0767218ec065f..5c68e67590833 100644 --- a/services/core/java/com/android/server/am/BroadcastProcessQueue.java +++ b/services/core/java/com/android/server/am/BroadcastProcessQueue.java @@ -242,8 +242,7 @@ class BroadcastProcessQueue { */ @Nullable public BroadcastRecord enqueueOrReplaceBroadcast(@NonNull BroadcastRecord record, - int recordIndex, boolean wouldBeSkipped, - @NonNull BroadcastConsumer deferredStatesApplyConsumer) { + int recordIndex, @NonNull BroadcastConsumer deferredStatesApplyConsumer) { // When updateDeferredStates() has already applied a deferred state to // all pending items, apply to this new broadcast too if (mLastDeferredStates && record.deferUntilActive @@ -252,8 +251,7 @@ class BroadcastProcessQueue { } if (record.isReplacePending()) { - final BroadcastRecord replacedBroadcastRecord = replaceBroadcast(record, recordIndex, - wouldBeSkipped); + final BroadcastRecord replacedBroadcastRecord = replaceBroadcast(record, recordIndex); if (replacedBroadcastRecord != null) { return replacedBroadcastRecord; } @@ -264,14 +262,13 @@ class BroadcastProcessQueue { SomeArgs newBroadcastArgs = SomeArgs.obtain(); newBroadcastArgs.arg1 = record; newBroadcastArgs.argi1 = recordIndex; - newBroadcastArgs.argi2 = (wouldBeSkipped ? 1 : 0); // Cross-broadcast prioritization policy: some broadcasts might warrant being // issued ahead of others that are already pending, for example if this new // broadcast is in a different delivery class or is tied to a direct user interaction // with implicit responsiveness expectations. getQueueForBroadcast(record).addLast(newBroadcastArgs); - onBroadcastEnqueued(record, recordIndex, wouldBeSkipped); + onBroadcastEnqueued(record, recordIndex); return null; } @@ -285,10 +282,9 @@ class BroadcastProcessQueue { * wasn't any broadcast that was replaced. */ @Nullable - private BroadcastRecord replaceBroadcast(@NonNull BroadcastRecord record, int recordIndex, - boolean wouldBeSkipped) { + private BroadcastRecord replaceBroadcast(@NonNull BroadcastRecord record, int recordIndex) { final ArrayDeque queue = getQueueForBroadcast(record); - return replaceBroadcastInQueue(queue, record, recordIndex, wouldBeSkipped); + return replaceBroadcastInQueue(queue, record, recordIndex); } /** @@ -302,15 +298,13 @@ class BroadcastProcessQueue { */ @Nullable private BroadcastRecord replaceBroadcastInQueue(@NonNull ArrayDeque queue, - @NonNull BroadcastRecord record, int recordIndex, - boolean wouldBeSkipped) { + @NonNull BroadcastRecord record, int recordIndex) { final Iterator it = queue.descendingIterator(); final Object receiver = record.receivers.get(recordIndex); while (it.hasNext()) { final SomeArgs args = it.next(); final BroadcastRecord testRecord = (BroadcastRecord) args.arg1; final int testRecordIndex = args.argi1; - final boolean testWouldBeSkipped = (args.argi2 == 1); final Object testReceiver = testRecord.receivers.get(testRecordIndex); if ((record.callingUid == testRecord.callingUid) && (record.userId == testRecord.userId) @@ -320,10 +314,9 @@ class BroadcastProcessQueue { // Exact match found; perform in-place swap args.arg1 = record; args.argi1 = recordIndex; - args.argi2 = (wouldBeSkipped ? 1 : 0); record.copyEnqueueTimeFrom(testRecord); - onBroadcastDequeued(testRecord, testRecordIndex, testWouldBeSkipped); - onBroadcastEnqueued(record, recordIndex, wouldBeSkipped); + onBroadcastDequeued(testRecord, testRecordIndex); + onBroadcastEnqueued(record, recordIndex); return testRecord; } } @@ -383,13 +376,12 @@ class BroadcastProcessQueue { final SomeArgs args = it.next(); final BroadcastRecord record = (BroadcastRecord) args.arg1; final int recordIndex = args.argi1; - final boolean recordWouldBeSkipped = (args.argi2 == 1); if (predicate.test(record, recordIndex)) { consumer.accept(record, recordIndex); if (andRemove) { args.recycle(); it.remove(); - onBroadcastDequeued(record, recordIndex, recordWouldBeSkipped); + onBroadcastDequeued(record, recordIndex); } else { // Even if we're leaving broadcast in queue, it may have // been mutated in such a way to change our runnable time @@ -539,12 +531,11 @@ class BroadcastProcessQueue { final SomeArgs next = removeNextBroadcast(); mActive = (BroadcastRecord) next.arg1; mActiveIndex = next.argi1; - final boolean wouldBeSkipped = (next.argi2 == 1); mActiveCountSinceIdle++; mActiveViaColdStart = false; mActiveWasStopped = false; next.recycle(); - onBroadcastDequeued(mActive, mActiveIndex, wouldBeSkipped); + onBroadcastDequeued(mActive, mActiveIndex); } /** @@ -561,8 +552,7 @@ class BroadcastProcessQueue { /** * Update summary statistics when the given record has been enqueued. */ - private void onBroadcastEnqueued(@NonNull BroadcastRecord record, int recordIndex, - boolean wouldBeSkipped) { + private void onBroadcastEnqueued(@NonNull BroadcastRecord record, int recordIndex) { mCountEnqueued++; if (record.deferUntilActive) { mCountDeferred++; @@ -594,8 +584,7 @@ class BroadcastProcessQueue { if (record.callerInstrumented) { mCountInstrumented++; } - if (!wouldBeSkipped - && (record.receivers.get(recordIndex) instanceof ResolveInfo)) { + if (record.receivers.get(recordIndex) instanceof ResolveInfo) { mCountManifest++; } invalidateRunnableAt(); @@ -604,8 +593,7 @@ class BroadcastProcessQueue { /** * Update summary statistics when the given record has been dequeued. */ - private void onBroadcastDequeued(@NonNull BroadcastRecord record, int recordIndex, - boolean wouldBeSkipped) { + private void onBroadcastDequeued(@NonNull BroadcastRecord record, int recordIndex) { mCountEnqueued--; if (record.deferUntilActive) { mCountDeferred--; @@ -637,8 +625,7 @@ class BroadcastProcessQueue { if (record.callerInstrumented) { mCountInstrumented--; } - if (!wouldBeSkipped - && (record.receivers.get(recordIndex) instanceof ResolveInfo)) { + if (record.receivers.get(recordIndex) instanceof ResolveInfo) { mCountManifest--; } invalidateRunnableAt(); diff --git a/services/core/java/com/android/server/am/BroadcastQueueModernImpl.java b/services/core/java/com/android/server/am/BroadcastQueueModernImpl.java index 8735f8a37b8bc..96e152320282c 100644 --- a/services/core/java/com/android/server/am/BroadcastQueueModernImpl.java +++ b/services/core/java/com/android/server/am/BroadcastQueueModernImpl.java @@ -606,24 +606,18 @@ class BroadcastQueueModernImpl extends BroadcastQueue { final BroadcastProcessQueue queue = getOrCreateProcessQueue( getReceiverProcessName(receiver), getReceiverUid(receiver)); - boolean wouldBeSkipped = false; - if (receiver instanceof ResolveInfo) { - // If the app is running but would not have been started if the process weren't - // running, we're going to deliver the broadcast but mark that it's not a manifest - // broadcast that would have started the app. This allows BroadcastProcessQueue to - // defer the broadcast as though it were a normal runtime receiver. - wouldBeSkipped = (mSkipPolicy.shouldSkipMessage(r, receiver) != null); - if (wouldBeSkipped && queue.app == null && !queue.getActiveViaColdStart()) { - // Skip receiver if there's no running app, the app is not being started, and - // the app wouldn't be launched for this broadcast - setDeliveryState(null, null, r, i, receiver, BroadcastRecord.DELIVERY_SKIPPED, - "skipped by policy to avoid cold start"); - continue; - } + // If this receiver is going to be skipped, skip it now itself and don't even enqueue + // it. + final boolean wouldBeSkipped = (mSkipPolicy.shouldSkipMessage(r, receiver) != null); + if (wouldBeSkipped) { + setDeliveryState(null, null, r, i, receiver, BroadcastRecord.DELIVERY_SKIPPED, + "skipped by policy at enqueue"); + continue; } + enqueuedBroadcast = true; final BroadcastRecord replacedBroadcast = queue.enqueueOrReplaceBroadcast( - r, i, wouldBeSkipped, mBroadcastConsumerDeferApply); + r, i, mBroadcastConsumerDeferApply); if (replacedBroadcast != null) { replacedBroadcasts.add(replacedBroadcast); } 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 3fb7fb4e6aeae..acfea85d60a2c 100644 --- a/services/tests/mockingservicestests/src/com/android/server/am/BroadcastQueueModernImplTest.java +++ b/services/tests/mockingservicestests/src/com/android/server/am/BroadcastQueueModernImplTest.java @@ -40,6 +40,7 @@ import static com.android.server.am.BroadcastQueueTest.PACKAGE_YELLOW; import static com.android.server.am.BroadcastQueueTest.getUidForPackage; import static com.android.server.am.BroadcastQueueTest.makeManifestReceiver; import static com.android.server.am.BroadcastQueueTest.withPriority; +import static com.android.server.am.BroadcastRecord.isReceiverEquals; import static com.google.common.truth.Truth.assertThat; @@ -49,13 +50,16 @@ import static org.junit.Assert.assertNotEquals; import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; +import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyInt; import static org.mockito.ArgumentMatchers.anyLong; import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.ArgumentMatchers.nullable; +import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.spy; import static org.mockito.Mockito.times; import android.annotation.NonNull; @@ -116,6 +120,7 @@ public final class BroadcastQueueModernImplTest { TestLooperManager mLooper; BroadcastConstants mConstants; + private BroadcastSkipPolicy mSkipPolicy; BroadcastQueueModernImpl mImpl; BroadcastProcessQueue mHead; @@ -139,29 +144,18 @@ public final class BroadcastQueueModernImplTest { mConstants.DELAY_NORMAL_MILLIS = 10_000; mConstants.DELAY_CACHED_MILLIS = 120_000; - final BroadcastSkipPolicy emptySkipPolicy = new BroadcastSkipPolicy(mAms) { - public boolean shouldSkip(BroadcastRecord r, Object o) { - // Ignored - return false; - } - public String shouldSkipMessage(BroadcastRecord r, Object o) { - // Ignored - return null; - } - public boolean disallowBackgroundStart(BroadcastRecord r) { - // Ignored - return false; - } - }; + mSkipPolicy = spy(new BroadcastSkipPolicy(mAms)); + doReturn(null).when(mSkipPolicy).shouldSkipMessage(any(), any()); + doReturn(false).when(mSkipPolicy).disallowBackgroundStart(any()); + final BroadcastHistory emptyHistory = new BroadcastHistory(mConstants) { public void addBroadcastToHistoryLocked(BroadcastRecord original) { // Ignored } }; - mImpl = new BroadcastQueueModernImpl(mAms, mHandlerThread.getThreadHandler(), - mConstants, mConstants, emptySkipPolicy, emptyHistory); + mConstants, mConstants, mSkipPolicy, emptyHistory); doReturn(1L).when(mQueue1).getRunnableAt(); doReturn(2L).when(mQueue2).getRunnableAt(); @@ -256,17 +250,12 @@ public final class BroadcastQueueModernImplTest { private void enqueueOrReplaceBroadcast(BroadcastProcessQueue queue, BroadcastRecord record, int recordIndex) { - enqueueOrReplaceBroadcast(queue, record, recordIndex, false, 42_000_000L); + enqueueOrReplaceBroadcast(queue, record, recordIndex, 42_000_000L); } private void enqueueOrReplaceBroadcast(BroadcastProcessQueue queue, BroadcastRecord record, int recordIndex, long enqueueTime) { - enqueueOrReplaceBroadcast(queue, record, recordIndex, false, enqueueTime); - } - - private void enqueueOrReplaceBroadcast(BroadcastProcessQueue queue, - BroadcastRecord record, int recordIndex, boolean wouldBeSkipped, long enqueueTime) { - queue.enqueueOrReplaceBroadcast(record, recordIndex, wouldBeSkipped, (r, i) -> { + queue.enqueueOrReplaceBroadcast(record, recordIndex, (r, i) -> { throw new UnsupportedOperationException(); }); record.enqueueTime = enqueueTime; @@ -1199,6 +1188,42 @@ public final class BroadcastQueueModernImplTest { assertEquals(ProcessList.SCHED_GROUP_DEFAULT, queue.getPreferredSchedulingGroupLocked()); } + @Test + public void testSkipPolicy_atEnqueueTime() throws Exception { + final Intent userPresent = new Intent(Intent.ACTION_USER_PRESENT); + final Object greenReceiver = makeManifestReceiver(PACKAGE_GREEN, CLASS_GREEN); + final Object redReceiver = makeManifestReceiver(PACKAGE_RED, CLASS_RED); + + final BroadcastRecord userPresentRecord = makeBroadcastRecord(userPresent, + List.of(greenReceiver, redReceiver)); + + final Intent timeTick = new Intent(Intent.ACTION_TIME_TICK); + final BroadcastRecord timeTickRecord = makeBroadcastRecord(timeTick, + List.of(greenReceiver, redReceiver)); + + doAnswer(invocation -> { + final BroadcastRecord r = invocation.getArgument(0); + final Object o = invocation.getArgument(1); + if (userPresent.getAction().equals(r.intent.getAction()) + && isReceiverEquals(o, greenReceiver)) { + return "receiver skipped by test"; + } + return null; + }).when(mSkipPolicy).shouldSkipMessage(any(BroadcastRecord.class), any()); + + mImpl.enqueueBroadcastLocked(userPresentRecord); + mImpl.enqueueBroadcastLocked(timeTickRecord); + + final BroadcastProcessQueue greenQueue = mImpl.getProcessQueue(PACKAGE_GREEN, + getUidForPackage(PACKAGE_GREEN)); + // There should be only one broadcast for green process as the other would have + // been skipped. + verifyPendingRecords(greenQueue, List.of(timeTick)); + final BroadcastProcessQueue redQueue = mImpl.getProcessQueue(PACKAGE_RED, + getUidForPackage(PACKAGE_RED)); + verifyPendingRecords(redQueue, List.of(userPresent, timeTick)); + } + private Intent createPackageChangedIntent(int uid, List componentNameList) { final Intent packageChangedIntent = new Intent(Intent.ACTION_PACKAGE_CHANGED); packageChangedIntent.putExtra(Intent.EXTRA_UID, uid);