From 67080b5caf4db39ecdefc9b92267dec0f20ed13d Mon Sep 17 00:00:00 2001 From: Jeff Sharkey Date: Thu, 20 Oct 2022 10:14:55 -0600 Subject: [PATCH 1/2] BroadcastQueue: misc fixes before dogfooding. Many test suites rely on broadcasts being processed quickly, and there's a wide spectrum of use-cases, where the sender and/or the receiver are being actively "instrumented". This change adds logic to make sure that no delays are applied in these cases. Also add a counter for the recently added "interactive" flag, since that's a signal that no delays should be applied. Update getPreferredSchedulingGroupLocked() to match the logic we use for selecting an ANR timeout duration, by only using the "default" scheduling group for foreground broadcasts. (Future changes can expand this to "interactive" broadcasts if desired.) Offer to deliver "resultTo" for non-ordered broadcasts, which will be needed as part of upcoming BOOT and SCREEN changes. Similar to above, this is yet another case where no delays should be applied, to ensure the "resultTo" doesn't get stuck waiting hours behind a frozen or cached app. Update KEY_DELAY constants to not use HW_TIMEOUT_MULTIPLIER, since these are delays, not timeouts. (Applying the multiplier causes tests failures due to delaying too long on cloud devices.) Fix obscure bug where callers were able register receivers without providing an IApplicationThread; we need that to find the relevant ProcessRecord for things like OOM adjustment, unfreezing, and more. Bug: 253906105 Test: atest FrameworksMockingServicesTests:BroadcastRecordTest Test: atest FrameworksMockingServicesTests:BroadcastQueueTest Test: atest FrameworksMockingServicesTests:BroadcastQueueModernImplTest Test: atest Launcher3Tests:AddConfigWidgetTest Test: atest CtsWindowManagerDeviceTestCases:ActivityVisibilityTests Test: atest CtsActivityManagerBackgroundActivityTestCases:BackgroundActivityLaunchTest Change-Id: I3424da03655457f58d66df761f63fd37ea2c9391 --- .../server/am/ActivityManagerService.java | 32 ++--- .../android/server/am/BroadcastConstants.java | 4 +- .../server/am/BroadcastProcessQueue.java | 111 ++++++++++++++---- .../server/am/BroadcastQueueModernImpl.java | 35 +++--- .../android/server/am/BroadcastRecord.java | 4 + .../android/server/am/BroadcastQueueTest.java | 44 +++++-- 6 files changed, 162 insertions(+), 68 deletions(-) diff --git a/services/core/java/com/android/server/am/ActivityManagerService.java b/services/core/java/com/android/server/am/ActivityManagerService.java index 7e2e932f04ef3..63f81822e8670 100644 --- a/services/core/java/com/android/server/am/ActivityManagerService.java +++ b/services/core/java/com/android/server/am/ActivityManagerService.java @@ -13373,27 +13373,19 @@ public class ActivityManagerService extends IActivityManager.Stub int callingPid; boolean instantApp; synchronized(this) { - if (caller != null) { - callerApp = getRecordForAppLOSP(caller); - if (callerApp == null) { - throw new SecurityException( - "Unable to find app for caller " + caller - + " (pid=" + Binder.getCallingPid() - + ") when registering receiver " + receiver); - } - if (callerApp.info.uid != SYSTEM_UID - && !callerApp.getPkgList().containsKey(callerPackage) - && !"android".equals(callerPackage)) { - throw new SecurityException("Given caller package " + callerPackage - + " is not running in process " + callerApp); - } - callingUid = callerApp.info.uid; - callingPid = callerApp.getPid(); - } else { - callerPackage = null; - callingUid = Binder.getCallingUid(); - callingPid = Binder.getCallingPid(); + callerApp = getRecordForAppLOSP(caller); + if (callerApp == null) { + Slog.w(TAG, "registerReceiverWithFeature: no app for " + caller); + return null; } + if (callerApp.info.uid != SYSTEM_UID + && !callerApp.getPkgList().containsKey(callerPackage) + && !"android".equals(callerPackage)) { + throw new SecurityException("Given caller package " + callerPackage + + " is not running in process " + callerApp); + } + callingUid = callerApp.info.uid; + callingPid = callerApp.getPid(); instantApp = isInstantApp(callerApp, callerPackage, callingUid); userId = mUserController.handleIncomingUser(callingPid, callingUid, userId, true, diff --git a/services/core/java/com/android/server/am/BroadcastConstants.java b/services/core/java/com/android/server/am/BroadcastConstants.java index a4a1c2f0d87ce..28a81e6ee1454 100644 --- a/services/core/java/com/android/server/am/BroadcastConstants.java +++ b/services/core/java/com/android/server/am/BroadcastConstants.java @@ -167,7 +167,7 @@ public class BroadcastConstants { */ public long DELAY_NORMAL_MILLIS = DEFAULT_DELAY_NORMAL_MILLIS; private static final String KEY_DELAY_NORMAL_MILLIS = "bcast_delay_normal_millis"; - private static final long DEFAULT_DELAY_NORMAL_MILLIS = 10_000 * Build.HW_TIMEOUT_MULTIPLIER; + private static final long DEFAULT_DELAY_NORMAL_MILLIS = 1_000; /** * For {@link BroadcastQueueModernImpl}: Delay to apply to broadcasts @@ -175,7 +175,7 @@ public class BroadcastConstants { */ public long DELAY_CACHED_MILLIS = DEFAULT_DELAY_CACHED_MILLIS; private static final String KEY_DELAY_CACHED_MILLIS = "bcast_delay_cached_millis"; - private static final long DEFAULT_DELAY_CACHED_MILLIS = 30_000 * Build.HW_TIMEOUT_MULTIPLIER; + private static final long DEFAULT_DELAY_CACHED_MILLIS = 10_000; /** * For {@link BroadcastQueueModernImpl}: Maximum number of complete diff --git a/services/core/java/com/android/server/am/BroadcastProcessQueue.java b/services/core/java/com/android/server/am/BroadcastProcessQueue.java index b57e3f9ed837a..868c3ae6da50e 100644 --- a/services/core/java/com/android/server/am/BroadcastProcessQueue.java +++ b/services/core/java/com/android/server/am/BroadcastProcessQueue.java @@ -147,12 +147,16 @@ class BroadcastProcessQueue { private int mCountOrdered; private int mCountAlarm; private int mCountPrioritized; + private int mCountInteractive; + private int mCountResultTo; + private int mCountInstrumented; private @UptimeMillisLong long mRunnableAt = Long.MAX_VALUE; private @Reason int mRunnableAtReason = REASON_EMPTY; private boolean mRunnableAtInvalidated; private boolean mProcessCached; + private boolean mProcessInstrumented; private String mCachedToString; private String mCachedToShortString; @@ -296,6 +300,18 @@ class BroadcastProcessQueue { return didSomething; } + /** + * Update the actively running "warm" process for this process. + */ + public void setProcess(@Nullable ProcessRecord app) { + this.app = app; + if (app != null) { + setProcessInstrumented(app.getActiveInstrumentation() != null); + } else { + setProcessInstrumented(false); + } + } + /** * Update if this process is in the "cached" state, typically signaling that * broadcast dispatch should be paused or delayed. @@ -307,6 +323,18 @@ class BroadcastProcessQueue { } } + /** + * Update if this process is in the "instrumented" state, typically + * signaling that broadcast dispatch should bypass all pauses or delays, to + * avoid holding up test suites. + */ + public void setProcessInstrumented(boolean instrumented) { + if (mProcessInstrumented != instrumented) { + mProcessInstrumented = instrumented; + invalidateRunnableAt(); + } + } + /** * Return if we know of an actively running "warm" process for this queue. */ @@ -315,13 +343,12 @@ class BroadcastProcessQueue { } public int getPreferredSchedulingGroupLocked() { - if (mCountForeground > 0 || mCountOrdered > 0 || mCountAlarm > 0) { - // We have an important broadcast somewhere down the queue, so + if (mCountForeground > 0) { + // We have a foreground broadcast somewhere down the queue, so // boost priority until we drain them all return ProcessList.SCHED_GROUP_DEFAULT; - } else if ((mActive != null) - && (mActive.isForeground() || mActive.ordered || mActive.alarm)) { - // We have an important broadcast right now, so boost priority + } else if ((mActive != null) && mActive.isForeground()) { + // We have a foreground broadcast right now, so boost priority return ProcessList.SCHED_GROUP_DEFAULT; } else if (!isIdle()) { return ProcessList.SCHED_GROUP_BACKGROUND; @@ -389,6 +416,15 @@ class BroadcastProcessQueue { if (record.prioritized) { mCountPrioritized++; } + if (record.interactive) { + mCountInteractive++; + } + if (record.resultTo != null) { + mCountResultTo++; + } + if (record.callerInstrumented) { + mCountInstrumented++; + } invalidateRunnableAt(); } @@ -408,6 +444,15 @@ class BroadcastProcessQueue { if (record.prioritized) { mCountPrioritized--; } + if (record.interactive) { + mCountInteractive--; + } + if (record.resultTo != null) { + mCountResultTo--; + } + if (record.callerInstrumented) { + mCountInstrumented--; + } invalidateRunnableAt(); } @@ -553,25 +598,33 @@ class BroadcastProcessQueue { } static final int REASON_EMPTY = 0; - static final int REASON_CONTAINS_FOREGROUND = 1; - static final int REASON_CONTAINS_ORDERED = 2; - static final int REASON_CONTAINS_ALARM = 3; - static final int REASON_CONTAINS_PRIORITIZED = 4; - static final int REASON_CACHED = 5; - static final int REASON_NORMAL = 6; - static final int REASON_MAX_PENDING = 7; - static final int REASON_BLOCKED = 8; + static final int REASON_CACHED = 1; + static final int REASON_NORMAL = 2; + static final int REASON_MAX_PENDING = 3; + static final int REASON_BLOCKED = 4; + static final int REASON_INSTRUMENTED = 5; + static final int REASON_CONTAINS_FOREGROUND = 10; + static final int REASON_CONTAINS_ORDERED = 11; + static final int REASON_CONTAINS_ALARM = 12; + static final int REASON_CONTAINS_PRIORITIZED = 13; + static final int REASON_CONTAINS_INTERACTIVE = 14; + static final int REASON_CONTAINS_RESULT_TO = 15; + static final int REASON_CONTAINS_INSTRUMENTED = 16; @IntDef(flag = false, prefix = { "REASON_" }, value = { REASON_EMPTY, - REASON_CONTAINS_FOREGROUND, - REASON_CONTAINS_ORDERED, - REASON_CONTAINS_ALARM, - REASON_CONTAINS_PRIORITIZED, REASON_CACHED, REASON_NORMAL, REASON_MAX_PENDING, REASON_BLOCKED, + REASON_INSTRUMENTED, + REASON_CONTAINS_FOREGROUND, + REASON_CONTAINS_ORDERED, + REASON_CONTAINS_ALARM, + REASON_CONTAINS_PRIORITIZED, + REASON_CONTAINS_INTERACTIVE, + REASON_CONTAINS_RESULT_TO, + REASON_CONTAINS_INSTRUMENTED, }) @Retention(RetentionPolicy.SOURCE) public @interface Reason {} @@ -579,14 +632,18 @@ class BroadcastProcessQueue { static @NonNull String reasonToString(@Reason int reason) { switch (reason) { case REASON_EMPTY: return "EMPTY"; - case REASON_CONTAINS_FOREGROUND: return "CONTAINS_FOREGROUND"; - case REASON_CONTAINS_ORDERED: return "CONTAINS_ORDERED"; - case REASON_CONTAINS_ALARM: return "CONTAINS_ALARM"; - case REASON_CONTAINS_PRIORITIZED: return "CONTAINS_PRIORITIZED"; case REASON_CACHED: return "CACHED"; case REASON_NORMAL: return "NORMAL"; case REASON_MAX_PENDING: return "MAX_PENDING"; case REASON_BLOCKED: return "BLOCKED"; + case REASON_INSTRUMENTED: return "INSTRUMENTED"; + case REASON_CONTAINS_FOREGROUND: return "CONTAINS_FOREGROUND"; + case REASON_CONTAINS_ORDERED: return "CONTAINS_ORDERED"; + case REASON_CONTAINS_ALARM: return "CONTAINS_ALARM"; + case REASON_CONTAINS_PRIORITIZED: return "CONTAINS_PRIORITIZED"; + case REASON_CONTAINS_INTERACTIVE: return "CONTAINS_INTERACTIVE"; + case REASON_CONTAINS_RESULT_TO: return "CONTAINS_RESULT_TO"; + case REASON_CONTAINS_INSTRUMENTED: return "CONTAINS_INSTRUMENTED"; default: return Integer.toString(reason); } } @@ -631,6 +688,18 @@ class BroadcastProcessQueue { } else if (mCountPrioritized > 0) { mRunnableAt = runnableAt; mRunnableAtReason = REASON_CONTAINS_PRIORITIZED; + } else if (mCountInteractive > 0) { + mRunnableAt = runnableAt; + mRunnableAtReason = REASON_CONTAINS_INTERACTIVE; + } else if (mCountResultTo > 0) { + mRunnableAt = runnableAt; + mRunnableAtReason = REASON_CONTAINS_RESULT_TO; + } else if (mCountInstrumented > 0) { + mRunnableAt = runnableAt; + mRunnableAtReason = REASON_CONTAINS_INSTRUMENTED; + } else if (mProcessInstrumented) { + mRunnableAt = runnableAt; + mRunnableAtReason = REASON_INSTRUMENTED; } else if (mProcessCached) { mRunnableAt = runnableAt + constants.DELAY_CACHED_MILLIS; mRunnableAtReason = REASON_CACHED; diff --git a/services/core/java/com/android/server/am/BroadcastQueueModernImpl.java b/services/core/java/com/android/server/am/BroadcastQueueModernImpl.java index e421c61a2bd6b..db3ef3d51b167 100644 --- a/services/core/java/com/android/server/am/BroadcastQueueModernImpl.java +++ b/services/core/java/com/android/server/am/BroadcastQueueModernImpl.java @@ -441,7 +441,7 @@ class BroadcastQueueModernImpl extends BroadcastQueue { // relevant per-process queue final BroadcastProcessQueue queue = getProcessQueue(app); if (queue != null) { - queue.app = app; + queue.setProcess(app); } boolean didSomething = false; @@ -478,7 +478,7 @@ class BroadcastQueueModernImpl extends BroadcastQueue { // relevant per-process queue final BroadcastProcessQueue queue = getProcessQueue(app); if (queue != null) { - queue.app = null; + queue.setProcess(null); } if ((mRunningColdStart != null) && (mRunningColdStart == queue)) { @@ -816,19 +816,21 @@ class BroadcastQueueModernImpl extends BroadcastQueue { } final BroadcastRecord r = queue.getActive(); - r.resultCode = resultCode; - r.resultData = resultData; - r.resultExtras = resultExtras; - if (!r.isNoAbort()) { - r.resultAbort = resultAbort; - } + if (r.ordered) { + r.resultCode = resultCode; + r.resultData = resultData; + r.resultExtras = resultExtras; + if (!r.isNoAbort()) { + r.resultAbort = resultAbort; + } - // When the caller aborted an ordered broadcast, we mark all remaining - // receivers as skipped - if (r.ordered && r.resultAbort) { - for (int i = r.terminalCount + 1; i < r.receivers.size(); i++) { - setDeliveryState(null, null, r, i, r.receivers.get(i), - BroadcastRecord.DELIVERY_SKIPPED); + // When the caller aborted an ordered broadcast, we mark all + // remaining receivers as skipped + if (r.resultAbort) { + for (int i = r.terminalCount + 1; i < r.receivers.size(); i++) { + setDeliveryState(null, null, r, i, r.receivers.get(i), + BroadcastRecord.DELIVERY_SKIPPED); + } } } @@ -925,7 +927,8 @@ class BroadcastQueueModernImpl extends BroadcastQueue { notifyFinishReceiver(queue, r, index, receiver); // When entire ordered broadcast finished, deliver final result - if (r.ordered && (r.terminalCount == r.receivers.size())) { + final boolean recordFinished = (r.terminalCount == r.receivers.size()); + if (recordFinished) { scheduleResultTo(r); } @@ -1217,7 +1220,7 @@ class BroadcastQueueModernImpl extends BroadcastQueue { private void updateWarmProcess(@NonNull BroadcastProcessQueue queue) { if (!queue.isProcessWarm()) { - queue.app = mService.getProcessRecordLocked(queue.processName, queue.uid); + queue.setProcess(mService.getProcessRecordLocked(queue.processName, queue.uid)); } } diff --git a/services/core/java/com/android/server/am/BroadcastRecord.java b/services/core/java/com/android/server/am/BroadcastRecord.java index 87c87e296538b..d7dc8b80931b9 100644 --- a/services/core/java/com/android/server/am/BroadcastRecord.java +++ b/services/core/java/com/android/server/am/BroadcastRecord.java @@ -78,6 +78,7 @@ final class BroadcastRecord extends Binder { final int callingPid; // the pid of who sent this final int callingUid; // the uid of who sent this final boolean callerInstantApp; // caller is an Instant App? + final boolean callerInstrumented; // caller is being instrumented final boolean ordered; // serialize the send to receivers? final boolean sticky; // originated from existing sticky data? final boolean alarm; // originated from an alarm triggering? @@ -365,6 +366,8 @@ final class BroadcastRecord extends Binder { callingPid = _callingPid; callingUid = _callingUid; callerInstantApp = _callerInstantApp; + callerInstrumented = (_callerApp != null) + ? (_callerApp.getActiveInstrumentation() != null) : false; resolvedType = _resolvedType; requiredPermissions = _requiredPermissions; excludedPermissions = _excludedPermissions; @@ -411,6 +414,7 @@ final class BroadcastRecord extends Binder { callingPid = from.callingPid; callingUid = from.callingUid; callerInstantApp = from.callerInstantApp; + callerInstrumented = from.callerInstrumented; ordered = from.ordered; sticky = from.sticky; initialSticky = from.initialSticky; 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 c125448979418..d9a26c68f3edf 100644 --- a/services/tests/mockingservicestests/src/com/android/server/am/BroadcastQueueTest.java +++ b/services/tests/mockingservicestests/src/com/android/server/am/BroadcastQueueTest.java @@ -549,25 +549,31 @@ public class BroadcastQueueTest { receivers, false, null, null, userId); } - private BroadcastRecord makeOrderedBroadcastRecord(Intent intent, ProcessRecord callerApp, - List receivers, IIntentReceiver orderedResultTo, Bundle orderedExtras) { - return makeBroadcastRecord(intent, callerApp, BroadcastOptions.makeBasic(), - receivers, true, orderedResultTo, orderedExtras, UserHandle.USER_SYSTEM); - } - private BroadcastRecord makeBroadcastRecord(Intent intent, ProcessRecord callerApp, BroadcastOptions options, List receivers) { return makeBroadcastRecord(intent, callerApp, options, receivers, false, null, null, UserHandle.USER_SYSTEM); } + private BroadcastRecord makeBroadcastRecord(Intent intent, ProcessRecord callerApp, + List receivers, IIntentReceiver resultTo) { + return makeBroadcastRecord(intent, callerApp, BroadcastOptions.makeBasic(), + receivers, false, resultTo, null, UserHandle.USER_SYSTEM); + } + + private BroadcastRecord makeOrderedBroadcastRecord(Intent intent, ProcessRecord callerApp, + List receivers, IIntentReceiver resultTo, Bundle resultExtras) { + return makeBroadcastRecord(intent, callerApp, BroadcastOptions.makeBasic(), + receivers, true, resultTo, resultExtras, UserHandle.USER_SYSTEM); + } + private BroadcastRecord makeBroadcastRecord(Intent intent, ProcessRecord callerApp, BroadcastOptions options, List receivers, boolean ordered, - IIntentReceiver orderedResultTo, Bundle orderedExtras, int userId) { + IIntentReceiver resultTo, Bundle resultExtras, int userId) { return new BroadcastRecord(mQueue, intent, callerApp, callerApp.info.packageName, null, callerApp.getPid(), callerApp.info.uid, false, null, null, null, null, - AppOpsManager.OP_NONE, options, receivers, callerApp, orderedResultTo, - Activity.RESULT_OK, null, orderedExtras, ordered, false, false, userId, false, null, + AppOpsManager.OP_NONE, options, receivers, callerApp, resultTo, + Activity.RESULT_OK, null, resultExtras, ordered, false, false, userId, false, null, false, null); } @@ -1346,6 +1352,26 @@ public class BroadcastQueueTest { anyBoolean(), eq(UserHandle.USER_SYSTEM), anyInt()); } + /** + * Verify that we deliver results for unordered broadcasts. + */ + @Test + public void testUnordered_ResultTo() throws Exception { + final ProcessRecord callerApp = makeActiveProcessRecord(PACKAGE_RED); + final IApplicationThread callerThread = callerApp.getThread(); + + final IIntentReceiver resultTo = mock(IIntentReceiver.class); + final Intent airplane = new Intent(Intent.ACTION_AIRPLANE_MODE_CHANGED); + enqueueBroadcast(makeBroadcastRecord(airplane, callerApp, + List.of(makeManifestReceiver(PACKAGE_GREEN, CLASS_GREEN), + makeManifestReceiver(PACKAGE_BLUE, CLASS_BLUE)), resultTo)); + + waitForIdle(); + verify(callerThread).scheduleRegisteredReceiver(any(), argThat(filterEquals(airplane)), + eq(Activity.RESULT_OK), any(), any(), eq(false), + anyBoolean(), eq(UserHandle.USER_SYSTEM), anyInt()); + } + /** * Verify that we're not surprised by a process attempting to finishing a * broadcast when none is in progress. From 093ace234fe3833d28d0e4ac91899d7d6a8b573f Mon Sep 17 00:00:00 2001 From: Jeff Sharkey Date: Wed, 19 Oct 2022 15:10:31 -0600 Subject: [PATCH 2/2] Enable "modern" BroadcastQueue by default. We've completed several weeks of teamfooding which has helped us gain confidence in the "modern" queue, so let's pivot it enabling it by default. Bug: 253906105 Test: TH Change-Id: Idcc7f7c930e917df37d97c5da3e8b1ffb115c9e1 --- .../core/java/com/android/server/am/BroadcastConstants.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/core/java/com/android/server/am/BroadcastConstants.java b/services/core/java/com/android/server/am/BroadcastConstants.java index 28a81e6ee1454..ebcd6239afd3c 100644 --- a/services/core/java/com/android/server/am/BroadcastConstants.java +++ b/services/core/java/com/android/server/am/BroadcastConstants.java @@ -133,7 +133,7 @@ public class BroadcastConstants { */ public boolean MODERN_QUEUE_ENABLED = DEFAULT_MODERN_QUEUE_ENABLED; private static final String KEY_MODERN_QUEUE_ENABLED = "modern_queue_enabled"; - private static final boolean DEFAULT_MODERN_QUEUE_ENABLED = false; + private static final boolean DEFAULT_MODERN_QUEUE_ENABLED = true; /** * For {@link BroadcastQueueModernImpl}: Maximum number of process queues to