Get and cache LinkProperties for network as querying them during the callback events is not consistent.

cf: https://developer.android.com/reference/android/net/ConnectivityManager.NetworkCallback#onLost(android.net.Network)

Bug: 312549751
Test: Covered by the existing test suite. Race condition bug.
Change-Id: I0d01f7a66d5933a522be4efb6ba3312a68e8ed59

2 files changed, 69 insertions, 30 deletions

diff --git a/MdnsOffloadManagerService/src/com/android/tv/mdnsoffloadmanager/MdnsOffloadManagerService.java b/MdnsOffloadManagerService/src/com/android/tv/mdnsoffloadmanager/MdnsOffloadManagerService.java
index 226fde3..b853185 100644
--- a/MdnsOffloadManagerService/src/com/android/tv/mdnsoffloadmanager/MdnsOffloadManagerService.java
+++ b/MdnsOffloadManagerService/src/com/android/tv/mdnsoffloadmanager/MdnsOffloadManagerService.java
@@ -50,6 +50,7 @@ import java.util.HashMap;
 import java.util.Map;
 import java.util.Objects;
 import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.TimeUnit;
 import java.util.stream.Collectors;
@@ -432,22 +433,48 @@ public class MdnsOffloadManagerService extends Service {
     }
 
     private class ConnectivityManagerNetworkCallback extends ConnectivityManager.NetworkCallback {
+        private Map<Network, LinkProperties> mLinkPropertiesMap = new ConcurrentHashMap<>();
+
         @Override
-        public void onAvailable(@NonNull Network network) {
-            LinkProperties linkProperties = mConnectivityManager.getLinkProperties(network);
-            String iface = linkProperties.getInterfaceName();
+        public void onLinkPropertiesChanged(Network network,
+                LinkProperties linkProperties) {
+            // We only want to know the interface name of a network. This method is
+            // called right after onAvailable() or any other important change during the lifecycle
+            // of the network.
+            LinkProperties previousProperties = mLinkPropertiesMap.put(network, linkProperties);
             mHandler.post(() -> {
-                InterfaceOffloadManager offloadManager = getInterfaceOffloadManager(iface);
+                if (previousProperties != null &&
+                        !previousProperties.getInterfaceName().equals(
+                                linkProperties.getInterfaceName())) {
+                    // This means that the interface changed names, which may happen
+                    // but very rarely.
+                    InterfaceOffloadManager offloadManager =
+                            getInterfaceOffloadManager(previousProperties.getInterfaceName());
+                    offloadManager.onNetworkLost();
+                }
+
+                // We trigger an onNetworkAvailable even if the existing is the same in case
+                // anything needs to be refreshed due to the LinkProperties change.
+                InterfaceOffloadManager offloadManager =
+                        getInterfaceOffloadManager(linkProperties.getInterfaceName());
                 offloadManager.onNetworkAvailable();
             });
+
         }
 
         @Override
         public void onLost(@NonNull Network network) {
-            LinkProperties linkProperties = mConnectivityManager.getLinkProperties(network);
-            String iface = linkProperties.getInterfaceName();
+            // Network object is guaranteed to match a network object from a previous
+            // onLinkPropertiesChanged() so the LinkProperties must be available to retrieve
+            // the associated iface.
+            LinkProperties previousProperties = mLinkPropertiesMap.remove(network);
+            if (previousProperties == null){
+                Log.w(TAG,"Network "+ network + " lost before being available.");
+                return;
+            }
             mHandler.post(() -> {
-                InterfaceOffloadManager offloadManager = getInterfaceOffloadManager(iface);
+                InterfaceOffloadManager offloadManager =
+                        getInterfaceOffloadManager(previousProperties.getInterfaceName());
                 offloadManager.onNetworkLost();
             });
         }
diff --git a/MdnsOffloadManagerService/tests/src/com/android/tv/mdnsoffloadmanager/MdnsOffloadManagerTest.java b/MdnsOffloadManagerService/tests/src/com/android/tv/mdnsoffloadmanager/MdnsOffloadManagerTest.java
index b2e578b..fe03990 100644
--- a/MdnsOffloadManagerService/tests/src/com/android/tv/mdnsoffloadmanager/MdnsOffloadManagerTest.java
+++ b/MdnsOffloadManagerService/tests/src/com/android/tv/mdnsoffloadmanager/MdnsOffloadManagerTest.java
@@ -18,6 +18,7 @@ package com.android.tv.mdnsoffloadmanager;
 
 import static android.net.NetworkCapabilities.TRANSPORT_ETHERNET;
 import static android.net.NetworkCapabilities.TRANSPORT_WIFI;
+
 import static com.android.tv.mdnsoffloadmanager.TestHelpers.SERVICE_AIRPLAY;
 import static com.android.tv.mdnsoffloadmanager.TestHelpers.SERVICE_ATV;
 import static com.android.tv.mdnsoffloadmanager.TestHelpers.SERVICE_GOOGLECAST;
@@ -27,6 +28,7 @@ import static com.android.tv.mdnsoffloadmanager.TestHelpers.makeLinkProperties;
 import static com.android.tv.mdnsoffloadmanager.TestHelpers.makeLowPowerStandbyPolicy;
 import static com.android.tv.mdnsoffloadmanager.TestHelpers.verifyOffloadedServices;
 import static com.android.tv.mdnsoffloadmanager.TestHelpers.verifyPassthroughQNames;
+
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotNull;
@@ -91,10 +93,10 @@ public class MdnsOffloadManagerTest {
 
     private static final String TAG = MdnsOffloadManagerTest.class.getSimpleName();
     private static final ComponentName VENDOR_SERVICE_COMPONENT =
-        ComponentName.unflattenFromString("test.vendor.offloadservice/.TestOffloadService");
+            ComponentName.unflattenFromString("test.vendor.offloadservice/.TestOffloadService");
     private static final String[] PRIORITY_LIST = {
-        "_googlecast._tcp.local.",
-        "_some._other._svc.local."
+            "_googlecast._tcp.local.",
+            "_some._other._svc.local."
     };
     private static final String IFC_0 = "imaginaryif0";
     private static final String IFC_1 = "imaginaryif1";
@@ -104,16 +106,26 @@ public class MdnsOffloadManagerTest {
     private static final String APP_PACKAGE_0 = "first.app.package";
     private static final String APP_PACKAGE_1 = "some.other.package";
 
-    @Mock Resources mResources;
-    @Mock IBinder mClientBinder0;
-    @Mock IBinder mClientBinder1;
-    @Mock ConnectivityManager mConnectivityManager;
-    @Mock Network mNetwork0;
-    @Mock Network mNetwork1;
-    @Mock PackageManager mPackageManager;
-    @Spy FakeMdnsOffloadService mVendorService = new FakeMdnsOffloadService();
-    @Captor ArgumentCaptor<NetworkCallback> mNetworkCallbackCaptor;
-    @Captor ArgumentCaptor<IBinder.DeathRecipient> mDeathRecipientCaptor;
+    @Mock
+    Resources mResources;
+    @Mock
+    IBinder mClientBinder0;
+    @Mock
+    IBinder mClientBinder1;
+    @Mock
+    ConnectivityManager mConnectivityManager;
+    @Mock
+    Network mNetwork0;
+    @Mock
+    Network mNetwork1;
+    @Mock
+    PackageManager mPackageManager;
+    @Spy
+    FakeMdnsOffloadService mVendorService = new FakeMdnsOffloadService();
+    @Captor
+    ArgumentCaptor<NetworkCallback> mNetworkCallbackCaptor;
+    @Captor
+    ArgumentCaptor<IBinder.DeathRecipient> mDeathRecipientCaptor;
 
     TestLooper mTestLooper;
     ServiceConnection mCapturedVendorServiceConnection;
@@ -130,9 +142,9 @@ public class MdnsOffloadManagerTest {
         mTestLooper = new TestLooper();
         MockitoAnnotations.initMocks(this);
         when(mResources.getString(eq(R.string.config_mdnsOffloadVendorServiceComponent)))
-            .thenReturn(VENDOR_SERVICE_COMPONENT.flattenToShortString());
+                .thenReturn(VENDOR_SERVICE_COMPONENT.flattenToShortString());
         when(mResources.getStringArray(eq(R.array.config_mdnsOffloadPriorityQnames)))
-            .thenReturn(PRIORITY_LIST);
+                .thenReturn(PRIORITY_LIST);
         when(mPackageManager.getPackageUid(eq(APP_PACKAGE_0), anyInt())).thenReturn(APP_UID_0);
         when(mPackageManager.getPackageUid(eq(APP_PACKAGE_1), anyInt())).thenReturn(APP_UID_1);
         mLowPowerStandbyPolicy = makeLowPowerStandbyPolicy(APP_PACKAGE_0);
@@ -220,7 +232,7 @@ public class MdnsOffloadManagerTest {
 
     private void bindVendorService() {
         mCapturedVendorServiceConnection.onServiceConnected(
-            VENDOR_SERVICE_COMPONENT, mVendorService);
+                VENDOR_SERVICE_COMPONENT, mVendorService);
         mTestLooper.dispatchAll();
     }
 
@@ -230,9 +242,8 @@ public class MdnsOffloadManagerTest {
     }
 
     private void registerNetwork(Network network, String networkInterface) {
-        when(mConnectivityManager.getLinkProperties(eq(network))).thenReturn(
+        mNetworkCallbackCaptor.getValue().onLinkPropertiesChanged(network,
                 makeLinkProperties(networkInterface));
-        mNetworkCallbackCaptor.getValue().onAvailable(network);
         mTestLooper.dispatchAll();
     }
 
@@ -358,7 +369,8 @@ public class MdnsOffloadManagerTest {
         mOffloadManagerBinder.addProtocolResponses(IFC_0, SERVICE_GOOGLECAST, mClientBinder0);
         mTestLooper.dispatchAll();
 
-        verifyOffloadedServices(mVendorService, IFC_0, SERVICE_GOOGLECAST, SERVICE_ATV, SERVICE_GTV);
+        verifyOffloadedServices(mVendorService, IFC_0, SERVICE_GOOGLECAST, SERVICE_ATV,
+                SERVICE_GTV);
     }
 
     @Test
@@ -377,7 +389,7 @@ public class MdnsOffloadManagerTest {
     @Test
     public void priorityListNamesAreCanonicalized() throws RemoteException {
         when(mResources.getStringArray(eq(R.array.config_mdnsOffloadPriorityQnames)))
-            .thenReturn(new String[]{"_googlecast._tcp.local"}); // Trailing dot is missing.
+                .thenReturn(new String[]{"_googlecast._tcp.local"}); // Trailing dot is missing.
         setupDefaultOffloadManager();
         mOffloadManagerBinder.addProtocolResponses(IFC_0, SERVICE_ATV, mClientBinder0);
         mOffloadManagerBinder.addProtocolResponses(IFC_0, SERVICE_GOOGLECAST, mClientBinder0);
@@ -388,7 +400,7 @@ public class MdnsOffloadManagerTest {
 
     @Test
     public void addingToPassthroughList_setsPassthroughBehaviorAndPropagatesToVendorService()
-        throws RemoteException {
+            throws RemoteException {
         setupDefaultOffloadManager();
         mOffloadManagerBinder.addToPassthroughList(IFC_0, "atv", mClientBinder0);
         mTestLooper.dispatchAll();
@@ -458,7 +470,7 @@ public class MdnsOffloadManagerTest {
 
     @Test
     public void removingPassthroughQNameHoldingInvalidClientBinder_doesNothing()
-        throws RemoteException {
+            throws RemoteException {
         setupDefaultOffloadManager();
         mOffloadManagerBinder.addToPassthroughList(IFC_0, "atv", mClientBinder0);
         mOffloadManagerBinder.addToPassthroughList(IFC_0, "gtv", mClientBinder0);
@@ -473,7 +485,7 @@ public class MdnsOffloadManagerTest {
 
     @Test
     public void removingAllEntriesFromPassthroughList_disablesPassthroughBehavior()
-        throws RemoteException {
+            throws RemoteException {
         setupDefaultOffloadManager();
         mOffloadManagerBinder.addToPassthroughList(IFC_0, "atv", mClientBinder0);
         mOffloadManagerBinder.addToPassthroughList(IFC_0, "gtv", mClientBinder0);