Fix legacy APIs when VPN switches to suspended underlying network.
Currently, when the VPN underlying network changes from a
network that is not suspended to one that is suspended (or vice
versa), some of the legacy APIs return incorrect results.
This is because the VPN's NetworkInfo can get into SUSPENDED
state even though the capabilities have the NOT_SUSPENDED
capability. This happens because the code in updateCapabilities
that checks for changes in NOT_SUSPENDED and NOT_ROAMING (which
are the capabilities that can affect the NetworkInfo state) is
only run when the capabilities change in a certain way.
Fix this by always checking for changes in these capabilities,
regardless of what else has changed.
This results in sending a lot more SUSPENDED and RESUMED
callbacks than the code sent previously. This should hopefully
not impact apps because those callback methods have never been
public API, though because they're just callbacks, it's possible
that apps found out via code inspection that the callbacks
existed and implemented them.
Bug: 172870110
Test: changes to existing tests in ConnectivityServiceTest
Change-Id: I6ec246a6a4e61f634956a165797fbb80296efd6a
Merged-In: I6ec246a6a4e61f634956a165797fbb80296efd6a
(cherry picked from commit 5b90ebaf4d9edefcd9648b46cd0226f882169476)
diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java
index cf14c63..a1cbd00 100644
--- a/services/core/java/com/android/server/ConnectivityService.java
+++ b/services/core/java/com/android/server/ConnectivityService.java
@@ -6318,6 +6318,25 @@
return newNc;
}
+ private void updateNetworkInfoForRoamingAndSuspended(NetworkAgentInfo nai,
+ NetworkCapabilities prevNc, NetworkCapabilities newNc) {
+ final boolean prevSuspended = !prevNc.hasCapability(NET_CAPABILITY_NOT_SUSPENDED);
+ final boolean suspended = !newNc.hasCapability(NET_CAPABILITY_NOT_SUSPENDED);
+ final boolean prevRoaming = !prevNc.hasCapability(NET_CAPABILITY_NOT_ROAMING);
+ final boolean roaming = !newNc.hasCapability(NET_CAPABILITY_NOT_ROAMING);
+ if (prevSuspended != suspended) {
+ // TODO (b/73132094) : remove this call once the few users of onSuspended and
+ // onResumed have been removed.
+ notifyNetworkCallbacks(nai, suspended ? ConnectivityManager.CALLBACK_SUSPENDED
+ : ConnectivityManager.CALLBACK_RESUMED);
+ }
+ if (prevSuspended != suspended || prevRoaming != roaming) {
+ // updateNetworkInfo will mix in the suspended info from the capabilities and
+ // take appropriate action for the network having possibly changed state.
+ updateNetworkInfo(nai, nai.networkInfo);
+ }
+ }
+
/**
* Update the NetworkCapabilities for {@code nai} to {@code nc}. Specifically:
*
@@ -6349,25 +6368,13 @@
// on this network. We might have been called by rematchNetworkAndRequests when a
// network changed foreground state.
processListenRequests(nai);
- final boolean prevSuspended = !prevNc.hasCapability(NET_CAPABILITY_NOT_SUSPENDED);
- final boolean suspended = !newNc.hasCapability(NET_CAPABILITY_NOT_SUSPENDED);
- final boolean prevRoaming = !prevNc.hasCapability(NET_CAPABILITY_NOT_ROAMING);
- final boolean roaming = !newNc.hasCapability(NET_CAPABILITY_NOT_ROAMING);
- if (prevSuspended != suspended || prevRoaming != roaming) {
- // TODO (b/73132094) : remove this call once the few users of onSuspended and
- // onResumed have been removed.
- notifyNetworkCallbacks(nai, suspended ? ConnectivityManager.CALLBACK_SUSPENDED
- : ConnectivityManager.CALLBACK_RESUMED);
- // updateNetworkInfo will mix in the suspended info from the capabilities and
- // take appropriate action for the network having possibly changed state.
- updateNetworkInfo(nai, nai.networkInfo);
- }
} else {
// If the requestable capabilities have changed or the score changed, we can't have been
// called by rematchNetworkAndRequests, so it's safe to start a rematch.
rematchAllNetworksAndRequests();
notifyNetworkCallbacks(nai, ConnectivityManager.CALLBACK_CAP_CHANGED);
}
+ updateNetworkInfoForRoamingAndSuspended(nai, prevNc, newNc);
// TODO : static analysis indicates that prevNc can't be null here (getAndSetNetworkCaps
// never returns null), so mark the relevant members and functions in nai as @NonNull and
diff --git a/tests/net/java/com/android/server/ConnectivityServiceTest.java b/tests/net/java/com/android/server/ConnectivityServiceTest.java
index 3d4ff41..a5554c7 100644
--- a/tests/net/java/com/android/server/ConnectivityServiceTest.java
+++ b/tests/net/java/com/android/server/ConnectivityServiceTest.java
@@ -5488,23 +5488,18 @@
callback.expectCapabilitiesThat(mMockVpn,
nc -> nc.hasCapability(NET_CAPABILITY_NOT_SUSPENDED)
&& nc.hasTransport(TRANSPORT_WIFI));
-
- // BUG: the VPN is no longer suspended, so a RESUMED callback should have been sent.
- // callback.expectCallback(CallbackEntry.RESUMED, mMockVpn);
+ callback.expectCallback(CallbackEntry.RESUMED, mMockVpn);
callback.assertNoCallback();
assertTrue(mCm.getNetworkCapabilities(mMockVpn.getNetwork())
.hasCapability(NET_CAPABILITY_NOT_SUSPENDED));
assertNetworkInfo(TYPE_MOBILE, DetailedState.DISCONNECTED);
assertNetworkInfo(TYPE_WIFI, DetailedState.CONNECTED);
- assertNetworkInfo(TYPE_VPN, DetailedState.SUSPENDED); // BUG: VPN caps have NOT_SUSPENDED.
+ assertNetworkInfo(TYPE_VPN, DetailedState.CONNECTED);
assertActiveNetworkInfo(TYPE_WIFI, DetailedState.CONNECTED);
- // BUG: the device has connectivity, so this should return true.
- assertGetNetworkInfoOfGetActiveNetworkIsConnected(false);
+ assertGetNetworkInfoOfGetActiveNetworkIsConnected(true);
- // Unsuspend cellular and then switch back to it.
- // The same bug happens in the opposite direction: the VPN's capabilities correctly have
- // NOT_SUSPENDED, but the VPN's NetworkInfo is in state SUSPENDED.
+ // Unsuspend cellular and then switch back to it. The VPN remains not suspended.
mCellNetworkAgent.resume();
mWiFiNetworkAgent.disconnect();
callback.expectCapabilitiesThat(mMockVpn,
@@ -5520,12 +5515,11 @@
.hasCapability(NET_CAPABILITY_NOT_SUSPENDED));
assertNetworkInfo(TYPE_MOBILE, DetailedState.CONNECTED);
assertNetworkInfo(TYPE_WIFI, DetailedState.DISCONNECTED);
- assertNetworkInfo(TYPE_VPN, DetailedState.SUSPENDED); // BUG: VPN caps have NOT_SUSPENDED.
+ assertNetworkInfo(TYPE_VPN, DetailedState.CONNECTED);
assertActiveNetworkInfo(TYPE_MOBILE, DetailedState.CONNECTED);
- // BUG: the device has connectivity, so this should return true.
- assertGetNetworkInfoOfGetActiveNetworkIsConnected(false);
+ assertGetNetworkInfoOfGetActiveNetworkIsConnected(true);
- // Re-suspending the current network fixes the problem.
+ // Suspend cellular and expect no connectivity.
mCellNetworkAgent.suspend();
callback.expectCapabilitiesThat(mMockVpn,
nc -> !nc.hasCapability(NET_CAPABILITY_NOT_SUSPENDED)
@@ -5541,6 +5535,7 @@
assertActiveNetworkInfo(TYPE_MOBILE, DetailedState.SUSPENDED);
assertGetNetworkInfoOfGetActiveNetworkIsConnected(false);
+ // Resume cellular and expect that connectivity comes back.
mCellNetworkAgent.resume();
callback.expectCapabilitiesThat(mMockVpn,
nc -> nc.hasCapability(NET_CAPABILITY_NOT_SUSPENDED)
@@ -5926,10 +5921,7 @@
&& caps.hasTransport(TRANSPORT_CELLULAR)
&& !caps.hasCapability(NET_CAPABILITY_NOT_METERED)
&& !caps.hasCapability(NET_CAPABILITY_NOT_SUSPENDED));
- // While the SUSPENDED callback should in theory be sent here, it is not. This is
- // a bug in ConnectivityService, but as the SUSPENDED and RESUMED callbacks have never
- // been public and are deprecated and slated for removal, there is no sense in spending
- // resources fixing this bug now.
+ vpnNetworkCallback.expectCallback(CallbackEntry.SUSPENDED, mMockVpn);
assertDefaultNetworkCapabilities(userId, mCellNetworkAgent, mWiFiNetworkAgent);
// Use both again.
@@ -5941,8 +5933,7 @@
&& caps.hasTransport(TRANSPORT_CELLULAR) && caps.hasTransport(TRANSPORT_WIFI)
&& !caps.hasCapability(NET_CAPABILITY_NOT_METERED)
&& caps.hasCapability(NET_CAPABILITY_NOT_SUSPENDED));
- // As above, the RESUMED callback not being sent here is a bug, but not a bug that's
- // worth anybody's time to fix.
+ vpnNetworkCallback.expectCallback(CallbackEntry.RESUMED, mMockVpn);
assertDefaultNetworkCapabilities(userId, mCellNetworkAgent, mWiFiNetworkAgent);
// Disconnect cell. Receive update without even removing the dead network from the
