Merge "Ensure VcnGatewayConnection#isQuitting never gets unset after being set" into sc-dev am: 0df66631c0

Original change: https://googleplex-android-review.googlesource.com/c/platform/frameworks/base/+/15316004

Change-Id: I151b67855b705197cb832caa32c91cc746569b56

9 files changed, 86 insertions, 23 deletions

diff --git a/services/core/java/com/android/server/VcnManagementService.java b/services/core/java/com/android/server/VcnManagementService.java
index 207a5e3db06e..b068f86ff0ab 100644
--- a/services/core/java/com/android/server/VcnManagementService.java
+++ b/services/core/java/com/android/server/VcnManagementService.java
@@ -550,12 +550,14 @@ public class VcnManagementService extends IVcnManagementService.Stub {
 
     @GuardedBy("mLock")
     private void stopVcnLocked(@NonNull ParcelUuid uuidToTeardown) {
-        final Vcn vcnToTeardown = mVcns.remove(uuidToTeardown);
+        // Remove in 2 steps. Make sure teardownAsync is triggered before removing from the map.
+        final Vcn vcnToTeardown = mVcns.get(uuidToTeardown);
         if (vcnToTeardown == null) {
             return;
         }
 
         vcnToTeardown.teardownAsynchronously();
+        mVcns.remove(uuidToTeardown);
 
         // Now that the VCN is removed, notify all registered listeners to refresh their
         // UnderlyingNetworkPolicy.
@@ -1054,18 +1056,15 @@ public class VcnManagementService extends IVcnManagementService.Stub {
     private void logVdbg(String msg) {
         if (VDBG) {
             Slog.v(TAG, msg);
-            LOCAL_LOG.log(TAG + " VDBG: " + msg);
         }
     }
 
     private void logDbg(String msg) {
         Slog.d(TAG, msg);
-        LOCAL_LOG.log(TAG + " DBG: " + msg);
     }
 
     private void logDbg(String msg, Throwable tr) {
         Slog.d(TAG, msg, tr);
-        LOCAL_LOG.log(TAG + " DBG: " + msg + tr);
     }
 
     private void logErr(String msg) {
diff --git a/services/core/java/com/android/server/vcn/Vcn.java b/services/core/java/com/android/server/vcn/Vcn.java
index 44a6d13153fd..9c3721b15f32 100644
--- a/services/core/java/com/android/server/vcn/Vcn.java
+++ b/services/core/java/com/android/server/vcn/Vcn.java
@@ -528,18 +528,15 @@ public class Vcn extends Handler {
     private void logVdbg(String msg) {
         if (VDBG) {
             Slog.v(TAG, getLogPrefix() + msg);
-            LOCAL_LOG.log(getLogPrefix() + "VDBG: " + msg);
         }
     }
 
     private void logDbg(String msg) {
         Slog.d(TAG, getLogPrefix() + msg);
-        LOCAL_LOG.log(getLogPrefix() + "DBG: " + msg);
     }
 
     private void logDbg(String msg, Throwable tr) {
         Slog.d(TAG, getLogPrefix() + msg, tr);
-        LOCAL_LOG.log(getLogPrefix() + "DBG: " + msg + tr);
     }
 
     private void logErr(String msg) {
diff --git a/services/core/java/com/android/server/vcn/VcnGatewayConnection.java b/services/core/java/com/android/server/vcn/VcnGatewayConnection.java
index 38427697a9cd..3c0a05b6edf7 100644
--- a/services/core/java/com/android/server/vcn/VcnGatewayConnection.java
+++ b/services/core/java/com/android/server/vcn/VcnGatewayConnection.java
@@ -92,6 +92,7 @@ import com.android.server.vcn.UnderlyingNetworkTracker.UnderlyingNetworkTrackerC
 import com.android.server.vcn.Vcn.VcnGatewayStatusCallback;
 import com.android.server.vcn.util.LogUtils;
 import com.android.server.vcn.util.MtuUtils;
+import com.android.server.vcn.util.OneWayBoolean;
 
 import java.io.IOException;
 import java.net.Inet4Address;
@@ -551,8 +552,13 @@ public class VcnGatewayConnection extends StateMachine {
      * <p>This variable is false for the lifecycle of the VcnGatewayConnection, until a command to
      * teardown has been received. This may be flipped due to events such as the Network becoming
      * unwanted, the owning VCN entering safe mode, or an irrecoverable internal failure.
+     *
+     * <p>WARNING: Assignments to this MUST ALWAYS (except for testing) use the or operator ("|="),
+     * otherwise the flag may be flipped back to false after having been set to true. This could
+     * lead to a case where the Vcn parent instance has commanded a teardown, but a spurious
+     * non-quitting disconnect request could flip this back to true.
      */
-    private boolean mIsQuitting = false;
+    private OneWayBoolean mIsQuitting = new OneWayBoolean();
 
     /**
      * Whether the VcnGatewayConnection is in safe mode.
@@ -794,7 +800,7 @@ public class VcnGatewayConnection extends StateMachine {
     private void acquireWakeLock() {
         mVcnContext.ensureRunningOnLooperThread();
 
-        if (!mIsQuitting) {
+        if (!mIsQuitting.getValue()) {
             mWakeLock.acquire();
 
             logVdbg("Wakelock acquired: " + mWakeLock);
@@ -1297,7 +1303,9 @@ public class VcnGatewayConnection extends StateMachine {
             // TODO(b/180526152): notify VcnStatusCallback for Network loss
 
             logDbg("Tearing down. Cause: " + info.reason);
-            mIsQuitting = info.shouldQuit;
+            if (info.shouldQuit) {
+                mIsQuitting.setTrue();
+            }
 
             teardownNetwork();
 
@@ -1341,7 +1349,7 @@ public class VcnGatewayConnection extends StateMachine {
     private class DisconnectedState extends BaseState {
         @Override
         protected void enterState() {
-            if (mIsQuitting) {
+            if (mIsQuitting.getValue()) {
                 quitNow(); // Ignore all queued events; cleanup is complete.
             }
 
@@ -1365,7 +1373,7 @@ public class VcnGatewayConnection extends StateMachine {
                     break;
                 case EVENT_DISCONNECT_REQUESTED:
                     if (((EventDisconnectRequestedInfo) msg.obj).shouldQuit) {
-                        mIsQuitting = true;
+                        mIsQuitting.setTrue();
 
                         quitNow();
                     }
@@ -1451,7 +1459,10 @@ public class VcnGatewayConnection extends StateMachine {
                     break;
                 case EVENT_DISCONNECT_REQUESTED:
                     EventDisconnectRequestedInfo info = ((EventDisconnectRequestedInfo) msg.obj);
-                    mIsQuitting = info.shouldQuit;
+                    if (info.shouldQuit) {
+                        mIsQuitting.setTrue();
+                    }
+
                     teardownNetwork();
 
                     if (info.reason.equals(DISCONNECT_REASON_UNDERLYING_NETWORK_LOST)) {
@@ -1467,7 +1478,7 @@ public class VcnGatewayConnection extends StateMachine {
                 case EVENT_SESSION_CLOSED:
                     mIkeSession = null;
 
-                    if (!mIsQuitting && mUnderlying != null) {
+                    if (!mIsQuitting.getValue() && mUnderlying != null) {
                         transitionTo(mSkipRetryTimeout ? mConnectingState : mRetryTimeoutState);
                     } else {
                         teardownNetwork();
@@ -1626,7 +1637,7 @@ public class VcnGatewayConnection extends StateMachine {
                                 teardownAsynchronously();
                             } /* networkUnwantedCallback */,
                             (status) -> {
-                                if (mIsQuitting) {
+                                if (mIsQuitting.getValue()) {
                                     return; // Ignore; VcnGatewayConnection quitting or already quit
                                 }
 
@@ -2180,18 +2191,15 @@ public class VcnGatewayConnection extends StateMachine {
     private void logVdbg(String msg) {
         if (VDBG) {
             Slog.v(TAG, getLogPrefix() + msg);
-            LOCAL_LOG.log(getLogPrefix() + "VDBG: " + msg);
         }
     }
 
     private void logDbg(String msg) {
         Slog.d(TAG, getLogPrefix() + msg);
-        LOCAL_LOG.log(getLogPrefix() + "DBG: " + msg);
     }
 
     private void logDbg(String msg, Throwable tr) {
         Slog.d(TAG, getLogPrefix() + msg, tr);
-        LOCAL_LOG.log(getLogPrefix() + "DBG: " + msg + tr);
     }
 
     private void logWarn(String msg) {
@@ -2238,7 +2246,7 @@ public class VcnGatewayConnection extends StateMachine {
                         + (getCurrentState() == null
                                 ? null
                                 : getCurrentState().getClass().getSimpleName()));
-        pw.println("mIsQuitting: " + mIsQuitting);
+        pw.println("mIsQuitting: " + mIsQuitting.getValue());
         pw.println("mIsInSafeMode: " + mIsInSafeMode);
         pw.println("mCurrentToken: " + mCurrentToken);
         pw.println("mFailedAttempts: " + mFailedAttempts);
@@ -2275,12 +2283,12 @@ public class VcnGatewayConnection extends StateMachine {
 
     @VisibleForTesting(visibility = Visibility.PRIVATE)
     boolean isQuitting() {
-        return mIsQuitting;
+        return mIsQuitting.getValue();
     }
 
     @VisibleForTesting(visibility = Visibility.PRIVATE)
-    void setIsQuitting(boolean isQuitting) {
-        mIsQuitting = isQuitting;
+    void setQuitting() {
+        mIsQuitting.setTrue();
     }
 
     @VisibleForTesting(visibility = Visibility.PRIVATE)
diff --git a/services/core/java/com/android/server/vcn/util/OneWayBoolean.java b/services/core/java/com/android/server/vcn/util/OneWayBoolean.java
new file mode 100644
index 000000000000..e79bb2d2547d
--- /dev/null
+++ b/services/core/java/com/android/server/vcn/util/OneWayBoolean.java
@@ -0,0 +1,39 @@
+/*
+ * Copyright (C) 2021 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.android.server.vcn.util;
+
+/**
+ * OneWayBoolean is an abstraction for a boolean that MUST only ever be flipped from false to true
+ *
+ * <p>This class allows the providing of a guarantee that a flag will never be flipped back after
+ * being set.
+ *
+ * @hide
+ */
+public class OneWayBoolean {
+    private boolean mValue = false;
+
+    /** Get boolean value. */
+    public boolean getValue() {
+        return mValue;
+    }
+
+    /** Sets the value to true. */
+    public void setTrue() {
+        mValue = true;
+    }
+}
diff --git a/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionConnectedStateTest.java b/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionConnectedStateTest.java
index 6bfbfb1c8496..0f84f6ebe522 100644
--- a/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionConnectedStateTest.java
+++ b/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionConnectedStateTest.java
@@ -578,6 +578,10 @@ public class VcnGatewayConnectionConnectedStateTest extends VcnGatewayConnection
         mGatewayConnection.teardownAsynchronously();
         mTestLooper.dispatchAll();
 
+        // Verify that sending a non-quitting disconnect request does not unset the isQuitting flag
+        mGatewayConnection.sendDisconnectRequestedAndAcquireWakelock("TEST", false);
+        mTestLooper.dispatchAll();
+
         assertEquals(mGatewayConnection.mDisconnectingState, mGatewayConnection.getCurrentState());
         assertTrue(mGatewayConnection.isQuitting());
     }
diff --git a/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionConnectingStateTest.java b/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionConnectingStateTest.java
index acc8bf98e95b..d1f3a210d870 100644
--- a/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionConnectingStateTest.java
+++ b/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionConnectingStateTest.java
@@ -127,6 +127,10 @@ public class VcnGatewayConnectionConnectingStateTest extends VcnGatewayConnectio
         mGatewayConnection.teardownAsynchronously();
         mTestLooper.dispatchAll();
 
+        // Verify that sending a non-quitting disconnect request does not unset the isQuitting flag
+        mGatewayConnection.sendDisconnectRequestedAndAcquireWakelock("TEST", false);
+        mTestLooper.dispatchAll();
+
         assertEquals(mGatewayConnection.mDisconnectingState, mGatewayConnection.getCurrentState());
         assertTrue(mGatewayConnection.isQuitting());
     }
diff --git a/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionDisconnectedStateTest.java b/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionDisconnectedStateTest.java
index ac0edaa3b579..2056eea42ce6 100644
--- a/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionDisconnectedStateTest.java
+++ b/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionDisconnectedStateTest.java
@@ -68,7 +68,7 @@ public class VcnGatewayConnectionDisconnectedStateTest extends VcnGatewayConnect
                         true /* isMobileDataEnabled */,
                         mDeps);
 
-        vgc.setIsQuitting(true);
+        vgc.setQuitting();
         vgc.transitionTo(vgc.mDisconnectedState);
         mTestLooper.dispatchAll();
 
@@ -102,6 +102,10 @@ public class VcnGatewayConnectionDisconnectedStateTest extends VcnGatewayConnect
         mGatewayConnection.teardownAsynchronously();
         mTestLooper.dispatchAll();
 
+        // Verify that sending a non-quitting disconnect request does not unset the isQuitting flag
+        mGatewayConnection.sendDisconnectRequestedAndAcquireWakelock("TEST", false);
+        mTestLooper.dispatchAll();
+
         assertNull(mGatewayConnection.getCurrentState());
         verify(mIpSecSvc).deleteTunnelInterface(eq(TEST_IPSEC_TUNNEL_RESOURCE_ID), any());
         verifySafeModeTimeoutAlarmAndGetCallback(true /* expectCanceled */);
diff --git a/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionDisconnectingStateTest.java b/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionDisconnectingStateTest.java
index 9da8b451c9fc..78aefad9f8ff 100644
--- a/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionDisconnectingStateTest.java
+++ b/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionDisconnectingStateTest.java
@@ -79,6 +79,10 @@ public class VcnGatewayConnectionDisconnectingStateTest extends VcnGatewayConnec
         mGatewayConnection.teardownAsynchronously();
         mTestLooper.dispatchAll();
 
+        // Verify that sending a non-quitting disconnect request does not unset the isQuitting flag
+        mGatewayConnection.sendDisconnectRequestedAndAcquireWakelock("TEST", false);
+        mTestLooper.dispatchAll();
+
         // Should do nothing; already tearing down.
         assertEquals(mGatewayConnection.mDisconnectingState, mGatewayConnection.getCurrentState());
         verifyTeardownTimeoutAlarmAndGetCallback(false /* expectCanceled */);
diff --git a/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionRetryTimeoutStateTest.java b/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionRetryTimeoutStateTest.java
index 69407657b3c4..1c859790a2fe 100644
--- a/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionRetryTimeoutStateTest.java
+++ b/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionRetryTimeoutStateTest.java
@@ -90,6 +90,10 @@ public class VcnGatewayConnectionRetryTimeoutStateTest extends VcnGatewayConnect
                 .onSelectedUnderlyingNetworkChanged(null);
         mTestLooper.dispatchAll();
 
+        // Verify that sending a non-quitting disconnect request does not unset the isQuitting flag
+        mGatewayConnection.sendDisconnectRequestedAndAcquireWakelock("TEST", false);
+        mTestLooper.dispatchAll();
+
         assertEquals(mGatewayConnection.mDisconnectedState, mGatewayConnection.getCurrentState());
         verifyRetryTimeoutAlarmAndGetCallback(mFirstRetryInterval, true /* expectCanceled */);