Merge "Fixing multithreading issues in Ethernet Factory"

service-t/src/com/android/server/ethernet/EthernetNetworkFactory.java

@@ -442,24 +442,44 @@ public class EthernetNetworkFactory extends NetworkFactory {
                 mIpClientShutdownCv.block();
             }
 
+            // At the time IpClient is stopped, an IpClient event may have already been posted on
+            // the back of the handler and is awaiting execution. Once that event is executed, the
+            // associated callback object may not be valid anymore
+            // (NetworkInterfaceState#mIpClientCallback points to a different object / null).
+            private boolean isCurrentCallback() {
+                return this == mIpClientCallback;
+            }
+
+            private void handleIpEvent(final @NonNull Runnable r) {
+                mHandler.post(() -> {
+                    if (!isCurrentCallback()) {
+                        Log.i(TAG, "Ignoring stale IpClientCallbacks " + this);
+                        return;
+                    }
+                    r.run();
+                });
+            }
+
             @Override
             public void onProvisioningSuccess(LinkProperties newLp) {
-                mHandler.post(() -> onIpLayerStarted(newLp, mNetworkManagementListener));
+                handleIpEvent(() -> onIpLayerStarted(newLp, mNetworkManagementListener));
             }
 
             @Override
             public void onProvisioningFailure(LinkProperties newLp) {
-                mHandler.post(() -> onIpLayerStopped(mNetworkManagementListener));
+                // This cannot happen due to provisioning timeout, because our timeout is 0. It can
+                // happen due to errors while provisioning or on provisioning loss.
+                handleIpEvent(() -> onIpLayerStopped(mNetworkManagementListener));
             }
 
             @Override
             public void onLinkPropertiesChange(LinkProperties newLp) {
-                mHandler.post(() -> updateLinkProperties(newLp));
+                handleIpEvent(() -> updateLinkProperties(newLp));
             }
 
             @Override
             public void onReachabilityLost(String logMsg) {
-                mHandler.post(() -> updateNeighborLostEvent(logMsg));
+                handleIpEvent(() -> updateNeighborLostEvent(logMsg));
             }
 
             @Override
@@ -561,14 +581,6 @@ public class EthernetNetworkFactory extends NetworkFactory {
 
         void onIpLayerStarted(@NonNull final LinkProperties linkProperties,
                 @Nullable final INetworkInterfaceOutcomeReceiver listener) {
-            if(mIpClient == null) {
-                // This call comes from a message posted on the handler thread, but the IpClient has
-                // since been stopped such as may be the case if updateInterfaceLinkState() is
-                // queued on the handler thread prior. As network management callbacks are sent in
-                // stop(), there is no need to send them again here.
-                if (DBG) Log.d(TAG, "IpClient is not initialized.");
-                return;
-            }
             if (mNetworkAgent != null) {
                 Log.e(TAG, "Already have a NetworkAgent - aborting new request");
                 stop();
@@ -604,12 +616,6 @@ public class EthernetNetworkFactory extends NetworkFactory {
         }
 
         void onIpLayerStopped(@Nullable final INetworkInterfaceOutcomeReceiver listener) {
-            // This cannot happen due to provisioning timeout, because our timeout is 0. It can
-            // happen due to errors while provisioning or on provisioning loss.
-            if(mIpClient == null) {
-                if (DBG) Log.d(TAG, "IpClient is not initialized.");
-                return;
-            }
             // There is no point in continuing if the interface is gone as stop() will be triggered
             // by removeInterface() when processed on the handler thread and start() won't
             // work for a non-existent interface.
@@ -651,10 +657,6 @@ public class EthernetNetworkFactory extends NetworkFactory {
         }
 
         void updateLinkProperties(LinkProperties linkProperties) {
-            if(mIpClient == null) {
-                if (DBG) Log.d(TAG, "IpClient is not initialized.");
-                return;
-            }
             mLinkProperties = linkProperties;
             if (mNetworkAgent != null) {
                 mNetworkAgent.sendLinkPropertiesImpl(linkProperties);
@@ -662,10 +664,6 @@ public class EthernetNetworkFactory extends NetworkFactory {
         }
 
         void updateNeighborLostEvent(String logMsg) {
-            if(mIpClient == null) {
-                if (DBG) Log.d(TAG, "IpClient is not initialized.");
-                return;
-            }
             Log.i(TAG, "updateNeighborLostEvent " + logMsg);
             // Reachability lost will be seen only if the gateway is not reachable.
             // Since ethernet FW doesn't have the mechanism to scan for new networks

tests/ethernet/java/com/android/server/ethernet/EthernetNetworkFactoryTest.java

@@ -21,6 +21,7 @@ import static com.android.testutils.DevSdkIgnoreRuleKt.SC_V2;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertNotSame;
 import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertTrue;
 import static org.mockito.ArgumentMatchers.any;
@@ -543,68 +544,59 @@ public class EthernetNetworkFactoryTest {
         verifyRestart(createDefaultIpConfig());
     }
 
-    @Test
+    private IpClientCallbacks getStaleIpClientCallbacks() throws Exception {
-    public void testIgnoreOnIpLayerStartedCallbackAfterIpClientHasStopped() throws Exception {
-        initEthernetNetworkFactory();
         createAndVerifyProvisionedInterface(TEST_IFACE);
-        mIpClientCallbacks.onProvisioningFailure(new LinkProperties());
+        final IpClientCallbacks staleIpClientCallbacks = mIpClientCallbacks;
-        mIpClientCallbacks.onProvisioningSuccess(new LinkProperties());
+        mNetFactory.removeInterface(TEST_IFACE);
-        mLooper.dispatchAll();
         verifyStop();
+        assertNotSame(mIpClientCallbacks, staleIpClientCallbacks);
+        return staleIpClientCallbacks;
+    }
+
+    @Test
+    public void testIgnoreOnIpLayerStartedCallbackForStaleCallback() throws Exception {
+        initEthernetNetworkFactory();
+        final IpClientCallbacks staleIpClientCallbacks = getStaleIpClientCallbacks();
+
+        staleIpClientCallbacks.onProvisioningSuccess(new LinkProperties());
+        mLooper.dispatchAll();
 
-        // ipClient has been shut down first, we should not retry
         verify(mIpClient, never()).startProvisioning(any());
         verify(mNetworkAgent, never()).register();
     }
 
     @Test
-    public void testIgnoreOnIpLayerStoppedCallbackAfterIpClientHasStopped() throws Exception {
+    public void testIgnoreOnIpLayerStoppedCallbackForStaleCallback() throws Exception {
         initEthernetNetworkFactory();
-        createAndVerifyProvisionedInterface(TEST_IFACE);
         when(mDeps.getNetworkInterfaceByName(TEST_IFACE)).thenReturn(mInterfaceParams);
-        mIpClientCallbacks.onProvisioningFailure(new LinkProperties());
+        final IpClientCallbacks staleIpClientCallbacks = getStaleIpClientCallbacks();
-        mIpClientCallbacks.onProvisioningFailure(new LinkProperties());
-        mLooper.dispatchAll();
-        verifyStop();
 
-        // ipClient has been shut down first, we should not retry
+        staleIpClientCallbacks.onProvisioningFailure(new LinkProperties());
-        verify(mIpClient).startProvisioning(any());
+        mLooper.dispatchAll();
+
+        verify(mIpClient, never()).startProvisioning(any());
     }
 
     @Test
-    public void testIgnoreLinkPropertiesCallbackAfterIpClientHasStopped() throws Exception {
+    public void testIgnoreLinkPropertiesCallbackForStaleCallback() throws Exception {
         initEthernetNetworkFactory();
-        createAndVerifyProvisionedInterface(TEST_IFACE);
+        final IpClientCallbacks staleIpClientCallbacks = getStaleIpClientCallbacks();
-        LinkProperties lp = new LinkProperties();
+        final LinkProperties lp = new LinkProperties();
 
-        // The test requires the two proceeding methods to happen one after the other in ENF and
+        staleIpClientCallbacks.onLinkPropertiesChange(lp);
-        // verifies onLinkPropertiesChange doesn't complete execution for a downed interface.
-        // Posting is necessary as updateInterfaceLinkState with false will set mIpClientCallbacks
-        // to null which will throw an NPE in the test if executed synchronously.
-        mHandler.post(() -> mNetFactory.updateInterfaceLinkState(TEST_IFACE, false, NULL_LISTENER));
-        mIpClientCallbacks.onLinkPropertiesChange(lp);
         mLooper.dispatchAll();
 
-        verifyStop();
+        verify(mNetworkAgent, never()).sendLinkPropertiesImpl(eq(lp));
-        // ipClient has been shut down first, we should not update
-        verify(mNetworkAgent, never()).sendLinkPropertiesImpl(same(lp));
     }
 
     @Test
-    public void testIgnoreNeighborLossCallbackAfterIpClientHasStopped() throws Exception {
+    public void testIgnoreNeighborLossCallbackForStaleCallback() throws Exception {
         initEthernetNetworkFactory();
-        createAndVerifyProvisionedInterface(TEST_IFACE);
+        final IpClientCallbacks staleIpClientCallbacks = getStaleIpClientCallbacks();
 
-        // The test requires the two proceeding methods to happen one after the other in ENF and
+        staleIpClientCallbacks.onReachabilityLost("Neighbor Lost");
-        // verifies onReachabilityLost doesn't complete execution for a downed interface.
-        // Posting is necessary as updateInterfaceLinkState with false will set mIpClientCallbacks
-        // to null which will throw an NPE in the test if executed synchronously.
-        mHandler.post(() -> mNetFactory.updateInterfaceLinkState(TEST_IFACE, false, NULL_LISTENER));
-        mIpClientCallbacks.onReachabilityLost("Neighbor Lost");
         mLooper.dispatchAll();
 
-        verifyStop();
-        // ipClient has been shut down first, we should not update
         verify(mIpClient, never()).startProvisioning(any());
         verify(mNetworkAgent, never()).register();
     }