Merge "Destroyed networks can't hope to beat champions" into main

service/src/com/android/server/ConnectivityService.java

@@ -5273,7 +5273,14 @@ public class ConnectivityService extends IConnectivityManager.Stub
 
     private boolean isNetworkPotentialSatisfier(
             @NonNull final NetworkAgentInfo candidate, @NonNull final NetworkRequestInfo nri) {
-        // listen requests won't keep up a network satisfying it. If this is not a multilayer
+        // While destroyed network sometimes satisfy requests (including occasionally newly
+        // satisfying requests), *potential* satisfiers are networks that might beat a current
+        // champion if they validate. As such, a destroyed network is never a potential satisfier,
+        // because it's never a good idea to keep a destroyed network in case it validates.
+        // For example, declaring it a potential satisfier would keep an unvalidated destroyed
+        // candidate after it's been replaced by another unvalidated network.
+        if (candidate.isDestroyed()) return false;
+        // Listen requests won't keep up a network satisfying it. If this is not a multilayer
         // request, return immediately. For multilayer requests, check to see if any of the
         // multilayer requests may have a potential satisfier.
         if (!nri.isMultilayerRequest() && (nri.mRequests.get(0).isListen()
@@ -5291,8 +5298,12 @@ public class ConnectivityService extends IConnectivityManager.Stub
             if (req.isListen() || req.isListenForBest()) {
                 continue;
             }
-            // If this Network is already the best Network for a request, or if
-            // there is hope for it to become one if it validated, then it is needed.
+            // If there is hope for this network might validate and subsequently become the best
+            // network for that request, then it is needed. Note that this network can't already
+            // be the best for this request, or it would be the current satisfier, and therefore
+            // there would be no need to call this method to find out if it is a *potential*
+            // satisfier ("unneeded", the only caller, only calls this if this network currently
+            // satisfies no request).
             if (candidate.satisfies(req)) {
                 // As soon as a network is found that satisfies a request, return. Specifically for
                 // multilayer requests, returning as soon as a NetworkAgentInfo satisfies a request

tests/unit/java/com/android/server/connectivityservice/CSDestroyedNetworkTests.kt

@@ -0,0 +1,56 @@
+/*
+ * Copyright (C) 2023 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.android.server
+
+import android.net.NetworkCapabilities
+import android.net.NetworkCapabilities.TRANSPORT_WIFI
+import android.net.NetworkRequest
+import com.android.testutils.RecorderCallback.CallbackEntry.Lost
+import com.android.testutils.TestableNetworkCallback
+import org.junit.Test
+
+private const val LONG_TIMEOUT_MS = 5_000
+
+class CSDestroyedNetworkTests : CSTest() {
+    @Test
+    fun testDestroyNetworkNotKeptWhenUnvalidated() {
+        val nc = NetworkCapabilities.Builder()
+                .addTransportType(TRANSPORT_WIFI)
+                .build()
+
+        val nr = NetworkRequest.Builder()
+                .clearCapabilities()
+                .addTransportType(TRANSPORT_WIFI)
+                .build()
+        val cbRequest = TestableNetworkCallback()
+        val cbCallback = TestableNetworkCallback()
+        cm.requestNetwork(nr, cbRequest)
+        cm.registerNetworkCallback(nr, cbCallback)
+
+        val firstAgent = Agent(nc = nc)
+        firstAgent.connect()
+        cbCallback.expectAvailableCallbacks(firstAgent.network, validated = false)
+
+        firstAgent.unregisterAfterReplacement(LONG_TIMEOUT_MS)
+
+        val secondAgent = Agent(nc = nc)
+        secondAgent.connect()
+        cbCallback.expectAvailableCallbacks(secondAgent.network, validated = false)
+
+        cbCallback.expect<Lost>(timeoutMs = 500) { it.network == firstAgent.network }
+    }
+}

tests/unit/java/com/android/server/connectivityservice/base/CSAgentWrapper.kt

@@ -34,7 +34,6 @@ import android.net.NetworkScore
 import android.net.NetworkTestResultParcelable
 import android.net.networkstack.NetworkStackClientBase
 import android.os.HandlerThread
-import com.android.modules.utils.build.SdkLevel
 import com.android.testutils.RecorderCallback.CallbackEntry.Available
 import com.android.testutils.RecorderCallback.CallbackEntry.Lost
 import com.android.testutils.TestableNetworkCallback
@@ -168,5 +167,6 @@ class CSAgentWrapper(
         cb.eventuallyExpect<Lost> { it.network == agent.network }
     }
 
+    fun unregisterAfterReplacement(timeoutMs: Int) = agent.unregisterAfterReplacement(timeoutMs)
     fun sendLocalNetworkConfig(lnc: LocalNetworkConfig) = agent.sendLocalNetworkConfig(lnc)
 }