Merge "NetworkStats: Fix race condition causing system server crashes"

2 files changed, 10 insertions, 73 deletions

diff --git a/services/core/java/com/android/server/net/NetworkStatsService.java b/services/core/java/com/android/server/net/NetworkStatsService.java
index 2fc78d6f4760..5505828063ed 100644
--- a/services/core/java/com/android/server/net/NetworkStatsService.java
+++ b/services/core/java/com/android/server/net/NetworkStatsService.java
@@ -373,14 +373,9 @@ public class NetworkStatsService extends INetworkStatsService.Stub {
     }
 
     public void systemReady() {
-        mSystemReady = true;
-
-        if (!isBandwidthControlEnabled()) {
-            Slog.w(TAG, "bandwidth controls disabled, unable to track stats");
-            return;
-        }
-
         synchronized (mStatsLock) {
+            mSystemReady = true;
+
             // create data recorders along with historical rotators
             mDevRecorder = buildRecorder(PREFIX_DEV, mSettings.getDevConfig(), false);
             mXtRecorder = buildRecorder(PREFIX_XT, mSettings.getXtConfig(), false);
@@ -426,7 +421,14 @@ public class NetworkStatsService extends INetworkStatsService.Stub {
             // ignored; service lives in system_server
         }
 
-        registerPollAlarmLocked();
+        //  schedule periodic pall alarm based on {@link NetworkStatsSettings#getPollInterval()}.
+        final PendingIntent pollIntent =
+                PendingIntent.getBroadcast(mContext, 0, new Intent(ACTION_NETWORK_STATS_POLL), 0);
+
+        final long currentRealtime = SystemClock.elapsedRealtime();
+        mAlarmManager.setInexactRepeating(AlarmManager.ELAPSED_REALTIME, currentRealtime,
+                mSettings.getPollInterval(), pollIntent);
+
         registerGlobalAlert();
     }
 
@@ -487,23 +489,6 @@ public class NetworkStatsService extends INetworkStatsService.Stub {
     }
 
     /**
-     * Clear any existing {@link #ACTION_NETWORK_STATS_POLL} alarms, and
-     * reschedule based on current {@link NetworkStatsSettings#getPollInterval()}.
-     */
-    private void registerPollAlarmLocked() {
-        if (mPollIntent != null) {
-            mAlarmManager.cancel(mPollIntent);
-        }
-
-        mPollIntent = PendingIntent.getBroadcast(
-                mContext, 0, new Intent(ACTION_NETWORK_STATS_POLL), 0);
-
-        final long currentRealtime = SystemClock.elapsedRealtime();
-        mAlarmManager.setInexactRepeating(AlarmManager.ELAPSED_REALTIME, currentRealtime,
-                mSettings.getPollInterval(), mPollIntent);
-    }
-
-    /**
      * Register for a global alert that is delivered through
      * {@link INetworkManagementEventObserver} once a threshold amount of data
      * has been transferred.
@@ -548,8 +533,6 @@ public class NetworkStatsService extends INetworkStatsService.Stub {
     }
 
     private INetworkStatsSession openSessionInternal(final int flags, final String callingPackage) {
-        assertBandwidthControlEnabled();
-
         final int callingUid = Binder.getCallingUid();
         final int usedFlags = isRateLimitedForPoll(callingUid)
                 ? flags & (~NetworkStatsManager.FLAG_POLL_ON_OPEN)
@@ -742,7 +725,6 @@ public class NetworkStatsService extends INetworkStatsService.Stub {
 
     private long getNetworkTotalBytes(NetworkTemplate template, long start, long end) {
         assertSystemReady();
-        assertBandwidthControlEnabled();
 
         // NOTE: if callers want to get non-augmented data, they should go
         // through the public API
@@ -753,7 +735,6 @@ public class NetworkStatsService extends INetworkStatsService.Stub {
 
     private NetworkStats getNetworkUidBytes(NetworkTemplate template, long start, long end) {
         assertSystemReady();
-        assertBandwidthControlEnabled();
 
         final NetworkStatsCollection uidComplete;
         synchronized (mStatsLock) {
@@ -768,7 +749,6 @@ public class NetworkStatsService extends INetworkStatsService.Stub {
         if (Binder.getCallingUid() != uid) {
             mContext.enforceCallingOrSelfPermission(ACCESS_NETWORK_STATE, TAG);
         }
-        assertBandwidthControlEnabled();
 
         // TODO: switch to data layer stats once kernel exports
         // for now, read network layer stats and flatten across all ifaces
@@ -855,7 +835,6 @@ public class NetworkStatsService extends INetworkStatsService.Stub {
             NetworkState[] networkStates,
             String activeIface) {
         checkNetworkStackPermission(mContext);
-        assertBandwidthControlEnabled();
 
         final long token = Binder.clearCallingIdentity();
         try {
@@ -868,7 +847,6 @@ public class NetworkStatsService extends INetworkStatsService.Stub {
     @Override
     public void forceUpdate() {
         mContext.enforceCallingOrSelfPermission(READ_NETWORK_USAGE_HISTORY, TAG);
-        assertBandwidthControlEnabled();
 
         final long token = Binder.clearCallingIdentity();
         try {
@@ -879,8 +857,6 @@ public class NetworkStatsService extends INetworkStatsService.Stub {
     }
 
     private void advisePersistThreshold(long thresholdBytes) {
-        assertBandwidthControlEnabled();
-
         // clamp threshold into safe range
         mPersistThreshold = MathUtils.constrain(thresholdBytes, 128 * KB_IN_BYTES, 2 * MB_IN_BYTES);
         if (LOGV) {
@@ -1739,24 +1715,6 @@ public class NetworkStatsService extends INetworkStatsService.Stub {
         }
     }
 
-    private void assertBandwidthControlEnabled() {
-        if (!isBandwidthControlEnabled()) {
-            throw new IllegalStateException("Bandwidth module disabled");
-        }
-    }
-
-    private boolean isBandwidthControlEnabled() {
-        final long token = Binder.clearCallingIdentity();
-        try {
-            return mNetworkManager.isBandwidthControlEnabled();
-        } catch (RemoteException e) {
-            // ignored; service lives in system_server
-            return false;
-        } finally {
-            Binder.restoreCallingIdentity(token);
-        }
-    }
-
     private class DropBoxNonMonotonicObserver implements NonMonotonicObserver<String> {
         @Override
         public void foundNonMonotonic(NetworkStats left, int leftIndex, NetworkStats right,
diff --git a/tests/net/java/com/android/server/net/NetworkStatsServiceTest.java b/tests/net/java/com/android/server/net/NetworkStatsServiceTest.java
index 1682dd17c4a3..adaef40b51e3 100644
--- a/tests/net/java/com/android/server/net/NetworkStatsServiceTest.java
+++ b/tests/net/java/com/android/server/net/NetworkStatsServiceTest.java
@@ -239,7 +239,6 @@ public class NetworkStatsServiceTest extends NetworkStatsBaseTest {
         NetworkState[] states = new NetworkState[] {buildWifiState()};
         expectNetworkStatsSummary(buildEmptyStats());
         expectNetworkStatsUidDetail(buildEmptyStats());
-        expectBandwidthControlCheck();
 
         mService.forceUpdateIfaces(NETWORKS_WIFI, states, getActiveIface(states));
 
@@ -283,7 +282,6 @@ public class NetworkStatsServiceTest extends NetworkStatsBaseTest {
         NetworkState[] states = new NetworkState[] {buildWifiState()};
         expectNetworkStatsSummary(buildEmptyStats());
         expectNetworkStatsUidDetail(buildEmptyStats());
-        expectBandwidthControlCheck();
 
         mService.forceUpdateIfaces(NETWORKS_WIFI, states, getActiveIface(states));
 
@@ -357,7 +355,6 @@ public class NetworkStatsServiceTest extends NetworkStatsBaseTest {
         NetworkState[] states = new NetworkState[] {buildWifiState()};
         expectNetworkStatsSummary(buildEmptyStats());
         expectNetworkStatsUidDetail(buildEmptyStats());
-        expectBandwidthControlCheck();
 
         mService.forceUpdateIfaces(NETWORKS_WIFI, states, getActiveIface(states));
 
@@ -398,7 +395,6 @@ public class NetworkStatsServiceTest extends NetworkStatsBaseTest {
         NetworkState[] states = new NetworkState[] {buildMobile3gState(IMSI_1)};
         expectNetworkStatsSummary(buildEmptyStats());
         expectNetworkStatsUidDetail(buildEmptyStats());
-        expectBandwidthControlCheck();
 
         mService.forceUpdateIfaces(NETWORKS_MOBILE, states, getActiveIface(states));
 
@@ -433,7 +429,6 @@ public class NetworkStatsServiceTest extends NetworkStatsBaseTest {
                 .addValues(TEST_IFACE, UID_RED, SET_DEFAULT, TAG_NONE, 1536L, 12L, 512L, 4L, 0L)
                 .addValues(TEST_IFACE, UID_RED, SET_DEFAULT, 0xF00D, 512L, 4L, 512L, 4L, 0L)
                 .addValues(TEST_IFACE, UID_BLUE, SET_DEFAULT, TAG_NONE, 512L, 4L, 0L, 0L, 0L));
-        expectBandwidthControlCheck();
 
         mService.forceUpdateIfaces(NETWORKS_MOBILE, states, getActiveIface(states));
         forcePollAndWaitForIdle();
@@ -473,7 +468,6 @@ public class NetworkStatsServiceTest extends NetworkStatsBaseTest {
         NetworkState[] states = new NetworkState[] {buildWifiState()};
         expectNetworkStatsSummary(buildEmptyStats());
         expectNetworkStatsUidDetail(buildEmptyStats());
-        expectBandwidthControlCheck();
 
         mService.forceUpdateIfaces(NETWORKS_WIFI, states, getActiveIface(states));
 
@@ -531,7 +525,6 @@ public class NetworkStatsServiceTest extends NetworkStatsBaseTest {
         NetworkState[] states = new NetworkState[] {buildMobile3gState(IMSI_1)};
         expectNetworkStatsSummary(buildEmptyStats());
         expectNetworkStatsUidDetail(buildEmptyStats());
-        expectBandwidthControlCheck();
 
         mService.forceUpdateIfaces(NETWORKS_MOBILE, states, getActiveIface(states));
 
@@ -558,7 +551,6 @@ public class NetworkStatsServiceTest extends NetworkStatsBaseTest {
         expectNetworkStatsUidDetail(new NetworkStats(getElapsedRealtime(), 1)
                 .addValues(TEST_IFACE, UID_RED, SET_DEFAULT, TAG_NONE, 1024L, 8L, 1024L, 8L, 0L)
                 .addValues(TEST_IFACE, UID_RED, SET_DEFAULT, 0xF00D, 512L, 4L, 512L, 4L, 0L));
-        expectBandwidthControlCheck();
 
         mService.forceUpdateIfaces(NETWORKS_MOBILE, states, getActiveIface(states));
         forcePollAndWaitForIdle();
@@ -588,7 +580,6 @@ public class NetworkStatsServiceTest extends NetworkStatsBaseTest {
         NetworkState[] states = new NetworkState[] {buildWifiState()};
         expectNetworkStatsSummary(buildEmptyStats());
         expectNetworkStatsUidDetail(buildEmptyStats());
-        expectBandwidthControlCheck();
 
         mService.forceUpdateIfaces(NETWORKS_WIFI, states, getActiveIface(states));
 
@@ -646,7 +637,6 @@ public class NetworkStatsServiceTest extends NetworkStatsBaseTest {
         NetworkState[] states = new NetworkState[] {buildWifiState()};
         expectNetworkStatsSummary(buildEmptyStats());
         expectNetworkStatsUidDetail(buildEmptyStats());
-        expectBandwidthControlCheck();
 
         mService.forceUpdateIfaces(NETWORKS_WIFI, states, getActiveIface(states));
 
@@ -690,7 +680,6 @@ public class NetworkStatsServiceTest extends NetworkStatsBaseTest {
 
         expectNetworkStatsSummary(buildEmptyStats());
         expectNetworkStatsUidDetail(buildEmptyStats());
-        expectBandwidthControlCheck();
 
         mService.forceUpdateIfaces(NETWORKS_WIFI, states, getActiveIface(states));
 
@@ -740,7 +729,6 @@ public class NetworkStatsServiceTest extends NetworkStatsBaseTest {
         NetworkState[] states = new NetworkState[] {buildWifiState()};
         expectNetworkStatsSummary(buildEmptyStats());
         expectNetworkStatsUidDetail(buildEmptyStats());
-        expectBandwidthControlCheck();
 
         mService.forceUpdateIfaces(NETWORKS_WIFI, states, getActiveIface(states));
 
@@ -797,7 +785,6 @@ public class NetworkStatsServiceTest extends NetworkStatsBaseTest {
         NetworkState[] states = new NetworkState[] {buildWifiState(true /* isMetered */)};
         expectNetworkStatsSummary(buildEmptyStats());
         expectNetworkStatsUidDetail(buildEmptyStats());
-        expectBandwidthControlCheck();
 
         mService.forceUpdateIfaces(NETWORKS_WIFI, states, getActiveIface(states));
 
@@ -837,7 +824,6 @@ public class NetworkStatsServiceTest extends NetworkStatsBaseTest {
             new NetworkState[] {buildMobile3gState(IMSI_1, true /* isRoaming */)};
         expectNetworkStatsSummary(buildEmptyStats());
         expectNetworkStatsUidDetail(buildEmptyStats());
-        expectBandwidthControlCheck();
 
         mService.forceUpdateIfaces(NETWORKS_MOBILE, states, getActiveIface(states));
 
@@ -875,7 +861,6 @@ public class NetworkStatsServiceTest extends NetworkStatsBaseTest {
         NetworkState[] states = new NetworkState[] {buildMobile3gState(IMSI_1)};
         expectNetworkStatsSummary(buildEmptyStats());
         expectNetworkStatsUidDetail(buildEmptyStats());
-        expectBandwidthControlCheck();
 
         mService.forceUpdateIfaces(NETWORKS_MOBILE, states, getActiveIface(states));
 
@@ -916,7 +901,6 @@ public class NetworkStatsServiceTest extends NetworkStatsBaseTest {
         NetworkState[] states = new NetworkState[] {buildWifiState()};
         expectNetworkStatsSummary(buildEmptyStats());
         expectNetworkStatsUidDetail(buildEmptyStats());
-        expectBandwidthControlCheck();
 
         mService.forceUpdateIfaces(NETWORKS_WIFI, states, getActiveIface(states));
 
@@ -1057,7 +1041,6 @@ public class NetworkStatsServiceTest extends NetworkStatsBaseTest {
 
     private void expectSystemReady() throws Exception {
         expectNetworkStatsSummary(buildEmptyStats());
-        expectBandwidthControlCheck();
     }
 
     private String getActiveIface(NetworkState... states) throws Exception {
@@ -1125,10 +1108,6 @@ public class NetworkStatsServiceTest extends NetworkStatsBaseTest {
         when(mSettings.getUidTagPersistBytes(anyLong())).thenReturn(MB_IN_BYTES);
     }
 
-    private void expectBandwidthControlCheck() throws Exception {
-        when(mNetManager.isBandwidthControlEnabled()).thenReturn(true);
-    }
-
     private void assertStatsFilesExist(boolean exist) {
         final File basePath = new File(mStatsDir, "netstats");
         if (exist) {