Make sure to cleanup when timing out from a single scan

Also fix general failure cases

Bug: 26095183
Change-Id: I88433ff77c0adb4073350ae7a4746b5ee4da5033

5 files changed, 173 insertions, 83 deletions

diff --git a/service/java/com/android/server/wifi/SupplicantWifiScannerImpl.java b/service/java/com/android/server/wifi/SupplicantWifiScannerImpl.java
index 9cc685ffc..7e466a5e6 100644
--- a/service/java/com/android/server/wifi/SupplicantWifiScannerImpl.java
+++ b/service/java/com/android/server/wifi/SupplicantWifiScannerImpl.java
@@ -48,6 +48,9 @@ public class SupplicantWifiScannerImpl extends WifiScannerImpl implements Handle
     private static final String TAG = "SupplicantWifiScannerImpl";
     private static final boolean DBG = false;
 
+    public static final String BACKGROUND_PERIOD_ALARM_TAG = TAG + " Background Scan Period";
+    public static final String TIMEOUT_ALARM_TAG = TAG + " Scan Timeout";
+
     private static final int SCAN_BUFFER_CAPACITY = 10;
     private static final int MAX_APS_PER_SCAN = 32;
     private static final int MAX_SCAN_BUCKETS = 16;
@@ -94,6 +97,14 @@ public class SupplicantWifiScannerImpl extends WifiScannerImpl implements Handle
     private boolean mHwPnoRunning = false;
     private final boolean mHwPnoScanSupported;
 
+    /**
+     * Duration to wait before timing out a scan.
+     *
+     * The expected behavior is that the hardware will return a failed scan if it does not
+     * complete, but timeout just in case it does not.
+     */
+    private static final long SCAN_TIMEOUT_MS = 10000;
+
     AlarmManager.OnAlarmListener mScanPeriodListener = new AlarmManager.OnAlarmListener() {
             public void onAlarm() {
                 synchronized (mSettingsLock) {
@@ -102,6 +113,14 @@ public class SupplicantWifiScannerImpl extends WifiScannerImpl implements Handle
             }
         };
 
+    AlarmManager.OnAlarmListener mScanTimeoutListener = new AlarmManager.OnAlarmListener() {
+            public void onAlarm() {
+                synchronized (mSettingsLock) {
+                    handleScanTimeout();
+                }
+            }
+        };
+
     public SupplicantWifiScannerImpl(Context context, WifiNative wifiNative,
             ChannelHelper channelHelper, Looper looper) {
         mContext = context;
@@ -292,6 +311,12 @@ public class SupplicantWifiScannerImpl extends WifiScannerImpl implements Handle
         }
     }
 
+    private void handleScanTimeout() {
+        Log.e(TAG, "Timed out waiting for scan result from supplicant");
+        reportScanFailure();
+        processPendingScans();
+    }
+
     private void processPendingScans() {
         synchronized (mSettingsLock) {
             // Wait for the active scan result to come back to reschedule other scans,
@@ -366,8 +391,7 @@ public class SupplicantWifiScannerImpl extends WifiScannerImpl implements Handle
                     mBackgroundScanPeriodPending = false;
                     mAlarmManager.set(AlarmManager.RTC_WAKEUP,
                             System.currentTimeMillis() + mBackgroundScanSettings.base_period_ms,
-                            "SupplicantWifiScannerImpl Period",
-                            mScanPeriodListener, mEventHandler);
+                            BACKGROUND_PERIOD_ALARM_TAG, mScanPeriodListener, mEventHandler);
                 }
             }
 
@@ -409,6 +433,9 @@ public class SupplicantWifiScannerImpl extends WifiScannerImpl implements Handle
                             + ", background=" + newScanSettings.backgroundScanActive
                             + ", single=" + newScanSettings.singleScanActive);
                     mLastScanSettings = newScanSettings;
+                    mAlarmManager.set(AlarmManager.RTC_WAKEUP,
+                            System.currentTimeMillis() + SCAN_TIMEOUT_MS,
+                            TIMEOUT_ALARM_TAG, mScanTimeoutListener, mEventHandler);
                 } else {
                     Log.e(TAG, "Failed to start scan, freqs=" + freqs);
                     // indicate scan failure async
@@ -421,7 +448,7 @@ public class SupplicantWifiScannerImpl extends WifiScannerImpl implements Handle
                                 }
                             }
                         });
-                    // TODO if scans fail enough background scans should be failed as well
+                    // TODO(b/27769665) background scans should be failed too if scans fail enough
                 }
             } else if (isHwPnoScanRequired()) {
                 newScanSettings.setHwPnoScan(mPnoEventHandler);
@@ -454,14 +481,13 @@ public class SupplicantWifiScannerImpl extends WifiScannerImpl implements Handle
     public boolean handleMessage(Message msg) {
         switch(msg.what) {
             case WifiMonitor.SCAN_FAILED_EVENT:
-                // TODO indicate failure to caller
                 Log.w(TAG, "Scan failed");
-                synchronized (mSettingsLock) {
-                    mLastScanSettings = null;
-                }
+                mAlarmManager.cancel(mScanTimeoutListener);
+                reportScanFailure();
                 processPendingScans();
                 break;
             case WifiMonitor.SCAN_RESULTS_EVENT:
+                mAlarmManager.cancel(mScanTimeoutListener);
                 pollLatestScanData();
                 processPendingScans();
                 break;
@@ -471,6 +497,19 @@ public class SupplicantWifiScannerImpl extends WifiScannerImpl implements Handle
         return true;
     }
 
+    private void reportScanFailure() {
+        synchronized (mSettingsLock) {
+            if (mLastScanSettings != null) {
+                if (mLastScanSettings.singleScanEventHandler != null) {
+                    mLastScanSettings.singleScanEventHandler
+                            .onScanStatus(WifiNative.WIFI_SCAN_FAILED);
+                }
+                // TODO(b/27769665) background scans should be failed too if scans fail enough
+                mLastScanSettings = null;
+            }
+        }
+    }
+
     private void pollLatestScanData() {
         synchronized (mSettingsLock) {
             if (mLastScanSettings == null) {
diff --git a/service/java/com/android/server/wifi/WifiScanningServiceImpl.java b/service/java/com/android/server/wifi/WifiScanningServiceImpl.java
index 98b4e1e78..0dc6ad733 100644
--- a/service/java/com/android/server/wifi/WifiScanningServiceImpl.java
+++ b/service/java/com/android/server/wifi/WifiScanningServiceImpl.java
@@ -40,7 +40,6 @@ import android.os.Looper;
 import android.os.Message;
 import android.os.Messenger;
 import android.os.RemoteException;
-import android.os.SystemClock;
 import android.os.WorkSource;
 import android.util.ArrayMap;
 import android.util.LocalLog;
@@ -53,7 +52,6 @@ import com.android.internal.util.AsyncChannel;
 import com.android.internal.util.Protocol;
 import com.android.internal.util.State;
 import com.android.internal.util.StateMachine;
-import com.android.internal.util.WakeupMessage;
 import com.android.server.wifi.scanner.BackgroundScanScheduler;
 import com.android.server.wifi.scanner.ChannelHelper;
 import com.android.server.wifi.scanner.ChannelHelper.ChannelCollection;
@@ -228,9 +226,8 @@ public class WifiScanningServiceImpl extends IWifiScanner.Stub {
     private static final int CMD_SCAN_PAUSED                         = BASE + 8;
     private static final int CMD_SCAN_RESTARTED                      = BASE + 9;
     private static final int CMD_SCAN_FAILED                         = BASE + 10;
-    private static final int CMD_SCAN_TIMEOUT                        = BASE + 11;
-    private static final int CMD_PNO_NETWORK_FOUND                   = BASE + 12;
-    private static final int CMD_PNO_SCAN_FAILED                     = BASE + 13;
+    private static final int CMD_PNO_NETWORK_FOUND                   = BASE + 11;
+    private static final int CMD_PNO_SCAN_FAILED                     = BASE + 12;
 
     private final Context mContext;
     private final Looper mLooper;
@@ -462,27 +459,8 @@ public class WifiScanningServiceImpl extends IWifiScanner.Stub {
         }
 
         class ScanningState extends State {
-            /**
-             * Duration to wait before timing out a scan.
-             *
-             * The expected behavior is that the hardware will return a failed scan if it does not
-             * complete, but timeout just in case it does not.
-             * It is possible that there may already be a scan in progress when the scan is
-             * scheduled, so make sure that there is time for two scans to complete.
-             */
-            private static final long SINGLE_SCAN_TIMEOUT_MS = 20000;
-            private WakeupMessage mTimeoutMessage = new WakeupMessage(mContext, getHandler(),
-                    "WifiScanner Single Scan Timeout", CMD_SCAN_TIMEOUT);
-
-            @Override
-            public void enter() {
-                mTimeoutMessage.schedule(
-                        SystemClock.elapsedRealtime() + SINGLE_SCAN_TIMEOUT_MS);
-            }
-
             @Override
             public void exit() {
-                mTimeoutMessage.cancel();
                 // if any scans are still active (never got results available then indicate failure)
                 sendOpFailedToAllAndClear(mActiveScans, WifiScanner.REASON_UNSPECIFIED,
                         "Scan was interrupted");
@@ -505,12 +483,6 @@ public class WifiScanningServiceImpl extends IWifiScanner.Stub {
                                 "Scan failed");
                         transitionTo(mIdleState);
                         return HANDLED;
-                    case CMD_SCAN_TIMEOUT:
-                        sendOpFailedToAllAndClear(mActiveScans, WifiScanner.REASON_UNSPECIFIED,
-                                "Timed out waiting for a scan result");
-                        loge("Timed out waiting for single scan result/failure");
-                        transitionTo(mIdleState);
-                        return HANDLED;
                     default:
                         return NOT_HANDLED;
                 }
diff --git a/tests/wifitests/src/com/android/server/wifi/BaseWifiScannerImplTest.java b/tests/wifitests/src/com/android/server/wifi/BaseWifiScannerImplTest.java
index 0a2671e45..4867e07ad 100644
--- a/tests/wifitests/src/com/android/server/wifi/BaseWifiScannerImplTest.java
+++ b/tests/wifitests/src/com/android/server/wifi/BaseWifiScannerImplTest.java
@@ -218,6 +218,74 @@ public abstract class BaseWifiScannerImplTest {
         verifyNoMoreInteractions(eventHandler);
     }
 
+    /**
+     * Test that a scan failure is reported if a scan times out
+     */
+    @Test
+    public void singleScanFailOnTimeout() {
+        WifiNative.ScanSettings settings = new NativeScanSettingsBuilder()
+                .withBasePeriod(10000)
+                .withMaxApPerScan(10)
+                .addBucketWithBand(10000, WifiScanner.REPORT_EVENT_AFTER_EACH_SCAN,
+                        WifiScanner.WIFI_BAND_24_GHZ)
+                .build();
+
+        WifiNative.ScanEventHandler eventHandler = mock(WifiNative.ScanEventHandler.class);
+
+        ScanResults results = ScanResults.create(0, 2400, 2450, 2450);
+
+        InOrder order = inOrder(eventHandler, mWifiNative);
+
+        // scan succeeds
+        when(mWifiNative.scan(any(Set.class), any(Set.class))).thenReturn(true);
+
+        // start scan
+        assertTrue(mScanner.startSingleScan(settings, eventHandler));
+        mLooper.dispatchAll();
+
+        // Fire timeout
+        mAlarmManager.dispatch(SupplicantWifiScannerImpl.TIMEOUT_ALARM_TAG);
+        mLooper.dispatchAll();
+
+        order.verify(eventHandler).onScanStatus(WifiNative.WIFI_SCAN_FAILED);
+
+        verifyNoMoreInteractions(eventHandler);
+    }
+
+    /**
+     * Test that a scan failure is reported if supplicant sends a scan failed event
+     */
+    @Test
+    public void singleScanFailOnFailedEvent() {
+        WifiNative.ScanSettings settings = new NativeScanSettingsBuilder()
+                .withBasePeriod(10000)
+                .withMaxApPerScan(10)
+                .addBucketWithBand(10000, WifiScanner.REPORT_EVENT_AFTER_EACH_SCAN,
+                        WifiScanner.WIFI_BAND_24_GHZ)
+                .build();
+
+        WifiNative.ScanEventHandler eventHandler = mock(WifiNative.ScanEventHandler.class);
+
+        ScanResults results = ScanResults.create(0, 2400, 2450, 2450);
+
+        InOrder order = inOrder(eventHandler, mWifiNative);
+
+        // scan succeeds
+        when(mWifiNative.scan(any(Set.class), any(Set.class))).thenReturn(true);
+
+        // start scan
+        assertTrue(mScanner.startSingleScan(settings, eventHandler));
+        mLooper.dispatchAll();
+
+        // Fire failed event
+        mWifiMonitor.sendMessage(mWifiNative.getInterfaceName(), WifiMonitor.SCAN_FAILED_EVENT);
+        mLooper.dispatchAll();
+
+        order.verify(eventHandler).onScanStatus(WifiNative.WIFI_SCAN_FAILED);
+
+        verifyNoMoreInteractions(eventHandler);
+    }
+
     @Test
     public void singleScanNullEventHandler() {
         WifiNative.ScanSettings settings = new NativeScanSettingsBuilder()
diff --git a/tests/wifitests/src/com/android/server/wifi/MockAlarmManager.java b/tests/wifitests/src/com/android/server/wifi/MockAlarmManager.java
index c8a1c913d..5edfb5e1b 100644
--- a/tests/wifitests/src/com/android/server/wifi/MockAlarmManager.java
+++ b/tests/wifitests/src/com/android/server/wifi/MockAlarmManager.java
@@ -31,6 +31,7 @@ import com.android.server.wifi.MockAnswerUtil.AnswerWithArguments;
 import java.util.ArrayList;
 import java.util.Iterator;
 import java.util.List;
+import java.util.Objects;
 
 /**
  * Creates an AlarmManager whose alarm dispatch can be controlled
@@ -60,23 +61,32 @@ public class MockAlarmManager {
     }
 
     /**
-     * Dispatch all pending alarms
-     * @return the number of alarms that were dispatched
+     * Dispatch a pending alarm with the given tag
+     * @return if any alarm was dispatched
      */
-    public int dispatchAll() {
-        int count = 0;
-        while (mPendingAlarms.size() > 0) {
-            mPendingAlarms.remove(0).dispatch();
-            ++count;
+    public boolean dispatch(String tag) {
+        for (int i = 0; i < mPendingAlarms.size(); ++i) {
+            PendingAlarm alarm = mPendingAlarms.get(i);
+            if (Objects.equals(tag, alarm.getTag())) {
+                mPendingAlarms.remove(i);
+                alarm.dispatch();
+                return true;
+            }
         }
-        return count;
+        return false;
     }
 
     /**
-     * @return the number of alarms that are currently pending
+     * @return if an alarm with the given tag is pending
      */
-    public int getPendingCount() {
-        return mPendingAlarms.size();
+    public boolean isPending(String tag) {
+        for (int i = 0; i < mPendingAlarms.size(); ++i) {
+            PendingAlarm alarm = mPendingAlarms.get(i);
+            if (Objects.equals(tag, alarm.getTag())) {
+                return true;
+            }
+        }
+        return false;
     }
 
     private static class PendingAlarm {
@@ -101,6 +111,10 @@ public class MockAlarmManager {
         public Runnable getCallback() {
             return mCallback;
         }
+
+        public String getTag() {
+            return mTag;
+        }
     }
 
     private class SetListenerAnswer extends AnswerWithArguments {
diff --git a/tests/wifitests/src/com/android/server/wifi/SupplicantWifiScannerTest.java b/tests/wifitests/src/com/android/server/wifi/SupplicantWifiScannerTest.java
index f3adb119e..cc3a20d69 100644
--- a/tests/wifitests/src/com/android/server/wifi/SupplicantWifiScannerTest.java
+++ b/tests/wifitests/src/com/android/server/wifi/SupplicantWifiScannerTest.java
@@ -20,13 +20,8 @@ import static com.android.server.wifi.ScanTestUtil.NativeScanSettingsBuilder;
 import static com.android.server.wifi.ScanTestUtil.assertScanDatasEquals;
 import static com.android.server.wifi.ScanTestUtil.createFreqSet;
 
-import static org.junit.Assert.assertEquals;
-import static org.mockito.Mockito.any;
-import static org.mockito.Mockito.eq;
-import static org.mockito.Mockito.inOrder;
-import static org.mockito.Mockito.mock;
-import static org.mockito.Mockito.verifyNoMoreInteractions;
-import static org.mockito.Mockito.when;
+import static org.junit.Assert.*;
+import static org.mockito.Mockito.*;
 
 import android.net.wifi.ScanResult;
 import android.net.wifi.WifiScanner;
@@ -324,15 +319,15 @@ public class SupplicantWifiScannerTest extends BaseWifiScannerImplTest {
         // Start scan
         mScanner.startBatchedScan(settings, eventHandler);
 
-        assertEquals("alarm for next period", 1, mAlarmManager.getPendingCount());
+        assertBackgroundPeriodAlarmPending();
 
         expectFailedScanStart(order, eventHandler,
                 expectedBandScanFreqs(WifiScanner.WIFI_BAND_24_GHZ), new HashSet<Integer>());
 
         // Fire alarm to start next scan
-        dispatchOnlyAlarm();
+        dispatchBackgroundPeriodAlarm();
 
-        assertEquals("alarm for next period", 1, mAlarmManager.getPendingCount());
+        assertBackgroundPeriodAlarmPending();
 
         expectFailedScanStart(order, eventHandler,
                 expectedBandScanFreqs(WifiScanner.WIFI_BAND_24_GHZ), new HashSet<Integer>());
@@ -359,15 +354,15 @@ public class SupplicantWifiScannerTest extends BaseWifiScannerImplTest {
         // Start scan
         mScanner.startBatchedScan(settings, eventHandler);
 
-        assertEquals("alarm for next period", 1, mAlarmManager.getPendingCount());
+        assertBackgroundPeriodAlarmPending();
 
         expectFailedEventScan(order, eventHandler,
                 expectedBandScanFreqs(WifiScanner.WIFI_BAND_24_GHZ), new HashSet<Integer>());
 
         // Fire alarm to start next scan
-        dispatchOnlyAlarm();
+        dispatchBackgroundPeriodAlarm();
 
-        assertEquals("alarm for next period", 1, mAlarmManager.getPendingCount());
+        assertBackgroundPeriodAlarmPending();
 
         expectFailedEventScan(order, eventHandler,
                 expectedBandScanFreqs(WifiScanner.WIFI_BAND_24_GHZ), new HashSet<Integer>());
@@ -406,19 +401,18 @@ public class SupplicantWifiScannerTest extends BaseWifiScannerImplTest {
         // Start scan
         mScanner.startBatchedScan(settings, eventHandler);
 
-        assertEquals("alarm for next period", 1, mAlarmManager.getPendingCount());
+        assertBackgroundPeriodAlarmPending();
 
         expectSuccessfulBackgroundScan(order, eventHandler, expectedPeriods[0], 0);
 
-        // alarm for next period
-        assertEquals(1, mAlarmManager.getPendingCount());
+        assertBackgroundPeriodAlarmPending();
 
         mScanner.pauseBatchedScan();
 
         // onPause callback (previous results were flushed)
         order.verify(eventHandler).onScanPaused(new WifiScanner.ScanData[0]);
 
-        assertEquals("no pending alarms", 0, mAlarmManager.getPendingCount());
+        assertBackgroundPeriodAlarmNotPending();
 
         mScanner.restartBatchedScan();
 
@@ -461,10 +455,7 @@ public class SupplicantWifiScannerTest extends BaseWifiScannerImplTest {
         // Start scan
         mScanner.startBatchedScan(settings, eventHandler);
 
-        assertEquals("alarm for next period", 1, mAlarmManager.getPendingCount());
-
-        // alarm for next period
-        assertEquals(1, mAlarmManager.getPendingCount());
+        assertBackgroundPeriodAlarmPending();
 
         order.verify(mWifiNative).scan(eq(expectedPeriods[0].getScanFreqs()), any(Set.class));
 
@@ -473,7 +464,7 @@ public class SupplicantWifiScannerTest extends BaseWifiScannerImplTest {
         // onPause callback (no pending results)
         order.verify(eventHandler).onScanPaused(new WifiScanner.ScanData[0]);
 
-        assertEquals("no pending alarms", 0, mAlarmManager.getPendingCount());
+        assertBackgroundPeriodAlarmNotPending();
 
         // Setup scan results
         when(mWifiNative.getScanResults()).thenReturn(expectedPeriods[0]
@@ -537,19 +528,18 @@ public class SupplicantWifiScannerTest extends BaseWifiScannerImplTest {
         // Start scan
         mScanner.startBatchedScan(settings, eventHandler);
 
-        assertEquals("alarm for next period", 1, mAlarmManager.getPendingCount());
+        assertBackgroundPeriodAlarmPending();
 
         expectSuccessfulBackgroundScan(order, eventHandler, expectedPeriods[0], 0);
 
-        // alarm for next period
-        assertEquals(1, mAlarmManager.getPendingCount());
+        assertBackgroundPeriodAlarmPending();
 
         mScanner.pauseBatchedScan();
 
         // onPause callback (previous results were flushed)
         order.verify(eventHandler).onScanPaused(new WifiScanner.ScanData[0]);
 
-        assertEquals("no pending alarms", 0, mAlarmManager.getPendingCount());
+        assertBackgroundPeriodAlarmNotPending();
 
         // Start new scan
         mScanner.startBatchedScan(settings2, eventHandler);
@@ -578,16 +568,12 @@ public class SupplicantWifiScannerTest extends BaseWifiScannerImplTest {
 
         for (int i = 0; i < expectedPeriods.length; ++i) {
             ScanPeriod period = expectedPeriods[i];
-            if (period == null) { // no scan should be scheduled
-                // alarm for next period
-                assertEquals("alarm for next period", 1, mAlarmManager.getPendingCount());
-            } else {
-                assertEquals("alarm for next period", 1, mAlarmManager.getPendingCount());
-
+            assertBackgroundPeriodAlarmPending();
+            if (period != null) { // scan should be scheduled
                 expectSuccessfulBackgroundScan(order, eventHandler, expectedPeriods[i], i);
             }
             if (i < expectedPeriods.length - 1) {
-                dispatchOnlyAlarm();
+                dispatchBackgroundPeriodAlarm();
             }
         }
 
@@ -675,8 +661,19 @@ public class SupplicantWifiScannerTest extends BaseWifiScannerImplTest {
         // TODO: verify failure event
     }
 
-    private void dispatchOnlyAlarm() {
-        assertEquals("dispatch only one alarm", 1, mAlarmManager.dispatchAll());
+    private void assertBackgroundPeriodAlarmPending() {
+        assertTrue("background period alarm not pending",
+                mAlarmManager.isPending(SupplicantWifiScannerImpl.BACKGROUND_PERIOD_ALARM_TAG));
+    }
+
+    private void assertBackgroundPeriodAlarmNotPending() {
+        assertFalse("background period alarm is pending",
+                mAlarmManager.isPending(SupplicantWifiScannerImpl.BACKGROUND_PERIOD_ALARM_TAG));
+    }
+
+    private void dispatchBackgroundPeriodAlarm() {
+        assertTrue("dispatch background period alarm",
+                mAlarmManager.dispatch(SupplicantWifiScannerImpl.BACKGROUND_PERIOD_ALARM_TAG));
         mLooper.dispatchAll();
     }