From ef3d71dfb4c5fb5b6473cd49b60572a5afbe3740 Mon Sep 17 00:00:00 2001 From: Evan Laird Date: Mon, 25 Apr 2022 13:18:10 -0400 Subject: [PATCH] Synchronize access to mCurrentState when updating WifiSignalController expects broadcast updates to come in off the main thread, and WifiStatusTracker updates to come in on the main thread. This change ensures data access synchronization by making sure that all data access happens on a known background thread. Bug: 229425925 Test: atest NetworkControllerWifiTest Change-Id: Ifc04b2e25b0cc08ad6406030fa9de3df0ba2df2d --- .../connectivity/NetworkControllerImpl.java | 16 ++--- .../connectivity/SignalController.java | 4 +- .../connectivity/WifiSignalController.java | 63 +++++++++++-------- .../connectivity/WifiStatusTrackerFactory.kt | 50 +++++++++++++++ .../NetworkControllerBaseTest.java | 6 +- .../NetworkControllerDataTest.java | 6 +- .../NetworkControllerSignalTest.java | 13 ++-- 7 files changed, 116 insertions(+), 42 deletions(-) create mode 100644 packages/SystemUI/src/com/android/systemui/statusbar/connectivity/WifiStatusTrackerFactory.kt diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/connectivity/NetworkControllerImpl.java b/packages/SystemUI/src/com/android/systemui/statusbar/connectivity/NetworkControllerImpl.java index dea429f6c617c..d49e1e6acc235 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/connectivity/NetworkControllerImpl.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/connectivity/NetworkControllerImpl.java @@ -33,7 +33,6 @@ import android.net.ConnectivityManager; import android.net.ConnectivityManager.NetworkCallback; import android.net.Network; import android.net.NetworkCapabilities; -import android.net.NetworkScoreManager; import android.net.wifi.ScanResult; import android.net.wifi.WifiManager; import android.os.AsyncTask; @@ -225,10 +224,10 @@ public class NetworkControllerImpl extends BroadcastReceiver TelephonyManager telephonyManager, TelephonyListenerManager telephonyListenerManager, @Nullable WifiManager wifiManager, - NetworkScoreManager networkScoreManager, AccessPointControllerImpl accessPointController, DemoModeController demoModeController, CarrierConfigTracker carrierConfigTracker, + WifiStatusTrackerFactory trackerFactory, @Main Handler handler, InternetDialogFactory internetDialogFactory, FeatureFlags featureFlags, @@ -237,7 +236,6 @@ public class NetworkControllerImpl extends BroadcastReceiver telephonyManager, telephonyListenerManager, wifiManager, - networkScoreManager, subscriptionManager, Config.readConfig(context), bgLooper, @@ -250,6 +248,7 @@ public class NetworkControllerImpl extends BroadcastReceiver broadcastDispatcher, demoModeController, carrierConfigTracker, + trackerFactory, handler, featureFlags, dumpManager); @@ -262,8 +261,9 @@ public class NetworkControllerImpl extends BroadcastReceiver TelephonyManager telephonyManager, TelephonyListenerManager telephonyListenerManager, WifiManager wifiManager, - NetworkScoreManager networkScoreManager, - SubscriptionManager subManager, Config config, Looper bgLooper, + SubscriptionManager subManager, + Config config, + Looper bgLooper, Executor bgExecutor, CallbackHandler callbackHandler, AccessPointControllerImpl accessPointController, @@ -273,6 +273,7 @@ public class NetworkControllerImpl extends BroadcastReceiver BroadcastDispatcher broadcastDispatcher, DemoModeController demoModeController, CarrierConfigTracker carrierConfigTracker, + WifiStatusTrackerFactory trackerFactory, @Main Handler handler, FeatureFlags featureFlags, DumpManager dumpManager @@ -315,9 +316,10 @@ public class NetworkControllerImpl extends BroadcastReceiver notifyControllersMobileDataChanged(); } }); + mWifiSignalController = new WifiSignalController(mContext, mHasMobileDataFeature, - mCallbackHandler, this, mWifiManager, mConnectivityManager, networkScoreManager, - mMainHandler, mReceiverHandler); + mCallbackHandler, this, mWifiManager, trackerFactory, + mReceiverHandler); mEthernetSignalController = new EthernetSignalController(mContext, mCallbackHandler, this); diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/connectivity/SignalController.java b/packages/SystemUI/src/com/android/systemui/statusbar/connectivity/SignalController.java index e2806a39130fe..7e8f04e4bc23a 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/connectivity/SignalController.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/connectivity/SignalController.java @@ -50,7 +50,7 @@ public abstract class SignalController private final MobileIconGroup mCarrierMergedWifiIconGroup = TelephonyIcons.CARRIER_MERGED_WIFI; private final WifiManager mWifiManager; + private final Handler mBgHandler; + public WifiSignalController( Context context, boolean hasMobileDataFeature, CallbackHandler callbackHandler, NetworkControllerImpl networkController, WifiManager wifiManager, - ConnectivityManager connectivityManager, - NetworkScoreManager networkScoreManager, - @Main Handler handler, - @Background Handler backgroundHandler) { + WifiStatusTrackerFactory trackerFactory, + @Background Handler bgHandler) { super("WifiSignalController", context, NetworkCapabilities.TRANSPORT_WIFI, callbackHandler, networkController); + mBgHandler = bgHandler; mWifiManager = wifiManager; - mWifiTracker = new WifiStatusTracker(mContext, wifiManager, networkScoreManager, - connectivityManager, this::handleStatusUpdated, handler, backgroundHandler); + mWifiTracker = trackerFactory.createTracker(this::handleStatusUpdated, bgHandler); mWifiTracker.setListening(true); mHasMobileDataFeature = hasMobileDataFeature; if (wifiManager != null) { @@ -181,33 +178,51 @@ public class WifiSignalController extends SignalController * Fetches wifi initial state replacing the initial sticky broadcast. */ public void fetchInitialState() { - mWifiTracker.fetchInitialState(); - copyWifiStates(); - notifyListenersIfNecessary(); + doInBackground(() -> { + mWifiTracker.fetchInitialState(); + copyWifiStates(); + notifyListenersIfNecessary(); + }); } /** * Extract wifi state directly from broadcasts about changes in wifi state. */ - public void handleBroadcast(Intent intent) { - mWifiTracker.handleBroadcast(intent); - copyWifiStates(); - notifyListenersIfNecessary(); + void handleBroadcast(Intent intent) { + doInBackground(() -> { + mWifiTracker.handleBroadcast(intent); + copyWifiStates(); + notifyListenersIfNecessary(); + }); } private void handleStatusUpdated() { - Assert.isMainThread(); - copyWifiStates(); - notifyListenersIfNecessary(); + // The WifiStatusTracker callback comes in on the main thread, but the rest of our data + // access happens on the bgHandler + doInBackground(() -> { + copyWifiStates(); + notifyListenersIfNecessary(); + }); + } + + private void doInBackground(Runnable action) { + if (Thread.currentThread() != mBgHandler.getLooper().getThread()) { + mBgHandler.post(action); + } else { + action.run(); + } } private void copyWifiStates() { + // Data access should only happen on our bg thread + Preconditions.checkState(mBgHandler.getLooper().isCurrentThread()); + mCurrentState.enabled = mWifiTracker.enabled; mCurrentState.isDefault = mWifiTracker.isDefaultNetwork; mCurrentState.connected = mWifiTracker.connected; mCurrentState.ssid = mWifiTracker.ssid; mCurrentState.rssi = mWifiTracker.rssi; - notifyWifiLevelChangeIfNecessary(mWifiTracker.level); + boolean levelChanged = mCurrentState.level != mWifiTracker.level; mCurrentState.level = mWifiTracker.level; mCurrentState.statusLabel = mWifiTracker.statusLabel; mCurrentState.isCarrierMerged = mWifiTracker.isCarrierMerged; @@ -215,11 +230,9 @@ public class WifiSignalController extends SignalController mCurrentState.iconGroup = mCurrentState.isCarrierMerged ? mCarrierMergedWifiIconGroup : mUnmergedWifiIconGroup; - } - void notifyWifiLevelChangeIfNecessary(int level) { - if (level != mCurrentState.level) { - mNetworkController.notifyWifiLevelChange(level); + if (levelChanged) { + mNetworkController.notifyWifiLevelChange(mCurrentState.level); } } diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/connectivity/WifiStatusTrackerFactory.kt b/packages/SystemUI/src/com/android/systemui/statusbar/connectivity/WifiStatusTrackerFactory.kt new file mode 100644 index 0000000000000..9dc17d0f50de4 --- /dev/null +++ b/packages/SystemUI/src/com/android/systemui/statusbar/connectivity/WifiStatusTrackerFactory.kt @@ -0,0 +1,50 @@ +/* + * Copyright (C) 2015 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package com.android.systemui.statusbar.connectivity + +import android.content.Context +import android.net.ConnectivityManager +import android.net.NetworkScoreManager +import android.net.wifi.WifiManager +import android.os.Handler + +import com.android.settingslib.wifi.WifiStatusTracker +import com.android.systemui.dagger.qualifiers.Main + +import javax.inject.Inject + +/** + * Factory class for [WifiStatusTracker] which lives in SettingsLib (and thus doesn't use Dagger). + * This enables the constructors for NetworkControllerImpl and WifiSignalController to be slightly + * nicer. + */ +internal class WifiStatusTrackerFactory @Inject constructor( + private val mContext: Context, + private val mWifiManager: WifiManager?, + private val mNetworkScoreManager: NetworkScoreManager, + private val mConnectivityManager: ConnectivityManager, + @Main private val mMainHandler: Handler +) { + fun createTracker(callback: Runnable?, bgHandler: Handler?): WifiStatusTracker { + return WifiStatusTracker(mContext, + mWifiManager, + mNetworkScoreManager, + mConnectivityManager, + callback, + mMainHandler, + bgHandler) + } +} diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/connectivity/NetworkControllerBaseTest.java b/packages/SystemUI/tests/src/com/android/systemui/statusbar/connectivity/NetworkControllerBaseTest.java index 2fe7c075bc18b..e01ebbdda3745 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/connectivity/NetworkControllerBaseTest.java +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/connectivity/NetworkControllerBaseTest.java @@ -127,6 +127,7 @@ public class NetworkControllerBaseTest extends SysuiTestCase { protected FakeExecutor mFakeExecutor = new FakeExecutor(new FakeSystemClock()); protected Handler mMainHandler; protected FeatureFlags mFeatureFlags; + protected WifiStatusTrackerFactory mWifiStatusTrackerFactory; protected int mSubId; @@ -220,12 +221,14 @@ public class NetworkControllerBaseTest extends SysuiTestCase { return null; }).when(mMockProvisionController).addCallback(any()); + mWifiStatusTrackerFactory = new WifiStatusTrackerFactory( + mContext, mMockWm, mMockNsm, mMockCm, mMainHandler); + mNetworkController = new NetworkControllerImpl(mContext, mMockCm, mMockTm, mTelephonyListenerManager, mMockWm, - mMockNsm, mMockSm, mConfig, TestableLooper.get(this).getLooper(), @@ -238,6 +241,7 @@ public class NetworkControllerBaseTest extends SysuiTestCase { mMockBd, mDemoModeController, mCarrierConfigTracker, + mWifiStatusTrackerFactory, mMainHandler, mFeatureFlags, mock(DumpManager.class) diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/connectivity/NetworkControllerDataTest.java b/packages/SystemUI/tests/src/com/android/systemui/statusbar/connectivity/NetworkControllerDataTest.java index ccfa1b31b7990..3a0c203f76e0c 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/connectivity/NetworkControllerDataTest.java +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/connectivity/NetworkControllerDataTest.java @@ -127,11 +127,13 @@ public class NetworkControllerDataTest extends NetworkControllerBaseTest { mConfig.show4gForLte = true; mNetworkController = new NetworkControllerImpl(mContext, mMockCm, mMockTm, mTelephonyListenerManager, mMockWm, - mMockNsm, mMockSm, mConfig, Looper.getMainLooper(), mFakeExecutor, mCallbackHandler, + mMockSm, mConfig, Looper.getMainLooper(), mFakeExecutor, mCallbackHandler, mock(AccessPointControllerImpl.class), mock(DataUsageController.class), mMockSubDefaults, mock(DeviceProvisionedController.class), mMockBd, mDemoModeController, - mock(CarrierConfigTracker.class), new Handler(TestableLooper.get(this).getLooper()), + mock(CarrierConfigTracker.class), + mWifiStatusTrackerFactory, + new Handler(TestableLooper.get(this).getLooper()), mFeatureFlags, mock(DumpManager.class)); setupNetworkController(); diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/connectivity/NetworkControllerSignalTest.java b/packages/SystemUI/tests/src/com/android/systemui/statusbar/connectivity/NetworkControllerSignalTest.java index b84750aa7ea53..ae1b3d1e1f42e 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/connectivity/NetworkControllerSignalTest.java +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/connectivity/NetworkControllerSignalTest.java @@ -71,7 +71,6 @@ public class NetworkControllerSignalTest extends NetworkControllerBaseTest { mMockTm, mTelephonyListenerManager, mMockWm, - mMockNsm, mMockSm, mConfig, TestableLooper.get(this).getLooper(), @@ -84,6 +83,7 @@ public class NetworkControllerSignalTest extends NetworkControllerBaseTest { mMockBd, mDemoModeController, mCarrierConfigTracker, + mWifiStatusTrackerFactory, mMainHandler, mFeatureFlags, mock(DumpManager.class) @@ -105,7 +105,6 @@ public class NetworkControllerSignalTest extends NetworkControllerBaseTest { mMockTm, mTelephonyListenerManager, mMockWm, - mMockNsm, mMockSm, mConfig, TestableLooper.get(this).getLooper(), @@ -118,6 +117,7 @@ public class NetworkControllerSignalTest extends NetworkControllerBaseTest { mMockBd, mDemoModeController, mCarrierConfigTracker, + mWifiStatusTrackerFactory, mMainHandler, mFeatureFlags, mock(DumpManager.class) @@ -134,11 +134,12 @@ public class NetworkControllerSignalTest extends NetworkControllerBaseTest { when(mMockTm.isDataCapable()).thenReturn(false); // Create a new NetworkController as this is currently handled in constructor. mNetworkController = new NetworkControllerImpl(mContext, mMockCm, mMockTm, - mTelephonyListenerManager, mMockWm, mMockNsm, mMockSm, mConfig, + mTelephonyListenerManager, mMockWm, mMockSm, mConfig, Looper.getMainLooper(), mFakeExecutor, mCallbackHandler, mock(AccessPointControllerImpl.class), mock(DataUsageController.class), mMockSubDefaults, mock(DeviceProvisionedController.class), mMockBd, mDemoModeController, mock(CarrierConfigTracker.class), + mWifiStatusTrackerFactory, mMainHandler, mFeatureFlags, mock(DumpManager.class)); setupNetworkController(); @@ -156,11 +157,12 @@ public class NetworkControllerSignalTest extends NetworkControllerBaseTest { when(mMockSm.getCompleteActiveSubscriptionInfoList()).thenReturn(Collections.emptyList()); mNetworkController = new NetworkControllerImpl(mContext, mMockCm, mMockTm, - mTelephonyListenerManager, mMockWm, mMockNsm, mMockSm, mConfig, + mTelephonyListenerManager, mMockWm, mMockSm, mConfig, Looper.getMainLooper(), mFakeExecutor, mCallbackHandler, mock(AccessPointControllerImpl.class), mock(DataUsageController.class), mMockSubDefaults, mock(DeviceProvisionedController.class), mMockBd, mDemoModeController, mock(CarrierConfigTracker.class), + mWifiStatusTrackerFactory, mMainHandler, mFeatureFlags, mock(DumpManager.class)); mNetworkController.registerListeners(); @@ -225,11 +227,12 @@ public class NetworkControllerSignalTest extends NetworkControllerBaseTest { when(mMockTm.isDataCapable()).thenReturn(false); // Create a new NetworkController as this is currently handled in constructor. mNetworkController = new NetworkControllerImpl(mContext, mMockCm, mMockTm, - mTelephonyListenerManager, mMockWm, mMockNsm, mMockSm, mConfig, + mTelephonyListenerManager, mMockWm, mMockSm, mConfig, Looper.getMainLooper(), mFakeExecutor, mCallbackHandler, mock(AccessPointControllerImpl.class), mock(DataUsageController.class), mMockSubDefaults, mock(DeviceProvisionedController.class), mMockBd, mDemoModeController, mock(CarrierConfigTracker.class), + mWifiStatusTrackerFactory, mMainHandler, mFeatureFlags, mock(DumpManager.class)); setupNetworkController();