Fix dangling NetworkAgent

This change fixes the potential for a networkAgent to be left dangling,
due to a situation where a VcnGatewayConnection shuts down, but fails to
unregister it's NetworkAgent.

The root cause was that the NetworkAgent was not unregistered when
moving to the DisconnectedState from the RetryTimeoutState, and the new
state assumed that there was no NetworkAgent, and thus failed to close
it when disconnecting.

Thus, this change ensures that the NetworkAgent is closed before moving
to the DisconnectedState. Additionally, it adds safety-checks to
onQuitting(), ensuring that if all else fails, these fields are cleaned
up.

Lastly, this change adds the specific gateway reference to facilitate
future debugging of issues such as this where there is potential for
duplicate networks, or gateway connections.

Bug: 191707296
Test: atest FrameworksVcnTests
Change-Id: I84cd43a0c136662f5c2d229650f1f5f889e6f144

services/core/java/com/android/server/vcn/Vcn.java

@@ -514,7 +514,11 @@ public class Vcn extends Handler {
     }
 
     private String getLogPrefix() {
-        return "[" + LogUtils.getHashedSubscriptionGroup(mSubscriptionGroup) + "] ";
+        return "["
+                + LogUtils.getHashedSubscriptionGroup(mSubscriptionGroup)
+                + "-"
+                + System.identityHashCode(this)
+                + "] ";
     }
 
     private void logVdbg(String msg) {

services/core/java/com/android/server/vcn/VcnGatewayConnection.java

@@ -714,6 +714,18 @@ public class VcnGatewayConnection extends StateMachine {
     protected void onQuitting() {
         logDbg("Quitting VcnGatewayConnection");
 
+        if (mNetworkAgent != null) {
+            logWtf("NetworkAgent was non-null in onQuitting");
+            mNetworkAgent.unregister();
+            mNetworkAgent = null;
+        }
+
+        if (mIkeSession != null) {
+            logWtf("IkeSession was non-null in onQuitting");
+            mIkeSession.kill();
+            mIkeSession = null;
+        }
+
         // No need to call setInterfaceDown(); the IpSecInterface is being fully torn down.
         if (mTunnelIface != null) {
             mTunnelIface.close();
@@ -1863,6 +1875,7 @@ public class VcnGatewayConnection extends StateMachine {
 
             if (mUnderlying == null) {
                 logWtf("Underlying network was null in retry state");
+                teardownNetwork();
                 transitionTo(mDisconnectedState);
             } else {
                 // Safe to blindly set up, as it is cancelled and cleared on exiting this state
@@ -1879,6 +1892,7 @@ public class VcnGatewayConnection extends StateMachine {
 
                     // If new underlying is null, all networks were lost; go back to disconnected.
                     if (mUnderlying == null) {
+                        teardownNetwork();
                         transitionTo(mDisconnectedState);
                         return;
                     } else if (oldUnderlying != null
@@ -2134,6 +2148,8 @@ public class VcnGatewayConnection extends StateMachine {
                 + LogUtils.getHashedSubscriptionGroup(mSubscriptionGroup)
                 + "-"
                 + mConnectionConfig.getGatewayConnectionName()
+                + "-"
+                + System.identityHashCode(this)
                 + "] ";
     }
 

tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionRetryTimeoutStateTest.java

@@ -16,10 +16,16 @@
 
 package com.android.server.vcn;
 
+import static com.android.server.vcn.VcnGatewayConnection.VcnNetworkAgent;
+
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertTrue;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.verify;
 
 import androidx.test.filters.SmallTest;
 import androidx.test.runner.AndroidJUnit4;
@@ -33,16 +39,20 @@ import org.junit.runner.RunWith;
 @SmallTest
 public class VcnGatewayConnectionRetryTimeoutStateTest extends VcnGatewayConnectionTestBase {
     private long mFirstRetryInterval;
+    private VcnNetworkAgent mNetworkAgent;
 
     @Before
     public void setUp() throws Exception {
         super.setUp();
 
         mFirstRetryInterval = mConfig.getRetryIntervalsMillis()[0];
+        mNetworkAgent = mock(VcnNetworkAgent.class);
 
         mGatewayConnection.setUnderlyingNetwork(TEST_UNDERLYING_NETWORK_RECORD_1);
         mGatewayConnection.transitionTo(mGatewayConnection.mRetryTimeoutState);
         mTestLooper.dispatchAll();
+
+        mGatewayConnection.setNetworkAgent(mNetworkAgent);
     }
 
     @Test
@@ -54,6 +64,9 @@ public class VcnGatewayConnectionRetryTimeoutStateTest extends VcnGatewayConnect
 
         assertEquals(mGatewayConnection.mConnectingState, mGatewayConnection.getCurrentState());
         verifyRetryTimeoutAlarmAndGetCallback(mFirstRetryInterval, true /* expectCanceled */);
+
+        assertNotNull(mGatewayConnection.getNetworkAgent());
+        verify(mNetworkAgent, never()).unregister();
     }
 
     @Test
@@ -65,6 +78,9 @@ public class VcnGatewayConnectionRetryTimeoutStateTest extends VcnGatewayConnect
 
         assertEquals(mGatewayConnection.mRetryTimeoutState, mGatewayConnection.getCurrentState());
         verifyRetryTimeoutAlarmAndGetCallback(mFirstRetryInterval, false /* expectCanceled */);
+
+        assertNotNull(mGatewayConnection.getNetworkAgent());
+        verify(mNetworkAgent, never()).unregister();
     }
 
     @Test
@@ -76,6 +92,9 @@ public class VcnGatewayConnectionRetryTimeoutStateTest extends VcnGatewayConnect
 
         assertEquals(mGatewayConnection.mDisconnectedState, mGatewayConnection.getCurrentState());
         verifyRetryTimeoutAlarmAndGetCallback(mFirstRetryInterval, true /* expectCanceled */);
+
+        assertNull(mGatewayConnection.getNetworkAgent());
+        verify(mNetworkAgent).unregister();
     }
 
     @Test
@@ -93,6 +112,9 @@ public class VcnGatewayConnectionRetryTimeoutStateTest extends VcnGatewayConnect
 
         assertEquals(mGatewayConnection.mConnectingState, mGatewayConnection.getCurrentState());
         verifyRetryTimeoutAlarmAndGetCallback(mFirstRetryInterval, true /* expectCanceled */);
+
+        assertNotNull(mGatewayConnection.getNetworkAgent());
+        verify(mNetworkAgent, never()).unregister();
     }
 
     @Test
@@ -108,6 +130,9 @@ public class VcnGatewayConnectionRetryTimeoutStateTest extends VcnGatewayConnect
 
         assertNull(mGatewayConnection.getCurrentState());
         assertTrue(mGatewayConnection.isQuitting());
+
+        assertNull(mGatewayConnection.getNetworkAgent());
+        verify(mNetworkAgent).unregister();
     }
 
     @Test
@@ -117,5 +142,8 @@ public class VcnGatewayConnectionRetryTimeoutStateTest extends VcnGatewayConnect
 
         assertEquals(mGatewayConnection.mDisconnectedState, mGatewayConnection.getCurrentState());
         assertFalse(mGatewayConnection.isQuitting());
+
+        assertNull(mGatewayConnection.getNetworkAgent());
+        verify(mNetworkAgent).unregister();
     }
 }

tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionTest.java

@@ -24,8 +24,12 @@ import static android.net.NetworkCapabilities.NET_CAPABILITY_NOT_VCN_MANAGED;
 import static android.net.NetworkCapabilities.TRANSPORT_CELLULAR;
 import static android.net.NetworkCapabilities.TRANSPORT_WIFI;
 
+import static com.android.server.vcn.VcnGatewayConnection.VcnIkeSession;
+import static com.android.server.vcn.VcnGatewayConnection.VcnNetworkAgent;
+
 import static org.junit.Assert.assertArrayEquals;
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertTrue;
 import static org.mockito.ArgumentMatchers.anyLong;
 import static org.mockito.Matchers.eq;
@@ -191,4 +195,23 @@ public class VcnGatewayConnectionTest extends VcnGatewayConnectionTestBase {
 
         verify(mDisconnectRequestAlarm).cancel();
     }
+
+    @Test
+    public void testQuittingCleansUpPersistentState() {
+        final VcnIkeSession vcnIkeSession = mock(VcnIkeSession.class);
+        final VcnNetworkAgent vcnNetworkAgent = mock(VcnNetworkAgent.class);
+
+        mGatewayConnection.setIkeSession(vcnIkeSession);
+        mGatewayConnection.setNetworkAgent(vcnNetworkAgent);
+
+        mGatewayConnection.quitNow();
+        mTestLooper.dispatchAll();
+
+        assertNull(mGatewayConnection.getIkeSession());
+        verify(vcnIkeSession).kill();
+        assertNull(mGatewayConnection.getNetworkAgent());
+        verify(vcnNetworkAgent).unregister();
+
+        verifyWakeLockReleased();
+    }
 }