From c54aa729c9eb6a0ce758fd5c214ae5522ffecdc4 Mon Sep 17 00:00:00 2001
From: Paul Hu <paulhu@google.com>
Date: Wed, 7 Jun 2023 16:21:34 +0800
Subject: [PATCH] Adjust the notifyNetworkUnrequested on handleRegisterListener

If the MdnsServiceTypeClient was removed when the
SocketCreationCallback#onAllSocketsDestroyed() was called, the
MdnsSocketClientBase#notifyNetworkUnrequested() will not be
called when a listener is unregistered from the
MdnsDiscoveryManager. This is because the serviceTypeClients will
be cleared after the socket is destroyed. However, these dead
listeners will be re-added to the MdnsServiceTypeClient when a
new socket is created. They will then continue to send callbacks
to the NsdService. Therefore, the notifyNetworkUnrequested()
should be moved to the beginning of the handleRegisterListener()
to ensure that the socket unregistration is properly notified.

Bug: 285997766
Test: atest FrameworksNetTests android.net.cts.NsdManagerTest
Change-Id: I31c766305018889f50a7c12c836174c340d01d7f
---
 .../mdns/MdnsDiscoveryManager.java            |  5 ++-
 .../mdns/MdnsDiscoveryManagerTests.java       | 41 +++++++++++++++++++
 2 files changed, 44 insertions(+), 2 deletions(-)

diff --git a/service-t/src/com/android/server/connectivity/mdns/MdnsDiscoveryManager.java b/service-t/src/com/android/server/connectivity/mdns/MdnsDiscoveryManager.java
index d3cbf822b7..39fceb948b 100644
--- a/service-t/src/com/android/server/connectivity/mdns/MdnsDiscoveryManager.java
+++ b/service-t/src/com/android/server/connectivity/mdns/MdnsDiscoveryManager.java
@@ -220,6 +220,9 @@ public class MdnsDiscoveryManager implements MdnsSocketClientBase.Callback {
 
     private void handleUnregisterListener(
             @NonNull String serviceType, @NonNull MdnsServiceBrowserListener listener) {
+        // Unrequested the network.
+        socketClient.notifyNetworkUnrequested(listener);
+
         final List<MdnsServiceTypeClient> serviceTypeClients =
                 perNetworkServiceTypeClients.getByServiceType(serviceType);
         if (serviceTypeClients.isEmpty()) {
@@ -237,8 +240,6 @@ public class MdnsDiscoveryManager implements MdnsSocketClientBase.Callback {
             // No discovery request. Stops the socket client.
             socketClient.stopDiscovery();
         }
-        // Unrequested the network.
-        socketClient.notifyNetworkUnrequested(listener);
     }
 
     @Override
diff --git a/tests/unit/java/com/android/server/connectivity/mdns/MdnsDiscoveryManagerTests.java b/tests/unit/java/com/android/server/connectivity/mdns/MdnsDiscoveryManagerTests.java
index a54a5210a2..a24664e4ac 100644
--- a/tests/unit/java/com/android/server/connectivity/mdns/MdnsDiscoveryManagerTests.java
+++ b/tests/unit/java/com/android/server/connectivity/mdns/MdnsDiscoveryManagerTests.java
@@ -294,6 +294,47 @@ public class MdnsDiscoveryManagerTests {
                 .processResponse(response, ifIndex, NETWORK_1);
     }
 
+    @Test
+    public void testUnregisterListenerAfterSocketDestroyed() throws IOException {
+        // Create a ServiceTypeClient for SERVICE_TYPE_1
+        final MdnsSearchOptions network1Options =
+                MdnsSearchOptions.newBuilder().setNetwork(null /* network */).build();
+        final SocketCreationCallback callback = expectSocketCreationCallback(
+                SERVICE_TYPE_1, mockListenerOne, network1Options);
+        runOnHandler(() -> callback.onSocketCreated(null /* network */));
+        verify(mockServiceTypeClientType1NullNetwork).startSendAndReceive(
+                mockListenerOne, network1Options);
+
+        // Receive a response, it should be processed on the client.
+        final MdnsPacket response = createMdnsPacket(SERVICE_TYPE_1);
+        final int ifIndex = 1;
+        runOnHandler(() -> discoveryManager.onResponseReceived(
+                response, ifIndex, null /* network */));
+        verify(mockServiceTypeClientType1NullNetwork).processResponse(
+                response, ifIndex, null /* network */);
+
+        runOnHandler(() -> callback.onAllSocketsDestroyed(null /* network */));
+        verify(mockServiceTypeClientType1NullNetwork).notifySocketDestroyed();
+
+        // Receive a response again, it should not be processed.
+        runOnHandler(() -> discoveryManager.onResponseReceived(
+                response, ifIndex, null /* network */));
+        // Still times(1) as a response was received once previously
+        verify(mockServiceTypeClientType1NullNetwork, times(1))
+                .processResponse(response, ifIndex, null /* network */);
+
+        // Unregister the listener, notifyNetworkUnrequested should be called but other stop methods
+        // won't be call because the service type client was unregistered and destroyed. But those
+        // cleanups were done in notifySocketDestroyed when the socket was destroyed.
+        runOnHandler(() -> discoveryManager.unregisterListener(SERVICE_TYPE_1, mockListenerOne));
+        verify(socketClient).notifyNetworkUnrequested(mockListenerOne);
+        verify(mockServiceTypeClientType1NullNetwork, never()).stopSendAndReceive(any());
+        // The stopDiscovery() is only used by MdnsSocketClient, which doesn't send
+        // onAllSocketsDestroyed(). So the socket clients that send onAllSocketsDestroyed() do not
+        // need to call stopDiscovery().
+        verify(socketClient, never()).stopDiscovery();
+    }
+
     private MdnsPacket createMdnsPacket(String serviceType) {
         final String[] type = TextUtils.split(serviceType, "\\.");
         final ArrayList<String> name = new ArrayList<>(type.length + 1);