From 0a2a20054d0107ef74f439af89fec87c6a9ee3c7 Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Fri, 29 Nov 2019 16:41:50 +0900 Subject: [PATCH 1/4] =?UTF-8?q?Add=20tests=20for=20ConnectivityService=20?= =?UTF-8?q?=E2=86=92=20BatteryStats=20messages?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Test: ConnectivityServiceTest Bug: 113554781 Change-Id: I7d3a16be76f606872f8edb84647b9ef94f36cba1 --- .../android/server/ConnectivityService.java | 8 ++- .../server/ConnectivityServiceTest.java | 60 +++++++++++++++++++ 2 files changed, 66 insertions(+), 2 deletions(-) diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index bb7406a567..851f3c17e8 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -932,6 +932,10 @@ public class ConnectivityService extends IConnectivityManager.Stub return IIpConnectivityMetrics.Stub.asInterface( ServiceManager.getService(IpConnectivityLog.SERVICE_NAME)); } + + public IBatteryStats getBatteryStatsService() { + return BatteryStatsService.getService(); + } } public ConnectivityService(Context context, INetworkManagementService netManager, @@ -2164,7 +2168,7 @@ public class ConnectivityService extends IConnectivityManager.Stub opts.setMaxManifestReceiverApiLevel(Build.VERSION_CODES.M); options = opts.toBundle(); } - final IBatteryStats bs = BatteryStatsService.getService(); + final IBatteryStats bs = mDeps.getBatteryStatsService(); try { bs.noteConnectivityChanged(intent.getIntExtra( ConnectivityManager.EXTRA_NETWORK_TYPE, ConnectivityManager.TYPE_NONE), @@ -6505,7 +6509,7 @@ public class ConnectivityService extends IConnectivityManager.Stub // interface and any stacked links. // TODO: Avoid redoing this; this must only be done once when a network comes online. try { - final IBatteryStats bs = BatteryStatsService.getService(); + final IBatteryStats bs = mDeps.getBatteryStatsService(); final int type = newNetwork.networkInfo.getType(); final String baseIface = newNetwork.linkProperties.getInterfaceName(); diff --git a/tests/net/java/com/android/server/ConnectivityServiceTest.java b/tests/net/java/com/android/server/ConnectivityServiceTest.java index 7ea9bcf36f..da83397b49 100644 --- a/tests/net/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/net/java/com/android/server/ConnectivityServiceTest.java @@ -96,6 +96,7 @@ import static org.junit.Assert.fail; import static org.mockito.ArgumentMatchers.anyLong; import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.ArgumentMatchers.startsWith; import static org.mockito.Matchers.anyInt; import static org.mockito.Mockito.any; import static org.mockito.Mockito.atLeastOnce; @@ -197,6 +198,7 @@ import androidx.test.filters.FlakyTest; import androidx.test.filters.SmallTest; import androidx.test.runner.AndroidJUnit4; +import com.android.internal.app.IBatteryStats; import com.android.internal.net.VpnConfig; import com.android.internal.net.VpnInfo; import com.android.internal.util.ArrayUtils; @@ -305,6 +307,7 @@ public class ConnectivityServiceTest { @Mock DefaultNetworkMetrics mDefaultNetworkMetrics; @Mock INetworkManagementService mNetworkManagementService; @Mock INetworkStatsService mStatsService; + @Mock IBatteryStats mBatteryStatsService; @Mock INetworkPolicyManager mNpm; @Mock IDnsResolver mMockDnsResolver; @Mock INetd mMockNetd; @@ -1135,6 +1138,7 @@ public class ConnectivityServiceTest { doReturn(mMetricsService).when(deps).getMetricsLogger(); doReturn(true).when(deps).queryUserAccess(anyInt(), anyInt()); doReturn(mIpConnectivityMetrics).when(deps).getIpConnectivityMetrics(); + doReturn(mBatteryStatsService).when(deps).getBatteryStatsService(); doReturn(true).when(deps).hasService(Context.ETHERNET_SERVICE); doAnswer(inv -> { mPolicyTracker = new WrappedMultinetworkPolicyTracker( @@ -5640,6 +5644,44 @@ public class ConnectivityServiceTest { mCm.unregisterNetworkCallback(defaultCallback); } + @Test + public final void testBatteryStatsNetworkType() throws Exception { + final LinkProperties cellLp = new LinkProperties(); + cellLp.setInterfaceName("cell0"); + mCellNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_CELLULAR, cellLp); + mCellNetworkAgent.connect(true); + waitForIdle(); + verify(mBatteryStatsService).noteNetworkInterfaceType(cellLp.getInterfaceName(), + TYPE_MOBILE); + reset(mBatteryStatsService); + + final LinkProperties wifiLp = new LinkProperties(); + wifiLp.setInterfaceName("wifi0"); + mWiFiNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_WIFI, wifiLp); + mWiFiNetworkAgent.connect(true); + waitForIdle(); + verify(mBatteryStatsService).noteNetworkInterfaceType(wifiLp.getInterfaceName(), + TYPE_WIFI); + reset(mBatteryStatsService); + + // TODO : In the current code, ConnectivityService only tells BatteryStatsService about + // the type of networks that satisfy a request. That is a bug in a sense, but it has no + // consequences because a network that never satisfies any request gets torn down right + // away. Because of this, in the context of this test, the cell network agent does not + // satisfy any request as long as WiFi is connected, so the test below would fail if + // the WiFi network agent is not disconnected first. When this bug is fixed, remove the + // WiFi disconnect for more precise testing. + mWiFiNetworkAgent.disconnect(); + mCellNetworkAgent.disconnect(); + + cellLp.setInterfaceName("wifi0"); + mCellNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_CELLULAR, cellLp); + mCellNetworkAgent.connect(true); + waitForIdle(); + verify(mBatteryStatsService).noteNetworkInterfaceType(cellLp.getInterfaceName(), + TYPE_MOBILE); + } + /** * Make simulated InterfaceConfig for Nat464Xlat to query clat lower layer info. */ @@ -5690,15 +5732,19 @@ public class ConnectivityServiceTest { reset(mNetworkManagementService); reset(mMockDnsResolver); reset(mMockNetd); + reset(mBatteryStatsService); when(mNetworkManagementService.getInterfaceConfig(CLAT_PREFIX + MOBILE_IFNAME)) .thenReturn(getClatInterfaceConfig(myIpv4)); // Connect with ipv6 link properties. Expect prefix discovery to be started. mCellNetworkAgent.sendLinkProperties(cellLp); mCellNetworkAgent.connect(true); + waitForIdle(); verify(mMockNetd, times(1)).networkCreatePhysical(eq(cellNetId), anyInt()); verify(mMockDnsResolver, times(1)).createNetworkCache(eq(cellNetId)); + verify(mBatteryStatsService).noteNetworkInterfaceType(cellLp.getInterfaceName(), + TYPE_MOBILE); networkCallback.expectAvailableThenValidatedCallbacks(mCellNetworkAgent); verify(mMockDnsResolver, times(1)).startPrefix64Discovery(cellNetId); @@ -5714,6 +5760,11 @@ public class ConnectivityServiceTest { verify(mMockDnsResolver, times(1)).stopPrefix64Discovery(cellNetId); verify(mMockDnsResolver, atLeastOnce()).setResolverConfiguration(any()); + // Make sure BatteryStats was not told about any v4- interfaces, as none should have + // come online yet. + waitForIdle(); + verify(mBatteryStatsService, never()).noteNetworkInterfaceType(startsWith("v4-"), anyInt()); + verifyNoMoreInteractions(mMockNetd); verifyNoMoreInteractions(mMockDnsResolver); reset(mMockNetd); @@ -5760,6 +5811,15 @@ public class ConnectivityServiceTest { assertEquals(1, resolvrParams.servers.length); assertTrue(ArrayUtils.contains(resolvrParams.servers, "8.8.8.8")); + // TODO : this should be invoked but in the current code there is no path to invoke + // it. In practice, it will be invoked next time this network changes what requests it + // satisfies through rematchNetworkAndRequests, which may in fact be too late. This code + // should be reinstated when the bug is fixed. +// for (final LinkProperties stackedLp : stackedLpsAfterChange) { +// verify(mBatteryStatsService).noteNetworkInterfaceType(stackedLp.getInterfaceName(), +// TYPE_MOBILE); +// } + // Add ipv4 address, expect that clatd and prefix discovery are stopped and stacked // linkproperties are cleaned up. cellLp.addLinkAddress(myIpv4); From 61c79256b12c323ff621d465553352cbf1eb3c19 Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Thu, 7 Nov 2019 23:07:32 +0900 Subject: [PATCH 2/4] [NS A12] Move some legacy type tracker handling to a function MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It's fine to do this out of the if() clause because : • If the network newly satisfies a request it is sure to have it in the list of requests it satisfies • If it does not newly satisfy a request and there is still a request with a legacy type that it satisfies, then it is already remembered by LegacyTypeTracker As for the VPN, the code always enters the condition anyway. Test: ConnectivityServiceTest Change-Id: I8bd668ad27043d6a5036b1b3c52fa5a3146abcfa --- .../android/server/ConnectivityService.java | 64 ++++++++++--------- 1 file changed, 34 insertions(+), 30 deletions(-) diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index 851f3c17e8..fc42982fb4 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -6520,37 +6520,23 @@ public class ConnectivityService extends IConnectivityManager.Stub } } catch (RemoteException ignored) { } - - // This has to happen after the notifyNetworkCallbacks as that tickles each - // ConnectivityManager instance so that legacy requests correctly bind dns - // requests to this network. The legacy users are listening for this broadcast - // and will generally do a dns request so they can ensureRouteToHost and if - // they do that before the callbacks happen they'll use the default network. - // - // TODO: Is there still a race here? We send the broadcast - // after sending the callback, but if the app can receive the - // broadcast before the callback, it might still break. - // - // This *does* introduce a race where if the user uses the new api - // (notification callbacks) and then uses the old api (getNetworkInfo(type)) - // they may get old info. Reverse this after the old startUsing api is removed. - // This is on top of the multiple intent sequencing referenced in the todo above. - for (int i = 0; i < newNetwork.numNetworkRequests(); i++) { - NetworkRequest nr = newNetwork.requestAt(i); - if (nr.legacyType != TYPE_NONE && nr.isRequest()) { - // legacy type tracker filters out repeat adds - mLegacyTypeTracker.add(nr.legacyType, newNetwork); - } - } - - // A VPN generally won't get added to the legacy tracker in the "for (nri)" loop above, - // because usually there are no NetworkRequests it satisfies (e.g., mDefaultRequest - // wants the NOT_VPN capability, so it will never be satisfied by a VPN). So, add the - // newNetwork to the tracker explicitly (it's a no-op if it has already been added). - if (newNetwork.isVPN()) { - mLegacyTypeTracker.add(TYPE_VPN, newNetwork); - } } + + // This has to happen after the notifyNetworkCallbacks as that tickles each + // ConnectivityManager instance so that legacy requests correctly bind dns + // requests to this network. The legacy users are listening for this broadcast + // and will generally do a dns request so they can ensureRouteToHost and if + // they do that before the callbacks happen they'll use the default network. + // + // TODO: Is there still a race here? We send the broadcast + // after sending the callback, but if the app can receive the + // broadcast before the callback, it might still break. + // + // This *does* introduce a race where if the user uses the new api + // (notification callbacks) and then uses the old api (getNetworkInfo(type)) + // they may get old info. Reverse this after the old startUsing api is removed. + // This is on top of the multiple intent sequencing referenced in the todo above. + addNetworkToLegacyTypeTracker(newNetwork); } /** @@ -6592,6 +6578,24 @@ public class ConnectivityService extends IConnectivityManager.Stub } } + private void addNetworkToLegacyTypeTracker(@NonNull final NetworkAgentInfo nai) { + for (int i = 0; i < nai.numNetworkRequests(); i++) { + NetworkRequest nr = nai.requestAt(i); + if (nr.legacyType != TYPE_NONE && nr.isRequest()) { + // legacy type tracker filters out repeat adds + mLegacyTypeTracker.add(nr.legacyType, nai); + } + } + + // A VPN generally won't get added to the legacy tracker in the "for (nri)" loop above, + // because usually there are no NetworkRequests it satisfies (e.g., mDefaultRequest + // wants the NOT_VPN capability, so it will never be satisfied by a VPN). So, add the + // newNetwork to the tracker explicitly (it's a no-op if it has already been added). + if (nai.isVPN()) { + mLegacyTypeTracker.add(TYPE_VPN, nai); + } + } + private void updateInetCondition(NetworkAgentInfo nai) { // Don't bother updating until we've graduated to validated at least once. if (!nai.everValidated) return; From f01b2efc1357030e8b15d876b604e3176b54e01b Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Thu, 7 Nov 2019 23:16:12 +0900 Subject: [PATCH 3/4] [NS A13] Move legacy broadcast handling after rematch. As opposed to other patches in this series, there is a logic change here. This will send all necessary legacy broadcasts after all matches have been done. This should be fine because the callbacks and the broadcasts are unordered anyway, and the broadcasts are still sent in the same order as before ; there should not be an observable change from apps besides some jitter. Bug: 113554781 Test: ConnectivityServiceTest Change-Id: Ibeab8d0a9106c5198228888ac33084238c0a4a1a --- .../android/server/ConnectivityService.java | 36 ++++++++++--------- 1 file changed, 20 insertions(+), 16 deletions(-) diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index fc42982fb4..2bbcf0dfa8 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -6521,22 +6521,6 @@ public class ConnectivityService extends IConnectivityManager.Stub } catch (RemoteException ignored) { } } - - // This has to happen after the notifyNetworkCallbacks as that tickles each - // ConnectivityManager instance so that legacy requests correctly bind dns - // requests to this network. The legacy users are listening for this broadcast - // and will generally do a dns request so they can ensureRouteToHost and if - // they do that before the callbacks happen they'll use the default network. - // - // TODO: Is there still a race here? We send the broadcast - // after sending the callback, but if the app can receive the - // broadcast before the callback, it might still break. - // - // This *does* introduce a race where if the user uses the new api - // (notification callbacks) and then uses the old api (getNetworkInfo(type)) - // they may get old info. Reverse this after the old startUsing api is removed. - // This is on top of the multiple intent sequencing referenced in the todo above. - addNetworkToLegacyTypeTracker(newNetwork); } /** @@ -6559,6 +6543,26 @@ public class ConnectivityService extends IConnectivityManager.Stub for (NetworkAgentInfo nai : nais) { rematchNetworkAndRequests(nai, now); } + + // Now that all the callbacks have been sent, send the legacy network broadcasts + // as needed. This is necessary so that legacy requests correctly bind dns + // requests to this network. The legacy users are listening for this broadcast + // and will generally do a dns request so they can ensureRouteToHost and if + // they do that before the callbacks happen they'll use the default network. + // + // TODO: Is there still a race here? The legacy broadcast will be sent after sending + // callbacks, but if apps can receive the broadcast before the callback, they still might + // have an inconsistent view of networking. + // + // This *does* introduce a race where if the user uses the new api + // (notification callbacks) and then uses the old api (getNetworkInfo(type)) + // they may get old info. Reverse this after the old startUsing api is removed. + // This is on top of the multiple intent sequencing referenced in the todo above. + for (NetworkAgentInfo nai : nais) { + addNetworkToLegacyTypeTracker(nai); + } + + // Tear down all unneeded networks. for (NetworkAgentInfo nai : mNetworkAgentInfos.values()) { if (unneeded(nai, UnneededFor.TEARDOWN)) { if (nai.getLingerExpiry() > 0) { From 9589e72241580281535a5b7c7469822db6643793 Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Tue, 19 Nov 2019 19:03:53 +0900 Subject: [PATCH 4/4] [NS A14] Move code notifying battery stats in its right place This should be done once every time an interface comes online. Doing this in updateLinkProperties guarantees this happens every time a new interface comes online, but it doesn't do it more often than needed. Test: FrameworksNetTests NetworkStackTests Change-Id: I0613c23f44192944266d76107308da8d1c541d1c --- .../android/server/ConnectivityService.java | 48 +++++++------------ .../server/ConnectivityServiceTest.java | 25 +++------- 2 files changed, 24 insertions(+), 49 deletions(-) diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index 2bbcf0dfa8..05ce2aef86 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -5651,7 +5651,8 @@ public class ConnectivityService extends IConnectivityManager.Stub // are accurate. networkAgent.clatd.fixupLinkProperties(oldLp, newLp); - updateInterfaces(newLp, oldLp, netId, networkAgent.networkCapabilities); + updateInterfaces(newLp, oldLp, netId, networkAgent.networkCapabilities, + networkAgent.networkInfo.getType()); // update filtering rules, need to happen after the interface update so netd knows about the // new interface (the interface name -> index map becomes initialized) @@ -5730,21 +5731,26 @@ public class ConnectivityService extends IConnectivityManager.Stub } - private void updateInterfaces(LinkProperties newLp, LinkProperties oldLp, int netId, - NetworkCapabilities caps) { - CompareResult interfaceDiff = new CompareResult<>( + private void updateInterfaces(final @Nullable LinkProperties newLp, + final @Nullable LinkProperties oldLp, final int netId, + final @Nullable NetworkCapabilities caps, final int legacyType) { + final CompareResult interfaceDiff = new CompareResult<>( oldLp != null ? oldLp.getAllInterfaceNames() : null, newLp != null ? newLp.getAllInterfaceNames() : null); - for (String iface : interfaceDiff.added) { - try { - if (DBG) log("Adding iface " + iface + " to network " + netId); - mNMS.addInterfaceToNetwork(iface, netId); - wakeupModifyInterface(iface, caps, true); - } catch (Exception e) { - loge("Exception adding interface: " + e); + if (!interfaceDiff.added.isEmpty()) { + final IBatteryStats bs = mDeps.getBatteryStatsService(); + for (final String iface : interfaceDiff.added) { + try { + if (DBG) log("Adding iface " + iface + " to network " + netId); + mNMS.addInterfaceToNetwork(iface, netId); + wakeupModifyInterface(iface, caps, true); + bs.noteNetworkInterfaceType(iface, legacyType); + } catch (Exception e) { + loge("Exception adding interface: " + e); + } } } - for (String iface : interfaceDiff.removed) { + for (final String iface : interfaceDiff.removed) { try { if (DBG) log("Removing iface " + iface + " from network " + netId); wakeupModifyInterface(iface, caps, false); @@ -6503,24 +6509,6 @@ public class ConnectivityService extends IConnectivityManager.Stub mLegacyTypeTracker.add(newNetwork.networkInfo.getType(), newNetwork); notifyLockdownVpn(newNetwork); } - - if (reassignedRequests.containsValue(newNetwork) || newNetwork.isVPN()) { - // Notify battery stats service about this network, both the normal - // interface and any stacked links. - // TODO: Avoid redoing this; this must only be done once when a network comes online. - try { - final IBatteryStats bs = mDeps.getBatteryStatsService(); - final int type = newNetwork.networkInfo.getType(); - - final String baseIface = newNetwork.linkProperties.getInterfaceName(); - bs.noteNetworkInterfaceType(baseIface, type); - for (LinkProperties stacked : newNetwork.linkProperties.getStackedLinks()) { - final String stackedIface = stacked.getInterfaceName(); - bs.noteNetworkInterfaceType(stackedIface, type); - } - } catch (RemoteException ignored) { - } - } } /** diff --git a/tests/net/java/com/android/server/ConnectivityServiceTest.java b/tests/net/java/com/android/server/ConnectivityServiceTest.java index da83397b49..9b0ccdd445 100644 --- a/tests/net/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/net/java/com/android/server/ConnectivityServiceTest.java @@ -5664,14 +5664,6 @@ public class ConnectivityServiceTest { TYPE_WIFI); reset(mBatteryStatsService); - // TODO : In the current code, ConnectivityService only tells BatteryStatsService about - // the type of networks that satisfy a request. That is a bug in a sense, but it has no - // consequences because a network that never satisfies any request gets torn down right - // away. Because of this, in the context of this test, the cell network agent does not - // satisfy any request as long as WiFi is connected, so the test below would fail if - // the WiFi network agent is not disconnected first. When this bug is fixed, remove the - // WiFi disconnect for more precise testing. - mWiFiNetworkAgent.disconnect(); mCellNetworkAgent.disconnect(); cellLp.setInterfaceName("wifi0"); @@ -5722,13 +5714,12 @@ public class ConnectivityServiceTest { mCm.registerNetworkCallback(networkRequest, networkCallback); // Prepare ipv6 only link properties. - mCellNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_CELLULAR); - final int cellNetId = mCellNetworkAgent.getNetwork().netId; final LinkProperties cellLp = new LinkProperties(); cellLp.setInterfaceName(MOBILE_IFNAME); cellLp.addLinkAddress(myIpv6); cellLp.addRoute(new RouteInfo((IpPrefix) null, myIpv6.getAddress(), MOBILE_IFNAME)); cellLp.addRoute(new RouteInfo(myIpv6, null, MOBILE_IFNAME)); + mCellNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_CELLULAR, cellLp); reset(mNetworkManagementService); reset(mMockDnsResolver); reset(mMockNetd); @@ -5737,8 +5728,8 @@ public class ConnectivityServiceTest { .thenReturn(getClatInterfaceConfig(myIpv4)); // Connect with ipv6 link properties. Expect prefix discovery to be started. - mCellNetworkAgent.sendLinkProperties(cellLp); mCellNetworkAgent.connect(true); + final int cellNetId = mCellNetworkAgent.getNetwork().netId; waitForIdle(); verify(mMockNetd, times(1)).networkCreatePhysical(eq(cellNetId), anyInt()); @@ -5811,14 +5802,10 @@ public class ConnectivityServiceTest { assertEquals(1, resolvrParams.servers.length); assertTrue(ArrayUtils.contains(resolvrParams.servers, "8.8.8.8")); - // TODO : this should be invoked but in the current code there is no path to invoke - // it. In practice, it will be invoked next time this network changes what requests it - // satisfies through rematchNetworkAndRequests, which may in fact be too late. This code - // should be reinstated when the bug is fixed. -// for (final LinkProperties stackedLp : stackedLpsAfterChange) { -// verify(mBatteryStatsService).noteNetworkInterfaceType(stackedLp.getInterfaceName(), -// TYPE_MOBILE); -// } + for (final LinkProperties stackedLp : stackedLpsAfterChange) { + verify(mBatteryStatsService).noteNetworkInterfaceType(stackedLp.getInterfaceName(), + TYPE_MOBILE); + } // Add ipv4 address, expect that clatd and prefix discovery are stopped and stacked // linkproperties are cleaned up.