Make unregister receiver "sync".

Before this CL, unregistering a receiver was done asynchronously in
BroadcastDispatcher. This meant that after a call to unregisterReceiver,
there was a period of time in which broadcasts may still be dispatched
for that receiver.

After this change, we mark receivers as pending removal, blocking any
received broadcast to be dispatched while its being removed.

Also, remove the double posting to the same handler thread. Instead, we
use the fact that we know that UserBroadcastDispatcher will only be
called from BroadcastDispatcher in the background thread to prevent
re-posting.

Fixes: 193941146
Test: atest SystemUITests
Test: everything seems to be working fine
Change-Id: I345f4d7ad918381188d553bd2b89643d11ae3d6c
This commit is contained in:
Fabian Kozynski
2022-04-08 13:57:06 -04:00
parent 3fda2bf279
commit 8745729592
9 changed files with 309 additions and 30 deletions

View File

@@ -39,6 +39,13 @@ import java.util.concurrent.atomic.AtomicInteger
*
* This class has no sync controls, so make sure to only make modifications from the background
* thread.
*
* This class takes the following actions:
* * [registerAction]: action to register this receiver (with the proper filter) with [Context].
* * [unregisterAction]: action to unregister this receiver with [Context].
* * [testPendingRemovalAction]: action to check if a particular [BroadcastReceiver] registered
* with [BroadcastDispatcher] has been unregistered and is pending removal. See
* [PendingRemovalStore].
*/
class ActionReceiver(
private val action: String,
@@ -46,7 +53,8 @@ class ActionReceiver(
private val registerAction: BroadcastReceiver.(IntentFilter) -> Unit,
private val unregisterAction: BroadcastReceiver.() -> Unit,
private val bgExecutor: Executor,
private val logger: BroadcastDispatcherLogger
private val logger: BroadcastDispatcherLogger,
private val testPendingRemovalAction: (BroadcastReceiver, Int) -> Boolean
) : BroadcastReceiver(), Dumpable {
companion object {
@@ -106,7 +114,8 @@ class ActionReceiver(
// Immediately return control to ActivityManager
bgExecutor.execute {
receiverDatas.forEach {
if (it.filter.matchCategories(intent.categories) == null) {
if (it.filter.matchCategories(intent.categories) == null &&
!testPendingRemovalAction(it.receiver, userId)) {
it.executor.execute {
it.receiver.pendingResult = pendingResult
it.receiver.onReceive(context, intent)

View File

@@ -63,13 +63,14 @@ private const val DEBUG = true
* Broadcast handling may be asynchronous *without* calling goAsync(), as it's running within sysui
* and doesn't need to worry about being killed.
*/
open class BroadcastDispatcher constructor (
open class BroadcastDispatcher @JvmOverloads constructor (
private val context: Context,
private val bgLooper: Looper,
private val bgExecutor: Executor,
private val dumpManager: DumpManager,
private val logger: BroadcastDispatcherLogger,
private val userTracker: UserTracker
private val userTracker: UserTracker,
private val removalPendingStore: PendingRemovalStore = PendingRemovalStore(logger)
) : Dumpable {
// Only modify in BG thread
@@ -167,6 +168,7 @@ open class BroadcastDispatcher constructor (
* @param receiver The receiver to unregister. It will be unregistered for all users.
*/
open fun unregisterReceiver(receiver: BroadcastReceiver) {
removalPendingStore.tagForRemoval(receiver, UserHandle.USER_ALL)
handler.obtainMessage(MSG_REMOVE_RECEIVER, receiver).sendToTarget()
}
@@ -177,13 +179,21 @@ open class BroadcastDispatcher constructor (
* @param user The user associated to the registered [receiver]. It can be [UserHandle.ALL].
*/
open fun unregisterReceiverForUser(receiver: BroadcastReceiver, user: UserHandle) {
removalPendingStore.tagForRemoval(receiver, user.identifier)
handler.obtainMessage(MSG_REMOVE_RECEIVER_FOR_USER, user.identifier, 0, receiver)
.sendToTarget()
}
@VisibleForTesting
protected open fun createUBRForUser(userId: Int) =
UserBroadcastDispatcher(context, userId, bgLooper, bgExecutor, logger)
UserBroadcastDispatcher(
context,
userId,
bgLooper,
bgExecutor,
logger,
removalPendingStore
)
override fun dump(pw: PrintWriter, args: Array<out String>) {
pw.println("Broadcast dispatcher:")
@@ -193,6 +203,8 @@ open class BroadcastDispatcher constructor (
ipw.println("User ${receiversByUser.keyAt(index)}")
receiversByUser.valueAt(index).dump(ipw, args)
}
ipw.println("Pending removal:")
removalPendingStore.dump(ipw, args)
ipw.decreaseIndent()
}
@@ -223,10 +235,20 @@ open class BroadcastDispatcher constructor (
for (it in 0 until receiversByUser.size()) {
receiversByUser.valueAt(it).unregisterReceiver(msg.obj as BroadcastReceiver)
}
removalPendingStore.clearPendingRemoval(
msg.obj as BroadcastReceiver,
UserHandle.USER_ALL
)
}
MSG_REMOVE_RECEIVER_FOR_USER -> {
receiversByUser.get(msg.arg1)?.unregisterReceiver(msg.obj as BroadcastReceiver)
val userId = if (msg.arg1 == UserHandle.USER_CURRENT) {
userTracker.userId
} else {
msg.arg1
}
receiversByUser.get(userId)?.unregisterReceiver(msg.obj as BroadcastReceiver)
removalPendingStore.clearPendingRemoval(msg.obj as BroadcastReceiver, userId)
}
else -> super.handleMessage(msg)
}

View File

@@ -0,0 +1,58 @@
package com.android.systemui.broadcast
import android.content.BroadcastReceiver
import android.os.UserHandle
import android.util.SparseSetArray
import androidx.annotation.GuardedBy
import com.android.systemui.Dumpable
import com.android.systemui.broadcast.logging.BroadcastDispatcherLogger
import com.android.systemui.util.indentIfPossible
import java.io.PrintWriter
/**
* Store information about requests for unregistering receivers from [BroadcastDispatcher], before
* they have been completely removed from the system.
*
* This helps make unregistering a receiver a *sync* operation.
*/
class PendingRemovalStore(
private val logger: BroadcastDispatcherLogger
) : Dumpable {
@GuardedBy("pendingRemoval")
private val pendingRemoval: SparseSetArray<BroadcastReceiver> = SparseSetArray()
fun tagForRemoval(broadcastReceiver: BroadcastReceiver, userId: Int) {
logger.logTagForRemoval(userId, broadcastReceiver)
synchronized(pendingRemoval) {
pendingRemoval.add(userId, broadcastReceiver)
}
}
fun isPendingRemoval(broadcastReceiver: BroadcastReceiver, userId: Int): Boolean {
return synchronized(pendingRemoval) {
pendingRemoval.contains(userId, broadcastReceiver) ||
pendingRemoval.contains(UserHandle.USER_ALL, broadcastReceiver)
}
}
fun clearPendingRemoval(broadcastReceiver: BroadcastReceiver, userId: Int) {
synchronized(pendingRemoval) {
pendingRemoval.remove(userId, broadcastReceiver)
}
logger.logClearedAfterRemoval(userId, broadcastReceiver)
}
override fun dump(pw: PrintWriter, args: Array<out String>) {
synchronized(pendingRemoval) {
pw.indentIfPossible {
val size = pendingRemoval.size()
for (i in 0 until size) {
val user = pendingRemoval.keyAt(i)
print(user)
print("->")
println(pendingRemoval.get(user))
}
}
}
}
}

View File

@@ -20,12 +20,12 @@ import android.content.BroadcastReceiver
import android.content.Context
import android.os.Handler
import android.os.Looper
import android.os.Message
import android.os.UserHandle
import android.util.ArrayMap
import android.util.ArraySet
import android.util.Log
import androidx.annotation.VisibleForTesting
import androidx.annotation.WorkerThread
import com.android.internal.util.Preconditions
import com.android.systemui.Dumpable
import com.android.systemui.broadcast.logging.BroadcastDispatcherLogger
@@ -34,8 +34,6 @@ import java.io.PrintWriter
import java.util.concurrent.Executor
import java.util.concurrent.atomic.AtomicInteger
private const val MSG_REGISTER_RECEIVER = 0
private const val MSG_UNREGISTER_RECEIVER = 1
private const val TAG = "UserBroadcastDispatcher"
private const val DEBUG = false
@@ -50,7 +48,8 @@ open class UserBroadcastDispatcher(
private val userId: Int,
private val bgLooper: Looper,
private val bgExecutor: Executor,
private val logger: BroadcastDispatcherLogger
private val logger: BroadcastDispatcherLogger,
private val removalPendingStore: PendingRemovalStore
) : Dumpable {
companion object {
@@ -60,16 +59,6 @@ open class UserBroadcastDispatcher(
val index = AtomicInteger(0)
}
private val bgHandler = object : Handler(bgLooper) {
override fun handleMessage(msg: Message) {
when (msg.what) {
MSG_REGISTER_RECEIVER -> handleRegisterReceiver(msg.obj as ReceiverData, msg.arg1)
MSG_UNREGISTER_RECEIVER -> handleUnregisterReceiver(msg.obj as BroadcastReceiver)
else -> Unit
}
}
}
// Used for key in actionsToActionsReceivers
internal data class ReceiverProperties(
val action: String,
@@ -77,6 +66,8 @@ open class UserBroadcastDispatcher(
val permission: String?
)
private val bgHandler = Handler(bgLooper)
// Only modify in BG thread
@VisibleForTesting
internal val actionsToActionsReceivers = ArrayMap<ReceiverProperties, ActionReceiver>()
@@ -92,19 +83,21 @@ open class UserBroadcastDispatcher(
/**
* Register a [ReceiverData] for this user.
*/
@WorkerThread
fun registerReceiver(receiverData: ReceiverData, flags: Int) {
bgHandler.obtainMessage(MSG_REGISTER_RECEIVER, flags, 0, receiverData).sendToTarget()
handleRegisterReceiver(receiverData, flags)
}
/**
* Unregister a given [BroadcastReceiver] for this user.
*/
@WorkerThread
fun unregisterReceiver(receiver: BroadcastReceiver) {
bgHandler.obtainMessage(MSG_UNREGISTER_RECEIVER, receiver).sendToTarget()
handleUnregisterReceiver(receiver)
}
private fun handleRegisterReceiver(receiverData: ReceiverData, flags: Int) {
Preconditions.checkState(bgHandler.looper.isCurrentThread,
Preconditions.checkState(bgLooper.isCurrentThread,
"This method should only be called from BG thread")
if (DEBUG) Log.w(TAG, "Register receiver: ${receiverData.receiver}")
receiverToActions
@@ -151,12 +144,13 @@ open class UserBroadcastDispatcher(
}
},
bgExecutor,
logger
logger,
removalPendingStore::isPendingRemoval
)
}
private fun handleUnregisterReceiver(receiver: BroadcastReceiver) {
Preconditions.checkState(bgHandler.looper.isCurrentThread,
Preconditions.checkState(bgLooper.isCurrentThread,
"This method should only be called from BG thread")
if (DEBUG) Log.w(TAG, "Unregister receiver: $receiver")
receiverToActions.getOrDefault(receiver, mutableSetOf()).forEach {

View File

@@ -87,6 +87,26 @@ class BroadcastDispatcherLogger @Inject constructor(
})
}
fun logTagForRemoval(user: Int, receiver: BroadcastReceiver) {
val receiverString = receiver.toString()
log(DEBUG, {
int1 = user
str1 = receiverString
}, {
"Receiver $str1 tagged for removal from user $int1"
})
}
fun logClearedAfterRemoval(user: Int, receiver: BroadcastReceiver) {
val receiverString = receiver.toString()
log(DEBUG, {
int1 = user
str1 = receiverString
}, {
"Receiver $str1 has been completely removed for user $int1"
})
}
fun logReceiverUnregistered(user: Int, receiver: BroadcastReceiver) {
val receiverString = receiver.toString()
log(INFO, {

View File

@@ -42,6 +42,7 @@ import org.mockito.ArgumentMatchers.anyString
import org.mockito.Captor
import org.mockito.Mock
import org.mockito.Mockito
import org.mockito.Mockito.`when`
import org.mockito.Mockito.mock
import org.mockito.Mockito.never
import org.mockito.Mockito.verify
@@ -81,6 +82,8 @@ class ActionReceiverTest : SysuiTestCase() {
@Mock
private lateinit var unregisterFunction: BroadcastReceiver.() -> Unit
@Mock
private lateinit var isPendingRemovalFunction: (BroadcastReceiver, Int) -> Boolean
@Mock
private lateinit var receiver1: BroadcastReceiver
@Mock
private lateinit var receiver2: BroadcastReceiver
@@ -98,13 +101,16 @@ class ActionReceiverTest : SysuiTestCase() {
MockitoAnnotations.initMocks(this)
executor = FakeExecutor(FakeSystemClock())
`when`(isPendingRemovalFunction(any(), anyInt())).thenReturn(false)
actionReceiver = ActionReceiver(
ACTION1,
USER.identifier,
registerFunction,
unregisterFunction,
executor,
logger
logger,
isPendingRemovalFunction
)
}
@@ -249,6 +255,20 @@ class ActionReceiverTest : SysuiTestCase() {
verify(logger).logBroadcastDispatched(anyInt(), eq(ACTION1), sameNotNull(receiver1))
}
@Test
fun testBroadcastNotDispatchingOnPendingRemoval() {
`when`(isPendingRemovalFunction(receiver1, USER.identifier)).thenReturn(true)
val receiverData = ReceiverData(receiver1, IntentFilter(ACTION1), directExecutor, USER)
actionReceiver.addReceiverData(receiverData)
val intent = Intent(ACTION1)
actionReceiver.onReceive(mContext, intent)
executor.runAllReady()
verify(receiver1, never()).onReceive(any(), eq(intent))
}
@Test(expected = IllegalStateException::class)
fun testBroadcastWithWrongAction_throwsException() {
actionReceiver.onReceive(mContext, Intent(ACTION2))

View File

@@ -41,6 +41,8 @@ import org.mockito.ArgumentCaptor
import org.mockito.Captor
import org.mockito.Mock
import org.mockito.Mockito.`when`
import org.mockito.Mockito.anyInt
import org.mockito.Mockito.inOrder
import org.mockito.Mockito.mock
import org.mockito.Mockito.never
import org.mockito.Mockito.verify
@@ -85,6 +87,8 @@ class BroadcastDispatcherTest : SysuiTestCase() {
private lateinit var logger: BroadcastDispatcherLogger
@Mock
private lateinit var userTracker: UserTracker
@Mock
private lateinit var removalPendingStore: PendingRemovalStore
private lateinit var executor: Executor
@@ -108,6 +112,7 @@ class BroadcastDispatcherTest : SysuiTestCase() {
mock(DumpManager::class.java),
logger,
userTracker,
removalPendingStore,
mapOf(0 to mockUBRUser0, 1 to mockUBRUser1))
// These should be valid filters
@@ -325,6 +330,57 @@ class BroadcastDispatcherTest : SysuiTestCase() {
broadcastDispatcher.registerReceiver(broadcastReceiver, testFilter)
}
@Test
fun testTaggedReceiverForRemovalImmediately_allUsers() {
broadcastDispatcher.unregisterReceiver(broadcastReceiver)
verify(removalPendingStore).tagForRemoval(broadcastReceiver, UserHandle.USER_ALL)
verify(removalPendingStore, never()).clearPendingRemoval(eq(broadcastReceiver), anyInt())
}
@Test
fun testTaggedReceiverForRemovalImmediately_singleUser() {
val user = 0
broadcastDispatcher.unregisterReceiverForUser(broadcastReceiver, UserHandle.of(user))
verify(removalPendingStore).tagForRemoval(broadcastReceiver, user)
verify(removalPendingStore, never()).clearPendingRemoval(eq(broadcastReceiver), anyInt())
}
@Test
fun testUnregisterReceiverClearsPendingRemovalAfterRemoving_allUsers() {
broadcastDispatcher.registerReceiver(broadcastReceiver, intentFilter, null, user0)
broadcastDispatcher.registerReceiver(broadcastReceiver, intentFilter, null, user1)
broadcastDispatcher.unregisterReceiver(broadcastReceiver)
testableLooper.processAllMessages()
val inOrderUser0 = inOrder(mockUBRUser0, removalPendingStore)
inOrderUser0.verify(mockUBRUser0).unregisterReceiver(broadcastReceiver)
inOrderUser0.verify(removalPendingStore)
.clearPendingRemoval(broadcastReceiver, UserHandle.USER_ALL)
val inOrderUser1 = inOrder(mockUBRUser1, removalPendingStore)
inOrderUser1.verify(mockUBRUser1).unregisterReceiver(broadcastReceiver)
inOrderUser1.verify(removalPendingStore)
.clearPendingRemoval(broadcastReceiver, UserHandle.USER_ALL)
}
@Test
fun testUnregisterReceiverclearPendingRemovalAfterRemoving_singleUser() {
broadcastDispatcher.registerReceiver(broadcastReceiver, intentFilter, null, user1)
broadcastDispatcher.unregisterReceiverForUser(broadcastReceiver, user1)
testableLooper.processAllMessages()
val inOrderUser1 = inOrder(mockUBRUser1, removalPendingStore)
inOrderUser1.verify(mockUBRUser1).unregisterReceiver(broadcastReceiver)
inOrderUser1.verify(removalPendingStore)
.clearPendingRemoval(broadcastReceiver, user1.identifier)
}
private fun setUserMock(mockContext: Context, user: UserHandle) {
`when`(mockContext.user).thenReturn(user)
`when`(mockContext.userId).thenReturn(user.identifier)
@@ -337,8 +393,17 @@ class BroadcastDispatcherTest : SysuiTestCase() {
dumpManager: DumpManager,
logger: BroadcastDispatcherLogger,
userTracker: UserTracker,
removalPendingStore: PendingRemovalStore,
var mockUBRMap: Map<Int, UserBroadcastDispatcher>
) : BroadcastDispatcher(context, bgLooper, executor, dumpManager, logger, userTracker) {
) : BroadcastDispatcher(
context,
bgLooper,
executor,
dumpManager,
logger,
userTracker,
removalPendingStore
) {
override fun createUBRForUser(userId: Int): UserBroadcastDispatcher {
return mockUBRMap.getOrDefault(userId, mock(UserBroadcastDispatcher::class.java))
}

View File

@@ -0,0 +1,81 @@
package com.android.systemui.broadcast
import android.content.BroadcastReceiver
import android.os.UserHandle
import android.testing.AndroidTestingRunner
import androidx.test.filters.SmallTest
import com.android.systemui.SysuiTestCase
import com.android.systemui.broadcast.logging.BroadcastDispatcherLogger
import com.google.common.truth.Truth.assertThat
import org.junit.Before
import org.junit.Test
import org.junit.runner.RunWith
import org.mockito.Mock
import org.mockito.Mockito.verify
import org.mockito.MockitoAnnotations
@RunWith(AndroidTestingRunner::class)
@SmallTest
class PendingRemovalStoreTest : SysuiTestCase() {
@Mock
private lateinit var logger: BroadcastDispatcherLogger
@Mock
private lateinit var receiverOne: BroadcastReceiver
@Mock
private lateinit var receiverTwo: BroadcastReceiver
private lateinit var store: PendingRemovalStore
@Before
fun setUp() {
MockitoAnnotations.initMocks(this)
store = PendingRemovalStore(logger)
}
@Test
fun testTagForRemoval_logged() {
val user = 10
store.tagForRemoval(receiverOne, 10)
verify(logger).logTagForRemoval(user, receiverOne)
}
@Test
fun testClearedPendingRemoval_logged() {
val user = UserHandle.USER_ALL
store.clearPendingRemoval(receiverOne, user)
verify(logger).logClearedAfterRemoval(user, receiverOne)
}
@Test
fun testTaggedReceiverMarkedAsPending_specificUser() {
val user = 10
store.tagForRemoval(receiverOne, user)
assertThat(store.isPendingRemoval(receiverOne, user)).isTrue()
assertThat(store.isPendingRemoval(receiverOne, user + 1)).isFalse()
assertThat(store.isPendingRemoval(receiverOne, UserHandle.USER_ALL)).isFalse()
}
@Test
fun testTaggedReceiverMarkedAsPending_allUsers() {
val user = 10
store.tagForRemoval(receiverOne, UserHandle.USER_ALL)
assertThat(store.isPendingRemoval(receiverOne, user)).isTrue()
assertThat(store.isPendingRemoval(receiverOne, user + 1)).isTrue()
assertThat(store.isPendingRemoval(receiverOne, UserHandle.USER_ALL)).isTrue()
}
@Test
fun testOnlyBlockCorrectReceiver() {
val user = 10
store.tagForRemoval(receiverOne, user)
assertThat(store.isPendingRemoval(receiverOne, user)).isTrue()
assertThat(store.isPendingRemoval(receiverTwo, user)).isFalse()
}
}

View File

@@ -68,6 +68,8 @@ class UserBroadcastDispatcherTest : SysuiTestCase() {
private lateinit var mockContext: Context
@Mock
private lateinit var logger: BroadcastDispatcherLogger
@Mock
private lateinit var removalPendingStore: PendingRemovalStore
private lateinit var testableLooper: TestableLooper
private lateinit var userBroadcastDispatcher: UserBroadcastDispatcher
@@ -84,7 +86,13 @@ class UserBroadcastDispatcherTest : SysuiTestCase() {
fakeExecutor = FakeExecutor(FakeSystemClock())
userBroadcastDispatcher = object : UserBroadcastDispatcher(
mockContext, USER_ID, testableLooper.looper, mock(Executor::class.java), logger) {
mockContext,
USER_ID,
testableLooper.looper,
mock(Executor::class.java),
logger,
removalPendingStore
) {
override fun createActionReceiver(
action: String,
permission: String?,
@@ -216,7 +224,8 @@ class UserBroadcastDispatcherTest : SysuiTestCase() {
USER_ID,
testableLooper.looper,
fakeExecutor,
logger
logger,
removalPendingStore
)
uBR.registerReceiver(
ReceiverData(
@@ -243,7 +252,8 @@ class UserBroadcastDispatcherTest : SysuiTestCase() {
USER_ID,
testableLooper.looper,
fakeExecutor,
logger
logger,
removalPendingStore
)
uBR.registerReceiver(
ReceiverData(