Deduplicate items after clear interface in NetworkStats
Follow-up from commit Ie60829a65d0d9b5b63ad353695a820c0586e3665,
the interface field was cleared before returning the result to
the caller. However, this can cause problems in the
NetworkStats#subtract method. If the interface field is cleared,
the findIndexHinted method can match to a wrong entry.
This is because the keys of multiple entries will now be the
same. This can cause the subtract result to be unexpectedly
large and the return value of getUidStatsForTransport to be
mismatched with the values retrieved from other APIs.
Test: atest FrameworksNetTests:android.net.connectivity.com.android.server.net.NetworkStatsServiceTest \
FrameworksNetTests:android.net.connectivity.android.net.NetworkStatsTest
Bug: 290728278
Change-Id: I891ab29b8a2902663febc7c32b04417caf510926
diff --git a/framework-t/src/android/net/NetworkStats.java b/framework-t/src/android/net/NetworkStats.java
index 0b5bb85..4f816c5 100644
--- a/framework-t/src/android/net/NetworkStats.java
+++ b/framework-t/src/android/net/NetworkStats.java
@@ -1147,7 +1147,8 @@
entry.txPackets = left.txPackets[i];
entry.operations = left.operations[i];
- // find remote row that matches, and subtract
+ // Find the remote row that matches and subtract.
+ // The returned row must be uniquely matched.
final int j = right.findIndexHinted(entry.iface, entry.uid, entry.set, entry.tag,
entry.metered, entry.roaming, entry.defaultNetwork, i);
if (j != -1) {
@@ -1304,13 +1305,14 @@
/**
* Removes the interface name from all entries.
- * This mutates the original structure in place.
+ * This returns a newly constructed object instead of mutating the original structure.
* @hide
*/
- public void clearInterfaces() {
- for (int i = 0; i < size; i++) {
- iface[i] = null;
- }
+ @NonNull
+ public NetworkStats clearInterfaces() {
+ final Entry temp = new Entry();
+ return map(entry -> temp.setKeys(IFACE_ALL, entry.getUid(), entry.getSet(),
+ entry.getTag(), entry.getMetered(), entry.getRoaming(), entry.getDefaultNetwork()));
}
/**
diff --git a/service-t/src/com/android/server/net/NetworkStatsService.java b/service-t/src/com/android/server/net/NetworkStatsService.java
index e7ef510..6635fd3 100644
--- a/service-t/src/com/android/server/net/NetworkStatsService.java
+++ b/service-t/src/com/android/server/net/NetworkStatsService.java
@@ -1744,8 +1744,7 @@
// information. This is because no caller needs this information for now, and it
// makes it easier to change the implementation later by using the histories in the
// recorder.
- stats.clearInterfaces();
- return stats;
+ return stats.clearInterfaces();
} catch (RemoteException e) {
Log.wtf(TAG, "Error compiling UID stats", e);
return new NetworkStats(0L, 0);
diff --git a/tests/unit/java/android/net/NetworkStatsTest.java b/tests/unit/java/android/net/NetworkStatsTest.java
index 126ad55..4ff131b 100644
--- a/tests/unit/java/android/net/NetworkStatsTest.java
+++ b/tests/unit/java/android/net/NetworkStatsTest.java
@@ -1073,30 +1073,35 @@
final NetworkStats.Entry entry1 = new NetworkStats.Entry(
"test1", 10100, SET_DEFAULT, TAG_NONE, METERED_NO, ROAMING_NO,
DEFAULT_NETWORK_NO, 1024L, 50L, 100L, 20L, 0L);
-
final NetworkStats.Entry entry2 = new NetworkStats.Entry(
"test2", 10101, SET_DEFAULT, 0xF0DD, METERED_NO, ROAMING_NO,
DEFAULT_NETWORK_NO, 51200, 25L, 101010L, 50L, 0L);
+ final NetworkStats.Entry entry3 = new NetworkStats.Entry(
+ "test3", 10101, SET_DEFAULT, 0xF0DD, METERED_NO, ROAMING_NO,
+ DEFAULT_NETWORK_NO, 1, 2L, 3L, 4L, 5L);
stats.insertEntry(entry1);
stats.insertEntry(entry2);
+ stats.insertEntry(entry3);
// Verify that the interfaces have indeed been recorded.
- assertEquals(2, stats.size());
+ assertEquals(3, stats.size());
assertValues(stats, 0, "test1", 10100, SET_DEFAULT, TAG_NONE, METERED_NO,
ROAMING_NO, DEFAULT_NETWORK_NO, 1024L, 50L, 100L, 20L, 0L);
assertValues(stats, 1, "test2", 10101, SET_DEFAULT, 0xF0DD, METERED_NO,
ROAMING_NO, DEFAULT_NETWORK_NO, 51200, 25L, 101010L, 50L, 0L);
+ assertValues(stats, 2, "test3", 10101, SET_DEFAULT, 0xF0DD, METERED_NO,
+ ROAMING_NO, DEFAULT_NETWORK_NO, 1, 2L, 3L, 4L, 5L);
// Clear interfaces.
- stats.clearInterfaces();
+ final NetworkStats ifaceClearedStats = stats.clearInterfaces();
- // Verify that the interfaces are cleared.
- assertEquals(2, stats.size());
- assertValues(stats, 0, null /* iface */, 10100, SET_DEFAULT, TAG_NONE, METERED_NO,
- ROAMING_NO, DEFAULT_NETWORK_NO, 1024L, 50L, 100L, 20L, 0L);
- assertValues(stats, 1, null /* iface */, 10101, SET_DEFAULT, 0xF0DD, METERED_NO,
- ROAMING_NO, DEFAULT_NETWORK_NO, 51200, 25L, 101010L, 50L, 0L);
+ // Verify that the interfaces are cleared, and key-duplicated items are merged.
+ assertEquals(2, ifaceClearedStats.size());
+ assertValues(ifaceClearedStats, 0, null /* iface */, 10100, SET_DEFAULT, TAG_NONE,
+ METERED_NO, ROAMING_NO, DEFAULT_NETWORK_NO, 1024L, 50L, 100L, 20L, 0L);
+ assertValues(ifaceClearedStats, 1, null /* iface */, 10101, SET_DEFAULT, 0xF0DD,
+ METERED_NO, ROAMING_NO, DEFAULT_NETWORK_NO, 51201, 27L, 101013L, 54L, 5L);
}
private static void assertContains(NetworkStats stats, String iface, int uid, int set,
diff --git a/tests/unit/java/com/android/server/net/NetworkStatsBaseTest.java b/tests/unit/java/com/android/server/net/NetworkStatsBaseTest.java
index a058a46..2c9f212 100644
--- a/tests/unit/java/com/android/server/net/NetworkStatsBaseTest.java
+++ b/tests/unit/java/com/android/server/net/NetworkStatsBaseTest.java
@@ -41,6 +41,7 @@
abstract class NetworkStatsBaseTest {
static final String TEST_IFACE = "test0";
static final String TEST_IFACE2 = "test1";
+ static final String TEST_IFACE3 = "test2";
static final String TUN_IFACE = "test_nss_tun0";
static final String TUN_IFACE2 = "test_nss_tun1";
diff --git a/tests/unit/java/com/android/server/net/NetworkStatsServiceTest.java b/tests/unit/java/com/android/server/net/NetworkStatsServiceTest.java
index b8b0289..6292d45 100644
--- a/tests/unit/java/com/android/server/net/NetworkStatsServiceTest.java
+++ b/tests/unit/java/com/android/server/net/NetworkStatsServiceTest.java
@@ -1250,8 +1250,9 @@
TEST_IFACE2, IMSI_1, null /* wifiNetworkKey */,
false /* isTemporarilyNotMetered */, false /* isRoaming */);
- final NetworkStateSnapshot[] states = new NetworkStateSnapshot[] {
- mobileState, buildWifiState()};
+ final NetworkStateSnapshot[] states = new NetworkStateSnapshot[]{
+ mobileState, buildWifiState(false, TEST_IFACE, null),
+ buildWifiState(false, TEST_IFACE3, null)};
mService.notifyNetworkStatus(NETWORKS_MOBILE, states, getActiveIface(states),
new UnderlyingNetworkInfo[0]);
setMobileRatTypeAndWaitForIdle(TelephonyManager.NETWORK_TYPE_LTE);
@@ -1266,16 +1267,22 @@
final NetworkStats.Entry entry3 = new NetworkStats.Entry(
TEST_IFACE, UID_BLUE, SET_DEFAULT, 0xBEEF, METERED_NO, ROAMING_NO,
DEFAULT_NETWORK_NO, 1024L, 8L, 512L, 4L, 2L);
+ // Add an entry that with different wifi interface, but expected to be merged into entry3
+ // after clearing interface information.
+ final NetworkStats.Entry entry4 = new NetworkStats.Entry(
+ TEST_IFACE3, UID_BLUE, SET_DEFAULT, 0xBEEF, METERED_NO, ROAMING_NO,
+ DEFAULT_NETWORK_NO, 1L, 2L, 3L, 4L, 5L);
final TetherStatsParcel[] emptyTetherStats = {};
// The interfaces that expect to be used to query the stats.
- final String[] wifiIfaces = {TEST_IFACE};
+ final String[] wifiIfaces = {TEST_IFACE, TEST_IFACE3};
incrementCurrentTime(HOUR_IN_MILLIS);
mockDefaultSettings();
- mockNetworkStatsUidDetail(new NetworkStats(getElapsedRealtime(), 3)
+ mockNetworkStatsUidDetail(new NetworkStats(getElapsedRealtime(), 4)
.insertEntry(entry1)
.insertEntry(entry2)
- .insertEntry(entry3), emptyTetherStats, wifiIfaces);
+ .insertEntry(entry3)
+ .insertEntry(entry4), emptyTetherStats, wifiIfaces);
// getUidStatsForTransport (through getNetworkStatsUidDetail) adds all operation counts
// with active interface, and the interface here is mobile interface, so this test makes
@@ -1293,7 +1300,7 @@
assertValues(wifiStats, null /* iface */, UID_RED, SET_DEFAULT, 0xF00D,
METERED_NO, ROAMING_NO, METERED_NO, 50L, 5L, 50L, 5L, 1L);
assertValues(wifiStats, null /* iface */, UID_BLUE, SET_DEFAULT, 0xBEEF,
- METERED_NO, ROAMING_NO, METERED_NO, 1024L, 8L, 512L, 4L, 2L);
+ METERED_NO, ROAMING_NO, METERED_NO, 1025L, 10L, 515L, 8L, 7L);
final String[] mobileIfaces = {TEST_IFACE2};
mockNetworkStatsUidDetail(buildEmptyStats(), emptyTetherStats, mobileIfaces);