From 9c2775572ead41a8006e2da768900d663f904ceb Mon Sep 17 00:00:00 2001 From: Christopher Tate Date: Wed, 27 Jan 2021 09:53:29 -0800 Subject: [PATCH] Put broadcast timekeeping on its own thread Broadcast handling has fairly tight real-time / interactivity requirements. However, some key parts of its processing have had an implicit dependency on the latency behaviors of shared looper threads within the system process. In practice this means that it's sensitive to the behavior of unrelated parts of the OS, and in particular parts of the system that perform long-running or otherwise "slow" work on that shared looper thread, inadvertently DOSing broadcast delivery. In order to tightly control the latencies and dependencies around broadcasts' asynchronous processing, it is now using its own dedicated thread for these operations instead of the shared Activity Manager looper. In addition, the framework coretest broadcast tests turned out to have issues. They were not properly up to date with respect to implicit broadcast delivery policy, and furthermore had been copy/pasted from a starting point that used multiple packages in tandem, so expectations about permission policy no longer held true. These issues have all been fixed, and the BroadcstTest suite passes both before and after the looper changes here. Bug: 177396523 Test: atest android.app.activity.BroadcastTest Test: atest CtsOsTestCases Change-Id: I95501aaa458793a258e429b540c7ed9e8e6ba355 --- core/tests/coretests/AndroidManifest.xml | 5 ++- .../android/app/activity/BroadcastTest.java | 39 +++++++++++++----- .../app/activity/LaunchpadActivity.java | 40 ++++++++++++------- .../app/activity/LocalDeniedReceiver.java | 2 +- .../app/activity/RemoteDeniedReceiver.java | 2 +- .../server/am/ActivityManagerService.java | 15 +++++-- 6 files changed, 71 insertions(+), 32 deletions(-) diff --git a/core/tests/coretests/AndroidManifest.xml b/core/tests/coretests/AndroidManifest.xml index bb826deb4eff4..151a320494b46 100644 --- a/core/tests/coretests/AndroidManifest.xml +++ b/core/tests/coretests/AndroidManifest.xml @@ -35,6 +35,8 @@ android:label="@string/permlab_testDenied" android:description="@string/permdesc_testDenied" /> + + @@ -76,7 +78,6 @@ - @@ -1455,6 +1456,7 @@ @@ -1462,6 +1464,7 @@ diff --git a/core/tests/coretests/src/android/app/activity/BroadcastTest.java b/core/tests/coretests/src/android/app/activity/BroadcastTest.java index 0f81896692c00..d79c2fe19ce28 100644 --- a/core/tests/coretests/src/android/app/activity/BroadcastTest.java +++ b/core/tests/coretests/src/android/app/activity/BroadcastTest.java @@ -56,6 +56,8 @@ public class BroadcastTest extends ActivityTestsBase { "com.android.frameworks.coretests.activity.BROADCAST_MULTI"; public static final String BROADCAST_ABORT = "com.android.frameworks.coretests.activity.BROADCAST_ABORT"; + public static final String BROADCAST_RESULT = + "com.android.frameworks.coretests.activity.BROADCAST_RESULT"; public static final String BROADCAST_STICKY1 = "com.android.frameworks.coretests.activity.BROADCAST_STICKY1"; @@ -106,7 +108,14 @@ public class BroadcastTest extends ActivityTestsBase { } public Intent makeBroadcastIntent(String action) { + return makeBroadcastIntent(action, false); + } + + public Intent makeBroadcastIntent(String action, boolean makeImplicit) { Intent intent = new Intent(action, null); + if (makeImplicit) { + intent.addFlags(intent.FLAG_RECEIVER_INCLUDE_BACKGROUND); + } intent.putExtra("caller", mCallTarget); return intent; } @@ -277,7 +286,7 @@ public class BroadcastTest extends ActivityTestsBase { map.putString("foo", "you"); map.putString("remove", "me"); getContext().sendOrderedBroadcast( - new Intent("com.android.frameworks.coretests.activity.BROADCAST_RESULT"), + makeBroadcastIntent(BROADCAST_RESULT, true), null, broadcastReceiver, null, 1, "foo", map); while (!broadcastReceiver.mHaveResult) { try { @@ -424,10 +433,13 @@ public class BroadcastTest extends ActivityTestsBase { public void testLocalReceivePermissionGranted() throws Exception { setExpectedReceivers(new String[]{RECEIVER_LOCAL}); - getContext().sendBroadcast(makeBroadcastIntent(BROADCAST_LOCAL_GRANTED)); + getContext().sendBroadcast(makeBroadcastIntent(BROADCAST_LOCAL_GRANTED, true)); waitForResultOrThrow(BROADCAST_TIMEOUT); } + /* + // TODO: multi-package test b/c self-target broadcasts are always allowed + // even when gated on ungranted permissions public void testLocalReceivePermissionDenied() throws Exception { setExpectedReceivers(new String[]{RECEIVER_RESULTS}); @@ -438,16 +450,17 @@ public class BroadcastTest extends ActivityTestsBase { }; getContext().sendOrderedBroadcast( - makeBroadcastIntent(BROADCAST_LOCAL_DENIED), + makeBroadcastIntent(BROADCAST_LOCAL_DENIED, true), null, finish, null, Activity.RESULT_CANCELED, null, null); waitForResultOrThrow(BROADCAST_TIMEOUT); } + */ public void testLocalBroadcastPermissionGranted() throws Exception { setExpectedReceivers(new String[]{RECEIVER_LOCAL}); getContext().sendBroadcast( - makeBroadcastIntent(BROADCAST_LOCAL), + makeBroadcastIntent(BROADCAST_LOCAL, true), PERMISSION_GRANTED); waitForResultOrThrow(BROADCAST_TIMEOUT); } @@ -462,7 +475,7 @@ public class BroadcastTest extends ActivityTestsBase { }; getContext().sendOrderedBroadcast( - makeBroadcastIntent(BROADCAST_LOCAL), + makeBroadcastIntent(BROADCAST_LOCAL, true), PERMISSION_DENIED, finish, null, Activity.RESULT_CANCELED, null, null); waitForResultOrThrow(BROADCAST_TIMEOUT); @@ -470,10 +483,13 @@ public class BroadcastTest extends ActivityTestsBase { public void testRemoteReceivePermissionGranted() throws Exception { setExpectedReceivers(new String[]{RECEIVER_REMOTE}); - getContext().sendBroadcast(makeBroadcastIntent(BROADCAST_REMOTE_GRANTED)); + getContext().sendBroadcast(makeBroadcastIntent(BROADCAST_REMOTE_GRANTED, true)); waitForResultOrThrow(BROADCAST_TIMEOUT); } + /* + // TODO: multi-package test b/c self-target broadcasts are always allowed + // even when gated on ungranted permissions public void testRemoteReceivePermissionDenied() throws Exception { setExpectedReceivers(new String[]{RECEIVER_RESULTS}); @@ -484,16 +500,17 @@ public class BroadcastTest extends ActivityTestsBase { }; getContext().sendOrderedBroadcast( - makeBroadcastIntent(BROADCAST_REMOTE_DENIED), + makeBroadcastIntent(BROADCAST_REMOTE_DENIED, true), null, finish, null, Activity.RESULT_CANCELED, null, null); waitForResultOrThrow(BROADCAST_TIMEOUT); } + */ public void testRemoteBroadcastPermissionGranted() throws Exception { setExpectedReceivers(new String[]{RECEIVER_REMOTE}); getContext().sendBroadcast( - makeBroadcastIntent(BROADCAST_REMOTE), + makeBroadcastIntent(BROADCAST_REMOTE, true), PERMISSION_GRANTED); waitForResultOrThrow(BROADCAST_TIMEOUT); } @@ -508,7 +525,7 @@ public class BroadcastTest extends ActivityTestsBase { }; getContext().sendOrderedBroadcast( - makeBroadcastIntent(BROADCAST_REMOTE), + makeBroadcastIntent(BROADCAST_REMOTE, true), PERMISSION_DENIED, finish, null, Activity.RESULT_CANCELED, null, null); waitForResultOrThrow(BROADCAST_TIMEOUT); @@ -516,13 +533,13 @@ public class BroadcastTest extends ActivityTestsBase { public void testReceiverCanNotRegister() throws Exception { setExpectedReceivers(new String[]{RECEIVER_LOCAL}); - getContext().sendBroadcast(makeBroadcastIntent(BROADCAST_FAIL_REGISTER)); + getContext().sendBroadcast(makeBroadcastIntent(BROADCAST_FAIL_REGISTER, true)); waitForResultOrThrow(BROADCAST_TIMEOUT); } public void testReceiverCanNotBind() throws Exception { setExpectedReceivers(new String[]{RECEIVER_LOCAL}); - getContext().sendBroadcast(makeBroadcastIntent(BROADCAST_FAIL_BIND)); + getContext().sendBroadcast(makeBroadcastIntent(BROADCAST_FAIL_BIND, true)); waitForResultOrThrow(BROADCAST_TIMEOUT); } diff --git a/core/tests/coretests/src/android/app/activity/LaunchpadActivity.java b/core/tests/coretests/src/android/app/activity/LaunchpadActivity.java index 766245600d13b..0b21fa90c1c4d 100644 --- a/core/tests/coretests/src/android/app/activity/LaunchpadActivity.java +++ b/core/tests/coretests/src/android/app/activity/LaunchpadActivity.java @@ -253,16 +253,16 @@ public class LaunchpadActivity extends Activity { sendBroadcast(makeBroadcastIntent(BROADCAST_REGISTERED)); } else if (BROADCAST_LOCAL.equals(action)) { setExpectedReceivers(new String[]{RECEIVER_LOCAL}); - sendBroadcast(makeBroadcastIntent(BROADCAST_LOCAL)); + sendBroadcast(makeBroadcastIntent(BROADCAST_LOCAL, true)); } else if (BROADCAST_REMOTE.equals(action)) { setExpectedReceivers(new String[]{RECEIVER_REMOTE}); - sendBroadcast(makeBroadcastIntent(BROADCAST_REMOTE)); + sendBroadcast(makeBroadcastIntent(BROADCAST_REMOTE, true)); } else if (BROADCAST_ALL.equals(action)) { setExpectedReceivers(new String[]{ RECEIVER_REMOTE, RECEIVER_REG, RECEIVER_LOCAL}); registerMyReceiver(new IntentFilter(BROADCAST_ALL)); sCallingTest.addIntermediate("after-register"); - sendOrderedBroadcast(makeBroadcastIntent(BROADCAST_ALL), null); + sendOrderedBroadcast(makeBroadcastIntent(BROADCAST_ALL, true), null); } else if (BROADCAST_MULTI.equals(action)) { setExpectedReceivers(new String[]{ RECEIVER_REMOTE, RECEIVER_REG, RECEIVER_LOCAL, @@ -277,23 +277,26 @@ public class LaunchpadActivity extends Activity { RECEIVER_REMOTE, RECEIVER_LOCAL}); registerMyReceiver(new IntentFilter(BROADCAST_ALL)); sCallingTest.addIntermediate("after-register"); - sendOrderedBroadcast(makeBroadcastIntent(BROADCAST_ALL), null); - sendOrderedBroadcast(makeBroadcastIntent(BROADCAST_ALL), null); - sendOrderedBroadcast(makeBroadcastIntent(BROADCAST_ALL), null); - sendOrderedBroadcast(makeBroadcastIntent(BROADCAST_LOCAL), null); - sendOrderedBroadcast(makeBroadcastIntent(BROADCAST_REMOTE), null); - sendOrderedBroadcast(makeBroadcastIntent(BROADCAST_LOCAL), null); - sendOrderedBroadcast(makeBroadcastIntent(BROADCAST_REMOTE), null); - sendOrderedBroadcast(makeBroadcastIntent(BROADCAST_ALL), null); - sendOrderedBroadcast(makeBroadcastIntent(BROADCAST_ALL), null); - sendOrderedBroadcast(makeBroadcastIntent(BROADCAST_ALL), null); - sendOrderedBroadcast(makeBroadcastIntent(BROADCAST_REPEAT), null); + final Intent allIntent = makeBroadcastIntent(BROADCAST_ALL, true); + final Intent localIntent = makeBroadcastIntent(BROADCAST_LOCAL, true); + final Intent remoteIntent = makeBroadcastIntent(BROADCAST_REMOTE, true); + sendOrderedBroadcast(allIntent, null); + sendOrderedBroadcast(allIntent, null); + sendOrderedBroadcast(allIntent, null); + sendOrderedBroadcast(localIntent, null); + sendOrderedBroadcast(remoteIntent, null); + sendOrderedBroadcast(localIntent, null); + sendOrderedBroadcast(remoteIntent, null); + sendOrderedBroadcast(allIntent, null); + sendOrderedBroadcast(allIntent, null); + sendOrderedBroadcast(allIntent, null); + sendOrderedBroadcast(makeBroadcastIntent(BROADCAST_REPEAT, true), null); } else if (BROADCAST_ABORT.equals(action)) { setExpectedReceivers(new String[]{ RECEIVER_REMOTE, RECEIVER_ABORT}); registerMyReceiver(new IntentFilter(BROADCAST_ABORT)); sCallingTest.addIntermediate("after-register"); - sendOrderedBroadcast(makeBroadcastIntent(BROADCAST_ABORT), null); + sendOrderedBroadcast(makeBroadcastIntent(BROADCAST_ABORT, true), null); } else if (BROADCAST_STICKY1.equals(action)) { setExpectedReceivers(new String[]{RECEIVER_REG}); setExpectedData(new String[]{DATA_1}); @@ -436,7 +439,14 @@ public class LaunchpadActivity extends Activity { } private Intent makeBroadcastIntent(String action) { + return makeBroadcastIntent(action, false); + } + + private Intent makeBroadcastIntent(String action, boolean makeImplicit) { Intent intent = new Intent(action, null); + if (makeImplicit) { + intent.addFlags(Intent.FLAG_RECEIVER_INCLUDE_BACKGROUND); + } intent.putExtra("caller", mCallTarget); return intent; } diff --git a/core/tests/coretests/src/android/app/activity/LocalDeniedReceiver.java b/core/tests/coretests/src/android/app/activity/LocalDeniedReceiver.java index 2120a1db463c0..16ea73f309376 100644 --- a/core/tests/coretests/src/android/app/activity/LocalDeniedReceiver.java +++ b/core/tests/coretests/src/android/app/activity/LocalDeniedReceiver.java @@ -23,7 +23,7 @@ import android.os.RemoteException; import android.os.IBinder; import android.os.Parcel; -class LocalDeniedReceiver extends BroadcastReceiver { +public class LocalDeniedReceiver extends BroadcastReceiver { public LocalDeniedReceiver() { } diff --git a/core/tests/coretests/src/android/app/activity/RemoteDeniedReceiver.java b/core/tests/coretests/src/android/app/activity/RemoteDeniedReceiver.java index 7c89346e820df..5c1ded93e1c34 100644 --- a/core/tests/coretests/src/android/app/activity/RemoteDeniedReceiver.java +++ b/core/tests/coretests/src/android/app/activity/RemoteDeniedReceiver.java @@ -23,7 +23,7 @@ import android.os.RemoteException; import android.os.IBinder; import android.os.Parcel; -class RemoteDeniedReceiver extends BroadcastReceiver { +public class RemoteDeniedReceiver extends BroadcastReceiver { public RemoteDeniedReceiver() { } diff --git a/services/core/java/com/android/server/am/ActivityManagerService.java b/services/core/java/com/android/server/am/ActivityManagerService.java index 05f2fa096e4d0..1396654e37c83 100644 --- a/services/core/java/com/android/server/am/ActivityManagerService.java +++ b/services/core/java/com/android/server/am/ActivityManagerService.java @@ -237,6 +237,7 @@ import android.os.DropBoxManager; import android.os.FactoryTest; import android.os.FileUtils; import android.os.Handler; +import android.os.HandlerThread; import android.os.IBinder; import android.os.IDeviceIdentifiersPolicyService; import android.os.IPermissionController; @@ -2089,11 +2090,19 @@ public class ActivityManagerService extends IActivityManager.Stub mEnableOffloadQueue = SystemProperties.getBoolean( "persist.device_config.activity_manager_native_boot.offload_queue_enabled", false); - mFgBroadcastQueue = new BroadcastQueue(this, mHandler, + // Decouple broadcast-related timing operations from other OS activity by + // using a dedicated thread. Sharing this thread between queues is safe + // because we know the nature of the activity on it and can't stall + // unexpectedly. + HandlerThread broadcastThread = new HandlerThread("broadcast"); + broadcastThread.start(); + Handler broadcastHandler = broadcastThread.getThreadHandler(); + + mFgBroadcastQueue = new BroadcastQueue(this, broadcastHandler, "foreground", foreConstants, false); - mBgBroadcastQueue = new BroadcastQueue(this, mHandler, + mBgBroadcastQueue = new BroadcastQueue(this, broadcastHandler, "background", backConstants, true); - mOffloadBroadcastQueue = new BroadcastQueue(this, mHandler, + mOffloadBroadcastQueue = new BroadcastQueue(this, broadcastHandler, "offload", offloadConstants, true); mBroadcastQueues[0] = mFgBroadcastQueue; mBroadcastQueues[1] = mBgBroadcastQueue;