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 7691bf445c..c3deecadf4 100644 --- a/service-t/src/com/android/server/connectivity/mdns/MdnsDiscoveryManager.java +++ b/service-t/src/com/android/server/connectivity/mdns/MdnsDiscoveryManager.java @@ -214,6 +214,9 @@ public class MdnsDiscoveryManager implements MdnsSocketClientBase.Callback { private void handleUnregisterListener( @NonNull String serviceType, @NonNull MdnsServiceBrowserListener listener) { + // Unrequested the network. + socketClient.notifyNetworkUnrequested(listener); + final List serviceTypeClients = perNetworkServiceTypeClients.getByServiceType(serviceType); if (serviceTypeClients.isEmpty()) { @@ -231,8 +234,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/service-t/src/com/android/server/connectivity/mdns/MdnsResponse.java b/service-t/src/com/android/server/connectivity/mdns/MdnsResponse.java index 3a98b4d130..e765d536f3 100644 --- a/service-t/src/com/android/server/connectivity/mdns/MdnsResponse.java +++ b/service-t/src/com/android/server/connectivity/mdns/MdnsResponse.java @@ -95,6 +95,14 @@ public class MdnsResponse { return !isSame; } + /** + * @return True if this response contains an identical (original TTL included) record. + */ + public boolean hasIdenticalRecord(@NonNull MdnsRecord record) { + final int existing = records.indexOf(record); + return existing >= 0 && recordsAreSame(record, records.get(existing)); + } + /** * Adds a pointer record. * diff --git a/service-t/src/com/android/server/connectivity/mdns/MdnsResponseDecoder.java b/service-t/src/com/android/server/connectivity/mdns/MdnsResponseDecoder.java index 21792e603d..66487297d0 100644 --- a/service-t/src/com/android/server/connectivity/mdns/MdnsResponseDecoder.java +++ b/service-t/src/com/android/server/connectivity/mdns/MdnsResponseDecoder.java @@ -20,6 +20,7 @@ import android.annotation.NonNull; import android.annotation.Nullable; import android.net.Network; import android.os.SystemClock; +import android.util.ArrayMap; import android.util.ArraySet; import android.util.Pair; @@ -139,8 +140,11 @@ public class MdnsResponseDecoder { final ArraySet modified = new ArraySet<>(); final ArrayList responses = new ArrayList<>(existingResponses.size()); + final ArrayMap augmentedToOriginal = new ArrayMap<>(); for (MdnsResponse existing : existingResponses) { - responses.add(new MdnsResponse(existing)); + final MdnsResponse copy = new MdnsResponse(existing); + responses.add(copy); + augmentedToOriginal.put(copy, existing); } // The response records are structured in a hierarchy, where some records reference // others, as follows: @@ -166,7 +170,7 @@ public class MdnsResponseDecoder { // A: host name -> IP address // Loop 1: find PTR records, which identify distinct service instances. - long now = SystemClock.elapsedRealtime(); + long now = clock.elapsedRealtime(); for (MdnsRecord record : records) { if (record instanceof MdnsPointerRecord) { String[] name = record.getName(); @@ -262,7 +266,11 @@ public class MdnsResponseDecoder { findResponsesWithHostName(responses, inetRecord.getName()); for (MdnsResponse response : matchingResponses) { if (assignInetRecord(response, inetRecord)) { - modified.add(response); + final MdnsResponse originalResponse = augmentedToOriginal.get(response); + if (originalResponse == null + || !originalResponse.hasIdenticalRecord(inetRecord)) { + modified.add(response); + } } } } else { @@ -270,12 +278,26 @@ public class MdnsResponseDecoder { findResponseWithHostName(responses, inetRecord.getName()); if (response != null) { if (assignInetRecord(response, inetRecord)) { - modified.add(response); + final MdnsResponse originalResponse = augmentedToOriginal.get(response); + if (originalResponse == null + || !originalResponse.hasIdenticalRecord(inetRecord)) { + modified.add(response); + } } } } } + // Only responses that have new or modified address records were added to the modified set. + // Make sure responses that have lost address records are added to the set too. + for (int i = 0; i < augmentedToOriginal.size(); i++) { + final MdnsResponse augmented = augmentedToOriginal.keyAt(i); + final MdnsResponse original = augmentedToOriginal.valueAt(i); + if (augmented.getRecords().size() != original.getRecords().size()) { + modified.add(augmented); + } + } + return Pair.create(modified, responses); } 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 aeaca06a7e..350bad0081 100644 --- a/tests/unit/java/com/android/server/connectivity/mdns/MdnsDiscoveryManagerTests.java +++ b/tests/unit/java/com/android/server/connectivity/mdns/MdnsDiscoveryManagerTests.java @@ -293,6 +293,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 name = new ArrayList<>(type.length + 1); diff --git a/tests/unit/java/com/android/server/connectivity/mdns/MdnsResponseDecoderTests.java b/tests/unit/java/com/android/server/connectivity/mdns/MdnsResponseDecoderTests.java index 31383d1583..0d479b1996 100644 --- a/tests/unit/java/com/android/server/connectivity/mdns/MdnsResponseDecoderTests.java +++ b/tests/unit/java/com/android/server/connectivity/mdns/MdnsResponseDecoderTests.java @@ -28,6 +28,7 @@ import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; +import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.mock; import android.net.Network; @@ -406,6 +407,31 @@ public class MdnsResponseDecoderTests { response.getInet4AddressRecord().getInet4Address()); } + @Test + public void testDecodeWithIpv4AddressRemove() throws IOException { + MdnsResponse response = makeMdnsResponse(0, DATAIN_SERVICE_NAME_1, List.of( + new PacketAndRecordClass(DATAIN_PTR_1, + MdnsPointerRecord.class), + new PacketAndRecordClass(DATAIN_SERVICE_1, + MdnsServiceRecord.class), + new PacketAndRecordClass(DATAIN_IPV4_1, + MdnsInet4AddressRecord.class), + new PacketAndRecordClass(DATAIN_IPV4_2, + MdnsInet4AddressRecord.class))); + // Now update the response removing the second v4 address + final MdnsResponseDecoder decoder = new MdnsResponseDecoder(mClock, null); + + final byte[] removedAddrResponse = makeResponsePacket( + List.of(DATAIN_PTR_1, DATAIN_SERVICE_1, DATAIN_IPV4_1)); + final ArraySet changes = decode( + decoder, removedAddrResponse, List.of(response)); + + assertEquals(1, changes.size()); + assertEquals(1, changes.valueAt(0).getInet4AddressRecords().size()); + assertEquals(parseNumericAddress("10.1.2.3"), + changes.valueAt(0).getInet4AddressRecords().get(0).getInet4Address()); + } + @Test public void testDecodeWithIpv6AddressChange() throws IOException { MdnsResponse response = makeMdnsResponse(0, DATAIN_SERVICE_NAME_1, List.of( @@ -426,6 +452,29 @@ public class MdnsResponseDecoderTests { response.getInet6AddressRecord().getInet6Address()); } + @Test + public void testDecodeWithIpv6AddressRemoved() throws IOException { + MdnsResponse response = makeMdnsResponse(0, DATAIN_SERVICE_NAME_1, List.of( + new PacketAndRecordClass(DATAIN_PTR_1, + MdnsPointerRecord.class), + new PacketAndRecordClass(DATAIN_SERVICE_1, + MdnsServiceRecord.class), + new PacketAndRecordClass(DATAIN_IPV6_1, + MdnsInet6AddressRecord.class), + new PacketAndRecordClass(DATAIN_IPV6_2, + MdnsInet6AddressRecord.class))); + // Now update the response adding an address + final MdnsResponseDecoder decoder = new MdnsResponseDecoder(mClock, null); + final byte[] removedAddrResponse = makeResponsePacket( + List.of(DATAIN_PTR_1, DATAIN_SERVICE_1, DATAIN_IPV6_1)); + final ArraySet updatedResponses = decode( + decoder, removedAddrResponse, List.of(response)); + assertEquals(1, updatedResponses.size()); + assertEquals(1, updatedResponses.valueAt(0).getInet6AddressRecords().size()); + assertEquals(parseNumericAddress("aabb:ccdd:1122:3344:a0b0:c0d0:1020:3040"), + updatedResponses.valueAt(0).getInet6AddressRecords().get(0).getInet6Address()); + } + @Test public void testDecodeWithChangeOnText() throws IOException { MdnsResponse response = makeMdnsResponse(0, DATAIN_SERVICE_NAME_1, List.of( @@ -493,11 +542,12 @@ public class MdnsResponseDecoderTests { new PacketAndRecordClass(DATAIN_IPV4_1, MdnsInet4AddressRecord.class), new PacketAndRecordClass(DATAIN_IPV6_1, MdnsInet6AddressRecord.class), new PacketAndRecordClass(DATAIN_PTR_1, MdnsPointerRecord.class), - new PacketAndRecordClass(DATAIN_SERVICE_2, MdnsServiceRecord.class), + new PacketAndRecordClass(DATAIN_SERVICE_1, MdnsServiceRecord.class), new PacketAndRecordClass(DATAIN_TEXT_1, MdnsTextRecord.class)); // Create a two identical responses. - MdnsResponse response = makeMdnsResponse(0, DATAIN_SERVICE_NAME_1, recordList); + MdnsResponse response = makeMdnsResponse(12L /* time */, DATAIN_SERVICE_NAME_1, recordList); + doReturn(34L).when(mClock).elapsedRealtime(); final MdnsResponseDecoder decoder = new MdnsResponseDecoder(mClock, null); final byte[] identicalResponse = makeResponsePacket( recordList.stream().map(p -> p.packetData).collect(Collectors.toList())); diff --git a/tests/unit/java/com/android/server/connectivity/mdns/MdnsServiceTypeClientTests.java b/tests/unit/java/com/android/server/connectivity/mdns/MdnsServiceTypeClientTests.java index f9c0a193cd..a61e8b23ce 100644 --- a/tests/unit/java/com/android/server/connectivity/mdns/MdnsServiceTypeClientTests.java +++ b/tests/unit/java/com/android/server/connectivity/mdns/MdnsServiceTypeClientTests.java @@ -1078,21 +1078,17 @@ public class MdnsServiceTypeClientTests { 0 /* flags */, Collections.emptyList() /* questions */, List.of( - // TODO: cacheFlush will cause addresses to be cleared and re-added every - // time, which is considered a change and triggers extra - // onServiceChanged callbacks. Sets cacheFlush bit to false until the - // issue is fixed. new MdnsServiceRecord(serviceName, updatedReceiptTime, - false /* cacheFlush */, TEST_TTL, 0 /* servicePriority */, + true /* cacheFlush */, TEST_TTL, 0 /* servicePriority */, 0 /* serviceWeight */, 1234 /* servicePort */, hostname), new MdnsTextRecord(serviceName, updatedReceiptTime, - false /* cacheFlush */, TEST_TTL, + true /* cacheFlush */, TEST_TTL, Collections.emptyList() /* entries */), new MdnsInetAddressRecord(hostname, updatedReceiptTime, - false /* cacheFlush */, TEST_TTL, + true /* cacheFlush */, TEST_TTL, InetAddresses.parseNumericAddress(ipV4Address)), new MdnsInetAddressRecord(hostname, updatedReceiptTime, - false /* cacheFlush */, TEST_TTL, + true /* cacheFlush */, TEST_TTL, InetAddresses.parseNumericAddress(ipV6Address))), Collections.emptyList() /* authorityRecords */, Collections.emptyList() /* additionalRecords */);