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

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 314791e3105a..04aaf8fca9c1 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 3e01b34765c0..154fde204c70 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 b938fe2ac2db..621041a4675d 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<AccessPoint>());
         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<AccessPoint>());
         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