Merge "Readability improvements to the Vpn class with no logic changes"

1 files changed, 45 insertions, 46 deletions

diff --git a/services/core/java/com/android/server/connectivity/Vpn.java b/services/core/java/com/android/server/connectivity/Vpn.java
index c7a43153c0aa..3c2d72407b4e 100644
--- a/services/core/java/com/android/server/connectivity/Vpn.java
+++ b/services/core/java/com/android/server/connectivity/Vpn.java
@@ -128,7 +128,7 @@ public class Vpn {
 
     // Length of time (in milliseconds) that an app hosting an always-on VPN is placed on
     // the device idle whitelist during service launch and VPN bootstrap.
-    private static final long VPN_LAUNCH_IDLE_WHITELIST_DURATION = 60 * 1000;
+    private static final long VPN_LAUNCH_IDLE_WHITELIST_DURATION_MS = 60 * 1000;
 
     // TODO: create separate trackers for each unique VPN to support
     // automated reconnection
@@ -183,10 +183,10 @@ public class Vpn {
     @GuardedBy("this")
     private Set<UidRange> mBlockedUsers = new ArraySet<>();
 
-    // Handle of user initiating VPN.
+    // Handle of the user initiating VPN.
     private final int mUserHandle;
 
-    // Listen to package remove and change event in this user
+    // Listen to package removal and change events (update/uninstall) for this user
     private final BroadcastReceiver mPackageIntentReceiver = new BroadcastReceiver() {
         @Override
         public void onReceive(Context context, Intent intent) {
@@ -197,14 +197,14 @@ public class Vpn {
             }
 
             synchronized (Vpn.this) {
-                // Avoid race that always-on package has been unset
+                // Avoid race where always-on package has been unset
                 if (!packageName.equals(getAlwaysOnPackage())) {
                     return;
                 }
 
                 final String action = intent.getAction();
-                Log.i(TAG, "Received broadcast " + action + " for always-on package " + packageName
-                        + " in user " + mUserHandle);
+                Log.i(TAG, "Received broadcast " + action + " for always-on VPN package "
+                        + packageName + " in user " + mUserHandle);
 
                 switch(action) {
                     case Intent.ACTION_PACKAGE_REPLACED:
@@ -248,7 +248,8 @@ public class Vpn {
             Log.wtf(TAG, "Problem registering observer", e);
         }
 
-        mNetworkInfo = new NetworkInfo(ConnectivityManager.TYPE_VPN, 0, NETWORKTYPE, "");
+        mNetworkInfo = new NetworkInfo(ConnectivityManager.TYPE_VPN, 0 /* subtype */, NETWORKTYPE,
+                "" /* subtypeName */);
         mNetworkCapabilities = new NetworkCapabilities();
         mNetworkCapabilities.addTransportType(NetworkCapabilities.TRANSPORT_VPN);
         mNetworkCapabilities.removeCapability(NetworkCapabilities.NET_CAPABILITY_NOT_VPN);
@@ -258,7 +259,7 @@ public class Vpn {
     }
 
     /**
-     * Set if this object is responsible for watching for {@link NetworkInfo}
+     * Set whether this object is responsible for watching for {@link NetworkInfo}
      * teardown. When {@code false}, teardown is handled externally by someone
      * else.
      */
@@ -481,7 +482,6 @@ public class Vpn {
     }
 
     private void unregisterPackageChangeReceiverLocked() {
-        // register previous intent filter
         if (mIsPackageIntentReceiverRegistered) {
             mContext.unregisterReceiver(mPackageIntentReceiver);
             mIsPackageIntentReceiverRegistered = false;
@@ -582,7 +582,7 @@ public class Vpn {
             DeviceIdleController.LocalService idleController =
                     LocalServices.getService(DeviceIdleController.LocalService.class);
             idleController.addPowerSaveTempWhitelistApp(Process.myUid(), alwaysOnPackage,
-                    VPN_LAUNCH_IDLE_WHITELIST_DURATION, mUserHandle, false, "vpn");
+                    VPN_LAUNCH_IDLE_WHITELIST_DURATION_MS, mUserHandle, false, "vpn");
 
             // Start the VPN service declared in the app's manifest.
             Intent serviceIntent = new Intent(VpnConfig.SERVICE_INTERFACE);
@@ -612,9 +612,10 @@ public class Vpn {
      * It uses {@link VpnConfig#LEGACY_VPN} as its package name, and
      * it can be revoked by itself.
      *
-     * Note: when we added VPN pre-consent in http://ag/522961 the names oldPackage
-     * and newPackage become misleading, because when an app is pre-consented, we
-     * actually prepare oldPackage, not newPackage.
+     * Note: when we added VPN pre-consent in
+     * https://android.googlesource.com/platform/frameworks/base/+/0554260
+     * the names oldPackage and newPackage became misleading, because when
+     * an app is pre-consented, we actually prepare oldPackage, not newPackage.
      *
      * Their meanings actually are:
      *
@@ -630,7 +631,7 @@ public class Vpn {
      * @param oldPackage The package name of the old VPN application
      * @param newPackage The package name of the new VPN application
      *
-     * @return true if the operation is succeeded.
+     * @return true if the operation succeeded.
      */
     public synchronized boolean prepare(String oldPackage, String newPackage) {
         if (oldPackage != null) {
@@ -639,7 +640,7 @@ public class Vpn {
                 return false;
             }
 
-            // Package is not same or old package was reinstalled.
+            // Package is not the same or old package was reinstalled.
             if (!isCurrentPreparedPackage(oldPackage)) {
                 // The package doesn't match. We return false (to obtain user consent) unless the
                 // user has already consented to that VPN package.
@@ -861,8 +862,8 @@ public class Vpn {
 
         long token = Binder.clearCallingIdentity();
         try {
-            mNetworkAgent = new NetworkAgent(mLooper, mContext, NETWORKTYPE,
-                    mNetworkInfo, mNetworkCapabilities, lp, 0, networkMisc) {
+            mNetworkAgent = new NetworkAgent(mLooper, mContext, NETWORKTYPE /* logtag */,
+                    mNetworkInfo, mNetworkCapabilities, lp, 0 /* score */, networkMisc) {
                             @Override
                             public void unwanted() {
                                 // We are user controlled, not driven by NetworkRequest.
@@ -936,7 +937,7 @@ public class Vpn {
             }
 
             ResolveInfo info = AppGlobals.getPackageManager().resolveService(intent,
-                                                                        null, 0, mUserHandle);
+                    null, 0, mUserHandle);
             if (info == null) {
                 throw new SecurityException("Cannot find " + config.user);
             }
@@ -944,7 +945,7 @@ public class Vpn {
                 throw new SecurityException(config.user + " does not require " + BIND_VPN_SERVICE);
             }
         } catch (RemoteException e) {
-                throw new SecurityException("Cannot find " + config.user);
+            throw new SecurityException("Cannot find " + config.user);
         } finally {
             Binder.restoreCallingIdentity(token);
         }
@@ -1337,7 +1338,7 @@ public class Vpn {
     }
 
     private void enforceControlPermissionOrInternalCaller() {
-        // Require caller to be either an application with CONTROL_VPN permission or a process
+        // Require the caller to be either an application with CONTROL_VPN permission or a process
         // in the system server.
         mContext.enforceCallingOrSelfPermission(Manifest.permission.CONTROL_VPN,
                 "Unauthorized Caller");
@@ -1417,7 +1418,7 @@ public class Vpn {
     }
 
     /**
-     * This method should only be called by ConnectivityService. Because it doesn't
+     * This method should only be called by ConnectivityService because it doesn't
      * have enough data to fill VpnInfo.primaryUnderlyingIface field.
      */
     public synchronized VpnInfo getVpnInfo() {
@@ -1768,7 +1769,7 @@ public class Vpn {
      * Bringing up a VPN connection takes time, and that is all this thread
      * does. Here we have plenty of time. The only thing we need to take
      * care of is responding to interruptions as soon as possible. Otherwise
-     * requests will be piled up. This can be done in a Handler as a state
+     * requests will pile up. This could be done in a Handler as a state
      * machine, but it is much easier to read in the current form.
      */
     private class LegacyVpnRunner extends Thread {
@@ -1781,7 +1782,7 @@ public class Vpn {
         private final AtomicInteger mOuterConnection =
                 new AtomicInteger(ConnectivityManager.TYPE_NONE);
 
-        private long mTimer = -1;
+        private long mBringupStartTime = -1;
 
         /**
          * Watch for the outer connection (passing in the constructor) going away.
@@ -1861,8 +1862,8 @@ public class Vpn {
             synchronized (TAG) {
                 Log.v(TAG, "Executing");
                 try {
-                    execute();
-                    monitorDaemons();
+                    bringup();
+                    waitForDaemonsToStop();
                     interrupted(); // Clear interrupt flag if execute called exit.
                 } catch (InterruptedException e) {
                 } finally {
@@ -1883,30 +1884,27 @@ public class Vpn {
             }
         }
 
-        private void checkpoint(boolean yield) throws InterruptedException {
+        private void checkInterruptAndDelay(boolean sleepLonger) throws InterruptedException {
             long now = SystemClock.elapsedRealtime();
-            if (mTimer == -1) {
-                mTimer = now;
-                Thread.sleep(1);
-            } else if (now - mTimer <= 60000) {
-                Thread.sleep(yield ? 200 : 1);
+            if (now - mBringupStartTime <= 60000) {
+                Thread.sleep(sleepLonger ? 200 : 1);
             } else {
                 updateState(DetailedState.FAILED, "checkpoint");
-                throw new IllegalStateException("Time is up");
+                throw new IllegalStateException("VPN bringup took too long");
             }
         }
 
-        private void execute() {
-            // Catch all exceptions so we can clean up few things.
+        private void bringup() {
+            // Catch all exceptions so we can clean up a few things.
             boolean initFinished = false;
             try {
                 // Initialize the timer.
-                checkpoint(false);
+                mBringupStartTime = SystemClock.elapsedRealtime();
 
                 // Wait for the daemons to stop.
                 for (String daemon : mDaemons) {
                     while (!SystemService.isStopped(daemon)) {
-                        checkpoint(true);
+                        checkInterruptAndDelay(true);
                     }
                 }
 
@@ -1943,7 +1941,7 @@ public class Vpn {
 
                     // Wait for the daemon to start.
                     while (!SystemService.isRunning(daemon)) {
-                        checkpoint(true);
+                        checkInterruptAndDelay(true);
                     }
 
                     // Create the control socket.
@@ -1959,7 +1957,7 @@ public class Vpn {
                         } catch (Exception e) {
                             // ignore
                         }
-                        checkpoint(true);
+                        checkInterruptAndDelay(true);
                     }
                     mSockets[i].setSoTimeout(500);
 
@@ -1973,7 +1971,7 @@ public class Vpn {
                         out.write(bytes.length >> 8);
                         out.write(bytes.length);
                         out.write(bytes);
-                        checkpoint(false);
+                        checkInterruptAndDelay(false);
                     }
                     out.write(0xFF);
                     out.write(0xFF);
@@ -1989,7 +1987,7 @@ public class Vpn {
                         } catch (Exception e) {
                             // ignore
                         }
-                        checkpoint(true);
+                        checkInterruptAndDelay(true);
                     }
                 }
 
@@ -2002,7 +2000,7 @@ public class Vpn {
                             throw new IllegalStateException(daemon + " is dead");
                         }
                     }
-                    checkpoint(true);
+                    checkInterruptAndDelay(true);
                 }
 
                 // Now we are connected. Read and parse the new state.
@@ -2058,8 +2056,8 @@ public class Vpn {
                     // Set the start time
                     mConfig.startTime = SystemClock.elapsedRealtime();
 
-                    // Check if the thread is interrupted while we are waiting.
-                    checkpoint(false);
+                    // Check if the thread was interrupted while we were waiting on the lock.
+                    checkInterruptAndDelay(false);
 
                     // Check if the interface is gone while we are waiting.
                     if (jniCheck(mConfig.interfaze) == 0) {
@@ -2082,10 +2080,11 @@ public class Vpn {
         }
 
         /**
-         * Monitor the daemons we started, moving to disconnected state if the
-         * underlying services fail.
+         * Check all daemons every two seconds. Return when one of them is stopped.
+         * The caller will move to the disconnected state when this function returns,
+         * which can happen if a daemon failed or if the VPN was torn down.
          */
-        private void monitorDaemons() throws InterruptedException{
+        private void waitForDaemonsToStop() throws InterruptedException {
             if (!mNetworkInfo.isConnected()) {
                 return;
             }