[SP16] Address comments on aosp/1172143

Test: atest FrameworksNetTests
Test: atest NetworkPolicyManagerServiceTest
Test: m doc-comment-check-docs
Bug: 130855321
Change-Id: Iccaab09f5b9668ec4a7249737c64a69cecb08d15

core/java/android/app/usage/NetworkStatsManager.java

@@ -526,15 +526,17 @@ public class NetworkStatsManager {
     }
 
     /**
-     * Registers a custom provider of {@link android.net.NetworkStats} to combine the network
+     * Registers a custom provider of {@link android.net.NetworkStats} to provide network statistics
-     * statistics that cannot be seen by the kernel to system. To unregister, invoke
+     * to the system. To unregister, invoke {@link NetworkStatsProviderCallback#unregister()}.
-     * {@link NetworkStatsProviderCallback#unregister()}.
+     * Note that no de-duplication of statistics between providers is performed, so each provider
+     * must only report network traffic that is not being reported by any other provider.
      *
-     * @param tag a human readable identifier of the custom network stats provider.
+     * @param tag a human readable identifier of the custom network stats provider. This is only
-     * @param provider a custom implementation of {@link AbstractNetworkStatsProvider} that needs to
+     *            used for debugging.
-     *                 be registered to the system.
+     * @param provider the subclass of {@link AbstractNetworkStatsProvider} that needs to be
+     *                 registered to the system.
      * @return a {@link NetworkStatsProviderCallback}, which can be used to report events to the
-     *         system.
+     *         system or unregister the provider.
      * @hide
      */
     @SystemApi

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

@@ -255,7 +255,6 @@ public class NetworkStatsService extends INetworkStatsService.Stub {
     }
 
     private final Object mStatsLock = new Object();
-    private final Object mStatsProviderLock = new Object();
 
     /** Set of currently active ifaces. */
     @GuardedBy("mStatsLock")
@@ -1501,8 +1500,8 @@ public class NetworkStatsService extends INetworkStatsService.Stub {
         }
 
         @Override
-        public void setStatsProviderLimit(@NonNull String iface, long quota) {
+        public void setStatsProviderLimitAsync(@NonNull String iface, long quota) {
-            Slog.v(TAG, "setStatsProviderLimit(" + iface + "," + quota + ")");
+            Slog.v(TAG, "setStatsProviderLimitAsync(" + iface + "," + quota + ")");
             invokeForAllStatsProviderCallbacks((cb) -> cb.mProvider.setLimit(iface, quota));
         }
     }
@@ -1783,9 +1782,9 @@ public class NetworkStatsService extends INetworkStatsService.Stub {
      * {@code unregister()} of the returned callback.
      *
      * @param tag a human readable identifier of the custom network stats provider.
-     * @param provider the binder interface of
+     * @param provider the {@link INetworkStatsProvider} binder corresponding to the
-     *                 {@link android.net.netstats.provider.AbstractNetworkStatsProvider} that
+     *                 {@link android.net.netstats.provider.AbstractNetworkStatsProvider} to be
-     *                 needs to be registered to the system.
+     *                 registered.
      *
      * @return a binder interface of
      *         {@link android.net.netstats.provider.NetworkStatsProviderCallback}, which can be
@@ -1823,7 +1822,7 @@ public class NetworkStatsService extends INetworkStatsService.Stub {
 
     private void invokeForAllStatsProviderCallbacks(
             @NonNull ThrowingConsumer<NetworkStatsProviderCallbackImpl, RemoteException> task) {
-        synchronized (mStatsProviderCbList) {
+        synchronized (mStatsLock) {
             final int length = mStatsProviderCbList.beginBroadcast();
             try {
                 for (int i = 0; i < length; i++) {
@@ -1844,13 +1843,15 @@ public class NetworkStatsService extends INetworkStatsService.Stub {
     private static class NetworkStatsProviderCallbackImpl extends INetworkStatsProviderCallback.Stub
             implements IBinder.DeathRecipient {
         @NonNull final String mTag;
-        @NonNull private final Object mProviderStatsLock = new Object();
+
         @NonNull final INetworkStatsProvider mProvider;
         @NonNull final INetworkManagementEventObserver mAlertObserver;
         @NonNull final RemoteCallbackList<NetworkStatsProviderCallbackImpl> mStatsProviderCbList;
 
+        @NonNull private final Object mProviderStatsLock = new Object();
+
         @GuardedBy("mProviderStatsLock")
-        // STATS_PER_IFACE and STATS_PER_UID
+        // Track STATS_PER_IFACE and STATS_PER_UID separately.
         private final NetworkStats mIfaceStats = new NetworkStats(0L, 0);
         @GuardedBy("mProviderStatsLock")
         private final NetworkStats mUidStats = new NetworkStats(0L, 0);
@@ -1881,7 +1882,8 @@ public class NetworkStatsService extends INetworkStatsService.Stub {
                     default:
                         throw new IllegalArgumentException("Invalid type: " + how);
                 }
-                // Return a defensive copy instead of local reference.
+                // Callers might be able to mutate the returned object. Return a defensive copy
+                // instead of local reference.
                 return stats.clone();
             }
         }