From 5f31d431720afee05d10c77c618dd52e2cb8243b Mon Sep 17 00:00:00 2001 From: Jeff Sharkey Date: Mon, 26 Sep 2022 11:23:24 -0600 Subject: [PATCH 1/4] BroadcastQueue: tests for failing starts, dead. Add tests that verify the behavior of failing cold process starts and handling of DeadObjectException. This generally means we skip the active broadcast and keep making progress on our queue, instead of risking stalling out. This testing uncovered a subtle bug in the "default" implementation, where we'd clean up a dead registered receiver, but leave a dead manifest receiver floating. We fix this by giving both cases the same treatment. Add string caching to improve performance. Bug: 245771249 Test: atest FrameworksMockingServicesTests:BroadcastQueueTest Change-Id: Ic7ece84d6b0404d49b2fdf586f4574efdd4175fa --- .../server/am/BroadcastProcessQueue.java | 17 +- .../android/server/am/BroadcastQueueImpl.java | 2 + .../server/am/BroadcastQueueModernImpl.java | 22 ++- .../android/server/am/BroadcastRecord.java | 29 ++- .../android/server/am/BroadcastQueueTest.java | 176 ++++++++++++++++-- 5 files changed, 209 insertions(+), 37 deletions(-) diff --git a/services/core/java/com/android/server/am/BroadcastProcessQueue.java b/services/core/java/com/android/server/am/BroadcastProcessQueue.java index 77eefb4e27433..379b494a86fa4 100644 --- a/services/core/java/com/android/server/am/BroadcastProcessQueue.java +++ b/services/core/java/com/android/server/am/BroadcastProcessQueue.java @@ -119,6 +119,9 @@ class BroadcastProcessQueue { private boolean mProcessCached; + private String mCachedToString; + private String mCachedToShortString; + public BroadcastProcessQueue(@NonNull BroadcastConstants constants, @NonNull String processName, int uid) { this.constants = Objects.requireNonNull(constants); @@ -452,13 +455,19 @@ class BroadcastProcessQueue { @Override public String toString() { - return "BroadcastProcessQueue{" - + Integer.toHexString(System.identityHashCode(this)) - + " " + processName + "/" + UserHandle.formatUid(uid) + "}"; + if (mCachedToString == null) { + mCachedToString = "BroadcastProcessQueue{" + + Integer.toHexString(System.identityHashCode(this)) + + " " + processName + "/" + UserHandle.formatUid(uid) + "}"; + } + return mCachedToString; } public String toShortString() { - return processName + "/" + UserHandle.formatUid(uid); + if (mCachedToShortString == null) { + mCachedToShortString = processName + "/" + UserHandle.formatUid(uid); + } + return mCachedToShortString; } public void dumpLocked(@NonNull IndentingPrintWriter pw) { diff --git a/services/core/java/com/android/server/am/BroadcastQueueImpl.java b/services/core/java/com/android/server/am/BroadcastQueueImpl.java index 28bd9c367fff7..5848011a508dd 100644 --- a/services/core/java/com/android/server/am/BroadcastQueueImpl.java +++ b/services/core/java/com/android/server/am/BroadcastQueueImpl.java @@ -1381,6 +1381,8 @@ public class BroadcastQueueImpl extends BroadcastQueue { } catch (RemoteException e) { Slog.w(TAG, "Exception when sending broadcast to " + r.curComponent, e); + app.scheduleCrashLocked("can't deliver broadcast", + CannotDeliverBroadcastException.TYPE_ID, /* extras=*/ null); } catch (RuntimeException e) { Slog.wtf(TAG, "Failed sending broadcast to " + r.curComponent + " with " + r.intent, e); diff --git a/services/core/java/com/android/server/am/BroadcastQueueModernImpl.java b/services/core/java/com/android/server/am/BroadcastQueueModernImpl.java index a36a9f64bded8..b265c5758f7d7 100644 --- a/services/core/java/com/android/server/am/BroadcastQueueModernImpl.java +++ b/services/core/java/com/android/server/am/BroadcastQueueModernImpl.java @@ -604,7 +604,7 @@ class BroadcastQueueModernImpl extends BroadcastQueue { } if (DEBUG_BROADCAST) logv("Scheduling " + r + " to warm " + app); - setDeliveryState(queue, r, index, receiver, BroadcastRecord.DELIVERY_SCHEDULED); + setDeliveryState(queue, app, r, index, receiver, BroadcastRecord.DELIVERY_SCHEDULED); final IApplicationThread thread = app.getThread(); if (thread != null) { @@ -628,8 +628,11 @@ class BroadcastQueueModernImpl extends BroadcastQueue { app.mState.getReportedProcState()); } } catch (RemoteException e) { - finishReceiverLocked(queue, BroadcastRecord.DELIVERY_FAILURE); + Slog.w(TAG, "Failed to schedule " + r + " to " + receiver + + " via " + app + ": " + e); app.scheduleCrashLocked(TAG, CannotDeliverBroadcastException.TYPE_ID, null); + app.setKilled(true); + finishReceiverLocked(queue, BroadcastRecord.DELIVERY_FAILURE); } } else { finishReceiverLocked(queue, BroadcastRecord.DELIVERY_FAILURE); @@ -652,6 +655,7 @@ class BroadcastQueueModernImpl extends BroadcastQueue { r.resultCode, r.resultData, r.resultExtras, false, r.initialSticky, r.userId, app.mState.getReportedProcState()); } catch (RemoteException e) { + Slog.w(TAG, "Failed to schedule result of " + r + " via " + app + ": " + e); app.scheduleCrashLocked(TAG, CannotDeliverBroadcastException.TYPE_ID, null); } } @@ -674,7 +678,8 @@ class BroadcastQueueModernImpl extends BroadcastQueue { // receivers as skipped if (r.ordered && r.resultAbort) { for (int i = r.finishedCount + 1; i < r.receivers.size(); i++) { - setDeliveryState(null, r, i, r.receivers.get(i), BroadcastRecord.DELIVERY_SKIPPED); + setDeliveryState(null, null, r, i, r.receivers.get(i), + BroadcastRecord.DELIVERY_SKIPPED); } } @@ -690,7 +695,7 @@ class BroadcastQueueModernImpl extends BroadcastQueue { final int index = queue.getActiveIndex(); final Object receiver = r.receivers.get(index); - setDeliveryState(queue, r, index, receiver, deliveryState); + setDeliveryState(queue, app, r, index, receiver, deliveryState); if (deliveryState == BroadcastRecord.DELIVERY_TIMEOUT) { if (app != null && !app.isDebugging()) { @@ -733,12 +738,13 @@ class BroadcastQueueModernImpl extends BroadcastQueue { * bookkeeping related to ordered broadcasts. */ private void setDeliveryState(@Nullable BroadcastProcessQueue queue, - @NonNull BroadcastRecord r, int index, @NonNull Object receiver, - @DeliveryState int newDeliveryState) { + @Nullable ProcessRecord app, @NonNull BroadcastRecord r, int index, + @NonNull Object receiver, @DeliveryState int newDeliveryState) { final int oldDeliveryState = getDeliveryState(r, index); if (newDeliveryState != BroadcastRecord.DELIVERY_DELIVERED) { - Slog.w(TAG, "Delivery state of " + r + " to " + receiver + " changed from " + Slog.w(TAG, "Delivery state of " + r + " to " + receiver + + " via " + app + " changed from " + deliveryStateToString(oldDeliveryState) + " to " + deliveryStateToString(newDeliveryState)); } @@ -837,7 +843,7 @@ class BroadcastQueueModernImpl extends BroadcastQueue { * of it matching a predicate. */ private final BroadcastConsumer mBroadcastConsumerSkip = (r, i) -> { - setDeliveryState(null, r, i, r.receivers.get(i), BroadcastRecord.DELIVERY_SKIPPED); + setDeliveryState(null, null, r, i, r.receivers.get(i), BroadcastRecord.DELIVERY_SKIPPED); }; private boolean skipMatchingBroadcasts( diff --git a/services/core/java/com/android/server/am/BroadcastRecord.java b/services/core/java/com/android/server/am/BroadcastRecord.java index ae7f2a5d8565a..33f74f3bd7794 100644 --- a/services/core/java/com/android/server/am/BroadcastRecord.java +++ b/services/core/java/com/android/server/am/BroadcastRecord.java @@ -128,6 +128,9 @@ final class BroadcastRecord extends Binder { @Nullable final BiFunction filterExtrasForReceiver; + String cachedToString; + String cachedToShortString; + static final int IDLE = 0; static final int APP_RECEIVE = 1; static final int CALL_IN_RECEIVE = 2; @@ -700,21 +703,27 @@ final class BroadcastRecord extends Binder { @Override public String toString() { - String label = intent.getAction(); - if (label == null) { - label = intent.toString(); + if (cachedToString == null) { + String label = intent.getAction(); + if (label == null) { + label = intent.toString(); + } + cachedToString = "BroadcastRecord{" + + Integer.toHexString(System.identityHashCode(this)) + + " u" + userId + " " + label + "}"; } - return "BroadcastRecord{" - + Integer.toHexString(System.identityHashCode(this)) - + " u" + userId + " " + label + "}"; + return cachedToString; } public String toShortString() { - String label = intent.getAction(); - if (label == null) { - label = intent.toString(); + if (cachedToShortString == null) { + String label = intent.getAction(); + if (label == null) { + label = intent.toString(); + } + cachedToShortString = label + "/u" + userId; } - return label + "/u" + userId; + return cachedToShortString; } public void dumpDebug(ProtoOutputStream proto, long fieldId) { 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 cf5d1133b741e..2a54f463f44ea 100644 --- a/services/tests/mockingservicestests/src/com/android/server/am/BroadcastQueueTest.java +++ b/services/tests/mockingservicestests/src/com/android/server/am/BroadcastQueueTest.java @@ -17,6 +17,7 @@ package com.android.server.am; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotEquals; import static org.junit.Assert.assertTrue; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyBoolean; @@ -42,6 +43,7 @@ import android.app.ActivityManager; import android.app.AppOpsManager; import android.app.BroadcastOptions; import android.app.IApplicationThread; +import android.app.RemoteServiceException.CannotDeliverBroadcastException; import android.app.usage.UsageEvents.Event; import android.app.usage.UsageStatsManagerInternal; import android.content.ComponentName; @@ -56,6 +58,7 @@ import android.content.pm.PackageManagerInternal; import android.content.pm.ResolveInfo; import android.os.Binder; import android.os.Bundle; +import android.os.DeadObjectException; import android.os.Handler; import android.os.HandlerThread; import android.os.IBinder; @@ -135,6 +138,12 @@ public class BroadcastQueueTest { private ActivityManagerService mAms; private BroadcastQueue mQueue; + /** + * When enabled {@link ActivityManagerService#startProcessLocked} will fail + * by returning {@code null}; otherwise it will spawn a new mock process. + */ + private boolean mFailStartProcess; + /** * Map from PID to registered registered runtime receivers. */ @@ -185,10 +194,13 @@ public class BroadcastQueueTest { doAnswer((invocation) -> { Log.v(TAG, "Intercepting startProcessLocked() for " + Arrays.toString(invocation.getArguments())); + if (mFailStartProcess) { + return null; + } final String processName = invocation.getArgument(0); final ApplicationInfo ai = invocation.getArgument(1); - final ProcessRecord res = makeActiveProcessRecord(ai, processName, false, - false, UnaryOperator.identity()); + final ProcessRecord res = makeActiveProcessRecord(ai, processName, + ProcessBehavior.NORMAL, UnaryOperator.identity()); mHandlerThread.getThreadHandler().post(() -> { synchronized (mAms) { mQueue.onApplicationAttachedLocked(res); @@ -278,25 +290,48 @@ public class BroadcastQueueTest { } } + private enum ProcessBehavior { + /** Process broadcasts normally */ + NORMAL, + /** Wedge and never confirm broadcast receipt */ + WEDGE, + /** Process broadcast by requesting abort */ + ABORT, + /** Appear to behave completely dead */ + DEAD, + } + private ProcessRecord makeActiveProcessRecord(String packageName) throws Exception { final ApplicationInfo ai = makeApplicationInfo(packageName); - return makeActiveProcessRecord(ai, ai.processName, false, false, + return makeActiveProcessRecord(ai, ai.processName, ProcessBehavior.NORMAL, UnaryOperator.identity()); } - private ProcessRecord makeWedgedActiveProcessRecord(String packageName) throws Exception { + private ProcessRecord makeActiveProcessRecord(String packageName, + ProcessBehavior behavior) throws Exception { final ApplicationInfo ai = makeApplicationInfo(packageName); - return makeActiveProcessRecord(ai, ai.processName, true, false, + return makeActiveProcessRecord(ai, ai.processName, behavior, UnaryOperator.identity()); } private ProcessRecord makeActiveProcessRecord(ApplicationInfo ai, String processName, - boolean wedged, boolean abort, UnaryOperator extrasOperator) throws Exception { + ProcessBehavior behavior, UnaryOperator extrasOperator) throws Exception { + final boolean wedge = (behavior == ProcessBehavior.WEDGE); + final boolean abort = (behavior == ProcessBehavior.ABORT); + final boolean dead = (behavior == ProcessBehavior.DEAD); + final ProcessRecord r = spy(new ProcessRecord(mAms, ai, processName, ai.uid)); r.setPid(mNextPid.getAndIncrement()); mActiveProcesses.add(r); - final IApplicationThread thread = mock(IApplicationThread.class); + final IApplicationThread thread; + if (dead) { + thread = mock(IApplicationThread.class, (invocation) -> { + throw new DeadObjectException(); + }); + } else { + thread = mock(IApplicationThread.class); + } final IBinder threadBinder = new Binder(); doReturn(threadBinder).when(thread).asBinder(); r.makeActive(thread, mAms.mProcessStats); @@ -309,18 +344,21 @@ public class BroadcastQueueTest { UserHandle.getUserId(r.info.uid), receiver); mRegisteredReceivers.put(r.getPid(), receiverList); + // If we're entirely dead, rely on default behaviors above + if (dead) return r; + doAnswer((invocation) -> { Log.v(TAG, "Intercepting scheduleReceiver() for " + Arrays.toString(invocation.getArguments())); final Bundle extras = invocation.getArgument(5); - if (!wedged) { + if (!wedge) { assertTrue(r.mReceivers.numberOfCurReceivers() > 0); assertTrue(mQueue.getPreferredSchedulingGroupLocked(r) != ProcessList.SCHED_GROUP_UNDEFINED); mHandlerThread.getThreadHandler().post(() -> { synchronized (mAms) { - mQueue.finishReceiverLocked(r, Activity.RESULT_OK, - null, extrasOperator.apply(extras), abort, false); + mQueue.finishReceiverLocked(r, Activity.RESULT_OK, null, + extrasOperator.apply(extras), abort, false); } }); } @@ -333,7 +371,7 @@ public class BroadcastQueueTest { + Arrays.toString(invocation.getArguments())); final Bundle extras = invocation.getArgument(4); final boolean ordered = invocation.getArgument(5); - if (!wedged && ordered) { + if (!wedge && ordered) { assertTrue(r.mReceivers.numberOfCurReceivers() > 0); assertTrue(mQueue.getPreferredSchedulingGroupLocked(r) != ProcessList.SCHED_GROUP_UNDEFINED); @@ -449,6 +487,13 @@ public class BroadcastQueueTest { any(), eq(false), eq(UserHandle.USER_SYSTEM), anyInt()); } + private void verifyScheduleReceiver(VerificationMode mode, ProcessRecord app, Intent intent) + throws Exception { + verify(app.getThread(), mode).scheduleReceiver( + argThat(filterEqualsIgnoringComponent(intent)), any(), any(), anyInt(), any(), + any(), eq(false), eq(UserHandle.USER_SYSTEM), anyInt()); + } + private void verifyScheduleReceiver(VerificationMode mode, ProcessRecord app, Intent intent, ComponentName component) throws Exception { final Intent targetedIntent = new Intent(intent); @@ -689,7 +734,8 @@ public class BroadcastQueueTest { @Test public void testWedged() throws Exception { final ProcessRecord callerApp = makeActiveProcessRecord(PACKAGE_RED); - final ProcessRecord receiverApp = makeWedgedActiveProcessRecord(PACKAGE_GREEN); + final ProcessRecord receiverApp = makeActiveProcessRecord(PACKAGE_GREEN, + ProcessBehavior.WEDGE); final Intent airplane = new Intent(Intent.ACTION_AIRPLANE_MODE_CHANGED); enqueueBroadcast(makeBroadcastRecord(airplane, callerApp, @@ -699,6 +745,106 @@ public class BroadcastQueueTest { verify(mAms).appNotResponding(eq(receiverApp), any()); } + /** + * Verify that we handle registered receivers in a process that always + * responds with {@link DeadObjectException}, recovering to restart the + * process and deliver their next broadcast. + */ + @Test + public void testDead_Registered() throws Exception { + final ProcessRecord callerApp = makeActiveProcessRecord(PACKAGE_RED); + final ProcessRecord receiverApp = makeActiveProcessRecord(PACKAGE_GREEN, + ProcessBehavior.DEAD); + + final Intent airplane = new Intent(Intent.ACTION_AIRPLANE_MODE_CHANGED); + enqueueBroadcast(makeBroadcastRecord(airplane, callerApp, + List.of(makeRegisteredReceiver(receiverApp)))); + final Intent timezone = new Intent(Intent.ACTION_TIMEZONE_CHANGED); + enqueueBroadcast(makeBroadcastRecord(timezone, callerApp, + List.of(makeManifestReceiver(PACKAGE_GREEN, CLASS_GREEN)))); + waitForIdle(); + + // First broadcast should have already been dead + verifyScheduleRegisteredReceiver(receiverApp, airplane); + verify(receiverApp).scheduleCrashLocked(any(), + eq(CannotDeliverBroadcastException.TYPE_ID), any()); + + // Second broadcast in new process should work fine + final ProcessRecord restartedReceiverApp = mAms.getProcessRecordLocked(PACKAGE_GREEN, + getUidForPackage(PACKAGE_GREEN)); + assertNotEquals(receiverApp, restartedReceiverApp); + verifyScheduleReceiver(restartedReceiverApp, timezone); + } + + /** + * Verify that we handle manifest receivers in a process that always + * responds with {@link DeadObjectException}, recovering to restart the + * process and deliver their next broadcast. + */ + @Test + public void testDead_Manifest() throws Exception { + final ProcessRecord callerApp = makeActiveProcessRecord(PACKAGE_RED); + final ProcessRecord receiverApp = makeActiveProcessRecord(PACKAGE_GREEN, + ProcessBehavior.DEAD); + + final Intent airplane = new Intent(Intent.ACTION_AIRPLANE_MODE_CHANGED); + enqueueBroadcast(makeBroadcastRecord(airplane, callerApp, + List.of(makeManifestReceiver(PACKAGE_GREEN, CLASS_GREEN)))); + final Intent timezone = new Intent(Intent.ACTION_TIMEZONE_CHANGED); + enqueueBroadcast(makeBroadcastRecord(timezone, callerApp, + List.of(makeManifestReceiver(PACKAGE_GREEN, CLASS_GREEN)))); + waitForIdle(); + + // First broadcast should have already been dead + verifyScheduleReceiver(receiverApp, airplane); + verify(receiverApp).scheduleCrashLocked(any(), + eq(CannotDeliverBroadcastException.TYPE_ID), any()); + + // Second broadcast in new process should work fine + final ProcessRecord restartedReceiverApp = mAms.getProcessRecordLocked(PACKAGE_GREEN, + getUidForPackage(PACKAGE_GREEN)); + assertNotEquals(receiverApp, restartedReceiverApp); + verifyScheduleReceiver(restartedReceiverApp, timezone); + } + + /** + * Verify that we handle the system failing to start a process. + */ + @Test + public void testFailStartProcess() throws Exception { + final ProcessRecord callerApp = makeActiveProcessRecord(PACKAGE_RED); + + // Send broadcast while process starts are failing + mFailStartProcess = true; + final Intent airplane = new Intent(Intent.ACTION_AIRPLANE_MODE_CHANGED); + enqueueBroadcast(makeBroadcastRecord(airplane, callerApp, + List.of(makeManifestReceiver(PACKAGE_GREEN, CLASS_GREEN), + makeManifestReceiver(PACKAGE_YELLOW, CLASS_YELLOW)))); + + // Confirm that queue goes idle, with no processes + waitForIdle(); + assertEquals(1, mActiveProcesses.size()); + + // Send more broadcasts with working process starts + mFailStartProcess = false; + final Intent timezone = new Intent(Intent.ACTION_TIMEZONE_CHANGED); + enqueueBroadcast(makeBroadcastRecord(timezone, callerApp, + List.of(makeManifestReceiver(PACKAGE_GREEN, CLASS_GREEN), + makeManifestReceiver(PACKAGE_YELLOW, CLASS_YELLOW)))); + + // Confirm that we only saw second broadcast + waitForIdle(); + assertEquals(3, mActiveProcesses.size()); + final ProcessRecord receiverGreenApp = mAms.getProcessRecordLocked(PACKAGE_GREEN, + getUidForPackage(PACKAGE_GREEN)); + final ProcessRecord receiverYellowApp = mAms.getProcessRecordLocked(PACKAGE_YELLOW, + getUidForPackage(PACKAGE_YELLOW)); + verifyScheduleReceiver(never(), receiverGreenApp, airplane); + verifyScheduleReceiver(never(), receiverYellowApp, airplane); + verifyScheduleReceiver(times(1), receiverGreenApp, timezone); + verifyScheduleReceiver(times(1), receiverYellowApp, timezone); + } + /** * Verify that we cleanup a disabled component, skipping a pending dispatch * of broadcast to that component. @@ -770,13 +916,13 @@ public class BroadcastQueueTest { // Purposefully warm-start the middle apps to make sure we dispatch to // both cold and warm apps in expected order makeActiveProcessRecord(makeApplicationInfo(PACKAGE_BLUE), PACKAGE_BLUE, - false, false, (extras) -> { + ProcessBehavior.NORMAL, (extras) -> { extras = clone(extras); extras.putBoolean(PACKAGE_BLUE, true); return extras; }); makeActiveProcessRecord(makeApplicationInfo(PACKAGE_YELLOW), PACKAGE_YELLOW, - false, false, (extras) -> { + ProcessBehavior.NORMAL, (extras) -> { extras = clone(extras); extras.putBoolean(PACKAGE_YELLOW, true); return extras; @@ -858,7 +1004,7 @@ public class BroadcastQueueTest { // Create a process that aborts any ordered broadcasts makeActiveProcessRecord(makeApplicationInfo(PACKAGE_GREEN), PACKAGE_GREEN, - false, true, (extras) -> { + ProcessBehavior.ABORT, (extras) -> { extras = clone(extras); extras.putBoolean(PACKAGE_GREEN, true); return extras; From 40fe3375ffec019606687f92d2fe02403ab219c5 Mon Sep 17 00:00:00 2001 From: Jeff Sharkey Date: Mon, 26 Sep 2022 13:16:23 -0600 Subject: [PATCH 2/4] BroadcastQueue: max pending, misc fixes. To avoid paused or delayed queues from getting backed up too far, add a "MAX_PENDING_BROADCASTS" release valve. Ignore ANR timeouts while the system is booting. Ignore broadcast state changes once it's already reached a terminal state to handle racing timeout messages. Fix isEmpty() bug where caller wanted to check for empty-and-idle. Bug: 245771249 Test: atest FrameworksMockingServicesTests:BroadcastQueueTest Change-Id: If5a5898bbfc0005e1c33a31aaaf05c8eab7f90fb --- .../com/android/server/am/BroadcastConstants.java | 10 ++++++++++ .../android/server/am/BroadcastProcessQueue.java | 8 +++++++- .../android/server/am/BroadcastQueueModernImpl.java | 13 +++++++++---- 3 files changed, 26 insertions(+), 5 deletions(-) diff --git a/services/core/java/com/android/server/am/BroadcastConstants.java b/services/core/java/com/android/server/am/BroadcastConstants.java index 3efb628a8b759..f9b0dd0d6f28d 100644 --- a/services/core/java/com/android/server/am/BroadcastConstants.java +++ b/services/core/java/com/android/server/am/BroadcastConstants.java @@ -136,6 +136,14 @@ public class BroadcastConstants { public int MAX_RUNNING_ACTIVE_BROADCASTS = DEFAULT_MAX_RUNNING_ACTIVE_BROADCASTS; private static final int DEFAULT_MAX_RUNNING_ACTIVE_BROADCASTS = 16; + /** + * For {@link BroadcastQueueModernImpl}: Maximum number of pending + * broadcasts to hold for a process before we ignore any delays that policy + * might have applied to that process. + */ + public int MAX_PENDING_BROADCASTS = DEFAULT_MAX_PENDING_BROADCASTS; + private static final int DEFAULT_MAX_PENDING_BROADCASTS = 256; + /** * For {@link BroadcastQueueModernImpl}: Default delay to apply to normal * broadcasts, giving a chance for debouncing of rapidly changing events. @@ -217,6 +225,8 @@ public class BroadcastConstants { DEFAULT_MAX_RUNNING_PROCESS_QUEUES); MAX_RUNNING_ACTIVE_BROADCASTS = properties.getInt("bcast_max_running_active_broadcasts", DEFAULT_MAX_RUNNING_ACTIVE_BROADCASTS); + MAX_PENDING_BROADCASTS = properties.getInt("bcast_max_pending_broadcasts", + DEFAULT_MAX_PENDING_BROADCASTS); DELAY_NORMAL_MILLIS = properties.getLong("bcast_delay_normal_millis", DEFAULT_DELAY_NORMAL_MILLIS); DELAY_CACHED_MILLIS = properties.getLong("bcast_delay_cached_millis", diff --git a/services/core/java/com/android/server/am/BroadcastProcessQueue.java b/services/core/java/com/android/server/am/BroadcastProcessQueue.java index 379b494a86fa4..342d1f2f3131e 100644 --- a/services/core/java/com/android/server/am/BroadcastProcessQueue.java +++ b/services/core/java/com/android/server/am/BroadcastProcessQueue.java @@ -330,7 +330,7 @@ class BroadcastProcessQueue { } public boolean isEmpty() { - return (mActive != null) && mPending.isEmpty(); + return mPending.isEmpty(); } public boolean isActive() { @@ -388,6 +388,12 @@ class BroadcastProcessQueue { } else { mRunnableAt = runnableAt + constants.DELAY_NORMAL_MILLIS; } + + // If we have too many broadcasts pending, bypass any delays that + // might have been applied above to aid draining + if (mPending.size() >= constants.MAX_PENDING_BROADCASTS) { + mRunnableAt = runnableAt; + } } else { mRunnableAt = Long.MAX_VALUE; } diff --git a/services/core/java/com/android/server/am/BroadcastQueueModernImpl.java b/services/core/java/com/android/server/am/BroadcastQueueModernImpl.java index b265c5758f7d7..a13f48728374c 100644 --- a/services/core/java/com/android/server/am/BroadcastQueueModernImpl.java +++ b/services/core/java/com/android/server/am/BroadcastQueueModernImpl.java @@ -290,7 +290,7 @@ class BroadcastQueueModernImpl extends BroadcastQueue { } // If app isn't running, and there's nothing in the queue, clean up - if (queue.isEmpty() && !queue.isProcessWarm()) { + if (queue.isEmpty() && !queue.isActive() && !queue.isProcessWarm()) { removeProcessQueue(queue.processName, queue.uid); } } @@ -578,7 +578,7 @@ class BroadcastQueueModernImpl extends BroadcastQueue { return; } - if (!r.timeoutExempt) { + if (mService.mProcessesReady && !r.timeoutExempt) { final long timeout = r.isForeground() ? mFgConstants.TIMEOUT : mBgConstants.TIMEOUT; mLocalHandler.sendMessageDelayed( Message.obtain(mLocalHandler, MSG_DELIVERY_TIMEOUT, queue), timeout); @@ -698,6 +698,7 @@ class BroadcastQueueModernImpl extends BroadcastQueue { setDeliveryState(queue, app, r, index, receiver, deliveryState); if (deliveryState == BroadcastRecord.DELIVERY_TIMEOUT) { + r.anrCount++; if (app != null && !app.isDebugging()) { mService.appNotResponding(queue.app, TimeoutRecord .forBroadcastReceiver("Broadcast of " + r.toShortString())); @@ -709,7 +710,7 @@ class BroadcastQueueModernImpl extends BroadcastQueue { // Even if we have more broadcasts, if we've made reasonable progress // and someone else is waiting, retire ourselves to avoid starvation final boolean shouldRetire = (mRunnableHead != null) - && (queue.getActiveCountSinceIdle() > mConstants.MAX_RUNNING_ACTIVE_BROADCASTS); + && (queue.getActiveCountSinceIdle() >= mConstants.MAX_RUNNING_ACTIVE_BROADCASTS); if (queue.isRunnable() && queue.isProcessWarm() && !shouldRetire) { // We're on a roll; move onto the next broadcast for this process @@ -749,7 +750,11 @@ class BroadcastQueueModernImpl extends BroadcastQueue { + deliveryStateToString(newDeliveryState)); } - r.setDeliveryState(index, newDeliveryState); + // Only apply state when we haven't already reached a terminal state; + // this is how we ignore racing timeout messages + if (!isDeliveryStateTerminal(oldDeliveryState)) { + r.setDeliveryState(index, newDeliveryState); + } // Emit any relevant tracing results when we're changing the delivery // state as part of running from a queue From e40ab122a16cafab2c4a25fec7dc44b35dd4f2eb Mon Sep 17 00:00:00 2001 From: Jeff Sharkey Date: Mon, 26 Sep 2022 15:05:47 -0600 Subject: [PATCH 3/4] BroadcastQueue: more exception details. Help understand more about CannotDeliverBroadcastException. Bug: 236556314 Test: atest FrameworksMockingServicesTests:BroadcastQueueTest Change-Id: I0dfeea12c7da1a5a2c33400ed9016b18c313a6e2 --- .../com/android/server/am/BroadcastQueueImpl.java | 14 ++++++++------ .../server/am/BroadcastQueueModernImpl.java | 12 +++++++----- 2 files changed, 15 insertions(+), 11 deletions(-) diff --git a/services/core/java/com/android/server/am/BroadcastQueueImpl.java b/services/core/java/com/android/server/am/BroadcastQueueImpl.java index 5848011a508dd..645836b831de2 100644 --- a/services/core/java/com/android/server/am/BroadcastQueueImpl.java +++ b/services/core/java/com/android/server/am/BroadcastQueueImpl.java @@ -732,9 +732,10 @@ public class BroadcastQueueImpl extends BroadcastQueue { } catch (RemoteException ex) { // Failed to call into the process. It's either dying or wedged. Kill it gently. synchronized (mService) { - Slog.w(TAG, "Can't deliver broadcast to " + app.processName - + " (pid " + app.getPid() + "). Crashing it."); - app.scheduleCrashLocked("can't deliver broadcast", + final String msg = "Failed to schedule " + intent + " to " + receiver + + " via " + app + ": " + ex; + Slog.w(TAG, msg); + app.scheduleCrashLocked(msg, CannotDeliverBroadcastException.TYPE_ID, /* extras=*/ null); } throw ex; @@ -1379,9 +1380,10 @@ public class BroadcastQueueImpl extends BroadcastQueue { processCurBroadcastLocked(r, app); return; } catch (RemoteException e) { - Slog.w(TAG, "Exception when sending broadcast to " - + r.curComponent, e); - app.scheduleCrashLocked("can't deliver broadcast", + final String msg = "Failed to schedule " + r.intent + " to " + info + + " via " + app + ": " + e; + Slog.w(TAG, msg); + app.scheduleCrashLocked(msg, CannotDeliverBroadcastException.TYPE_ID, /* extras=*/ null); } catch (RuntimeException e) { Slog.wtf(TAG, "Failed sending broadcast to " diff --git a/services/core/java/com/android/server/am/BroadcastQueueModernImpl.java b/services/core/java/com/android/server/am/BroadcastQueueModernImpl.java index a13f48728374c..d6884633521e5 100644 --- a/services/core/java/com/android/server/am/BroadcastQueueModernImpl.java +++ b/services/core/java/com/android/server/am/BroadcastQueueModernImpl.java @@ -628,9 +628,10 @@ class BroadcastQueueModernImpl extends BroadcastQueue { app.mState.getReportedProcState()); } } catch (RemoteException e) { - Slog.w(TAG, "Failed to schedule " + r + " to " + receiver - + " via " + app + ": " + e); - app.scheduleCrashLocked(TAG, CannotDeliverBroadcastException.TYPE_ID, null); + final String msg = "Failed to schedule " + r + " to " + receiver + + " via " + app + ": " + e; + Slog.w(TAG, msg); + app.scheduleCrashLocked(msg, CannotDeliverBroadcastException.TYPE_ID, null); app.setKilled(true); finishReceiverLocked(queue, BroadcastRecord.DELIVERY_FAILURE); } @@ -655,8 +656,9 @@ class BroadcastQueueModernImpl extends BroadcastQueue { r.resultCode, r.resultData, r.resultExtras, false, r.initialSticky, r.userId, app.mState.getReportedProcState()); } catch (RemoteException e) { - Slog.w(TAG, "Failed to schedule result of " + r + " via " + app + ": " + e); - app.scheduleCrashLocked(TAG, CannotDeliverBroadcastException.TYPE_ID, null); + final String msg = "Failed to schedule result of " + r + " via " + app + ": " + e; + Slog.w(TAG, msg); + app.scheduleCrashLocked(msg, CannotDeliverBroadcastException.TYPE_ID, null); } } } From a4c77ee3f94446b52249c6b90c9ab604d116296e Mon Sep 17 00:00:00 2001 From: Jeff Sharkey Date: Mon, 26 Sep 2022 12:53:22 -0600 Subject: [PATCH 4/4] BroadcastQueue: ignore dead registered receivers. When a process dies (or is restarted), any registered receivers that came from the old PID are no longer valid, and should be skipped. Tests to verify, along with slight update to "default" implementation to skip sending to a known-dead process. Bug: 248605002 Test: atest FrameworksMockingServicesTests:BroadcastQueueTest Change-Id: I9c82595fe2c729cb50213377e1808ffd4bebaa75 --- .../com/android/server/am/BroadcastQueue.java | 9 ++- .../android/server/am/BroadcastQueueImpl.java | 18 +++--- .../server/am/BroadcastQueueModernImpl.java | 38 ++++++++---- .../android/server/am/BroadcastQueueTest.java | 58 ++++++++++++++++++- 4 files changed, 101 insertions(+), 22 deletions(-) diff --git a/services/core/java/com/android/server/am/BroadcastQueue.java b/services/core/java/com/android/server/am/BroadcastQueue.java index 972a1cea682fb..b46a2b2575feb 100644 --- a/services/core/java/com/android/server/am/BroadcastQueue.java +++ b/services/core/java/com/android/server/am/BroadcastQueue.java @@ -114,6 +114,9 @@ public abstract class BroadcastQueue { /** * Signal from OS internals that the given process has just been actively * attached, and is ready to begin receiving broadcasts. + * + * @return if the queue performed an action on the given process, such as + * dispatching a pending broadcast */ @GuardedBy("mService") public abstract boolean onApplicationAttachedLocked(@NonNull ProcessRecord app); @@ -123,7 +126,7 @@ public abstract class BroadcastQueue { * an attempted start and attachment. */ @GuardedBy("mService") - public abstract boolean onApplicationTimeoutLocked(@NonNull ProcessRecord app); + public abstract void onApplicationTimeoutLocked(@NonNull ProcessRecord app); /** * Signal from OS internals that the given process, which had already been @@ -131,14 +134,14 @@ public abstract class BroadcastQueue { * not responding. */ @GuardedBy("mService") - public abstract boolean onApplicationProblemLocked(@NonNull ProcessRecord app); + public abstract void onApplicationProblemLocked(@NonNull ProcessRecord app); /** * Signal from OS internals that the given process has been killed, and is * no longer actively running. */ @GuardedBy("mService") - public abstract boolean onApplicationCleanupLocked(@NonNull ProcessRecord app); + public abstract void onApplicationCleanupLocked(@NonNull ProcessRecord app); /** * Signal from OS internals that the given package (or some subset of that diff --git a/services/core/java/com/android/server/am/BroadcastQueueImpl.java b/services/core/java/com/android/server/am/BroadcastQueueImpl.java index 645836b831de2..16711853267ec 100644 --- a/services/core/java/com/android/server/am/BroadcastQueueImpl.java +++ b/services/core/java/com/android/server/am/BroadcastQueueImpl.java @@ -426,16 +426,16 @@ public class BroadcastQueueImpl extends BroadcastQueue { } } - public boolean onApplicationTimeoutLocked(ProcessRecord app) { - return skipCurrentOrPendingReceiverLocked(app); + public void onApplicationTimeoutLocked(ProcessRecord app) { + skipCurrentOrPendingReceiverLocked(app); } - public boolean onApplicationProblemLocked(ProcessRecord app) { - return skipCurrentOrPendingReceiverLocked(app); + public void onApplicationProblemLocked(ProcessRecord app) { + skipCurrentOrPendingReceiverLocked(app); } - public boolean onApplicationCleanupLocked(ProcessRecord app) { - return skipCurrentOrPendingReceiverLocked(app); + public void onApplicationCleanupLocked(ProcessRecord app) { + skipCurrentOrPendingReceiverLocked(app); } public boolean sendPendingBroadcastsLocked(ProcessRecord app) { @@ -815,7 +815,11 @@ public class BroadcastQueueImpl extends BroadcastQueue { try { if (DEBUG_BROADCAST_LIGHT) Slog.i(TAG_BROADCAST, "Delivering to " + filter + " : " + r); - if (filter.receiverList.app != null && filter.receiverList.app.isInFullBackup()) { + final boolean isInFullBackup = (filter.receiverList.app != null) + && filter.receiverList.app.isInFullBackup(); + final boolean isKilled = (filter.receiverList.app != null) + && filter.receiverList.app.isKilled(); + if (isInFullBackup || isKilled) { // Skip delivery if full backup in progress // If it's an ordered broadcast, we need to continue to the next receiver. if (ordered) { diff --git a/services/core/java/com/android/server/am/BroadcastQueueModernImpl.java b/services/core/java/com/android/server/am/BroadcastQueueModernImpl.java index d6884633521e5..7c236ffe7ba18 100644 --- a/services/core/java/com/android/server/am/BroadcastQueueModernImpl.java +++ b/services/core/java/com/android/server/am/BroadcastQueueModernImpl.java @@ -420,18 +420,17 @@ class BroadcastQueueModernImpl extends BroadcastQueue { } @Override - public boolean onApplicationTimeoutLocked(@NonNull ProcessRecord app) { - return onApplicationCleanupLocked(app); + public void onApplicationTimeoutLocked(@NonNull ProcessRecord app) { + onApplicationCleanupLocked(app); } @Override - public boolean onApplicationProblemLocked(@NonNull ProcessRecord app) { - return onApplicationCleanupLocked(app); + public void onApplicationProblemLocked(@NonNull ProcessRecord app) { + onApplicationCleanupLocked(app); } @Override - public boolean onApplicationCleanupLocked(@NonNull ProcessRecord app) { - boolean didSomething = false; + public void onApplicationCleanupLocked(@NonNull ProcessRecord app) { if ((mRunningColdStart != null) && (mRunningColdStart.app == app)) { // We've been waiting for this app to cold start, and it had // trouble; clear the slot and fail delivery below @@ -439,7 +438,6 @@ class BroadcastQueueModernImpl extends BroadcastQueue { // We might be willing to kick off another cold start enqueueUpdateRunningList(); - didSomething = true; } final BroadcastProcessQueue queue = getProcessQueue(app); @@ -449,16 +447,19 @@ class BroadcastQueueModernImpl extends BroadcastQueue { // If queue was running a broadcast, fail it if (queue.isActive()) { finishReceiverLocked(queue, BroadcastRecord.DELIVERY_FAILURE); - didSomething = true; } + // Skip any pending registered receivers, since the old process + // would never be around to receive them + queue.removeMatchingBroadcasts((r, i) -> { + return (r.receivers.get(i) instanceof BroadcastFilter); + }, mBroadcastConsumerSkip); + // If queue has nothing else pending, consider cleaning it if (queue.isEmpty()) { updateRunnableList(queue); } } - - return didSomething; } @Override @@ -515,6 +516,13 @@ class BroadcastQueueModernImpl extends BroadcastQueue { final int index = queue.getActiveIndex(); final Object receiver = r.receivers.get(index); + // Ignore registered receivers from a previous PID + if (receiver instanceof BroadcastFilter) { + mRunningColdStart = null; + finishReceiverLocked(queue, BroadcastRecord.DELIVERY_SKIPPED); + return; + } + final ApplicationInfo info = ((ResolveInfo) receiver).activityInfo.applicationInfo; final ComponentName component = ((ResolveInfo) receiver).activityInfo.getComponentName(); @@ -536,6 +544,7 @@ class BroadcastQueueModernImpl extends BroadcastQueue { } else { mRunningColdStart = null; finishReceiverLocked(queue, BroadcastRecord.DELIVERY_FAILURE); + return; } } @@ -563,7 +572,7 @@ class BroadcastQueueModernImpl extends BroadcastQueue { return; } - // Consider additional cases where we'd want fo finish immediately + // Consider additional cases where we'd want to finish immediately if (app.isInFullBackup()) { finishReceiverLocked(queue, BroadcastRecord.DELIVERY_SKIPPED); return; @@ -578,6 +587,13 @@ class BroadcastQueueModernImpl extends BroadcastQueue { return; } + // Ignore registered receivers from a previous PID + if ((receiver instanceof BroadcastFilter) + && ((BroadcastFilter) receiver).receiverList.pid != app.getPid()) { + finishReceiverLocked(queue, BroadcastRecord.DELIVERY_SKIPPED); + return; + } + if (mService.mProcessesReady && !r.timeoutExempt) { final long timeout = r.isForeground() ? mFgConstants.TIMEOUT : mBgConstants.TIMEOUT; mLocalHandler.sendMessageDelayed( 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 2a54f463f44ea..89accf877f998 100644 --- a/services/tests/mockingservicestests/src/com/android/server/am/BroadcastQueueTest.java +++ b/services/tests/mockingservicestests/src/com/android/server/am/BroadcastQueueTest.java @@ -209,6 +209,16 @@ public class BroadcastQueueTest { return res; }).when(mAms).startProcessLocked(any(), any(), anyBoolean(), anyInt(), any(), anyInt(), anyBoolean(), anyBoolean()); + doAnswer((invocation) -> { + final String processName = invocation.getArgument(0); + final int uid = invocation.getArgument(1); + for (ProcessRecord r : mActiveProcesses) { + if (Objects.equals(r.processName, processName) && r.uid == uid) { + return r; + } + } + return null; + }).when(mAms).getProcessRecordLocked(any(), anyInt()); doNothing().when(mAms).appNotResponding(any(), any()); final BroadcastConstants constants = new BroadcastConstants( @@ -335,7 +345,6 @@ public class BroadcastQueueTest { final IBinder threadBinder = new Binder(); doReturn(threadBinder).when(thread).asBinder(); r.makeActive(thread, mAms.mProcessStats); - doReturn(r).when(mAms).getProcessRecordLocked(eq(r.info.processName), eq(r.info.uid)); final IIntentReceiver receiver = mock(IIntentReceiver.class); final IBinder receiverBinder = new Binder(); @@ -344,6 +353,14 @@ public class BroadcastQueueTest { UserHandle.getUserId(r.info.uid), receiver); mRegisteredReceivers.put(r.getPid(), receiverList); + doAnswer((invocation) -> { + Log.v(TAG, "Intercepting killLocked() for " + + Arrays.toString(invocation.getArguments())); + mActiveProcesses.remove(r); + mRegisteredReceivers.remove(r.getPid()); + return invocation.callRealMethod(); + }).when(r).killLocked(any(), any(), anyInt(), anyInt(), anyBoolean()); + // If we're entirely dead, rely on default behaviors above if (dead) return r; @@ -887,6 +904,45 @@ public class BroadcastQueueTest { new ComponentName(PACKAGE_GREEN, CLASS_BLUE)); } + /** + * Verify that killing a running process skips registered receivers. + */ + @Test + public void testKill() throws Exception { + final ProcessRecord callerApp = makeActiveProcessRecord(PACKAGE_RED); + final ProcessRecord oldApp = makeActiveProcessRecord(PACKAGE_GREEN); + + final Intent airplane = new Intent(Intent.ACTION_AIRPLANE_MODE_CHANGED); + try (SyncBarrier b = new SyncBarrier()) { + enqueueBroadcast(makeBroadcastRecord(airplane, callerApp, new ArrayList<>( + List.of(makeRegisteredReceiver(oldApp), + makeManifestReceiver(PACKAGE_GREEN, CLASS_GREEN))))); + + synchronized (mAms) { + oldApp.killLocked(TAG, 42, false); + mQueue.onApplicationCleanupLocked(oldApp); + } + } + waitForIdle(); + + // Confirm that we cold-started after the kill + final ProcessRecord newApp = mAms.getProcessRecordLocked(PACKAGE_GREEN, + getUidForPackage(PACKAGE_GREEN)); + assertNotEquals(oldApp, newApp); + + // Confirm that we saw no registered receiver traffic + final IApplicationThread oldThread = oldApp.getThread(); + verify(oldThread, never()).scheduleRegisteredReceiver(any(), + any(), anyInt(), any(), any(), anyBoolean(), anyBoolean(), anyInt(), anyInt()); + final IApplicationThread newThread = newApp.getThread(); + verify(newThread, never()).scheduleRegisteredReceiver(any(), + any(), anyInt(), any(), any(), anyBoolean(), anyBoolean(), anyInt(), anyInt()); + + // Confirm that we saw final manifest broadcast + verifyScheduleReceiver(times(1), newApp, airplane, + new ComponentName(PACKAGE_GREEN, CLASS_GREEN)); + } + /** * Verify that we skip broadcasts to an app being backed up. */