From 71f4a82b51f53ba7b99fc7f1d1f81a29b7c88072 Mon Sep 17 00:00:00 2001 From: Sundeep Ghuman Date: Tue, 18 Apr 2017 19:51:46 -0700 Subject: [PATCH 1/2] Remove all pending callbacks in stopTracking. This prevents callbacks occurring on the Settings UI thread in the WifiSettings fragment after its Activity is already destroyed. When removing pending ACCESS_POINTS_CHANGED_MESSAGES, release the lock. Also fixed some flakey tests due to threading issues. Bug: b/37472533 Test: runtest --path frameworks/base/packages/SettingsLib/tests/integ/src/com/android/settingslib/wifi/WifiTrackerTest.java Change-Id: I764caf52e38b7f7e33c67c7ea3aa3d2301095dca --- .../android/settingslib/wifi/WifiTracker.java | 39 +++++++-- .../settingslib/wifi/AccessPointTest.java | 3 +- .../settingslib/wifi/WifiTrackerTest.java | 82 ++++++++++++++++--- 3 files changed, 101 insertions(+), 23 deletions(-) diff --git a/packages/SettingsLib/src/com/android/settingslib/wifi/WifiTracker.java b/packages/SettingsLib/src/com/android/settingslib/wifi/WifiTracker.java index 314791e3105ab..04aaf8fca9c1a 100644 --- a/packages/SettingsLib/src/com/android/settingslib/wifi/WifiTracker.java +++ b/packages/SettingsLib/src/com/android/settingslib/wifi/WifiTracker.java @@ -16,7 +16,6 @@ package com.android.settingslib.wifi; import android.content.BroadcastReceiver; -import android.content.ContentResolver; import android.content.Context; import android.content.Intent; import android.content.IntentFilter; @@ -91,7 +90,7 @@ public class WifiTracker { private final boolean mIncludeSaved; private final boolean mIncludeScans; private final boolean mIncludePasspoints; - private final MainHandler mMainHandler; + @VisibleForTesting final MainHandler mMainHandler; private final WorkHandler mWorkHandler; private WifiTrackerNetworkCallback mNetworkCallback; @@ -335,13 +334,13 @@ public class WifiTracker { */ public void stopTracking() { if (mRegistered) { - mWorkHandler.removeMessages(WorkHandler.MSG_UPDATE_ACCESS_POINTS); - mWorkHandler.removeMessages(WorkHandler.MSG_UPDATE_NETWORK_INFO); - mWorkHandler.removeMessages(WorkHandler.MSG_UPDATE_NETWORK_SCORES); mContext.unregisterReceiver(mReceiver); mConnectivityManager.unregisterNetworkCallback(mNetworkCallback); mRegistered = false; } + mWorkHandler.removePendingMessages(); + mMainHandler.removePendingMessages(); + pauseScanning(); unregisterAndClearScoreCache(); @@ -749,10 +748,11 @@ public class WifiTracker { } } - private final class MainHandler extends Handler { - private static final int MSG_CONNECTED_CHANGED = 0; - private static final int MSG_WIFI_STATE_CHANGED = 1; - private static final int MSG_ACCESS_POINT_CHANGED = 2; + @VisibleForTesting + final class MainHandler extends Handler { + @VisibleForTesting static final int MSG_CONNECTED_CHANGED = 0; + @VisibleForTesting static final int MSG_WIFI_STATE_CHANGED = 1; + @VisibleForTesting static final int MSG_ACCESS_POINT_CHANGED = 2; private static final int MSG_RESUME_SCANNING = 3; private static final int MSG_PAUSE_SCANNING = 4; @@ -794,6 +794,19 @@ public class WifiTracker { mInternalAccessPointsWriteLock.close(); sendEmptyMessage(MSG_ACCESS_POINT_CHANGED); } + + void removePendingMessages() { + // Only release the lock if there was a pending message which would have done the same + if (mMainHandler.hasMessages(MSG_ACCESS_POINT_CHANGED)) { + mMainHandler.removeMessages(MSG_ACCESS_POINT_CHANGED); + mInternalAccessPointsWriteLock.open(); + } + + removeMessages(MSG_CONNECTED_CHANGED); + removeMessages(MSG_WIFI_STATE_CHANGED); + removeMessages(MSG_PAUSE_SCANNING); + removeMessages(MSG_RESUME_SCANNING); + } } private final class WorkHandler extends Handler { @@ -841,6 +854,14 @@ public class WifiTracker { break; } } + + private void removePendingMessages() { + removeMessages(MSG_UPDATE_ACCESS_POINTS); + removeMessages(MSG_UPDATE_NETWORK_INFO); + removeMessages(MSG_RESUME); + removeMessages(MSG_UPDATE_WIFI_STATE); + removeMessages(MSG_UPDATE_NETWORK_SCORES); + } } @VisibleForTesting diff --git a/packages/SettingsLib/tests/integ/src/com/android/settingslib/wifi/AccessPointTest.java b/packages/SettingsLib/tests/integ/src/com/android/settingslib/wifi/AccessPointTest.java index 3e01b34765c03..154fde204c70e 100644 --- a/packages/SettingsLib/tests/integ/src/com/android/settingslib/wifi/AccessPointTest.java +++ b/packages/SettingsLib/tests/integ/src/com/android/settingslib/wifi/AccessPointTest.java @@ -17,6 +17,7 @@ package com.android.settingslib.wifi; import static com.google.common.truth.Truth.assertThat; import static com.google.common.truth.Truth.assertWithMessage; + import static org.junit.Assert.assertTrue; import static org.mockito.Mockito.any; import static org.mockito.Mockito.when; @@ -24,7 +25,6 @@ import static org.mockito.Mockito.when; import android.content.Context; import android.net.ConnectivityManager; import android.net.NetworkInfo; -import android.net.NetworkKey; import android.net.ScoredNetwork; import android.net.wifi.ScanResult; import android.net.wifi.WifiConfiguration; @@ -40,6 +40,7 @@ import android.support.test.filters.SmallTest; import android.support.test.runner.AndroidJUnit4; import android.text.SpannableString; import android.text.style.TtsSpan; + import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; diff --git a/packages/SettingsLib/tests/integ/src/com/android/settingslib/wifi/WifiTrackerTest.java b/packages/SettingsLib/tests/integ/src/com/android/settingslib/wifi/WifiTrackerTest.java index b938fe2ac2db4..621041a4675d8 100644 --- a/packages/SettingsLib/tests/integ/src/com/android/settingslib/wifi/WifiTrackerTest.java +++ b/packages/SettingsLib/tests/integ/src/com/android/settingslib/wifi/WifiTrackerTest.java @@ -20,13 +20,15 @@ import static com.google.common.truth.Truth.assertThat; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; import static org.mockito.Mockito.any; import static org.mockito.Mockito.anyInt; -import static org.mockito.Mockito.atLeast; +import static org.mockito.Mockito.atMost; import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.doNothing; import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyNoMoreInteractions; import static org.mockito.Mockito.when; import android.content.Context; @@ -60,7 +62,6 @@ import org.junit.After; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; - import org.mockito.ArgumentCaptor; import org.mockito.Captor; import org.mockito.Matchers; @@ -129,7 +130,7 @@ public class WifiTrackerTest { private Handler mScannerHandler; private HandlerThread mMainThread; private HandlerThread mWorkerThread; - private Looper mLooper; + private Looper mWorkerLooper; private Looper mMainLooper; private int mOriginalScoringUiSettingValue; @@ -141,7 +142,7 @@ public class WifiTrackerTest { mWorkerThread = new HandlerThread("TestHandlerWorkerThread"); mWorkerThread.start(); - mLooper = mWorkerThread.getLooper(); + mWorkerLooper = mWorkerThread.getLooper(); mMainThread = new HandlerThread("TestHandlerThread"); mMainThread.start(); mMainLooper = mMainThread.getLooper(); @@ -264,7 +265,7 @@ public class WifiTrackerTest { new WifiTracker( mContext, mockWifiListener, - mLooper, + mWorkerLooper, true, true, true, @@ -349,7 +350,7 @@ public class WifiTrackerTest { scanResult.capabilities = ""; WifiTracker tracker = new WifiTracker( - InstrumentationRegistry.getTargetContext(), null, mLooper, true, true); + InstrumentationRegistry.getTargetContext(), null, mWorkerLooper, true, true); AccessPoint result = tracker.getCachedOrCreate(scanResult, new ArrayList()); assertTrue(result.mAccessPointListener != null); @@ -365,7 +366,7 @@ public class WifiTrackerTest { configuration.allowedKeyManagement.set(WifiConfiguration.KeyMgmt.WPA_PSK); WifiTracker tracker = new WifiTracker( - InstrumentationRegistry.getTargetContext(), null, mLooper, true, true); + InstrumentationRegistry.getTargetContext(), null, mWorkerLooper, true, true); AccessPoint result = tracker.getCachedOrCreate(configuration, new ArrayList()); assertTrue(result.mAccessPointListener != null); @@ -433,7 +434,8 @@ public class WifiTrackerTest { } @Test - public void startTrackingShouldRequestScoresForCurrentAccessPoints() throws InterruptedException { + public void startTrackingAfterStopTracking_shouldRequestNewScores() + throws InterruptedException { // Start the tracker and inject the initial scan results and then stop tracking WifiTracker tracker = createTrackerWithImmediateBroadcastsAndInjectInitialScanResults(); @@ -442,6 +444,7 @@ public class WifiTrackerTest { mRequestScoresLatch = new CountDownLatch(1); startTracking(tracker); + tracker.forceUpdate(); assertTrue("Latch timed out", mRequestScoresLatch.await(LATCH_TIMEOUT, TimeUnit.MILLISECONDS)); @@ -657,18 +660,29 @@ public class WifiTrackerTest { int newRssi = CONNECTED_RSSI + 10; WifiInfo info = new WifiInfo(CONNECTED_AP_1_INFO); info.setRssi(newRssi); - when(mockWifiManager.getConnectionInfo()).thenReturn(info); - mAccessPointsChangedLatch = new CountDownLatch(1); + CountDownLatch latch = new CountDownLatch(1); + + // Once the new info has been fetched, we need to wait for the access points to be copied + doAnswer(invocation -> { + latch.countDown(); + mAccessPointsChangedLatch = new CountDownLatch(1); + return info; + }).when(mockWifiManager).getConnectionInfo(); + tracker.mReceiver.onReceive(mContext, new Intent(WifiManager.RSSI_CHANGED_ACTION)); - assertTrue(mAccessPointsChangedLatch.await(LATCH_TIMEOUT, TimeUnit.MILLISECONDS)); + assertTrue("New connection info never retrieved", + latch.await(LATCH_TIMEOUT, TimeUnit.MILLISECONDS)); + assertTrue("onAccessPointsChanged never called", + mAccessPointsChangedLatch.await(LATCH_TIMEOUT, TimeUnit.MILLISECONDS)); - verify(mockWifiManager, atLeast(2)).getConnectionInfo(); assertThat(tracker.getAccessPoints().get(0).getRssi()).isEqualTo(newRssi); } @Test public void forceUpdateShouldSynchronouslyFetchLatestInformation() throws Exception { + // TODO(sghuman): Fix flakiness of this test + when(mockWifiManager.getConnectionInfo()).thenReturn(CONNECTED_AP_1_INFO); WifiConfiguration configuration = new WifiConfiguration(); @@ -682,7 +696,6 @@ public class WifiTrackerTest { networkInfo.setDetailedState(NetworkInfo.DetailedState.CONNECTED, "connected", "test"); when(mockConnectivityManager.getNetworkInfo(any(Network.class))).thenReturn(networkInfo); - WifiTracker tracker = createMockedWifiTracker(); startTracking(tracker); tracker.forceUpdate(); @@ -691,4 +704,47 @@ public class WifiTrackerTest { assertThat(tracker.getAccessPoints().size()).isEqualTo(2); assertThat(tracker.getAccessPoints().get(0).isActive()).isTrue(); } + + @Test + public void stopTrackingShouldRemoveWifiListenerCallbacks() throws Exception { + WifiTracker tracker = createMockedWifiTracker(); + startTracking(tracker); + + CountDownLatch latch = new CountDownLatch(1); + CountDownLatch lock = new CountDownLatch(1); + tracker.mMainHandler.post(() -> { + try { + lock.await(); + latch.countDown(); + } catch (InterruptedException e) { + fail("Interrupted Exception while awaiting lock release: " + e); + } + }); + + // Enqueue messages + tracker.mMainHandler.sendEmptyMessage( + WifiTracker.MainHandler.MSG_ACCESS_POINT_CHANGED); + tracker.mMainHandler.sendEmptyMessage( + WifiTracker.MainHandler.MSG_CONNECTED_CHANGED); + tracker.mMainHandler.sendEmptyMessage( + WifiTracker.MainHandler.MSG_WIFI_STATE_CHANGED); + + tracker.stopTracking(); + + verify(mockWifiListener, atMost(1)).onAccessPointsChanged(); + verify(mockWifiListener, atMost(1)).onConnectedChanged(); + verify(mockWifiListener, atMost(1)).onWifiStateChanged(anyInt()); + + lock.countDown(); + assertTrue(latch.await(LATCH_TIMEOUT, TimeUnit.MILLISECONDS)); + + assertThat(tracker.mMainHandler.hasMessages( + WifiTracker.MainHandler.MSG_ACCESS_POINT_CHANGED)).isFalse(); + assertThat(tracker.mMainHandler.hasMessages( + WifiTracker.MainHandler.MSG_CONNECTED_CHANGED)).isFalse(); + assertThat(tracker.mMainHandler.hasMessages( + WifiTracker.MainHandler.MSG_WIFI_STATE_CHANGED)).isFalse(); + + verifyNoMoreInteractions(mockWifiListener); + } } \ No newline at end of file From ce6bab87e6e37c124f4a23cbbacaf04f23fa95fc Mon Sep 17 00:00:00 2001 From: Sundeep Ghuman Date: Wed, 19 Apr 2017 16:21:46 -0700 Subject: [PATCH 2/2] Use synchronization rather than locking in WifiTracker. This vastly simplifies WifiTracker usage. The existing locking behavior closed a lock on thread A, depended on thread B to open it, and would block on Thread A until thread B did. However, thread B can also block on this lock, hence if Thread A closes the lock between thread B opening it and blocking on it, and deadlock would result in an ANR that would crash WifiSettings (see b/37530557 for another example). All work on the WorkHandler is now synchronized, as a preliminary step to removing the worker thread altogether, pending discussions with original author on the threads creation. Also fix test flakiness, an indirect byproduct of now simplifying concurrency issues in this class. Fixes b/37581732. Together with the other changes in this topic, this CL resolves all known wifi picker jank and no ANRs have been witnessed. Bug: b/37504190 Test: runtest --path frameworks/base/packages/SettingsLib/tests/integ/src/com/android/settingslib/wifi/WifiTrackerTest.java Change-Id: I0e47a4d50372beb2d141189276b1a4d9230c0d98 --- .../android/settingslib/wifi/WifiTracker.java | 297 +++++++++--------- .../settingslib/wifi/WifiTrackerFactory.java | 4 +- .../settingslib/wifi/WifiTrackerTest.java | 25 +- 3 files changed, 161 insertions(+), 165 deletions(-) diff --git a/packages/SettingsLib/src/com/android/settingslib/wifi/WifiTracker.java b/packages/SettingsLib/src/com/android/settingslib/wifi/WifiTracker.java index 04aaf8fca9c1a..646b6ba84ebb5 100644 --- a/packages/SettingsLib/src/com/android/settingslib/wifi/WifiTracker.java +++ b/packages/SettingsLib/src/com/android/settingslib/wifi/WifiTracker.java @@ -15,6 +15,7 @@ */ package com.android.settingslib.wifi; +import android.annotation.MainThread; import android.content.BroadcastReceiver; import android.content.Context; import android.content.Intent; @@ -40,6 +41,7 @@ import android.os.Handler; import android.os.Looper; import android.os.Message; import android.provider.Settings; +import android.support.annotation.GuardedBy; import android.util.ArraySet; import android.util.Log; import android.util.SparseArray; @@ -64,8 +66,6 @@ import java.util.concurrent.atomic.AtomicBoolean; * Tracks saved or available wifi networks and their state. */ public class WifiTracker { - // TODO(sghuman): Document remaining methods with @UiThread and @WorkerThread where possible. - // TODO(sghuman): Refactor to avoid calling certain methods on the UiThread. // TODO(b/36733768): Remove flag includeSaved and includePasspoints. private static final String TAG = "WifiTracker"; @@ -96,34 +96,34 @@ public class WifiTracker { private WifiTrackerNetworkCallback mNetworkCallback; private int mNumSavedNetworks; + + @GuardedBy("mLock") private boolean mRegistered; - /** Updated using main handler. Clone of this collection is returned - * from {@link #getAccessPoints()} + /** + * The externally visible access point list. + * + * Updated using main handler. Clone of this collection is returned from + * {@link #getAccessPoints()} */ private final List mAccessPoints = new ArrayList<>(); /** - * Protects APs contained in {@link #mInternalAccessPoints} from being modified concurrently - * while its being read. Usage contract: + * The internal list of access points, synchronized on itself. * - * 1. MainHandler opens the condition after copying the states thereby - * allowing WorkerHandler to mutate the contents. - * 2. WorkerHandler after mutating the contents, sends a message to MainHandler to copy the - * states and closes the condition. - * - * This is better than synchronizing on a variable because it prevents MainHandler from - * unnecessarily blocking/waiting to acquire lock for copying states. When MainHandler is about - * to access {@link #mInternalAccessPoints}, it is assumed that it has exclusively lock on the - * contents. - */ - private final ConditionVariable mInternalAccessPointsWriteLock = new ConditionVariable(true); - - /** Guarded by mInternalAccessPointsWriteLock, updated using worker handler. * Never exposed outside this class. */ + @GuardedBy("mLock") private final List mInternalAccessPoints = new ArrayList<>(); + /** + * Synchronization lock for managing concurrency between main and worker threads. + * + *

This lock should be held for all background work. + * TODO(b/37674366): Remove the worker thread so synchronization is no longer necessary. + */ + private final Object mLock = new Object(); + //visible to both worker and main thread. Guarded by #mInternalAccessPoints private final AccessPointListenerAdapter mAccessPointListenerAdapter = new AccessPointListenerAdapter(); @@ -137,10 +137,12 @@ public class WifiTracker { private final NetworkScoreManager mNetworkScoreManager; private final WifiNetworkScoreCache mScoreCache; - private final Set mRequestedScores = new ArraySet<>(); private boolean mNetworkScoringUiEnabled; private final ContentObserver mObserver; + @GuardedBy("mLock") + private final Set mRequestedScores = new ArraySet<>(); + @VisibleForTesting Scanner mScanner; @@ -211,13 +213,17 @@ public class WifiTracker { mNetworkScoreManager = networkScoreManager; - mScoreCache = new WifiNetworkScoreCache(context, new CacheListener(mMainHandler) { + mScoreCache = new WifiNetworkScoreCache(context, new CacheListener(mWorkHandler) { @Override public void networkCacheUpdated(List networks) { + synchronized (mLock) { + if (!mRegistered) return; + } + if (Log.isLoggable(TAG, Log.VERBOSE)) { Log.v(TAG, "Score cache was updated with networks: " + networks); } - Message.obtain(mWorkHandler, WorkHandler.MSG_UPDATE_NETWORK_SCORES).sendToTarget(); + updateNetworkScores(); } }); @@ -235,17 +241,19 @@ public class WifiTracker { /** * Synchronously update the list of access points with the latest information. */ + @MainThread public void forceUpdate() { - mWorkHandler.removeMessages(WorkHandler.MSG_UPDATE_ACCESS_POINTS); + synchronized (mLock) { + mWorkHandler.removeMessages(WorkHandler.MSG_UPDATE_ACCESS_POINTS); + mLastInfo = mWifiManager.getConnectionInfo(); + mLastNetworkInfo = mConnectivityManager.getNetworkInfo(mWifiManager.getCurrentNetwork()); + updateAccessPointsLocked(); - mLastInfo = mWifiManager.getConnectionInfo(); - mLastNetworkInfo = mConnectivityManager.getNetworkInfo(mWifiManager.getCurrentNetwork()); - updateAccessPoints(); - - // Synchronously copy access points - mMainHandler.removeMessages(MainHandler.MSG_ACCESS_POINT_CHANGED); - mMainHandler.handleMessage( - Message.obtain(mMainHandler, MainHandler.MSG_ACCESS_POINT_CHANGED)); + // Synchronously copy access points + mMainHandler.removeMessages(MainHandler.MSG_ACCESS_POINT_CHANGED); + mMainHandler.handleMessage( + Message.obtain(mMainHandler, MainHandler.MSG_ACCESS_POINT_CHANGED)); + } } /** @@ -289,22 +297,25 @@ public class WifiTracker { *

Registers listeners and starts scanning for wifi networks. If this is not called * then forceUpdate() must be called to populate getAccessPoints(). */ + @MainThread public void startTracking() { - registerScoreCache(); + synchronized (mLock) { + registerScoreCache(); - mContext.getContentResolver().registerContentObserver( - Settings.Global.getUriFor(Settings.Global.NETWORK_SCORING_UI_ENABLED), - false /* notifyForDescendants */, - mObserver); - mObserver.onChange(false /* selfChange */); // Set the initial value for mScoringUiEnabled + mContext.getContentResolver().registerContentObserver( + Settings.Global.getUriFor(Settings.Global.NETWORK_SCORING_UI_ENABLED), + false /* notifyForDescendants */, + mObserver); + mObserver.onChange(false /* selfChange */); // Set mScoringUiEnabled - resumeScanning(); - if (!mRegistered) { - mContext.registerReceiver(mReceiver, mFilter); - // NetworkCallback objects cannot be reused. http://b/20701525 . - mNetworkCallback = new WifiTrackerNetworkCallback(); - mConnectivityManager.registerNetworkCallback(mNetworkRequest, mNetworkCallback); - mRegistered = true; + resumeScanning(); + if (!mRegistered) { + mContext.registerReceiver(mReceiver, mFilter); + // NetworkCallback objects cannot be reused. http://b/20701525 . + mNetworkCallback = new WifiTrackerNetworkCallback(); + mConnectivityManager.registerNetworkCallback(mNetworkRequest, mNetworkCallback); + mRegistered = true; + } } } @@ -322,44 +333,49 @@ public class WifiTracker { Log.d(TAG, "Requesting scores for Network Keys: " + keys); } mNetworkScoreManager.requestScores(keys.toArray(new NetworkKey[keys.size()])); - mRequestedScores.addAll(keys); + synchronized (mLock) { + mRequestedScores.addAll(keys); + } } /** * Stop tracking wifi networks and scores. * - *

Unregisters all listeners and stops scanning for wifi networks. This should always - * be called when done with a WifiTracker (if startTracking was called) to ensure - * proper cleanup. + *

This should always be called when done with a WifiTracker (if startTracking was called) to + * ensure proper cleanup and prevent any further callbacks from occuring. */ + @MainThread public void stopTracking() { - if (mRegistered) { - mContext.unregisterReceiver(mReceiver); - mConnectivityManager.unregisterNetworkCallback(mNetworkCallback); - mRegistered = false; + synchronized (mLock) { + if (mRegistered) { + mContext.unregisterReceiver(mReceiver); + mConnectivityManager.unregisterNetworkCallback(mNetworkCallback); + mRegistered = false; + } + unregisterAndClearScoreCache(); + pauseScanning(); + mContext.getContentResolver().unregisterContentObserver(mObserver); + + mWorkHandler.removePendingMessages(); + mMainHandler.removePendingMessages(); } - mWorkHandler.removePendingMessages(); - mMainHandler.removePendingMessages(); - - pauseScanning(); - - unregisterAndClearScoreCache(); - - mContext.getContentResolver().unregisterContentObserver(mObserver); } private void unregisterAndClearScoreCache() { mNetworkScoreManager.unregisterNetworkScoreCache(NetworkKey.TYPE_WIFI, mScoreCache); mScoreCache.clearScores(); - // Clear the scores on the work handler to avoid concurrent modification exceptions - mWorkHandler.post(() -> mRequestedScores.clear()); + // Synchronize on mLock to avoid concurrent modification during updateAccessPointsLocked + synchronized (mLock) { + mRequestedScores.clear(); + } } /** * Gets the current list of access points. Should be called from main thread, otherwise * expect inconsistencies */ + @MainThread public List getAccessPoints() { return new ArrayList<>(mAccessPoints); } @@ -440,16 +456,20 @@ public class WifiTracker { return null; } - private void updateAccessPoints() { - // Wait until main worker is done copying the states. This is done to prevent propagation - // of accesspoint states while the update is in progress. - long before = System.currentTimeMillis(); - mInternalAccessPointsWriteLock.block(); - if (DBG) { - Log.d(TAG, "Acquired AP lock on WorkerHandler. Time to wait = " - + (System.currentTimeMillis() - before) + " ms."); + /** Safely modify {@link #mInternalAccessPoints} by acquiring {@link #mLock} first. */ + private void updateAccessPointsLocked() { + synchronized (mLock) { + updateAccessPoints(); } + } + /** + * Update the internal list of access points. + * + *

Should never be called directly, use {@link #updateAccessPointsLocked()} instead. + */ + @GuardedBy("mLock") + private void updateAccessPoints() { // Swap the current access points into a cached list. List cachedAccessPoints = new ArrayList<>(mInternalAccessPoints); ArrayList accessPoints = new ArrayList<>(); @@ -459,8 +479,8 @@ public class WifiTracker { accessPoint.clearConfig(); } - /* Lookup table to more quickly update AccessPoints by only considering objects with the - * correct SSID. Maps SSID -> List of AccessPoints with the given SSID. */ + /* Lookup table to more quickly update AccessPoints by only considering objects with the + * correct SSID. Maps SSID -> List of AccessPoints with the given SSID. */ Multimap apMap = new Multimap(); WifiConfiguration connectionConfig = null; if (mLastInfo != null) { @@ -583,7 +603,8 @@ public class WifiTracker { mInternalAccessPoints.clear(); mInternalAccessPoints.addAll(accessPoints); - mMainHandler.scheduleAPCopyingAndCloseWriteLock(); + + mMainHandler.sendEmptyMessage(MainHandler.MSG_ACCESS_POINT_CHANGED); } @VisibleForTesting @@ -640,59 +661,48 @@ public class WifiTracker { connectionConfig = getWifiConfigurationForNetworkId(mLastInfo.getNetworkId()); } - // Lock required to prevent accidental copying of AccessPoint states while the modification - // is in progress. see #copyAndNotifyListeners - long before = System.currentTimeMillis(); - mInternalAccessPointsWriteLock.block(); - if (DBG) { - Log.d(TAG, "Acquired AP lock on WorkerHandler for updating NetworkInfo. Wait time = " + - (System.currentTimeMillis() - before) + "ms."); - } - boolean updated = false; boolean reorder = false; // Only reorder if connected AP was changed - for (int i = mInternalAccessPoints.size() - 1; i >= 0; --i) { - AccessPoint ap = mInternalAccessPoints.get(i); - boolean previouslyConnected = ap.isActive(); - if (ap.update(connectionConfig, mLastInfo, mLastNetworkInfo)) { - updated = true; - if (previouslyConnected != ap.isActive()) reorder = true; - } - if (ap.update(mScoreCache, mNetworkScoringUiEnabled)) { - reorder = true; - updated = true; + + synchronized (mLock) { + for (int i = mInternalAccessPoints.size() - 1; i >= 0; --i) { + AccessPoint ap = mInternalAccessPoints.get(i); + boolean previouslyConnected = ap.isActive(); + if (ap.update(connectionConfig, mLastInfo, mLastNetworkInfo)) { + updated = true; + if (previouslyConnected != ap.isActive()) reorder = true; + } + if (ap.update(mScoreCache, mNetworkScoringUiEnabled)) { + reorder = true; + updated = true; + } } + + if (reorder) Collections.sort(mInternalAccessPoints); + + if (updated) mMainHandler.sendEmptyMessage(MainHandler.MSG_ACCESS_POINT_CHANGED); } - - if (reorder) Collections.sort(mInternalAccessPoints); - - if (updated) mMainHandler.scheduleAPCopyingAndCloseWriteLock(); } /** * Update all the internal access points rankingScores, badge and metering. * *

Will trigger a resort and notify listeners of changes if applicable. + * + *

Synchronized on {@link #mLock}. */ private void updateNetworkScores() { - // Lock required to prevent accidental copying of AccessPoint states while the modification - // is in progress. see #copyAndNotifyListeners - long before = System.currentTimeMillis(); - mInternalAccessPointsWriteLock.block(); - if (DBG) { - Log.d(TAG, "Acquired AP lock on WorkerHandler for inserting NetworkScores. Wait time = " + - (System.currentTimeMillis() - before) + "ms."); - } - - boolean reorder = false; - for (int i = 0; i < mInternalAccessPoints.size(); i++) { - if (mInternalAccessPoints.get(i).update(mScoreCache, mNetworkScoringUiEnabled)) { - reorder = true; + synchronized (mLock) { + boolean updated = false; + for (int i = 0; i < mInternalAccessPoints.size(); i++) { + if (mInternalAccessPoints.get(i).update(mScoreCache, mNetworkScoringUiEnabled)) { + updated = true; + } + } + if (updated) { + Collections.sort(mInternalAccessPoints); + mMainHandler.sendEmptyMessage(MainHandler.MSG_ACCESS_POINT_CHANGED); } - } - if (reorder) { - Collections.sort(mInternalAccessPoints); - mMainHandler.scheduleAPCopyingAndCloseWriteLock(); } } @@ -732,7 +742,10 @@ public class WifiTracker { .sendToTarget(); mWorkHandler.sendEmptyMessage(WorkHandler.MSG_UPDATE_ACCESS_POINTS); } else if (WifiManager.RSSI_CHANGED_ACTION.equals(action)) { - mWorkHandler.sendEmptyMessage(WorkHandler.MSG_UPDATE_NETWORK_INFO); + NetworkInfo info = + mConnectivityManager.getNetworkInfo(mWifiManager.getCurrentNetwork()); + mWorkHandler.obtainMessage(WorkHandler.MSG_UPDATE_NETWORK_INFO, info) + .sendToTarget(); } } }; @@ -789,19 +802,8 @@ public class WifiTracker { } } - void scheduleAPCopyingAndCloseWriteLock() { - //prevent worker thread from modifying mInternalAccessPoints - mInternalAccessPointsWriteLock.close(); - sendEmptyMessage(MSG_ACCESS_POINT_CHANGED); - } - void removePendingMessages() { - // Only release the lock if there was a pending message which would have done the same - if (mMainHandler.hasMessages(MSG_ACCESS_POINT_CHANGED)) { - mMainHandler.removeMessages(MSG_ACCESS_POINT_CHANGED); - mInternalAccessPointsWriteLock.open(); - } - + removeMessages(MSG_ACCESS_POINT_CHANGED); removeMessages(MSG_CONNECTED_CHANGED); removeMessages(MSG_WIFI_STATE_CHANGED); removeMessages(MSG_PAUSE_SCANNING); @@ -814,7 +816,6 @@ public class WifiTracker { private static final int MSG_UPDATE_NETWORK_INFO = 1; private static final int MSG_RESUME = 2; private static final int MSG_UPDATE_WIFI_STATE = 3; - private static final int MSG_UPDATE_NETWORK_SCORES = 4; public WorkHandler(Looper looper) { super(looper); @@ -822,9 +823,18 @@ public class WifiTracker { @Override public void handleMessage(Message msg) { + synchronized (mLock) { + processMessage(msg); + } + } + + @GuardedBy("mLock") + private void processMessage(Message msg) { + if (!mRegistered) return; + switch (msg.what) { case MSG_UPDATE_ACCESS_POINTS: - updateAccessPoints(); + updateAccessPointsLocked(); break; case MSG_UPDATE_NETWORK_INFO: updateNetworkInfo((NetworkInfo) msg.obj); @@ -849,9 +859,6 @@ public class WifiTracker { mMainHandler.obtainMessage(MainHandler.MSG_WIFI_STATE_CHANGED, msg.arg1, 0) .sendToTarget(); break; - case MSG_UPDATE_NETWORK_SCORES: - updateNetworkScores(); - break; } } @@ -860,7 +867,6 @@ public class WifiTracker { removeMessages(MSG_UPDATE_NETWORK_INFO); removeMessages(MSG_RESUME); removeMessages(MSG_UPDATE_WIFI_STATE); - removeMessages(MSG_UPDATE_NETWORK_SCORES); } } @@ -980,11 +986,12 @@ public class WifiTracker { /** * Responsible for copying access points from {@link #mInternalAccessPoints} and notifying - * accesspoint listeners. Mutation of the accesspoints returned is done on the main thread. + * accesspoint listeners. * * @param notifyListeners if true, accesspoint listeners are notified, otherwise notifications * dropped. */ + @MainThread private void copyAndNotifyListeners(boolean notifyListeners) { // Need to watch out for memory allocations on main thread. SparseArray oldAccessPoints = new SparseArray<>(); @@ -995,17 +1002,15 @@ public class WifiTracker { oldAccessPoints.put(accessPoint.mId, accessPoint); } - //synchronize to prevent modification of "mInternalAccessPoints" by worker thread. - long before = System.currentTimeMillis(); - try { - if (DBG) { - Log.d(TAG, "Starting to copy AP items on the MainHandler"); - } - if (notifyListeners) { - notificationMap = mAccessPointListenerAdapter.mPendingNotifications.clone(); - } + if (DBG) { + Log.d(TAG, "Starting to copy AP items on the MainHandler"); + } + if (notifyListeners) { + notificationMap = mAccessPointListenerAdapter.mPendingNotifications.clone(); + } - mAccessPointListenerAdapter.mPendingNotifications.clear(); + mAccessPointListenerAdapter.mPendingNotifications.clear(); + synchronized (mLock) { for (AccessPoint internalAccessPoint : mInternalAccessPoints) { AccessPoint accessPoint = oldAccessPoints.get(internalAccessPoint.mId); if (accessPoint == null) { @@ -1015,12 +1020,6 @@ public class WifiTracker { } updatedAccessPoints.add(accessPoint); } - } finally { - mInternalAccessPointsWriteLock.open(); - if (DBG) { - Log.d(TAG, "Opened AP Write lock on the MainHandler. Time to copy = " + - (System.currentTimeMillis() - before) + " ms."); - } } mAccessPoints.clear(); diff --git a/packages/SettingsLib/src/com/android/settingslib/wifi/WifiTrackerFactory.java b/packages/SettingsLib/src/com/android/settingslib/wifi/WifiTrackerFactory.java index a2becf717e899..79cee046140d8 100644 --- a/packages/SettingsLib/src/com/android/settingslib/wifi/WifiTrackerFactory.java +++ b/packages/SettingsLib/src/com/android/settingslib/wifi/WifiTrackerFactory.java @@ -23,8 +23,6 @@ import android.support.annotation.Keep; * Factory method used to inject WifiTracker instances. */ public class WifiTrackerFactory { - private static boolean sTestingMode = false; - private static WifiTracker sTestingWifiTracker; @Keep // Keep proguard from stripping this method since it is only used in tests @@ -35,7 +33,7 @@ public class WifiTrackerFactory { public static WifiTracker create( Context context, WifiTracker.WifiListener wifiListener, Looper workerLooper, boolean includeSaved, boolean includeScans, boolean includePasspoints) { - if(sTestingMode) { + if(sTestingWifiTracker != null) { return sTestingWifiTracker; } return new WifiTracker( diff --git a/packages/SettingsLib/tests/integ/src/com/android/settingslib/wifi/WifiTrackerTest.java b/packages/SettingsLib/tests/integ/src/com/android/settingslib/wifi/WifiTrackerTest.java index 621041a4675d8..f519a906eab5d 100644 --- a/packages/SettingsLib/tests/integ/src/com/android/settingslib/wifi/WifiTrackerTest.java +++ b/packages/SettingsLib/tests/integ/src/com/android/settingslib/wifi/WifiTrackerTest.java @@ -27,6 +27,8 @@ import static org.mockito.Mockito.atMost; import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.doNothing; import static org.mockito.Mockito.doThrow; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoMoreInteractions; import static org.mockito.Mockito.when; @@ -253,9 +255,7 @@ public class WifiTrackerTest { tracker.mReceiver.onReceive(mContext, intent); } - mAccessPointsChangedLatch = new CountDownLatch(1); sendScanResultsAndProcess(tracker); - assertTrue(mAccessPointsChangedLatch.await(LATCH_TIMEOUT, TimeUnit.MILLISECONDS)); return tracker; } @@ -465,9 +465,10 @@ public class WifiTrackerTest { private void updateScoresAndWaitForAccessPointsChangedCallback() throws InterruptedException { // Updating scores can happen together or one after the other, so the latch countdown is set // to 2. - mAccessPointsChangedLatch = new CountDownLatch(2); + mAccessPointsChangedLatch = new CountDownLatch(3); updateScores(); - mAccessPointsChangedLatch.await(LATCH_TIMEOUT, TimeUnit.MILLISECONDS); + assertTrue("onAccessPointChanged was not called three times", + mAccessPointsChangedLatch.await(LATCH_TIMEOUT, TimeUnit.MILLISECONDS)); } @Test @@ -651,12 +652,6 @@ public class WifiTrackerTest { WifiTracker tracker = createTrackerWithScanResultsAndAccessPoint1Connected(); assertThat(tracker.getAccessPoints().get(0).isActive()).isTrue(); - WifiConfiguration configuration = new WifiConfiguration(); - configuration.SSID = SSID_1; - configuration.BSSID = BSSID_1; - configuration.networkId = CONNECTED_NETWORK_ID; - when(mockWifiManager.getConfiguredNetworks()).thenReturn(Arrays.asList(configuration)); - int newRssi = CONNECTED_RSSI + 10; WifiInfo info = new WifiInfo(CONNECTED_AP_1_INFO); info.setRssi(newRssi); @@ -681,7 +676,8 @@ public class WifiTrackerTest { @Test public void forceUpdateShouldSynchronouslyFetchLatestInformation() throws Exception { - // TODO(sghuman): Fix flakiness of this test + Network mockNetwork = mock(Network.class); + when(mockWifiManager.getCurrentNetwork()).thenReturn(mockNetwork); when(mockWifiManager.getConnectionInfo()).thenReturn(CONNECTED_AP_1_INFO); @@ -697,9 +693,12 @@ public class WifiTrackerTest { when(mockConnectivityManager.getNetworkInfo(any(Network.class))).thenReturn(networkInfo); WifiTracker tracker = createMockedWifiTracker(); - startTracking(tracker); tracker.forceUpdate(); + verify(mockWifiManager).getConnectionInfo(); + verify(mockWifiManager, times(2)).getConfiguredNetworks(); + verify(mockConnectivityManager).getNetworkInfo(any(Network.class)); + verify(mockWifiListener).onAccessPointsChanged(); assertThat(tracker.getAccessPoints().size()).isEqualTo(2); assertThat(tracker.getAccessPoints().get(0).isActive()).isTrue(); @@ -736,7 +735,7 @@ public class WifiTrackerTest { verify(mockWifiListener, atMost(1)).onWifiStateChanged(anyInt()); lock.countDown(); - assertTrue(latch.await(LATCH_TIMEOUT, TimeUnit.MILLISECONDS)); + assertTrue("Latch timed out", latch.await(LATCH_TIMEOUT, TimeUnit.MILLISECONDS)); assertThat(tracker.mMainHandler.hasMessages( WifiTracker.MainHandler.MSG_ACCESS_POINT_CHANGED)).isFalse();