Merge "Ignore re-enqueued bcast delivery failure to avoid repeated attempts." into main
diff --git a/services/core/java/com/android/server/am/BroadcastProcessQueue.java b/services/core/java/com/android/server/am/BroadcastProcessQueue.java
index c37e619..d1c8c30 100644
--- a/services/core/java/com/android/server/am/BroadcastProcessQueue.java
+++ b/services/core/java/com/android/server/am/BroadcastProcessQueue.java
@@ -152,6 +152,11 @@
private int mActiveIndex;
/**
+ * True if the broadcast actively being dispatched to this process was re-enqueued previously.
+ */
+ private boolean mActiveReEnqueued;
+
+ /**
* Count of {@link #mActive} broadcasts that have been dispatched since this
* queue was last idle.
*/
@@ -312,6 +317,7 @@
final SomeArgs broadcastArgs = SomeArgs.obtain();
broadcastArgs.arg1 = record;
broadcastArgs.argi1 = recordIndex;
+ broadcastArgs.argi2 = 1;
getQueueForBroadcast(record).addFirst(broadcastArgs);
onBroadcastEnqueued(record, recordIndex);
}
@@ -609,6 +615,7 @@
final SomeArgs next = removeNextBroadcast();
mActive = (BroadcastRecord) next.arg1;
mActiveIndex = next.argi1;
+ mActiveReEnqueued = (next.argi2 == 1);
mActiveCountSinceIdle++;
mActiveAssumedDeliveryCountSinceIdle +=
(mActive.isAssumedDelivered(mActiveIndex) ? 1 : 0);
@@ -624,12 +631,21 @@
public void makeActiveIdle() {
mActive = null;
mActiveIndex = 0;
+ mActiveReEnqueued = false;
mActiveCountSinceIdle = 0;
mActiveAssumedDeliveryCountSinceIdle = 0;
mActiveViaColdStart = false;
invalidateRunnableAt();
}
+ public boolean wasActiveBroadcastReEnqueued() {
+ // If the flag is not enabled, treat as if the broadcast was never re-enqueued.
+ if (!Flags.avoidRepeatedBcastReEnqueues()) {
+ return false;
+ }
+ return mActiveReEnqueued;
+ }
+
/**
* Update summary statistics when the given record has been enqueued.
*/
@@ -1476,6 +1492,9 @@
if (runningOomAdjusted) {
pw.print("runningOomAdjusted:"); pw.println(runningOomAdjusted);
}
+ if (mActiveReEnqueued) {
+ pw.print("activeReEnqueued:"); pw.println(mActiveReEnqueued);
+ }
}
@NeverCompile
diff --git a/services/core/java/com/android/server/am/BroadcastQueueModernImpl.java b/services/core/java/com/android/server/am/BroadcastQueueModernImpl.java
index db0f03f..98263df 100644
--- a/services/core/java/com/android/server/am/BroadcastQueueModernImpl.java
+++ b/services/core/java/com/android/server/am/BroadcastQueueModernImpl.java
@@ -542,8 +542,8 @@
updateOomAdj |= queue.runningOomAdjusted;
try {
completed = scheduleReceiverWarmLocked(queue);
- } catch (BroadcastDeliveryFailedException e) {
- reEnqueueActiveBroadcast(queue);
+ } catch (BroadcastRetryException e) {
+ finishOrReEnqueueActiveBroadcast(queue);
completed = true;
}
} else {
@@ -586,7 +586,12 @@
private void clearInvalidPendingColdStart() {
logw("Clearing invalid pending cold start: " + mRunningColdStart);
- mRunningColdStart.reEnqueueActiveBroadcast();
+ if (mRunningColdStart.wasActiveBroadcastReEnqueued()) {
+ finishReceiverActiveLocked(mRunningColdStart, BroadcastRecord.DELIVERY_FAILURE,
+ "invalid start with re-enqueued broadcast");
+ } else {
+ mRunningColdStart.reEnqueueActiveBroadcast();
+ }
demoteFromRunningLocked(mRunningColdStart);
clearRunningColdStart();
enqueueUpdateRunningList();
@@ -613,19 +618,26 @@
}
}
- private void reEnqueueActiveBroadcast(@NonNull BroadcastProcessQueue queue) {
+ private void finishOrReEnqueueActiveBroadcast(@NonNull BroadcastProcessQueue queue) {
checkState(queue.isActive(), "isActive");
- final BroadcastRecord record = queue.getActive();
- final int index = queue.getActiveIndex();
- setDeliveryState(queue, queue.app, record, index, record.receivers.get(index),
- BroadcastRecord.DELIVERY_PENDING, "reEnqueueActiveBroadcast");
- queue.reEnqueueActiveBroadcast();
+ if (queue.wasActiveBroadcastReEnqueued()) {
+ // If the broadcast was already re-enqueued previously, finish it to avoid repeated
+ // delivery attempts
+ finishReceiverActiveLocked(queue, BroadcastRecord.DELIVERY_FAILURE,
+ "re-enqueued broadcast delivery failed");
+ } else {
+ final BroadcastRecord record = queue.getActive();
+ final int index = queue.getActiveIndex();
+ setDeliveryState(queue, queue.app, record, index, record.receivers.get(index),
+ BroadcastRecord.DELIVERY_PENDING, "reEnqueueActiveBroadcast");
+ queue.reEnqueueActiveBroadcast();
+ }
}
@Override
public boolean onApplicationAttachedLocked(@NonNull ProcessRecord app)
- throws BroadcastDeliveryFailedException {
+ throws BroadcastRetryException {
if (DEBUG_BROADCAST) {
logv("Process " + app + " is attached");
}
@@ -653,8 +665,8 @@
if (scheduleReceiverWarmLocked(queue)) {
demoteFromRunningLocked(queue);
}
- } catch (BroadcastDeliveryFailedException e) {
- reEnqueueActiveBroadcast(queue);
+ } catch (BroadcastRetryException e) {
+ finishOrReEnqueueActiveBroadcast(queue);
demoteFromRunningLocked(queue);
throw e;
}
@@ -983,7 +995,7 @@
@CheckResult
@GuardedBy("mService")
private boolean scheduleReceiverWarmLocked(@NonNull BroadcastProcessQueue queue)
- throws BroadcastDeliveryFailedException {
+ throws BroadcastRetryException {
checkState(queue.isActive(), "isActive");
final int cookie = traceBegin("scheduleReceiverWarmLocked");
@@ -1065,7 +1077,7 @@
*/
@CheckResult
private boolean dispatchReceivers(@NonNull BroadcastProcessQueue queue,
- @NonNull BroadcastRecord r, int index) throws BroadcastDeliveryFailedException {
+ @NonNull BroadcastRecord r, int index) throws BroadcastRetryException {
final ProcessRecord app = queue.app;
final Object receiver = r.receivers.get(index);
@@ -1157,7 +1169,7 @@
// to try redelivering the broadcast to this receiver.
if (receiver instanceof ResolveInfo) {
cancelDeliveryTimeoutLocked(queue);
- throw new BroadcastDeliveryFailedException(e);
+ throw new BroadcastRetryException(e);
}
finishReceiverActiveLocked(queue, BroadcastRecord.DELIVERY_FAILURE,
"remote app");
@@ -1316,8 +1328,8 @@
demoteFromRunningLocked(queue);
return true;
}
- } catch (BroadcastDeliveryFailedException e) {
- reEnqueueActiveBroadcast(queue);
+ } catch (BroadcastRetryException e) {
+ finishOrReEnqueueActiveBroadcast(queue);
demoteFromRunningLocked(queue);
return true;
}
diff --git a/services/core/java/com/android/server/am/BroadcastRetryException.java b/services/core/java/com/android/server/am/BroadcastRetryException.java
new file mode 100644
index 0000000..8bd6664
--- /dev/null
+++ b/services/core/java/com/android/server/am/BroadcastRetryException.java
@@ -0,0 +1,30 @@
+/*
+ * Copyright (C) 2024 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.android.server.am;
+
+/**
+ * Exception to represent that broadcast delivery failed and we should try redelivering it.
+ */
+public class BroadcastRetryException extends BroadcastDeliveryFailedException {
+ public BroadcastRetryException(String name) {
+ super(name);
+ }
+
+ public BroadcastRetryException(Exception cause) {
+ super(cause);
+ }
+}
diff --git a/services/core/java/com/android/server/am/flags.aconfig b/services/core/java/com/android/server/am/flags.aconfig
index 31d9cc9..16dbe18 100644
--- a/services/core/java/com/android/server/am/flags.aconfig
+++ b/services/core/java/com/android/server/am/flags.aconfig
@@ -42,3 +42,14 @@
description: "Optimize the service bindings by different policies like skipping oom adjuster"
bug: "318717054"
}
+
+flag {
+ namespace: "backstage_power"
+ name: "avoid_repeated_bcast_re_enqueues"
+ description: "Avoid re-enqueueing a broadcast repeatedly"
+ bug: "319225224"
+ is_fixed_read_only: true
+ metadata {
+ purpose: PURPOSE_BUGFIX
+ }
+}
diff --git a/services/tests/mockingservicestests/Android.bp b/services/tests/mockingservicestests/Android.bp
index d928306..9f97551 100644
--- a/services/tests/mockingservicestests/Android.bp
+++ b/services/tests/mockingservicestests/Android.bp
@@ -73,6 +73,7 @@
"testng",
"compatibility-device-util-axt",
"flag-junit",
+ "am_flags_lib",
],
libs: [
diff --git a/services/tests/mockingservicestests/src/com/android/server/am/BaseBroadcastQueueTest.java b/services/tests/mockingservicestests/src/com/android/server/am/BaseBroadcastQueueTest.java
index f875f65..fb47aa8 100644
--- a/services/tests/mockingservicestests/src/com/android/server/am/BaseBroadcastQueueTest.java
+++ b/services/tests/mockingservicestests/src/com/android/server/am/BaseBroadcastQueueTest.java
@@ -38,6 +38,8 @@
import android.os.HandlerThread;
import android.os.TestLooperManager;
import android.os.UserHandle;
+import android.platform.test.flag.junit.CheckFlagsRule;
+import android.platform.test.flag.junit.DeviceFlagsValueProvider;
import android.provider.Settings;
import android.util.SparseArray;
@@ -93,6 +95,9 @@
.spyStatic(ProcessList.class)
.build();
+ @Rule
+ public final CheckFlagsRule mCheckFlagsRule = DeviceFlagsValueProvider.createCheckFlagsRule();
+
final BroadcastQueue[] mBroadcastQueues = new BroadcastQueue[1];
@Mock
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 75409d9..3f6117b 100644
--- a/services/tests/mockingservicestests/src/com/android/server/am/BroadcastQueueTest.java
+++ b/services/tests/mockingservicestests/src/com/android/server/am/BroadcastQueueTest.java
@@ -75,6 +75,8 @@
import android.os.PowerExemptionManager;
import android.os.SystemClock;
import android.os.UserHandle;
+import android.platform.test.annotations.RequiresFlagsEnabled;
+import android.util.ArrayMap;
import android.util.Log;
import android.util.Pair;
import android.util.proto.ProtoOutputStream;
@@ -135,6 +137,17 @@
ProcessStartBehavior.SUCCESS);
/**
+ * Map of processes to behaviors indicating how the new processes should behave as needed
+ * by the tests.
+ */
+ private ArrayMap<String, ProcessBehavior> mNewProcessBehaviors = new ArrayMap<>();
+
+ /**
+ * Map of processes to behaviors indicating how the new process starts should result in.
+ */
+ private ArrayMap<String, ProcessStartBehavior> mNewProcessStartBehaviors = new ArrayMap<>();
+
+ /**
* Collection of all active processes during current test run.
*/
private List<ProcessRecord> mActiveProcesses = new ArrayList<>();
@@ -161,15 +174,17 @@
Log.v(TAG, "Intercepting startProcessLocked() for "
+ Arrays.toString(invocation.getArguments()));
assertHealth();
- final ProcessStartBehavior behavior = mNextProcessStartBehavior
- .getAndSet(ProcessStartBehavior.SUCCESS);
+ final String processName = invocation.getArgument(0);
+ final ProcessStartBehavior behavior = mNewProcessStartBehaviors.getOrDefault(
+ processName, mNextProcessStartBehavior.getAndSet(ProcessStartBehavior.SUCCESS));
if (behavior == ProcessStartBehavior.FAIL_NULL) {
return null;
}
- final String processName = invocation.getArgument(0);
final ApplicationInfo ai = invocation.getArgument(1);
+ final ProcessBehavior processBehavior = mNewProcessBehaviors.getOrDefault(
+ processName, ProcessBehavior.NORMAL);
final ProcessRecord res = makeActiveProcessRecord(ai, processName,
- ProcessBehavior.NORMAL, UnaryOperator.identity());
+ processBehavior, UnaryOperator.identity());
final ProcessRecord deliverRes;
switch (behavior) {
case SUCCESS_PREDECESSOR:
@@ -274,6 +289,8 @@
assertEquals(app.toShortString(), ProcessList.SCHED_GROUP_UNDEFINED,
mQueue.getPreferredSchedulingGroupLocked(app));
}
+ mNewProcessBehaviors.clear();
+ mNewProcessStartBehaviors.clear();
}
@Override
@@ -955,6 +972,40 @@
}
/**
+ * Verify that we handle manifest receivers in a process that always
+ * responds with {@link DeadObjectException} even after restarting.
+ */
+ @Test
+ @RequiresFlagsEnabled(Flags.FLAG_AVOID_REPEATED_BCAST_RE_ENQUEUES)
+ public void testRepeatedDead_Manifest() throws Exception {
+ final ProcessRecord callerApp = makeActiveProcessRecord(PACKAGE_RED);
+ mNewProcessBehaviors.put(PACKAGE_GREEN, ProcessBehavior.DEAD);
+
+ final Intent airplane = new Intent(Intent.ACTION_AIRPLANE_MODE_CHANGED);
+ enqueueBroadcast(makeBroadcastRecord(airplane, callerApp, List.of(
+ withPriority(makeManifestReceiver(PACKAGE_GREEN, CLASS_GREEN), 10),
+ withPriority(makeManifestReceiver(PACKAGE_BLUE, CLASS_BLUE), 0))));
+ final Intent timezone = new Intent(Intent.ACTION_TIMEZONE_CHANGED);
+ enqueueBroadcast(makeBroadcastRecord(timezone, callerApp,
+ List.of(makeManifestReceiver(PACKAGE_YELLOW, CLASS_YELLOW))));
+ waitForIdle();
+
+ final ProcessRecord receiverGreenApp = mAms.getProcessRecordLocked(PACKAGE_GREEN,
+ getUidForPackage(PACKAGE_GREEN));
+ // Modern queue always kills the target process when broadcast delivery fails, where as
+ // the legacy queue leaves the process killing task to AMS
+ if (mImpl == Impl.MODERN) {
+ assertNull(receiverGreenApp);
+ }
+ final ProcessRecord receiverBlueApp = mAms.getProcessRecordLocked(PACKAGE_BLUE,
+ getUidForPackage(PACKAGE_BLUE));
+ verifyScheduleReceiver(receiverBlueApp, airplane);
+ final ProcessRecord receiverYellowApp = mAms.getProcessRecordLocked(PACKAGE_YELLOW,
+ getUidForPackage(PACKAGE_YELLOW));
+ verifyScheduleReceiver(receiverYellowApp, timezone);
+ }
+
+ /**
* Verify that we handle the system failing to start a process.
*/
@Test
@@ -1142,6 +1193,49 @@
}
/**
+ * Verify that when BroadcastQueue doesn't get notified when a process gets killed repeatedly,
+ * it doesn't get stuck.
+ */
+ @Test
+ @RequiresFlagsEnabled(Flags.FLAG_AVOID_REPEATED_BCAST_RE_ENQUEUES)
+ public void testRepeatedKillWithoutNotify() throws Exception {
+ // Legacy queue does not handle repeated kills that don't get notified.
+ Assume.assumeTrue(mImpl == Impl.MODERN);
+
+ final ProcessRecord callerApp = makeActiveProcessRecord(PACKAGE_RED);
+ final ProcessRecord receiverBlueApp = makeActiveProcessRecord(PACKAGE_BLUE);
+
+ mNewProcessStartBehaviors.put(PACKAGE_GREEN, ProcessStartBehavior.KILLED_WITHOUT_NOTIFY);
+
+ final Intent airplane = new Intent(Intent.ACTION_AIRPLANE_MODE_CHANGED);
+ enqueueBroadcast(makeBroadcastRecord(airplane, callerApp, List.of(
+ withPriority(makeManifestReceiver(PACKAGE_GREEN, CLASS_GREEN), 10),
+ withPriority(makeRegisteredReceiver(receiverBlueApp), 5),
+ withPriority(makeManifestReceiver(PACKAGE_YELLOW, CLASS_YELLOW), 0))));
+
+ final Intent timezone = new Intent(Intent.ACTION_TIMEZONE_CHANGED);
+ enqueueBroadcast(makeBroadcastRecord(timezone, callerApp,
+ List.of(makeManifestReceiver(PACKAGE_ORANGE, CLASS_ORANGE))));
+
+ waitForIdle();
+ final ProcessRecord receiverGreenApp = mAms.getProcessRecordLocked(PACKAGE_GREEN,
+ getUidForPackage(PACKAGE_GREEN));
+ final ProcessRecord receiverYellowApp = mAms.getProcessRecordLocked(PACKAGE_YELLOW,
+ getUidForPackage(PACKAGE_YELLOW));
+ final ProcessRecord receiverOrangeApp = mAms.getProcessRecordLocked(PACKAGE_ORANGE,
+ getUidForPackage(PACKAGE_ORANGE));
+
+ // Modern queue always kills the target process when broadcast delivery fails, where as
+ // the legacy queue leaves the process killing task to AMS
+ if (mImpl == Impl.MODERN) {
+ assertNull(receiverGreenApp);
+ }
+ verifyScheduleRegisteredReceiver(times(1), receiverBlueApp, airplane);
+ verifyScheduleReceiver(times(1), receiverYellowApp, airplane);
+ verifyScheduleReceiver(times(1), receiverOrangeApp, timezone);
+ }
+
+ /**
* Verify that a broadcast sent to a frozen app, which gets killed as part of unfreezing
* process due to pending sync binder transactions, is delivered as expected.
*/