Check that network evaluators have reported their choice.

Network evaluators are supposed to call onConnectableListener to report
all networks they want as candidates. This should include the one that
they return.

Add a check in WifiNetworkSelector to ensure this is the case, and
Log.wtf if not.

Add checks to ScoredNetworkEvaluatorTest for all the non-null choices
in the previously existing unit tests. Add a new unit test to cover
the case where a new ephemeral network is created.

Fix ScoredNetworkEvaluator to call onConnectableListener in case of
a new ephemeral network.

Bug: 128938468
Test: atest ScoredNetworkEvaluatorTest
Change-Id: I539dfaa7e5951e19d319a95a90d0b7cc5f3d1a06

3 files changed, 110 insertions, 6 deletions

diff --git a/service/java/com/android/server/wifi/ScoredNetworkEvaluator.java b/service/java/com/android/server/wifi/ScoredNetworkEvaluator.java
index 0d6af75b0..66222691f 100644
--- a/service/java/com/android/server/wifi/ScoredNetworkEvaluator.java
+++ b/service/java/com/android/server/wifi/ScoredNetworkEvaluator.java
@@ -154,7 +154,7 @@ public class ScoredNetworkEvaluator implements WifiNetworkSelector.NetworkEvalua
             // Track scan results for open wifi networks
             if (configuredNetwork == null) {
                 if (ScanResultUtil.isScanResultForOpenNetwork(scanResult)) {
-                    scoreTracker.trackUntrustedCandidate(scanResult);
+                    scoreTracker.trackUntrustedCandidate(scanDetail);
                 }
                 continue;
             }
@@ -184,7 +184,8 @@ public class ScoredNetworkEvaluator implements WifiNetworkSelector.NetworkEvalua
             onConnectableListener.onConnectable(scanDetail, configuredNetwork, 0);
         }
 
-        return scoreTracker.getCandidateConfiguration();
+
+        return scoreTracker.getCandidateConfiguration(onConnectableListener);
     }
 
     /** Used to track the network with the highest score. */
@@ -198,6 +199,7 @@ public class ScoredNetworkEvaluator implements WifiNetworkSelector.NetworkEvalua
         private WifiConfiguration mEphemeralConfig;
         private WifiConfiguration mSavedConfig;
         private ScanResult mScanResultCandidate;
+        private ScanDetail mScanDetailCandidate;
 
         /**
          * Returns the available external network score or null if no score is available.
@@ -219,12 +221,14 @@ public class ScoredNetworkEvaluator implements WifiNetworkSelector.NetworkEvalua
             return null;
         }
 
-        /** Track an untrusted {@link ScanResult}. */
-        void trackUntrustedCandidate(ScanResult scanResult) {
+        /** Track an untrusted {@link ScanDetail}. */
+        void trackUntrustedCandidate(ScanDetail scanDetail) {
+            ScanResult scanResult = scanDetail.getScanResult();
             Integer score = getNetworkScore(scanResult, false /* isCurrentNetwork */);
             if (score != null && score > mHighScore) {
                 mHighScore = score;
                 mScanResultCandidate = scanResult;
+                mScanDetailCandidate = scanDetail;
                 mBestCandidateType = EXTERNAL_SCORED_UNTRUSTED_NETWORK;
                 debugLog(WifiNetworkSelector.toScanId(scanResult)
                         + " becomes the new untrusted candidate.");
@@ -241,6 +245,7 @@ public class ScoredNetworkEvaluator implements WifiNetworkSelector.NetworkEvalua
             if (score != null && score > mHighScore) {
                 mHighScore = score;
                 mScanResultCandidate = scanResult;
+                mScanDetailCandidate = null;
                 mBestCandidateType = EXTERNAL_SCORED_UNTRUSTED_NETWORK;
                 mEphemeralConfig = config;
                 mWifiConfigManager.setNetworkCandidateScanResult(config.networkId, scanResult, 0);
@@ -262,6 +267,7 @@ public class ScoredNetworkEvaluator implements WifiNetworkSelector.NetworkEvalua
                 mHighScore = score;
                 mSavedConfig = config;
                 mScanResultCandidate = scanResult;
+                mScanDetailCandidate = null;
                 mBestCandidateType = EXTERNAL_SCORED_SAVED_NETWORK;
                 mWifiConfigManager.setNetworkCandidateScanResult(config.networkId, scanResult, 0);
                 debugLog(WifiNetworkSelector.toScanId(scanResult)
@@ -271,7 +277,8 @@ public class ScoredNetworkEvaluator implements WifiNetworkSelector.NetworkEvalua
 
         /** Returns the best candidate network tracked by this {@link ScoreTracker}. */
         @Nullable
-        WifiConfiguration getCandidateConfiguration() {
+        WifiConfiguration getCandidateConfiguration(
+                @NonNull OnConnectableListener onConnectableListener) {
             int candidateNetworkId = WifiConfiguration.INVALID_NETWORK_ID;
             switch (mBestCandidateType) {
                 case ScoreTracker.EXTERNAL_SCORED_UNTRUSTED_NETWORK:
@@ -305,6 +312,11 @@ public class ScoredNetworkEvaluator implements WifiNetworkSelector.NetworkEvalua
                         break;
                     }
                     candidateNetworkId = result.getNetworkId();
+                    if (mScanDetailCandidate == null) {
+                        // This should never happen, but if it does, WNS will log a wtf.
+                        // A message here might help with the diagnosis.
+                        Log.e(TAG, "mScanDetailCandidate is null!");
+                    }
                     mWifiConfigManager.setNetworkCandidateScanResult(candidateNetworkId,
                             mScanResultCandidate, 0);
                     mLocalLog.log(String.format("new ephemeral candidate %s network ID:%d, "
@@ -324,7 +336,13 @@ public class ScoredNetworkEvaluator implements WifiNetworkSelector.NetworkEvalua
                     mLocalLog.log("ScoredNetworkEvaluator did not see any good candidates.");
                     break;
             }
-            return mWifiConfigManager.getConfiguredNetwork(candidateNetworkId);
+            WifiConfiguration ans = mWifiConfigManager.getConfiguredNetwork(
+                    candidateNetworkId);
+            if (ans != null && mScanDetailCandidate != null) {
+                // This is a newly created config, so we need to call onConnectable.
+                onConnectableListener.onConnectable(mScanDetailCandidate, ans, 0);
+            }
+            return ans;
         }
     }
 
diff --git a/service/java/com/android/server/wifi/WifiNetworkSelector.java b/service/java/com/android/server/wifi/WifiNetworkSelector.java
index a96f457bd..0b6f03e19 100644
--- a/service/java/com/android/server/wifi/WifiNetworkSelector.java
+++ b/service/java/com/android/server/wifi/WifiNetworkSelector.java
@@ -30,6 +30,7 @@ import android.net.wifi.WifiInfo;
 import android.os.Process;
 import android.text.TextUtils;
 import android.util.ArrayMap;
+import android.util.ArraySet;
 import android.util.LocalLog;
 import android.util.Log;
 import android.util.Pair;
@@ -659,6 +660,7 @@ public class WifiNetworkSelector {
         // Determine the weight for the last user selection
         final int lastUserSelectedNetworkId = mWifiConfigManager.getLastSelectedNetwork();
         final double lastSelectionWeight = calculateLastSelectionWeight();
+        final ArraySet<Integer> mNetworkIds = new ArraySet<>();
 
         // Go through the registered network evaluators in order
         WifiConfiguration selectedNetwork = null;
@@ -674,6 +676,7 @@ public class WifiNetworkSelector {
                     (scanDetail, config, score) -> {
                         if (config != null) {
                             mConnectableNetworks.add(Pair.create(scanDetail, config));
+                            mNetworkIds.add(config.networkId);
                             if (config.networkId == lastUserSelectedNetworkId) {
                                 wifiCandidates.add(scanDetail, config,
                                         registeredEvaluator.getId(), score, lastSelectionWeight);
@@ -685,6 +688,10 @@ public class WifiNetworkSelector {
                                     evaluatorIdToNominatorId(registeredEvaluator.getId()));
                         }
                     });
+            if (choice != null && !mNetworkIds.contains(choice.networkId)) {
+                Log.wtf(TAG, registeredEvaluator.getName()
+                        + " failed to report choice with noConnectibleListener");
+            }
             if (selectedNetwork == null && choice != null) {
                 selectedNetwork = choice; // First one wins
                 localLog(registeredEvaluator.getName() + " selects "
diff --git a/tests/wifitests/src/com/android/server/wifi/ScoredNetworkEvaluatorTest.java b/tests/wifitests/src/com/android/server/wifi/ScoredNetworkEvaluatorTest.java
index 4643876c0..d1de03cbc 100644
--- a/tests/wifitests/src/com/android/server/wifi/ScoredNetworkEvaluatorTest.java
+++ b/tests/wifitests/src/com/android/server/wifi/ScoredNetworkEvaluatorTest.java
@@ -75,6 +75,7 @@ public class ScoredNetworkEvaluatorTest {
     @Mock private WifiPermissionsUtil mWifiPermissionsUtil;
     @Mock private OnConnectableListener mOnConnectableListener;
     @Captor private ArgumentCaptor<NetworkKey[]> mNetworkKeyArrayCaptor;
+    @Captor private ArgumentCaptor<WifiConfiguration> mWifiConfigCaptor;
 
     private WifiNetworkScoreCache mScoreCache;
     private ScoredNetworkEvaluator mScoredNetworkEvaluator;
@@ -272,6 +273,51 @@ public class ScoredNetworkEvaluatorTest {
     }
 
     /**
+     * When we have created a new ephemeral network, make sure that mOnConnectableListener
+     * is called.
+     */
+    @Test
+    public void testEvaluateNetworks_newEphemeralNetworkMustBeReportedAsConnectable() {
+        String[] ssids = {"\"test1\"", "\"test2\""};
+        String[] bssids = {"6c:f3:7f:ae:8c:f3", "6c:f3:7f:ae:8c:f4"};
+        int[] freqs = {2470, 2437};
+        String[] caps = {"[WPA2-PSK][ESS]", "[ESS]"};
+        int[] levels = {mThresholdQualifiedRssi2G + 8, mThresholdQualifiedRssi2G + 10};
+        Integer[] scores = {null, 120};
+        boolean[] meteredHints = {false, false};
+
+        List<ScanDetail> scanDetails = WifiNetworkSelectorTestUtil.buildScanDetails(
+                ssids, bssids, freqs, caps, levels, mClock);
+        WifiNetworkSelectorTestUtil.configureScoreCache(mScoreCache,
+                scanDetails, scores, meteredHints);
+
+        ScanResult scanResult = scanDetails.get(1).getScanResult();
+        WifiConfiguration ephemeralNetworkConfig = WifiNetworkSelectorTestUtil
+                .setupEphemeralNetwork(mWifiConfigManager, 1, scanDetails.get(1), meteredHints[1]);
+
+        // No saved networks.
+        when(mWifiConfigManager.getConfiguredNetworkForScanDetailAndCache(any(ScanDetail.class)))
+                .thenReturn(null);
+
+        // But when we create one, this is should be it.
+        when(mWifiConfigManager.addOrUpdateNetwork(any(), anyInt()))
+                .thenReturn(new NetworkUpdateResult(1));
+
+        // Untrusted networks allowed.
+        WifiConfiguration candidate = mScoredNetworkEvaluator.evaluateNetworks(scanDetails,
+                null, null, false, true, mOnConnectableListener);
+
+        WifiConfigurationTestUtil.assertConfigurationEqual(ephemeralNetworkConfig, candidate);
+        WifiNetworkSelectorTestUtil.verifySelectedScanResult(mWifiConfigManager,
+                scanResult, candidate);
+        assertEquals(meteredHints[1], candidate.meteredHint);
+        verify(mOnConnectableListener, atLeastOnce())
+                .onConnectable(any(), mWifiConfigCaptor.capture(), anyInt());
+        assertTrue(mWifiConfigCaptor.getAllValues().stream()
+                .anyMatch(c -> c.networkId == candidate.networkId));
+    }
+
+    /**
      * When no saved networks available, choose the available ephemeral networks
      * if untrusted networks are allowed.
      */
@@ -306,6 +352,10 @@ public class ScoredNetworkEvaluatorTest {
         WifiNetworkSelectorTestUtil.verifySelectedScanResult(mWifiConfigManager,
                 scanResult, candidate);
         assertEquals(meteredHints[1], candidate.meteredHint);
+        verify(mOnConnectableListener, atLeastOnce())
+                .onConnectable(any(), mWifiConfigCaptor.capture(), anyInt());
+        assertTrue(mWifiConfigCaptor.getAllValues().stream()
+                .anyMatch(c -> c.networkId == candidate.networkId));
     }
 
     /**
@@ -346,6 +396,10 @@ public class ScoredNetworkEvaluatorTest {
         WifiNetworkSelectorTestUtil.verifySelectedScanResult(mWifiConfigManager,
                 scanResults[1], candidate);
         assertEquals(meteredHints[1], candidate.meteredHint);
+        verify(mOnConnectableListener, atLeastOnce())
+                .onConnectable(any(), mWifiConfigCaptor.capture(), anyInt());
+        assertTrue(mWifiConfigCaptor.getAllValues().stream()
+                .anyMatch(c -> c.networkId == candidate.networkId));
     }
 
     /**
@@ -411,6 +465,10 @@ public class ScoredNetworkEvaluatorTest {
         WifiConfigurationTestUtil.assertConfigurationEqual(savedConfigs[0], candidate);
         WifiNetworkSelectorTestUtil.verifySelectedScanResult(mWifiConfigManager,
                 scanDetails.get(0).getScanResult(), candidate);
+        verify(mOnConnectableListener, atLeastOnce())
+                .onConnectable(any(), mWifiConfigCaptor.capture(), anyInt());
+        assertTrue(mWifiConfigCaptor.getAllValues().stream()
+                .anyMatch(c -> c.networkId == candidate.networkId));
     }
 
     /**
@@ -443,6 +501,10 @@ public class ScoredNetworkEvaluatorTest {
         WifiConfigurationTestUtil.assertConfigurationEqual(savedConfigs[1], candidate);
         WifiNetworkSelectorTestUtil.verifySelectedScanResult(mWifiConfigManager,
                 scanDetails.get(1).getScanResult(), candidate);
+        verify(mOnConnectableListener, atLeastOnce())
+                .onConnectable(any(), mWifiConfigCaptor.capture(), anyInt());
+        assertTrue(mWifiConfigCaptor.getAllValues().stream()
+                .anyMatch(c -> c.networkId == candidate.networkId));
     }
 
     /**
@@ -476,6 +538,10 @@ public class ScoredNetworkEvaluatorTest {
         WifiConfigurationTestUtil.assertConfigurationEqual(savedConfigs[0], candidate);
         WifiNetworkSelectorTestUtil.verifySelectedScanResult(mWifiConfigManager,
                 scanDetails.get(0).getScanResult(), candidate);
+        verify(mOnConnectableListener, atLeastOnce())
+                .onConnectable(any(), mWifiConfigCaptor.capture(), anyInt());
+        assertTrue(mWifiConfigCaptor.getAllValues().stream()
+                .anyMatch(c -> c.networkId == candidate.networkId));
     }
 
     /**
@@ -529,6 +595,10 @@ public class ScoredNetworkEvaluatorTest {
         WifiConfigurationTestUtil.assertConfigurationEqual(ephemeralNetworkConfig, candidate);
         WifiNetworkSelectorTestUtil.verifySelectedScanResult(mWifiConfigManager,
                 ephemeralScanResult, candidate);
+        verify(mOnConnectableListener, atLeastOnce())
+                .onConnectable(any(), mWifiConfigCaptor.capture(), anyInt());
+        assertTrue(mWifiConfigCaptor.getAllValues().stream()
+                .anyMatch(c -> c.networkId == candidate.networkId));
     }
 
     /**
@@ -601,6 +671,10 @@ public class ScoredNetworkEvaluatorTest {
         WifiNetworkSelectorTestUtil.verifySelectedScanResult(mWifiConfigManager,
                 scanResults[1], candidate);
         assertEquals(meteredHints[1], candidate.meteredHint);
+        verify(mOnConnectableListener, atLeastOnce())
+                .onConnectable(any(), mWifiConfigCaptor.capture(), anyInt());
+        assertTrue(mWifiConfigCaptor.getAllValues().stream()
+                .anyMatch(c -> c.networkId == candidate.networkId));
     }
 
     /**
@@ -633,5 +707,10 @@ public class ScoredNetworkEvaluatorTest {
         WifiConfigurationTestUtil.assertConfigurationEqual(savedConfigs[1], candidate);
         WifiNetworkSelectorTestUtil.verifySelectedScanResult(mWifiConfigManager,
                 scanDetails.get(1).getScanResult(), candidate);
+        verify(mOnConnectableListener, atLeastOnce())
+                .onConnectable(any(), mWifiConfigCaptor.capture(), anyInt());
+        assertTrue(mWifiConfigCaptor.getAllValues().stream()
+                .anyMatch(c -> c.networkId == candidate.networkId));
+
     }
 }