From 46a36cb0a9a8b72462ee68b2cf1d008209bc8966 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Brzezi=C5=84ski?= Date: Tue, 1 Nov 2022 11:58:36 +0000 Subject: [PATCH 1/3] Revert "BroadcastQueue: skip ANRs when assuming success." Revert submission 20329504 Reason for revert: Broken SystemServicesTestRuleTest Bug: 256741859 Reverted Changes: I1a74109e7:BroadcastQueue: return reasons from skip policy. I7fe33cd17:BroadcastQueue: better state transition logging. I9899e805f:BroadcastQueue: skip ANRs when assuming success. Change-Id: I13e432a0928d919476c1d644c8c2ad5ed6943805 --- .../com/android/server/am/BroadcastQueueModernImpl.java | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/services/core/java/com/android/server/am/BroadcastQueueModernImpl.java b/services/core/java/com/android/server/am/BroadcastQueueModernImpl.java index e956fb0859d22..21700a3a08ca2 100644 --- a/services/core/java/com/android/server/am/BroadcastQueueModernImpl.java +++ b/services/core/java/com/android/server/am/BroadcastQueueModernImpl.java @@ -736,10 +736,7 @@ class BroadcastQueueModernImpl extends BroadcastQueue { return; } - // Skip ANR tracking early during boot, when requested, or when we - // immediately assume delivery success - final boolean assumeDelivered = (receiver instanceof BroadcastFilter) && !r.ordered; - if (mService.mProcessesReady && !r.timeoutExempt && !assumeDelivered) { + if (mService.mProcessesReady && !r.timeoutExempt) { queue.lastCpuDelayTime = queue.app.getCpuDelayTime(); final long timeout = r.isForeground() ? mFgConstants.TIMEOUT : mBgConstants.TIMEOUT; @@ -782,7 +779,7 @@ class BroadcastQueueModernImpl extends BroadcastQueue { // TODO: consider making registered receivers of unordered // broadcasts report results to detect ANRs - if (assumeDelivered) { + if (!r.ordered) { finishReceiverLocked(queue, BroadcastRecord.DELIVERY_DELIVERED, "assuming delivered"); } From 8ef5a2e3ae9fe53a71c8fc7c4a1e931b445fd06a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Brzezi=C5=84ski?= Date: Tue, 1 Nov 2022 11:58:36 +0000 Subject: [PATCH 2/3] Revert "BroadcastQueue: better state transition logging." Revert submission 20329504 Reason for revert: Broken SystemServicesTestRuleTest Bug: 256741859 Reverted Changes: I1a74109e7:BroadcastQueue: return reasons from skip policy. I7fe33cd17:BroadcastQueue: better state transition logging. I9899e805f:BroadcastQueue: skip ANRs when assuming success. Change-Id: I31ad4804cde2e16d5ecffb8ea8c9cfa9fec4f23d --- .../server/JobSchedulerBackgroundThread.java | 3 - .../com/android/server/ServiceThread.java | 10 ---- .../android/server/StorageManagerService.java | 2 - .../server/am/ActivityManagerService.java | 2 - .../android/server/am/BroadcastLoopers.java | 3 - .../server/am/BroadcastQueueModernImpl.java | 60 +++++++++---------- 6 files changed, 29 insertions(+), 51 deletions(-) diff --git a/apex/jobscheduler/service/java/com/android/server/JobSchedulerBackgroundThread.java b/apex/jobscheduler/service/java/com/android/server/JobSchedulerBackgroundThread.java index 6b01a9f446af7..a413f7b1f3ca8 100644 --- a/apex/jobscheduler/service/java/com/android/server/JobSchedulerBackgroundThread.java +++ b/apex/jobscheduler/service/java/com/android/server/JobSchedulerBackgroundThread.java @@ -22,8 +22,6 @@ import android.os.HandlerThread; import android.os.Looper; import android.os.Trace; -import com.android.server.am.BroadcastLoopers; - import java.util.concurrent.Executor; /** @@ -47,7 +45,6 @@ public final class JobSchedulerBackgroundThread extends HandlerThread { sInstance = new JobSchedulerBackgroundThread(); sInstance.start(); final Looper looper = sInstance.getLooper(); - BroadcastLoopers.addLooper(looper); looper.setTraceTag(Trace.TRACE_TAG_SYSTEM_SERVER); looper.setSlowLogThresholdMs( SLOW_DISPATCH_THRESHOLD_MS, SLOW_DELIVERY_THRESHOLD_MS); diff --git a/services/core/java/com/android/server/ServiceThread.java b/services/core/java/com/android/server/ServiceThread.java index 3ea4b86d52964..6d8e49c7c8692 100644 --- a/services/core/java/com/android/server/ServiceThread.java +++ b/services/core/java/com/android/server/ServiceThread.java @@ -22,8 +22,6 @@ import android.os.Looper; import android.os.Process; import android.os.StrictMode; -import com.android.server.am.BroadcastLoopers; - /** * Special handler thread that we create for system services that require their own loopers. */ @@ -48,14 +46,6 @@ public class ServiceThread extends HandlerThread { super.run(); } - @Override - protected void onLooperPrepared() { - // Almost all service threads are used for dispatching broadcast - // intents, so register ourselves to ensure that "wait-for-broadcast" - // shell commands are able to drain any pending broadcasts - BroadcastLoopers.addLooper(getLooper()); - } - protected static Handler makeSharedHandler(Looper looper) { return new Handler(looper, /*callback=*/ null, /* async=*/ false, /* shared=*/ true); } diff --git a/services/core/java/com/android/server/StorageManagerService.java b/services/core/java/com/android/server/StorageManagerService.java index c280719b06a3a..72876f669f013 100644 --- a/services/core/java/com/android/server/StorageManagerService.java +++ b/services/core/java/com/android/server/StorageManagerService.java @@ -147,7 +147,6 @@ import com.android.internal.util.IndentingPrintWriter; import com.android.internal.util.Preconditions; import com.android.modules.utils.TypedXmlPullParser; import com.android.modules.utils.TypedXmlSerializer; -import com.android.server.am.BroadcastLoopers; import com.android.server.pm.Installer; import com.android.server.pm.UserManagerInternal; import com.android.server.storage.AppFuseBridge; @@ -1810,7 +1809,6 @@ class StorageManagerService extends IStorageManager.Stub HandlerThread hthread = new HandlerThread(TAG); hthread.start(); - BroadcastLoopers.addLooper(hthread.getLooper()); mHandler = new StorageManagerServiceHandler(hthread.getLooper()); // Add OBB Action Handler to StorageManagerService thread. diff --git a/services/core/java/com/android/server/am/ActivityManagerService.java b/services/core/java/com/android/server/am/ActivityManagerService.java index f865b8a377f6a..3032f17c25976 100644 --- a/services/core/java/com/android/server/am/ActivityManagerService.java +++ b/services/core/java/com/android/server/am/ActivityManagerService.java @@ -2508,8 +2508,6 @@ public class ActivityManagerService extends IActivityManager.Stub Watchdog.getInstance().addMonitor(this); Watchdog.getInstance().addThread(mHandler); - BroadcastLoopers.addLooper(BackgroundThread.getHandler().getLooper()); - // bind background threads to little cores // this is expected to fail inside of framework tests because apps can't touch cpusets directly // make sure we've already adjusted system_server's internal view of itself first diff --git a/services/core/java/com/android/server/am/BroadcastLoopers.java b/services/core/java/com/android/server/am/BroadcastLoopers.java index b828720c9162e..bebb48473fc3c 100644 --- a/services/core/java/com/android/server/am/BroadcastLoopers.java +++ b/services/core/java/com/android/server/am/BroadcastLoopers.java @@ -25,8 +25,6 @@ import android.os.SystemClock; import android.util.ArraySet; import android.util.Slog; -import com.android.internal.annotations.GuardedBy; - import java.io.PrintWriter; import java.util.Objects; import java.util.concurrent.CountDownLatch; @@ -39,7 +37,6 @@ import java.util.concurrent.CountDownLatch; public class BroadcastLoopers { private static final String TAG = "BroadcastLoopers"; - @GuardedBy("sLoopers") private static final ArraySet sLoopers = new ArraySet<>(); /** diff --git a/services/core/java/com/android/server/am/BroadcastQueueModernImpl.java b/services/core/java/com/android/server/am/BroadcastQueueModernImpl.java index 21700a3a08ca2..9e9eb71db4e52 100644 --- a/services/core/java/com/android/server/am/BroadcastQueueModernImpl.java +++ b/services/core/java/com/android/server/am/BroadcastQueueModernImpl.java @@ -144,6 +144,14 @@ class BroadcastQueueModernImpl extends BroadcastQueue { mRunning = new BroadcastProcessQueue[mConstants.MAX_RUNNING_PROCESS_QUEUES]; } + // TODO: add support for replacing pending broadcasts + // TODO: add support for merging pending broadcasts + + // TODO: consider reordering foreground broadcasts within queue + + // TODO: pause queues when background services are running + // TODO: pause queues when processes are frozen + /** * Map from UID to per-process broadcast queues. If a UID hosts more than * one process, each additional process is stored as a linked list using @@ -501,8 +509,7 @@ class BroadcastQueueModernImpl extends BroadcastQueue { if (queue != null) { // If queue was running a broadcast, fail it if (queue.isActive()) { - finishReceiverLocked(queue, BroadcastRecord.DELIVERY_FAILURE, - "onApplicationCleanupLocked"); + finishReceiverLocked(queue, BroadcastRecord.DELIVERY_FAILURE); } // Skip any pending registered receivers, since the old process @@ -654,8 +661,7 @@ class BroadcastQueueModernImpl extends BroadcastQueue { // Ignore registered receivers from a previous PID if (receiver instanceof BroadcastFilter) { mRunningColdStart = null; - finishReceiverLocked(queue, BroadcastRecord.DELIVERY_SKIPPED, - "BroadcastFilter for cold app"); + finishReceiverLocked(queue, BroadcastRecord.DELIVERY_SKIPPED); return; } @@ -677,8 +683,7 @@ class BroadcastQueueModernImpl extends BroadcastQueue { hostingRecord, zygotePolicyFlags, allowWhileBooting, false); if (queue.app == null) { mRunningColdStart = null; - finishReceiverLocked(queue, BroadcastRecord.DELIVERY_FAILURE, - "startProcessLocked failed"); + finishReceiverLocked(queue, BroadcastRecord.DELIVERY_FAILURE); return; } } @@ -709,30 +714,29 @@ class BroadcastQueueModernImpl extends BroadcastQueue { // If someone already finished this broadcast, finish immediately final int oldDeliveryState = getDeliveryState(r, index); if (isDeliveryStateTerminal(oldDeliveryState)) { - finishReceiverLocked(queue, oldDeliveryState, "already terminal state"); + finishReceiverLocked(queue, oldDeliveryState); return; } // Consider additional cases where we'd want to finish immediately if (app.isInFullBackup()) { - finishReceiverLocked(queue, BroadcastRecord.DELIVERY_SKIPPED, "isInFullBackup"); + finishReceiverLocked(queue, BroadcastRecord.DELIVERY_SKIPPED); return; } if (mSkipPolicy.shouldSkip(r, receiver)) { - finishReceiverLocked(queue, BroadcastRecord.DELIVERY_SKIPPED, "mSkipPolicy"); + finishReceiverLocked(queue, BroadcastRecord.DELIVERY_SKIPPED); return; } final Intent receiverIntent = r.getReceiverIntent(receiver); if (receiverIntent == null) { - finishReceiverLocked(queue, BroadcastRecord.DELIVERY_SKIPPED, "isInFullBackup"); + finishReceiverLocked(queue, BroadcastRecord.DELIVERY_SKIPPED); return; } // Ignore registered receivers from a previous PID if ((receiver instanceof BroadcastFilter) && ((BroadcastFilter) receiver).receiverList.pid != app.getPid()) { - finishReceiverLocked(queue, BroadcastRecord.DELIVERY_SKIPPED, - "BroadcastFilter for mismatched PID"); + finishReceiverLocked(queue, BroadcastRecord.DELIVERY_SKIPPED); return; } @@ -764,8 +768,7 @@ class BroadcastQueueModernImpl extends BroadcastQueue { } if (DEBUG_BROADCAST) logv("Scheduling " + r + " to warm " + app); - setDeliveryState(queue, app, r, index, receiver, BroadcastRecord.DELIVERY_SCHEDULED, - "scheduleReceiverWarmLocked"); + setDeliveryState(queue, app, r, index, receiver, BroadcastRecord.DELIVERY_SCHEDULED); final IApplicationThread thread = app.getOnewayThread(); if (thread != null) { @@ -780,8 +783,7 @@ class BroadcastQueueModernImpl extends BroadcastQueue { // TODO: consider making registered receivers of unordered // broadcasts report results to detect ANRs if (!r.ordered) { - finishReceiverLocked(queue, BroadcastRecord.DELIVERY_DELIVERED, - "assuming delivered"); + finishReceiverLocked(queue, BroadcastRecord.DELIVERY_DELIVERED); } } else { notifyScheduleReceiver(app, r, (ResolveInfo) receiver); @@ -795,11 +797,10 @@ class BroadcastQueueModernImpl extends BroadcastQueue { logw(msg); app.scheduleCrashLocked(msg, CannotDeliverBroadcastException.TYPE_ID, null); app.setKilled(true); - finishReceiverLocked(queue, BroadcastRecord.DELIVERY_FAILURE, "remote app"); + finishReceiverLocked(queue, BroadcastRecord.DELIVERY_FAILURE); } } else { - finishReceiverLocked(queue, BroadcastRecord.DELIVERY_FAILURE, - "missing IApplicationThread"); + finishReceiverLocked(queue, BroadcastRecord.DELIVERY_FAILURE); } } @@ -843,8 +844,7 @@ class BroadcastQueueModernImpl extends BroadcastQueue { } private void deliveryTimeoutHardLocked(@NonNull BroadcastProcessQueue queue) { - finishReceiverLocked(queue, BroadcastRecord.DELIVERY_TIMEOUT, - "deliveryTimeoutHardLocked"); + finishReceiverLocked(queue, BroadcastRecord.DELIVERY_TIMEOUT); } @Override @@ -871,16 +871,16 @@ class BroadcastQueueModernImpl extends BroadcastQueue { 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, "resultAbort"); + BroadcastRecord.DELIVERY_SKIPPED); } } } - return finishReceiverLocked(queue, BroadcastRecord.DELIVERY_DELIVERED, "remote app"); + return finishReceiverLocked(queue, BroadcastRecord.DELIVERY_DELIVERED); } private boolean finishReceiverLocked(@NonNull BroadcastProcessQueue queue, - @DeliveryState int deliveryState, @NonNull String reason) { + @DeliveryState int deliveryState) { checkState(queue.isActive(), "isActive"); final ProcessRecord app = queue.app; @@ -888,7 +888,7 @@ class BroadcastQueueModernImpl extends BroadcastQueue { final int index = queue.getActiveIndex(); final Object receiver = r.receivers.get(index); - setDeliveryState(queue, app, r, index, receiver, deliveryState, reason); + setDeliveryState(queue, app, r, index, receiver, deliveryState); if (deliveryState == BroadcastRecord.DELIVERY_TIMEOUT) { r.anrCount++; @@ -935,7 +935,7 @@ class BroadcastQueueModernImpl extends BroadcastQueue { */ private void setDeliveryState(@Nullable BroadcastProcessQueue queue, @Nullable ProcessRecord app, @NonNull BroadcastRecord r, int index, - @NonNull Object receiver, @DeliveryState int newDeliveryState, String reason) { + @NonNull Object receiver, @DeliveryState int newDeliveryState) { final int oldDeliveryState = getDeliveryState(r, index); // Only apply state when we haven't already reached a terminal state; @@ -963,7 +963,7 @@ class BroadcastQueueModernImpl extends BroadcastQueue { logw("Delivery state of " + r + " to " + receiver + " via " + app + " changed from " + deliveryStateToString(oldDeliveryState) + " to " - + deliveryStateToString(newDeliveryState) + " because " + reason); + + deliveryStateToString(newDeliveryState)); } r.terminalCount++; @@ -1053,8 +1053,7 @@ class BroadcastQueueModernImpl extends BroadcastQueue { * of it matching a predicate. */ private final BroadcastConsumer mBroadcastConsumerSkip = (r, i) -> { - setDeliveryState(null, null, r, i, r.receivers.get(i), BroadcastRecord.DELIVERY_SKIPPED, - "mBroadcastConsumerSkip"); + setDeliveryState(null, null, r, i, r.receivers.get(i), BroadcastRecord.DELIVERY_SKIPPED); }; /** @@ -1062,8 +1061,7 @@ class BroadcastQueueModernImpl extends BroadcastQueue { * cancelled, usually as a result of it matching a predicate. */ private final BroadcastConsumer mBroadcastConsumerSkipAndCanceled = (r, i) -> { - setDeliveryState(null, null, r, i, r.receivers.get(i), BroadcastRecord.DELIVERY_SKIPPED, - "mBroadcastConsumerSkipAndCanceled"); + setDeliveryState(null, null, r, i, r.receivers.get(i), BroadcastRecord.DELIVERY_SKIPPED); r.resultCode = Activity.RESULT_CANCELED; r.resultData = null; r.resultExtras = null; From 6d8daf68f0021a056a44e68da367c22bb0271f97 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Brzezi=C5=84ski?= Date: Tue, 1 Nov 2022 11:58:36 +0000 Subject: [PATCH 3/3] Revert "BroadcastQueue: return reasons from skip policy." Revert submission 20329504 Reason for revert: Broken SystemServicesTestRuleTest Bug: 256741859 Reverted Changes: I1a74109e7:BroadcastQueue: return reasons from skip policy. I7fe33cd17:BroadcastQueue: better state transition logging. I9899e805f:BroadcastQueue: skip ANRs when assuming success. Change-Id: Ia7601026df1997bcde8f7392b5bdbb6e06e2253b --- .../server/am/BroadcastSkipPolicy.java | 234 +++++++++--------- .../android/server/am/BroadcastQueueTest.java | 6 +- 2 files changed, 122 insertions(+), 118 deletions(-) diff --git a/services/core/java/com/android/server/am/BroadcastSkipPolicy.java b/services/core/java/com/android/server/am/BroadcastSkipPolicy.java index 481ab17b609eb..60fddf0c7f227 100644 --- a/services/core/java/com/android/server/am/BroadcastSkipPolicy.java +++ b/services/core/java/com/android/server/am/BroadcastSkipPolicy.java @@ -21,7 +21,6 @@ import static com.android.server.am.ActivityManagerService.checkComponentPermiss import static com.android.server.am.BroadcastQueue.TAG; import android.annotation.NonNull; -import android.annotation.Nullable; import android.app.ActivityManager; import android.app.AppGlobals; import android.app.AppOpsManager; @@ -43,8 +42,6 @@ import android.util.Slog; import com.android.internal.util.ArrayUtils; -import java.util.Objects; - /** * Policy logic that decides if delivery of a particular {@link BroadcastRecord} * should be skipped for a given {@link ResolveInfo} or {@link BroadcastFilter}. @@ -54,8 +51,8 @@ import java.util.Objects; public class BroadcastSkipPolicy { private final ActivityManagerService mService; - public BroadcastSkipPolicy(@NonNull ActivityManagerService service) { - mService = Objects.requireNonNull(service); + public BroadcastSkipPolicy(ActivityManagerService service) { + mService = service; } /** @@ -63,39 +60,18 @@ public class BroadcastSkipPolicy { * the given {@link BroadcastFilter} or {@link ResolveInfo}. */ public boolean shouldSkip(@NonNull BroadcastRecord r, @NonNull Object target) { - final String msg = shouldSkipMessage(r, target); - if (msg != null) { - Slog.w(TAG, msg); - return true; - } else { - return false; - } - } - - /** - * Determine if the given {@link BroadcastRecord} is eligible to be sent to - * the given {@link BroadcastFilter} or {@link ResolveInfo}. - * - * @return message indicating why the argument should be skipped, otherwise - * {@code null} if it can proceed. - */ - public @Nullable String shouldSkipMessage(@NonNull BroadcastRecord r, @NonNull Object target) { if (target instanceof BroadcastFilter) { - return shouldSkipMessage(r, (BroadcastFilter) target); + return shouldSkip(r, (BroadcastFilter) target); } else { - return shouldSkipMessage(r, (ResolveInfo) target); + return shouldSkip(r, (ResolveInfo) target); } } /** * Determine if the given {@link BroadcastRecord} is eligible to be sent to * the given {@link ResolveInfo}. - * - * @return message indicating why the argument should be skipped, otherwise - * {@code null} if it can proceed. */ - private @Nullable String shouldSkipMessage(@NonNull BroadcastRecord r, - @NonNull ResolveInfo info) { + public boolean shouldSkip(@NonNull BroadcastRecord r, @NonNull ResolveInfo info) { final BroadcastOptions brOptions = r.options; final ComponentName component = new ComponentName( info.activityInfo.applicationInfo.packageName, @@ -106,52 +82,58 @@ public class BroadcastSkipPolicy { < brOptions.getMinManifestReceiverApiLevel() || info.activityInfo.applicationInfo.targetSdkVersion > brOptions.getMaxManifestReceiverApiLevel())) { - return "Target SDK mismatch: receiver " + info.activityInfo + Slog.w(TAG, "Target SDK mismatch: receiver " + info.activityInfo + " targets " + info.activityInfo.applicationInfo.targetSdkVersion + " but delivery restricted to [" + brOptions.getMinManifestReceiverApiLevel() + ", " + brOptions.getMaxManifestReceiverApiLevel() - + "] broadcasting " + broadcastDescription(r, component); + + "] broadcasting " + broadcastDescription(r, component)); + return true; } if (brOptions != null && !brOptions.testRequireCompatChange(info.activityInfo.applicationInfo.uid)) { - return "Compat change filtered: broadcasting " + broadcastDescription(r, component) + Slog.w(TAG, "Compat change filtered: broadcasting " + broadcastDescription(r, component) + " to uid " + info.activityInfo.applicationInfo.uid + " due to compat change " - + r.options.getRequireCompatChangeId(); + + r.options.getRequireCompatChangeId()); + return true; } if (!mService.validateAssociationAllowedLocked(r.callerPackage, r.callingUid, component.getPackageName(), info.activityInfo.applicationInfo.uid)) { - return "Association not allowed: broadcasting " - + broadcastDescription(r, component); + Slog.w(TAG, "Association not allowed: broadcasting " + + broadcastDescription(r, component)); + return true; } if (!mService.mIntentFirewall.checkBroadcast(r.intent, r.callingUid, r.callingPid, r.resolvedType, info.activityInfo.applicationInfo.uid)) { - return "Firewall blocked: broadcasting " - + broadcastDescription(r, component); + Slog.w(TAG, "Firewall blocked: broadcasting " + + broadcastDescription(r, component)); + return true; } int perm = checkComponentPermission(info.activityInfo.permission, r.callingPid, r.callingUid, info.activityInfo.applicationInfo.uid, info.activityInfo.exported); if (perm != PackageManager.PERMISSION_GRANTED) { if (!info.activityInfo.exported) { - return "Permission Denial: broadcasting " + Slog.w(TAG, "Permission Denial: broadcasting " + broadcastDescription(r, component) - + " is not exported from uid " + info.activityInfo.applicationInfo.uid; + + " is not exported from uid " + info.activityInfo.applicationInfo.uid); } else { - return "Permission Denial: broadcasting " + Slog.w(TAG, "Permission Denial: broadcasting " + broadcastDescription(r, component) - + " requires " + info.activityInfo.permission; + + " requires " + info.activityInfo.permission); } + return true; } else if (info.activityInfo.permission != null) { final int opCode = AppOpsManager.permissionToOpCode(info.activityInfo.permission); if (opCode != AppOpsManager.OP_NONE && mService.getAppOpsManager().noteOpNoThrow(opCode, r.callingUid, r.callerPackage, r.callerFeatureId, "Broadcast delivered to " + info.activityInfo.name) != AppOpsManager.MODE_ALLOWED) { - return "Appop Denial: broadcasting " + Slog.w(TAG, "Appop Denial: broadcasting " + broadcastDescription(r, component) + " requires appop " + AppOpsManager.permissionToOp( - info.activityInfo.permission); + info.activityInfo.permission)); + return true; } } @@ -160,34 +142,38 @@ public class BroadcastSkipPolicy { android.Manifest.permission.INTERACT_ACROSS_USERS, info.activityInfo.applicationInfo.uid) != PackageManager.PERMISSION_GRANTED) { - return "Permission Denial: Receiver " + component.flattenToShortString() + Slog.w(TAG, "Permission Denial: Receiver " + component.flattenToShortString() + " requests FLAG_SINGLE_USER, but app does not hold " - + android.Manifest.permission.INTERACT_ACROSS_USERS; + + android.Manifest.permission.INTERACT_ACROSS_USERS); + return true; } } if (info.activityInfo.applicationInfo.isInstantApp() && r.callingUid != info.activityInfo.applicationInfo.uid) { - return "Instant App Denial: receiving " + Slog.w(TAG, "Instant App Denial: receiving " + r.intent + " to " + component.flattenToShortString() + " due to sender " + r.callerPackage + " (uid " + r.callingUid + ")" - + " Instant Apps do not support manifest receivers"; + + " Instant Apps do not support manifest receivers"); + return true; } if (r.callerInstantApp && (info.activityInfo.flags & ActivityInfo.FLAG_VISIBLE_TO_INSTANT_APP) == 0 && r.callingUid != info.activityInfo.applicationInfo.uid) { - return "Instant App Denial: receiving " + Slog.w(TAG, "Instant App Denial: receiving " + r.intent + " to " + component.flattenToShortString() + " requires receiver have visibleToInstantApps set" + " due to sender " + r.callerPackage - + " (uid " + r.callingUid + ")"; + + " (uid " + r.callingUid + ")"); + return true; } if (r.curApp != null && r.curApp.mErrorState.isCrashing()) { // If the target process is crashing, just skip it. - return "Skipping deliver ordered [" + r.queue.toString() + "] " + r - + " to " + r.curApp + ": process crashing"; + Slog.w(TAG, "Skipping deliver ordered [" + r.queue.toString() + "] " + r + + " to " + r.curApp + ": process crashing"); + return true; } boolean isAvailable = false; @@ -197,13 +183,15 @@ public class BroadcastSkipPolicy { UserHandle.getUserId(info.activityInfo.applicationInfo.uid)); } catch (Exception e) { // all such failures mean we skip this receiver - return "Exception getting recipient info for " - + info.activityInfo.packageName; + Slog.w(TAG, "Exception getting recipient info for " + + info.activityInfo.packageName, e); } if (!isAvailable) { - return "Skipping delivery to " + info.activityInfo.packageName + " / " + Slog.w(TAG, + "Skipping delivery to " + info.activityInfo.packageName + " / " + info.activityInfo.applicationInfo.uid - + " : package no longer available"; + + " : package no longer available"); + return true; } // If permissions need a review before any of the app components can run, we drop @@ -213,8 +201,10 @@ public class BroadcastSkipPolicy { if (!requestStartTargetPermissionsReviewIfNeededLocked(r, info.activityInfo.packageName, UserHandle.getUserId( info.activityInfo.applicationInfo.uid))) { - return "Skipping delivery: permission review required for " - + broadcastDescription(r, component); + Slog.w(TAG, + "Skipping delivery: permission review required for " + + broadcastDescription(r, component)); + return true; } final int allowed = mService.getAppStartModeLOSP( @@ -226,9 +216,10 @@ public class BroadcastSkipPolicy { // to it and the app is in a state that should not receive it // (depending on how getAppStartModeLOSP has determined that). if (allowed == ActivityManager.APP_START_MODE_DISABLED) { - return "Background execution disabled: receiving " + Slog.w(TAG, "Background execution disabled: receiving " + r.intent + " to " - + component.flattenToShortString(); + + component.flattenToShortString()); + return true; } else if (((r.intent.getFlags()&Intent.FLAG_RECEIVER_EXCLUDE_BACKGROUND) != 0) || (r.intent.getComponent() == null && r.intent.getPackage() == null @@ -237,9 +228,10 @@ public class BroadcastSkipPolicy { && !isSignaturePerm(r.requiredPermissions))) { mService.addBackgroundCheckViolationLocked(r.intent.getAction(), component.getPackageName()); - return "Background execution not allowed: receiving " + Slog.w(TAG, "Background execution not allowed: receiving " + r.intent + " to " - + component.flattenToShortString(); + + component.flattenToShortString()); + return true; } } @@ -247,8 +239,10 @@ public class BroadcastSkipPolicy { && !mService.mUserController .isUserRunning(UserHandle.getUserId(info.activityInfo.applicationInfo.uid), 0 /* flags */)) { - return "Skipping delivery to " + info.activityInfo.packageName + " / " - + info.activityInfo.applicationInfo.uid + " : user is not running"; + Slog.w(TAG, + "Skipping delivery to " + info.activityInfo.packageName + " / " + + info.activityInfo.applicationInfo.uid + " : user is not running"); + return true; } if (r.excludedPermissions != null && r.excludedPermissions.length > 0) { @@ -274,15 +268,13 @@ public class BroadcastSkipPolicy { info.activityInfo.applicationInfo.uid, info.activityInfo.packageName) == AppOpsManager.MODE_ALLOWED)) { - return "Skipping delivery to " + info.activityInfo.packageName - + " due to excluded permission " + excludedPermission; + return true; } } else { // When there is no app op associated with the permission, // skip when permission is granted. if (perm == PackageManager.PERMISSION_GRANTED) { - return "Skipping delivery to " + info.activityInfo.packageName - + " due to excluded permission " + excludedPermission; + return true; } } } @@ -291,12 +283,13 @@ public class BroadcastSkipPolicy { // Check that the receiver does *not* belong to any of the excluded packages if (r.excludedPackages != null && r.excludedPackages.length > 0) { if (ArrayUtils.contains(r.excludedPackages, component.getPackageName())) { - return "Skipping delivery of excluded package " + Slog.w(TAG, "Skipping delivery of excluded package " + r.intent + " to " + component.flattenToShortString() + " excludes package " + component.getPackageName() + " due to sender " + r.callerPackage - + " (uid " + r.callingUid + ")"; + + " (uid " + r.callingUid + ")"); + return true; } } @@ -314,94 +307,95 @@ public class BroadcastSkipPolicy { perm = PackageManager.PERMISSION_DENIED; } if (perm != PackageManager.PERMISSION_GRANTED) { - return "Permission Denial: receiving " + Slog.w(TAG, "Permission Denial: receiving " + r.intent + " to " + component.flattenToShortString() + " requires " + requiredPermission + " due to sender " + r.callerPackage - + " (uid " + r.callingUid + ")"; + + " (uid " + r.callingUid + ")"); + return true; } int appOp = AppOpsManager.permissionToOpCode(requiredPermission); if (appOp != AppOpsManager.OP_NONE && appOp != r.appOp) { if (!noteOpForManifestReceiver(appOp, r, info, component)) { - return "Skipping delivery to " + info.activityInfo.packageName - + " due to required appop " + appOp; + return true; } } } } if (r.appOp != AppOpsManager.OP_NONE) { if (!noteOpForManifestReceiver(r.appOp, r, info, component)) { - return "Skipping delivery to " + info.activityInfo.packageName - + " due to required appop " + r.appOp; + return true; } } - return null; + return false; } /** * Determine if the given {@link BroadcastRecord} is eligible to be sent to * the given {@link BroadcastFilter}. - * - * @return message indicating why the argument should be skipped, otherwise - * {@code null} if it can proceed. */ - private @Nullable String shouldSkipMessage(@NonNull BroadcastRecord r, - @NonNull BroadcastFilter filter) { + public boolean shouldSkip(@NonNull BroadcastRecord r, @NonNull BroadcastFilter filter) { if (r.options != null && !r.options.testRequireCompatChange(filter.owningUid)) { - return "Compat change filtered: broadcasting " + r.intent.toString() + Slog.w(TAG, "Compat change filtered: broadcasting " + r.intent.toString() + " to uid " + filter.owningUid + " due to compat change " - + r.options.getRequireCompatChangeId(); + + r.options.getRequireCompatChangeId()); + return true; } if (!mService.validateAssociationAllowedLocked(r.callerPackage, r.callingUid, filter.packageName, filter.owningUid)) { - return "Association not allowed: broadcasting " + Slog.w(TAG, "Association not allowed: broadcasting " + r.intent.toString() + " from " + r.callerPackage + " (pid=" + r.callingPid + ", uid=" + r.callingUid + ") to " + filter.packageName + " through " - + filter; + + filter); + return true; } if (!mService.mIntentFirewall.checkBroadcast(r.intent, r.callingUid, r.callingPid, r.resolvedType, filter.receiverList.uid)) { - return "Firewall blocked: broadcasting " + Slog.w(TAG, "Firewall blocked: broadcasting " + r.intent.toString() + " from " + r.callerPackage + " (pid=" + r.callingPid + ", uid=" + r.callingUid + ") to " + filter.packageName + " through " - + filter; + + filter); + return true; } // Check that the sender has permission to send to this receiver if (filter.requiredPermission != null) { int perm = checkComponentPermission(filter.requiredPermission, r.callingPid, r.callingUid, -1, true); if (perm != PackageManager.PERMISSION_GRANTED) { - return "Permission Denial: broadcasting " + Slog.w(TAG, "Permission Denial: broadcasting " + r.intent.toString() + " from " + r.callerPackage + " (pid=" + r.callingPid + ", uid=" + r.callingUid + ")" + " requires " + filter.requiredPermission - + " due to registered receiver " + filter; + + " due to registered receiver " + filter); + return true; } else { final int opCode = AppOpsManager.permissionToOpCode(filter.requiredPermission); if (opCode != AppOpsManager.OP_NONE && mService.getAppOpsManager().noteOpNoThrow(opCode, r.callingUid, r.callerPackage, r.callerFeatureId, "Broadcast sent to protected receiver") != AppOpsManager.MODE_ALLOWED) { - return "Appop Denial: broadcasting " + Slog.w(TAG, "Appop Denial: broadcasting " + r.intent.toString() + " from " + r.callerPackage + " (pid=" + r.callingPid + ", uid=" + r.callingUid + ")" + " requires appop " + AppOpsManager.permissionToOp( filter.requiredPermission) - + " due to registered receiver " + filter; + + " due to registered receiver " + filter); + return true; } } } if ((filter.receiverList.app == null || filter.receiverList.app.isKilled() || filter.receiverList.app.mErrorState.isCrashing())) { - return "Skipping deliver [" + r.queue.toString() + "] " + r - + " to " + filter.receiverList + ": process gone or crashing"; + Slog.w(TAG, "Skipping deliver [" + r.queue.toString() + "] " + r + + " to " + filter.receiverList + ": process gone or crashing"); + return true; } // Ensure that broadcasts are only sent to other Instant Apps if they are marked as @@ -411,26 +405,28 @@ public class BroadcastSkipPolicy { if (!visibleToInstantApps && filter.instantApp && filter.receiverList.uid != r.callingUid) { - return "Instant App Denial: receiving " + Slog.w(TAG, "Instant App Denial: receiving " + r.intent.toString() + " to " + filter.receiverList.app + " (pid=" + filter.receiverList.pid + ", uid=" + filter.receiverList.uid + ")" + " due to sender " + r.callerPackage + " (uid " + r.callingUid + ")" - + " not specifying FLAG_RECEIVER_VISIBLE_TO_INSTANT_APPS"; + + " not specifying FLAG_RECEIVER_VISIBLE_TO_INSTANT_APPS"); + return true; } if (!filter.visibleToInstantApp && r.callerInstantApp && filter.receiverList.uid != r.callingUid) { - return "Instant App Denial: receiving " + Slog.w(TAG, "Instant App Denial: receiving " + r.intent.toString() + " to " + filter.receiverList.app + " (pid=" + filter.receiverList.pid + ", uid=" + filter.receiverList.uid + ")" + " requires receiver be visible to instant apps" + " due to sender " + r.callerPackage - + " (uid " + r.callingUid + ")"; + + " (uid " + r.callingUid + ")"); + return true; } // Check that the receiver has the required permission(s) to receive this broadcast. @@ -440,14 +436,15 @@ public class BroadcastSkipPolicy { int perm = checkComponentPermission(requiredPermission, filter.receiverList.pid, filter.receiverList.uid, -1, true); if (perm != PackageManager.PERMISSION_GRANTED) { - return "Permission Denial: receiving " + Slog.w(TAG, "Permission Denial: receiving " + r.intent.toString() + " to " + filter.receiverList.app + " (pid=" + filter.receiverList.pid + ", uid=" + filter.receiverList.uid + ")" + " requires " + requiredPermission + " due to sender " + r.callerPackage - + " (uid " + r.callingUid + ")"; + + " (uid " + r.callingUid + ")"); + return true; } int appOp = AppOpsManager.permissionToOpCode(requiredPermission); if (appOp != AppOpsManager.OP_NONE && appOp != r.appOp @@ -455,7 +452,7 @@ public class BroadcastSkipPolicy { filter.receiverList.uid, filter.packageName, filter.featureId, "Broadcast delivered to registered receiver " + filter.receiverId) != AppOpsManager.MODE_ALLOWED) { - return "Appop Denial: receiving " + Slog.w(TAG, "Appop Denial: receiving " + r.intent.toString() + " to " + filter.receiverList.app + " (pid=" + filter.receiverList.pid @@ -463,7 +460,8 @@ public class BroadcastSkipPolicy { + " requires appop " + AppOpsManager.permissionToOp( requiredPermission) + " due to sender " + r.callerPackage - + " (uid " + r.callingUid + ")"; + + " (uid " + r.callingUid + ")"); + return true; } } } @@ -471,13 +469,14 @@ public class BroadcastSkipPolicy { int perm = checkComponentPermission(null, filter.receiverList.pid, filter.receiverList.uid, -1, true); if (perm != PackageManager.PERMISSION_GRANTED) { - return "Permission Denial: security check failed when receiving " + Slog.w(TAG, "Permission Denial: security check failed when receiving " + r.intent.toString() + " to " + filter.receiverList.app + " (pid=" + filter.receiverList.pid + ", uid=" + filter.receiverList.uid + ")" + " due to sender " + r.callerPackage - + " (uid " + r.callingUid + ")"; + + " (uid " + r.callingUid + ")"); + return true; } } // Check that the receiver does *not* have any excluded permissions @@ -497,7 +496,7 @@ public class BroadcastSkipPolicy { filter.receiverList.uid, filter.packageName) == AppOpsManager.MODE_ALLOWED)) { - return "Appop Denial: receiving " + Slog.w(TAG, "Appop Denial: receiving " + r.intent.toString() + " to " + filter.receiverList.app + " (pid=" + filter.receiverList.pid @@ -505,20 +504,22 @@ public class BroadcastSkipPolicy { + " excludes appop " + AppOpsManager.permissionToOp( excludedPermission) + " due to sender " + r.callerPackage - + " (uid " + r.callingUid + ")"; + + " (uid " + r.callingUid + ")"); + return true; } } else { // When there is no app op associated with the permission, // skip when permission is granted. if (perm == PackageManager.PERMISSION_GRANTED) { - return "Permission Denial: receiving " + Slog.w(TAG, "Permission Denial: receiving " + r.intent.toString() + " to " + filter.receiverList.app + " (pid=" + filter.receiverList.pid + ", uid=" + filter.receiverList.uid + ")" + " excludes " + excludedPermission + " due to sender " + r.callerPackage - + " (uid " + r.callingUid + ")"; + + " (uid " + r.callingUid + ")"); + return true; } } } @@ -527,14 +528,15 @@ public class BroadcastSkipPolicy { // Check that the receiver does *not* belong to any of the excluded packages if (r.excludedPackages != null && r.excludedPackages.length > 0) { if (ArrayUtils.contains(r.excludedPackages, filter.packageName)) { - return "Skipping delivery of excluded package " + Slog.w(TAG, "Skipping delivery of excluded package " + r.intent.toString() + " to " + filter.receiverList.app + " (pid=" + filter.receiverList.pid + ", uid=" + filter.receiverList.uid + ")" + " excludes package " + filter.packageName + " due to sender " + r.callerPackage - + " (uid " + r.callingUid + ")"; + + " (uid " + r.callingUid + ")"); + return true; } } @@ -544,14 +546,15 @@ public class BroadcastSkipPolicy { filter.receiverList.uid, filter.packageName, filter.featureId, "Broadcast delivered to registered receiver " + filter.receiverId) != AppOpsManager.MODE_ALLOWED) { - return "Appop Denial: receiving " + Slog.w(TAG, "Appop Denial: receiving " + r.intent.toString() + " to " + filter.receiverList.app + " (pid=" + filter.receiverList.pid + ", uid=" + filter.receiverList.uid + ")" + " requires appop " + AppOpsManager.opToName(r.appOp) + " due to sender " + r.callerPackage - + " (uid " + r.callingUid + ")"; + + " (uid " + r.callingUid + ")"); + return true; } // Ensure that broadcasts are only sent to other apps if they are explicitly marked as @@ -559,14 +562,15 @@ public class BroadcastSkipPolicy { if (!filter.exported && checkComponentPermission(null, r.callingPid, r.callingUid, filter.receiverList.uid, filter.exported) != PackageManager.PERMISSION_GRANTED) { - return "Exported Denial: sending " + Slog.w(TAG, "Exported Denial: sending " + r.intent.toString() + ", action: " + r.intent.getAction() + " from " + r.callerPackage + " (uid=" + r.callingUid + ")" + " due to receiver " + filter.receiverList.app + " (uid " + filter.receiverList.uid + ")" - + " not specifying RECEIVER_EXPORTED"; + + " not specifying RECEIVER_EXPORTED"); + return true; } // If permissions need a review before any of the app components can run, we drop @@ -575,10 +579,10 @@ public class BroadcastSkipPolicy { // broadcast. if (!requestStartTargetPermissionsReviewIfNeededLocked(r, filter.packageName, filter.owningUserId)) { - return "Skipping delivery to " + filter.packageName + " due to permissions review"; + return true; } - return null; + return false; } private static String broadcastDescription(BroadcastRecord r, ComponentName component) { 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 de5960363fa5e..e1a4c1dd72566 100644 --- a/services/tests/mockingservicestests/src/com/android/server/am/BroadcastQueueTest.java +++ b/services/tests/mockingservicestests/src/com/android/server/am/BroadcastQueueTest.java @@ -280,13 +280,13 @@ public class BroadcastQueueTest { constants.TIMEOUT = 100; constants.ALLOW_BG_ACTIVITY_START_TIMEOUT = 0; final BroadcastSkipPolicy emptySkipPolicy = new BroadcastSkipPolicy(mAms) { - public boolean shouldSkip(BroadcastRecord r, Object o) { + public boolean shouldSkip(BroadcastRecord r, ResolveInfo info) { // Ignored return false; } - public String shouldSkipMessage(BroadcastRecord r, Object o) { + public boolean shouldSkip(BroadcastRecord r, BroadcastFilter filter) { // Ignored - return null; + return false; } }; final BroadcastHistory emptyHistory = new BroadcastHistory(constants) {