From 1bcbb88955ab48797c50d705fa263debb2f22461 Mon Sep 17 00:00:00 2001 From: "Chaitanya Cheemala (xWF)" Date: Tue, 8 Jul 2025 03:54:19 -0700 Subject: [PATCH] Revert "Fix onActiveDataSubscriptionIdChanged is not received in multi SIM mode" This reverts commit aecb2b65bacfa7f4620c2db188ab26e600d36acf. Reason for revert: Likely culprit for b/430219293 - verifying through ABTD before revert submission. This is part of the standard investigation process, and does not mean your CL will be reverted. Bug: 430219293 Change-Id: I9b786abf784de8c34b9f517236305330b79b227e --- .../MultinetworkPolicyTracker.java | 60 ++----------------- .../server/ConnectivityServiceTest.java | 5 -- .../MultinetworkPolicyTrackerTest.kt | 8 +-- .../server/connectivityservice/base/CSTest.kt | 5 -- 4 files changed, 6 insertions(+), 72 deletions(-) diff --git a/service/src/com/android/server/connectivity/MultinetworkPolicyTracker.java b/service/src/com/android/server/connectivity/MultinetworkPolicyTracker.java index a614cc4008..cd17bbde26 100644 --- a/service/src/com/android/server/connectivity/MultinetworkPolicyTracker.java +++ b/service/src/com/android/server/connectivity/MultinetworkPolicyTracker.java @@ -114,7 +114,6 @@ public class MultinetworkPolicyTracker { private final Context mContext; private final ConnectivityResources mResources; private final Handler mHandler; - private final HandlerExecutor mExecutor; private final Runnable mAvoidBadWifiCallback; private final List mSettingsUris; private final ContentResolver mResolver; @@ -129,8 +128,6 @@ public class MultinetworkPolicyTracker { private volatile int mMeteredMultipathPreference; private int mActiveSubId = SubscriptionManager.INVALID_SUBSCRIPTION_ID; private volatile long mTestAllowBadWifiUntilMs = 0; - // This field should only be accessed on the handler thread - private TelephonyCallback mTelephonyCallback; /** * Dependencies for testing @@ -183,36 +180,6 @@ public class MultinetworkPolicyTracker { } private final Dependencies mDeps; - private void unregisterTelephonyCallback( - int subId, @NonNull final TelephonyCallback telephonyCallback - ) { - final TelephonyManager telephonyManager = mContext.getSystemService(TelephonyManager.class); - if (null == telephonyManager) return; - - final TelephonyManager telephonyManagerForSubId = - telephonyManager.createForSubscriptionId(subId); - - if (null == telephonyManagerForSubId) return; - - Log.d(TAG, "unregisterTelephonyCallback for subId:" + subId); - telephonyManagerForSubId.unregisterTelephonyCallback(telephonyCallback); - } - - private void registerTelephonyCallback( - int subId, @NonNull final TelephonyCallback telephonyCallback - ) { - final TelephonyManager telephonyManager = mContext.getSystemService(TelephonyManager.class); - if (null == telephonyManager) return; - - final TelephonyManager telephonyManagerForSubId = - telephonyManager.createForSubscriptionId(subId); - - if (null == telephonyManagerForSubId) return; - - Log.d(TAG, "registerTelephonyCallback for subId:" + subId); - telephonyManagerForSubId.registerTelephonyCallback(mExecutor, telephonyCallback); - } - /** * Whether to prefer bad wifi to a network that yields to bad wifis, even if it never validated * @@ -256,22 +223,7 @@ public class MultinetworkPolicyTracker { implements TelephonyCallback.ActiveDataSubscriptionIdListener { @Override public void onActiveDataSubscriptionIdChanged(final int subId) { - if (mActiveSubId == subId) return; - - if (subId == SubscriptionManager.INVALID_SUBSCRIPTION_ID) return; - - Log.d(TAG, "onActiveDataSubscriptionIdChanged from " + mActiveSubId - + " to " + subId); - // register new subId callback first to prevent subscription changes in between - final int oldSubId = mActiveSubId; - final TelephonyCallback oldCallback = mTelephonyCallback; - - mTelephonyCallback = - new MultinetworkPolicyTracker.ActiveDataSubscriptionIdListener(); - registerTelephonyCallback(subId, mTelephonyCallback); // new mActiveSubId = subId; - - unregisterTelephonyCallback(oldSubId, oldCallback); // old reevaluateInternal(); } } @@ -285,7 +237,6 @@ public class MultinetworkPolicyTracker { mContext = ctx; mResources = new ConnectivityResources(ctx); mHandler = handler; - mExecutor = new HandlerExecutor(mHandler); mAvoidBadWifiCallback = avoidBadWifiCallback; mDeps = deps; mSettingsUris = Arrays.asList( @@ -329,20 +280,19 @@ public class MultinetworkPolicyTracker { mContext.registerReceiverForAllUsers(mBroadcastReceiver, intentFilter, null /* broadcastPermission */, mHandler); - mHandler.post(() -> { - mTelephonyCallback = new ActiveDataSubscriptionIdListener(); - registerTelephonyCallback(mActiveSubId, mTelephonyCallback); - }); + final Executor handlerExecutor = new HandlerExecutor(mHandler); + mContext.getSystemService(TelephonyManager.class).registerTelephonyCallback( + handlerExecutor, new ActiveDataSubscriptionIdListener()); if (mCarrierConfigManager != null) { mCarrierConfigManager.registerCarrierConfigChangeListener( - mExecutor, mCarrierConfigChangeListener + handlerExecutor, mCarrierConfigChangeListener ); // This ensures the latest carrier configuration is read. mHandler.post(() -> reevaluateInternal()); } else { - mDeps.addOnDevicePropertiesChangedListener(mExecutor, + mDeps.addOnDevicePropertiesChangedListener(handlerExecutor, properties -> reevaluateInternal()); } diff --git a/tests/unit/java/com/android/server/ConnectivityServiceTest.java b/tests/unit/java/com/android/server/ConnectivityServiceTest.java index 4783d28aac..08be2ecf03 100644 --- a/tests/unit/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/unit/java/com/android/server/ConnectivityServiceTest.java @@ -1899,11 +1899,6 @@ public class ConnectivityServiceTest { mockHasSystemFeature(FEATURE_WIFI_DIRECT, true); mockHasSystemFeature(FEATURE_ETHERNET, true); doReturn(true).when(mTelephonyManager).isDataCapable(); - // This will return the same object for all subscription IDs. This is - // fine for all tests at the time of this writing, but if the difference - // becomes important for all tests, then it may be necessary to create a - // new one per subId. See [CSTest#createContextAsUser] above for a model. - doReturn(mTelephonyManager).when(mTelephonyManager).createForSubscriptionId(anyInt()); FakeSettingsProvider.clearSettingsProvider(); mServiceContext = new MockContext(InstrumentationRegistry.getContext(), diff --git a/tests/unit/java/com/android/server/connectivity/MultinetworkPolicyTrackerTest.kt b/tests/unit/java/com/android/server/connectivity/MultinetworkPolicyTrackerTest.kt index 372bd88c3e..75349cf1b2 100644 --- a/tests/unit/java/com/android/server/connectivity/MultinetworkPolicyTrackerTest.kt +++ b/tests/unit/java/com/android/server/connectivity/MultinetworkPolicyTrackerTest.kt @@ -107,13 +107,7 @@ class MultinetworkPolicyTrackerTest(private val supportCarrierConfigManager: Boo doReturn(0).`when`(it).getInteger(R.integer.config_activelyPreferBadWifi) } - private val telephonyManager = mock(TelephonyManager::class.java).also { - // This will return the same object for all subscription IDs. This is - // fine for all tests at the time of this writing, but if the difference - // becomes important for all tests, then it may be necessary to create a - // new one per subId. See [CSTest#createContextAsUser] above for a model. - doReturn(it).`when`(it).createForSubscriptionId(anyInt()) - } + private val telephonyManager = mock(TelephonyManager::class.java) private val subscriptionManager = mock(SubscriptionManager::class.java).also { doReturn(null).`when`(it).getActiveSubscriptionInfo(anyInt()) } diff --git a/tests/unit/java/com/android/server/connectivityservice/base/CSTest.kt b/tests/unit/java/com/android/server/connectivityservice/base/CSTest.kt index 82cba718fd..1a97e3a9c0 100644 --- a/tests/unit/java/com/android/server/connectivityservice/base/CSTest.kt +++ b/tests/unit/java/com/android/server/connectivityservice/base/CSTest.kt @@ -229,11 +229,6 @@ open class CSTest { val appOpsManager = mock() val telephonyManager = mock().also { doReturn(true).`when`(it).isDataCapable() - // This will return the same object for all subscription IDs. This is - // fine for all tests at the time of this writing, but if the difference - // becomes important for all tests, then it may be necessary to create a - // new one per subId. See [createContextAsUser] above for a model. - doReturn(it).`when`(it).createForSubscriptionId(any()) } val subscriptionManager = mock() val bluetoothManager = mock()