diff --git a/packages/SettingsLib/src/com/android/settingslib/wifi/WifiTracker.java b/packages/SettingsLib/src/com/android/settingslib/wifi/WifiTracker.java index 314791e3105ab..646b6ba84ebb5 100644 --- a/packages/SettingsLib/src/com/android/settingslib/wifi/WifiTracker.java +++ b/packages/SettingsLib/src/com/android/settingslib/wifi/WifiTracker.java @@ -15,8 +15,8 @@ */ package com.android.settingslib.wifi; +import android.annotation.MainThread; import android.content.BroadcastReceiver; -import android.content.ContentResolver; import android.content.Context; import android.content.Intent; import android.content.IntentFilter; @@ -41,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; @@ -65,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"; @@ -91,40 +90,40 @@ 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; 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(); @@ -138,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; @@ -212,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(); } }); @@ -236,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)); + } } /** @@ -290,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; + } } } @@ -323,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) { - 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; + synchronized (mLock) { + if (mRegistered) { + mContext.unregisterReceiver(mReceiver); + mConnectivityManager.unregisterNetworkCallback(mNetworkCallback); + mRegistered = false; + } + unregisterAndClearScoreCache(); + pauseScanning(); + mContext.getContentResolver().unregisterContentObserver(mObserver); + + 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); } @@ -441,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<>(); @@ -460,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) { @@ -584,7 +603,8 @@ public class WifiTracker { mInternalAccessPoints.clear(); mInternalAccessPoints.addAll(accessPoints); - mMainHandler.scheduleAPCopyingAndCloseWriteLock(); + + mMainHandler.sendEmptyMessage(MainHandler.MSG_ACCESS_POINT_CHANGED); } @VisibleForTesting @@ -641,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(); } } @@ -733,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(); } } }; @@ -749,10 +761,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; @@ -789,10 +802,12 @@ public class WifiTracker { } } - void scheduleAPCopyingAndCloseWriteLock() { - //prevent worker thread from modifying mInternalAccessPoints - mInternalAccessPointsWriteLock.close(); - sendEmptyMessage(MSG_ACCESS_POINT_CHANGED); + void removePendingMessages() { + removeMessages(MSG_ACCESS_POINT_CHANGED); + removeMessages(MSG_CONNECTED_CHANGED); + removeMessages(MSG_WIFI_STATE_CHANGED); + removeMessages(MSG_PAUSE_SCANNING); + removeMessages(MSG_RESUME_SCANNING); } } @@ -801,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); @@ -809,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); @@ -836,11 +859,15 @@ public class WifiTracker { mMainHandler.obtainMessage(MainHandler.MSG_WIFI_STATE_CHANGED, msg.arg1, 0) .sendToTarget(); break; - case MSG_UPDATE_NETWORK_SCORES: - updateNetworkScores(); - break; } } + + private void removePendingMessages() { + removeMessages(MSG_UPDATE_ACCESS_POINTS); + removeMessages(MSG_UPDATE_NETWORK_INFO); + removeMessages(MSG_RESUME); + removeMessages(MSG_UPDATE_WIFI_STATE); + } } @VisibleForTesting @@ -959,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<>(); @@ -974,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) { @@ -994,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/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..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 @@ -20,13 +20,17 @@ 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.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; import android.content.Context; @@ -60,7 +64,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 +132,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 +144,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(); @@ -252,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; } @@ -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)); @@ -462,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 @@ -648,27 +652,33 @@ 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); - 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 { + Network mockNetwork = mock(Network.class); + when(mockWifiManager.getCurrentNetwork()).thenReturn(mockNetwork); + when(mockWifiManager.getConnectionInfo()).thenReturn(CONNECTED_AP_1_INFO); WifiConfiguration configuration = new WifiConfiguration(); @@ -682,13 +692,58 @@ 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(); + 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(); } + + @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 timed out", 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