Use CopyOnWriteArrayList to store list of NetworkStatsProviderCb

In NetworkStatsService, mStatsLock will be held when
iterating through the provider list. This is to protect the list
from concurrent broadcast that triggered by
NetworkPolicyManagerService. This is not good since the binder
call is oneway, it does not make sense to block every
access to the providers.

This change also remove unuse variable and reduce verbose log.

Test: atest FrameworksNetTests TetheringTests
Bug: 150418178
Change-Id: If74e9f2ea597a0d5ae4668c3358bc687f342bbb5
Merged-In: If74e9f2ea597a0d5ae4668c3358bc687f342bbb5
(cherry picked from commit 5d8f96c6967d5b1a42ba374521078e75a9e6f187)

services/core/java/com/android/server/net/NetworkStatsService.java

@@ -119,7 +119,6 @@ import android.os.Looper;
 import android.os.Message;
 import android.os.Messenger;
 import android.os.PowerManager;
-import android.os.RemoteCallbackList;
 import android.os.RemoteException;
 import android.os.SystemClock;
 import android.os.Trace;
@@ -160,6 +159,7 @@ import java.util.Arrays;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Objects;
+import java.util.concurrent.CopyOnWriteArrayList;
 import java.util.concurrent.Executor;
 import java.util.concurrent.Semaphore;
 import java.util.concurrent.TimeUnit;
@@ -226,12 +226,6 @@ public class NetworkStatsService extends INetworkStatsService.Stub {
     private static final String PREFIX_UID = "uid";
     private static final String PREFIX_UID_TAG = "uid_tag";
 
-    /**
-     * Virtual network interface for video telephony. This is for VT data usage counting purpose.
-     */
-    // TODO: Remove this after no one is using it.
-    public static final String VT_INTERFACE = NetworkStats.IFACE_VT;
-
     /**
      * Settings that can be changed externally.
      */
@@ -304,8 +298,8 @@ public class NetworkStatsService extends INetworkStatsService.Stub {
             new DropBoxNonMonotonicObserver();
 
     private static final int MAX_STATS_PROVIDER_POLL_WAIT_TIME_MS = 100;
-    private final RemoteCallbackList<NetworkStatsProviderCallbackImpl> mStatsProviderCbList =
+    private final CopyOnWriteArrayList<NetworkStatsProviderCallbackImpl> mStatsProviderCbList =
-            new RemoteCallbackList<>();
+            new CopyOnWriteArrayList<>();
     /** Semaphore used to wait for stats provider to respond to request stats update. */
     private final Semaphore mStatsProviderSem = new Semaphore(0, true);
 
@@ -1466,10 +1460,14 @@ public class NetworkStatsService extends INetworkStatsService.Stub {
         final boolean persistForce = (flags & FLAG_PERSIST_FORCE) != 0;
 
         // Request asynchronous stats update from all providers for next poll. And wait a bit of
-        // time to allow providers report-in given that normally binder call should be fast.
+        // time to allow providers report-in given that normally binder call should be fast. Note
+        // that size of list might be changed because addition/removing at the same time. For
+        // addition, the stats of the missed provider can only be collected in next poll;
+        // for removal, wait might take up to MAX_STATS_PROVIDER_POLL_WAIT_TIME_MS
+        // once that happened.
         // TODO: request with a valid token.
         Trace.traceBegin(TRACE_TAG_NETWORK, "provider.requestStatsUpdate");
-        final int registeredCallbackCount = mStatsProviderCbList.getRegisteredCallbackCount();
+        final int registeredCallbackCount = mStatsProviderCbList.size();
         mStatsProviderSem.drainPermits();
         invokeForAllStatsProviderCallbacks(
                 (cb) -> cb.mProvider.onRequestStatsUpdate(0 /* unused */));
@@ -1643,7 +1641,7 @@ public class NetworkStatsService extends INetworkStatsService.Stub {
 
         @Override
         public void setStatsProviderLimitAsync(@NonNull String iface, long quota) {
-            Slog.v(TAG, "setStatsProviderLimitAsync(" + iface + "," + quota + ")");
+            if (LOGV) Slog.v(TAG, "setStatsProviderLimitAsync(" + iface + "," + quota + ")");
             invokeForAllStatsProviderCallbacks((cb) -> cb.mProvider.onSetLimit(iface, quota));
         }
     }
@@ -1934,7 +1932,7 @@ public class NetworkStatsService extends INetworkStatsService.Stub {
             NetworkStatsProviderCallbackImpl callback = new NetworkStatsProviderCallbackImpl(
                     tag, provider, mStatsProviderSem, mAlertObserver,
                     mStatsProviderCbList);
-            mStatsProviderCbList.register(callback);
+            mStatsProviderCbList.add(callback);
             Log.d(TAG, "registerNetworkStatsProvider from " + callback.mTag + " uid/pid="
                     + getCallingUid() + "/" + getCallingPid());
             return callback;
@@ -1958,22 +1956,13 @@ public class NetworkStatsService extends INetworkStatsService.Stub {
 
     private void invokeForAllStatsProviderCallbacks(
             @NonNull ThrowingConsumer<NetworkStatsProviderCallbackImpl, RemoteException> task) {
-        synchronized (mStatsLock) {
+        for (final NetworkStatsProviderCallbackImpl cb : mStatsProviderCbList) {
-            final int length = mStatsProviderCbList.beginBroadcast();
-            try {
-                for (int i = 0; i < length; i++) {
-                    final NetworkStatsProviderCallbackImpl cb =
-                            mStatsProviderCbList.getBroadcastItem(i);
             try {
                 task.accept(cb);
             } catch (RemoteException e) {
                 Log.e(TAG, "Fail to broadcast to provider: " + cb.mTag, e);
             }
         }
-            } finally {
-                mStatsProviderCbList.finishBroadcast();
-            }
-        }
     }
 
     private static class NetworkStatsProviderCallbackImpl extends INetworkStatsProviderCallback.Stub
@@ -1983,7 +1972,7 @@ public class NetworkStatsService extends INetworkStatsService.Stub {
         @NonNull final INetworkStatsProvider mProvider;
         @NonNull private final Semaphore mSemaphore;
         @NonNull final INetworkManagementEventObserver mAlertObserver;
-        @NonNull final RemoteCallbackList<NetworkStatsProviderCallbackImpl> mStatsProviderCbList;
+        @NonNull final CopyOnWriteArrayList<NetworkStatsProviderCallbackImpl> mStatsProviderCbList;
 
         @NonNull private final Object mProviderStatsLock = new Object();
 
@@ -1997,7 +1986,7 @@ public class NetworkStatsService extends INetworkStatsService.Stub {
                 @NonNull String tag, @NonNull INetworkStatsProvider provider,
                 @NonNull Semaphore semaphore,
                 @NonNull INetworkManagementEventObserver alertObserver,
-                @NonNull RemoteCallbackList<NetworkStatsProviderCallbackImpl> cbList)
+                @NonNull CopyOnWriteArrayList<NetworkStatsProviderCallbackImpl> cbList)
                 throws RemoteException {
             mTag = tag;
             mProvider = provider;
@@ -2054,13 +2043,13 @@ public class NetworkStatsService extends INetworkStatsService.Stub {
         @Override
         public void binderDied() {
             Log.d(TAG, mTag + ": binderDied");
-            mStatsProviderCbList.unregister(this);
+            mStatsProviderCbList.remove(this);
         }
 
         @Override
         public void unregister() {
             Log.d(TAG, mTag + ": unregister");
-            mStatsProviderCbList.unregister(this);
+            mStatsProviderCbList.remove(this);
         }
 
     }