Merge "Readability improvements to the Vpn class with no logic changes"
diff --git a/services/core/java/com/android/server/connectivity/Vpn.java b/services/core/java/com/android/server/connectivity/Vpn.java
index c7a4315..3c2d724 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 @@
// 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 @@
@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 @@
}
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 @@
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 @@
}
/**
- * 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 @@
}
private void unregisterPackageChangeReceiverLocked() {
- // register previous intent filter
if (mIsPackageIntentReceiverRegistered) {
mContext.unregisterReceiver(mPackageIntentReceiver);
mIsPackageIntentReceiverRegistered = false;
@@ -582,7 +582,7 @@
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 @@
* 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 @@
* @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 @@
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 @@
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 @@
}
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 @@
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 @@
}
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 @@
}
/**
- * 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 @@
* 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 @@
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 @@
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 @@
}
}
- 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 @@
// Wait for the daemon to start.
while (!SystemService.isRunning(daemon)) {
- checkpoint(true);
+ checkInterruptAndDelay(true);
}
// Create the control socket.
@@ -1959,7 +1957,7 @@
} catch (Exception e) {
// ignore
}
- checkpoint(true);
+ checkInterruptAndDelay(true);
}
mSockets[i].setSoTimeout(500);
@@ -1973,7 +1971,7 @@
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 @@
} catch (Exception e) {
// ignore
}
- checkpoint(true);
+ checkInterruptAndDelay(true);
}
}
@@ -2002,7 +2000,7 @@
throw new IllegalStateException(daemon + " is dead");
}
}
- checkpoint(true);
+ checkInterruptAndDelay(true);
}
// Now we are connected. Read and parse the new state.
@@ -2058,8 +2056,8 @@
// 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 @@
}
/**
- * 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;
}