From 9bd526d7d41ee2d8ff7cdd247b914df5df6710ee Mon Sep 17 00:00:00 2001 From: Quang Anh Luong Date: Mon, 14 Apr 2025 16:33:02 +0900 Subject: [PATCH] Fix carrier privilege race condition CarrierConfigRule cleanup does not wait for carrier privilege value to change before continuing to the next test, which may cause test failures if the value isn't fully reset on time. Make sure the value has time to properly be reset by waiting for onCarrierPrivilegesChanged before proceeding. Flag: EXEMPT test only change. Bug: 409710349 Test: atest TetheringManagerTest#testCarrierPrivilegedIsTetheringSupported TetheringManagerTest#testCarrierPrivilegedStopTetheringWifiWithConfigSucceeds Change-Id: If67884514a2c1678834ab9ff67947e75e37db0a7 --- .../android/testutils/CarrierConfigRule.kt | 70 +++++++++++++++---- 1 file changed, 55 insertions(+), 15 deletions(-) diff --git a/staticlibs/testutils/devicetests/com/android/testutils/CarrierConfigRule.kt b/staticlibs/testutils/devicetests/com/android/testutils/CarrierConfigRule.kt index 0d70d24e3d..d9803999ba 100644 --- a/staticlibs/testutils/devicetests/com/android/testutils/CarrierConfigRule.kt +++ b/staticlibs/testutils/devicetests/com/android/testutils/CarrierConfigRule.kt @@ -36,11 +36,11 @@ import android.telephony.TelephonyManager.CarrierPrivilegesCallback import android.util.Log import androidx.test.platform.app.InstrumentationRegistry import com.android.modules.utils.build.SdkLevel +import com.android.net.module.util.ArrayTrackRecord import com.android.testutils.runAsShell import com.android.testutils.runCommandInShell import com.android.testutils.tryTest import java.security.MessageDigest -import kotlin.test.assertEquals import kotlin.test.assertNotNull import kotlin.test.assertTrue import org.junit.rules.TestRule @@ -66,6 +66,9 @@ class CarrierConfigRule : TestRule { // Map of (subId) -> (original values of overridden settings) private val originalConfigs = mutableMapOf() + // Map of (subId) -> (original values of carrier privilege) + private val originalCarrierPrivileges = mutableMapOf() + // Map of (subId) -> (original values of carrier service package) private val originalCarrierServicePackages = mutableMapOf() @@ -176,8 +179,7 @@ class CarrierConfigRule : TestRule { val tm = context.getSystemService(TelephonyManager::class.java)!! - val cv = ConditionVariable() - val cpb = PrivilegeWaiterCallback(cv) + val cpb = PrivilegeWaiterCallback() // The lambda below is capturing |cpb|, whose type inherits from a class that appeared in // T. This means the lambda will compile as a private method of this class taking a // PrivilegeWaiterCallback argument. As JUnit uses reflection to enumerate all methods @@ -189,10 +191,8 @@ class CarrierConfigRule : TestRule { runAsShell(READ_PRIVILEGED_PHONE_STATE) @JvmSerializableLambda { tm.registerCarrierPrivilegesCallback(slotIndex, { it.run() }, cpb) } - // Wait for the callback to be registered - assertTrue(cv.block(CARRIER_CONFIG_CHANGE_TIMEOUT_MS), - "Can't register CarrierPrivilegesCallback") - if (cpb.hasPrivilege == hold) { + val currentPrivilege = cpb.expectCarrierPrivilegesChanged() + if (currentPrivilege == hold) { if (hold) { Log.w(TAG, "Package ${context.opPackageName} already is privileged") } else { @@ -200,7 +200,9 @@ class CarrierConfigRule : TestRule { } return@tryTest } - cv.close() + if (!originalCarrierPrivileges.contains(subId)) { + originalCarrierPrivileges.put(subId, currentPrivilege) + } if (hold) { addConfigOverrides(subId, PersistableBundle().also { it.putStringArray(CarrierConfigManager.KEY_CARRIER_CERTIFICATE_STRING_ARRAY, @@ -209,9 +211,7 @@ class CarrierConfigRule : TestRule { } else { cleanUpNow() } - assertTrue(cv.block(CARRIER_CONFIG_CHANGE_TIMEOUT_MS), - "Timed out waiting for CarrierPrivilegesCallback") - assertEquals(cpb.hasPrivilege, hold, "Couldn't set carrier privilege") + cpb.eventuallyExpectCarrierPrivilegesChanged(hold) } cleanup @JvmSerializableLambda { runAsShell(READ_PRIVILEGED_PHONE_STATE) @JvmSerializableLambda { tm.unregisterCarrierPrivilegesCallback(cpb) @@ -301,12 +301,25 @@ class CarrierConfigRule : TestRule { } } - private class PrivilegeWaiterCallback(private val cv: ConditionVariable) : + private class PrivilegeWaiterCallback() : CarrierPrivilegesCallback { - var hasPrivilege = false + private val history = ArrayTrackRecord().ReadHead() override fun onCarrierPrivilegesChanged(p: MutableSet, uids: MutableSet) { - hasPrivilege = uids.contains(Process.myUid()) - cv.open() + history.add(uids.contains(Process.myUid())) + } + + fun expectCarrierPrivilegesChanged(): Boolean { + val result = history.poll(CARRIER_CONFIG_CHANGE_TIMEOUT_MS) + assertNotNull(result, "onCarrierPrivilegesChanged not received!") + return result + } + + fun eventuallyExpectCarrierPrivilegesChanged(privileged: Boolean) { + val result = history.poll( + CARRIER_CONFIG_CHANGE_TIMEOUT_MS, + { it == privileged } + ) + assertNotNull(result, "Carrier privilege did not change to $privileged!") } } @@ -340,6 +353,33 @@ class CarrierConfigRule : TestRule { } originalConfigs.clear() } + + // Carrier privilege status may not be reset yet even after receiving + // ACTION_CARRIER_CONFIG_CHANGED, so make sure to also wait for + // CarrierPrivilegesCallback before proceeding. + val tm = context.getSystemService(TelephonyManager::class.java)!! + originalCarrierPrivileges.forEach { (subId, privileged) -> + val cpb = PrivilegeWaiterCallback() + // The lambda below is capturing |cpb|, whose type inherits from a class that appeared in + // T. This means the lambda will compile as a private method of this class taking a + // PrivilegeWaiterCallback argument. As JUnit uses reflection to enumerate all methods + // including private methods, this would fail with a link error when running on S-. + // To solve this, make the lambda serializable, which causes the compiler to emit a + // synthetic class instead of a synthetic method. + tryTest @JvmSerializableLambda { + val slotIndex = SubscriptionManager.getSlotIndex(subId)!! + runAsShell(READ_PRIVILEGED_PHONE_STATE) @JvmSerializableLambda { + tm.registerCarrierPrivilegesCallback(slotIndex, { it.run() }, cpb) + } + cpb.eventuallyExpectCarrierPrivilegesChanged(privileged) + } cleanup @JvmSerializableLambda { + runAsShell(READ_PRIVILEGED_PHONE_STATE) @JvmSerializableLambda { + tm.unregisterCarrierPrivilegesCallback(cpb) + } + } + originalCarrierPrivileges.clear() + } + originalCarrierServicePackages.forEach { (subId, pkg) -> setCarrierServicePackageOverride(subId, pkg) }