From ac7b43616e8329fb19c1cfd6ec33637f4b0a1090 Mon Sep 17 00:00:00 2001 From: Sundeep Ghuman Date: Wed, 8 Feb 2017 17:19:27 -0800 Subject: [PATCH] Serially (un/)register score cache in WifiTracker. Before the cache unregistration logic was posted to the worker thread. However, when WifiSettings is quit, the onDestroy method immediately kills the WorkHandler. This caused unregistration to never occur, which left the cache registered and the listener to try to post work on the dead worker thread. The cache is now registered and unregistered serially in start and stop tracking, respectively. Furthermore, the cachelistener is running on the mainHandler as it is a lightweight task (posting to work handler). Bug: 34891717 Test: runtest --path frameworks/base/packages/SettingsLib/tests/integ/src/com/android/settingslib/wifi/WifiTrackerTest.java Change-Id: I7739fdbe036be5b2870be45f33f670fd69d69af1 --- .../android/settingslib/wifi/WifiTracker.java | 24 ++++------- .../settingslib/wifi/WifiTrackerTest.java | 42 +++++++++++++++---- 2 files changed, 40 insertions(+), 26 deletions(-) diff --git a/packages/SettingsLib/src/com/android/settingslib/wifi/WifiTracker.java b/packages/SettingsLib/src/com/android/settingslib/wifi/WifiTracker.java index c6179943b4d31..0ec16ae268432 100644 --- a/packages/SettingsLib/src/com/android/settingslib/wifi/WifiTracker.java +++ b/packages/SettingsLib/src/com/android/settingslib/wifi/WifiTracker.java @@ -41,7 +41,6 @@ import android.os.Handler; import android.os.Looper; import android.os.Message; import android.provider.Settings; -import android.support.annotation.WorkerThread; import android.util.ArraySet; import android.util.Log; import android.util.SparseArray; @@ -211,7 +210,7 @@ public class WifiTracker { mNetworkScoreManager = networkScoreManager; - mScoreCache = new WifiNetworkScoreCache(context, new CacheListener(mWorkHandler) { + mScoreCache = new WifiNetworkScoreCache(context, new CacheListener(mMainHandler) { @Override public void networkCacheUpdated(List networks) { if (Log.isLoggable(TAG, Log.VERBOSE)) { @@ -282,12 +281,7 @@ public class WifiTracker { * then forceUpdate() must be called to populate getAccessPoints(). */ public void startTracking() { - mWorkHandler.post(new Runnable() { - @Override - public void run() { - registerScoreCache(); - } - }); + registerScoreCache(); mContext.getContentResolver().registerContentObserver( Settings.Global.getUriFor(Settings.Global.NETWORK_SCORING_UI_ENABLED), @@ -305,7 +299,6 @@ public class WifiTracker { } } - @WorkerThread private void registerScoreCache() { mNetworkScoreManager.registerNetworkScoreCache( NetworkKey.TYPE_WIFI, @@ -341,20 +334,17 @@ public class WifiTracker { } pauseScanning(); - mWorkHandler.post(new Runnable() { - @Override - public void run() { - unregisterAndClearScoreCache(); - } - }); + unregisterAndClearScoreCache(); + mContext.getContentResolver().unregisterContentObserver(mObserver); } - @WorkerThread private void unregisterAndClearScoreCache() { - mRequestedScores.clear(); mNetworkScoreManager.unregisterNetworkScoreCache(NetworkKey.TYPE_WIFI, mScoreCache); mScoreCache.clearScores(); + + // Clear the scores on the work handler to avoid concurrent modification exceptions + mWorkHandler.post(() -> mRequestedScores.clear()); } /** 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 08736c77c409a..0f220aaf5574e 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 @@ -72,7 +72,7 @@ import java.util.concurrent.TimeUnit; public class WifiTrackerTest { private static final String TAG = "WifiTrackerTest"; - private static final int LATCH_TIMEOUT = 2000; + private static final int LATCH_TIMEOUT = 4000; private static final String SSID_1 = "ssid1"; private static final String BSSID_1 = "00:00:00:00:00:00"; @@ -226,7 +226,7 @@ public class WifiTrackerTest { mAccessPointsChangedLatch = new CountDownLatch(1); sendScanResultsAndProcess(tracker); - mAccessPointsChangedLatch.await(LATCH_TIMEOUT, TimeUnit.MILLISECONDS); + assertTrue(mAccessPointsChangedLatch.await(LATCH_TIMEOUT, TimeUnit.MILLISECONDS)); return tracker; } @@ -242,7 +242,8 @@ public class WifiTrackerTest { true, mockWifiManager, mockConnectivityManager, - mockNetworkScoreManager, mMainLooper + mockNetworkScoreManager, + mMainLooper ); return tracker; @@ -257,7 +258,7 @@ public class WifiTrackerTest { latch.countDown(); } }); - latch.await(LATCH_TIMEOUT, TimeUnit.MILLISECONDS); + assertTrue("Latch timed out", latch.await(LATCH_TIMEOUT, TimeUnit.MILLISECONDS)); } private void sendScanResultsAndProcess(WifiTracker tracker) throws InterruptedException { @@ -265,7 +266,8 @@ public class WifiTrackerTest { Intent i = new Intent(WifiManager.SCAN_RESULTS_AVAILABLE_ACTION); tracker.mReceiver.onReceive(mContext, i); - mAccessPointsChangedLatch.await(LATCH_TIMEOUT, TimeUnit.MILLISECONDS); + assertTrue("Latch timed out", + mAccessPointsChangedLatch.await(LATCH_TIMEOUT, TimeUnit.MILLISECONDS)); } private void updateScores() { @@ -348,7 +350,7 @@ public class WifiTrackerTest { // Test unregister tracker.stopTracking(); - latch.await(LATCH_TIMEOUT, TimeUnit.MILLISECONDS); + assertTrue("Latch timed out", latch.await(LATCH_TIMEOUT, TimeUnit.MILLISECONDS)); verify(mockNetworkScoreManager) .unregisterNetworkScoreCache(NetworkKey.TYPE_WIFI, scoreCache); } @@ -359,12 +361,15 @@ public class WifiTrackerTest { WifiTracker tracker = createTrackerAndInjectInitialScanResults(); tracker.stopTracking(); + android.util.Log.d("WifiTrackerTest", "Clearing previously captured requested keys"); mRequestedKeys.clear(); - mRequestScoresLatch = new CountDownLatch(2); + mRequestScoresLatch = new CountDownLatch(1); startTracking(tracker); - mRequestScoresLatch.await(LATCH_TIMEOUT, TimeUnit.MILLISECONDS); + assertTrue("Latch timed out", + mRequestScoresLatch.await(LATCH_TIMEOUT, TimeUnit.MILLISECONDS)); + android.util.Log.d("WifiTrackerTest", "requested keys: " + mRequestedKeys); assertTrue(mRequestedKeys.contains(NETWORK_KEY_1)); assertTrue(mRequestedKeys.contains(NETWORK_KEY_2)); } @@ -379,6 +384,8 @@ 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); updateScores(); mAccessPointsChangedLatch.await(LATCH_TIMEOUT, TimeUnit.MILLISECONDS); @@ -462,6 +469,8 @@ public class WifiTrackerTest { @Test public void scoresShouldBeRequestedForNewScanResultOnly() throws InterruptedException { + // Scores can be requested together or serially depending on how the scan results are + // processed. mRequestScoresLatch = new CountDownLatch(2); WifiTracker tracker = createTrackerAndInjectInitialScanResults(); mRequestScoresLatch.await(LATCH_TIMEOUT, TimeUnit.MILLISECONDS); @@ -484,9 +493,24 @@ public class WifiTrackerTest { mRequestScoresLatch = new CountDownLatch(1); sendScanResultsAndProcess(tracker); - mRequestScoresLatch.await(LATCH_TIMEOUT, TimeUnit.MILLISECONDS); + assertTrue(mRequestScoresLatch.await(LATCH_TIMEOUT, TimeUnit.MILLISECONDS)); assertEquals(1, mRequestedKeys.size()); assertTrue(mRequestedKeys.contains(new NetworkKey(new WifiKey('"' + ssid + '"', bssid)))); } + + @Test + public void scoreCacheAndListenerShouldBeUnregisteredWhenStopTrackingIsCalled() throws Exception + { + WifiTracker tracker = createTrackerAndInjectInitialScanResults(); + WifiNetworkScoreCache cache = mScoreCacheCaptor.getValue(); + + tracker.stopTracking(); + verify(mockNetworkScoreManager).unregisterNetworkScoreCache(NetworkKey.TYPE_WIFI, cache); + + // Verify listener is unregistered so updating a score does not throw an error by posting + // a message to the dead work handler + mWorkerThread.quit(); + updateScores(); + } }