Better errors from unregisterNetworkCallback

This patch changes the validation of unregisterNetworkCallback in
ConnectivityManager so that the caller can better distinguish the case
of a callback that was never registered from the case of a callback that
has already been unregistered.

Bug: 62497809
Test: runtest frameworks-net passes
Change-Id: I58eda22725dd4e67dc4b64207e38cf482032df10

1 files changed, 16 insertions, 8 deletions

diff --git a/core/java/android/net/ConnectivityManager.java b/core/java/android/net/ConnectivityManager.java
index 9e8acd0c47ad..943c6813004c 100644
--- a/core/java/android/net/ConnectivityManager.java
+++ b/core/java/android/net/ConnectivityManager.java
@@ -599,6 +599,15 @@ public class ConnectivityManager {
     public final static int REQUEST_ID_UNSET = 0;
 
     /**
+     * Static unique request used as a tombstone for NetworkCallbacks that have been unregistered.
+     * This allows to distinguish when unregistering NetworkCallbacks those that were never
+     * registered and those that were already unregistered.
+     * @hide
+     */
+    private final static NetworkRequest ALREADY_UNREGISTERED =
+            new NetworkRequest.Builder().clearCapabilities().build();
+
+    /**
      * A NetID indicating no Network is selected.
      * Keep in sync with bionic/libc/dns/include/resolv_netid.h
      * @hide
@@ -2691,10 +2700,6 @@ public class ConnectivityManager {
         public void onNetworkResumed(Network network) {}
 
         private NetworkRequest networkRequest;
-
-        private boolean isRegistered() {
-            return (networkRequest != null) && (networkRequest.requestId != REQUEST_ID_UNSET);
-        }
     }
 
     /**
@@ -2861,7 +2866,8 @@ public class ConnectivityManager {
         final NetworkRequest request;
         try {
             synchronized(sCallbacks) {
-                if (callback.isRegistered()) {
+                if (callback.networkRequest != null
+                        && callback.networkRequest != ALREADY_UNREGISTERED) {
                     // TODO: throw exception instead and enforce 1:1 mapping of callbacks
                     // and requests (http://b/20701525).
                     Log.e(TAG, "NetworkCallback was already registered");
@@ -3312,8 +3318,10 @@ public class ConnectivityManager {
         // Find all requests associated to this callback and stop callback triggers immediately.
         // Callback is reusable immediately. http://b/20701525, http://b/35921499.
         synchronized (sCallbacks) {
-            Preconditions.checkArgument(
-                    networkCallback.isRegistered(), "NetworkCallback was not registered");
+            Preconditions.checkArgument(networkCallback.networkRequest != null,
+                    "NetworkCallback was not registered");
+            Preconditions.checkArgument(networkCallback.networkRequest != ALREADY_UNREGISTERED,
+                    "NetworkCallback was already unregistered");
             for (Map.Entry<NetworkRequest, NetworkCallback> e : sCallbacks.entrySet()) {
                 if (e.getValue() == networkCallback) {
                     reqs.add(e.getKey());
@@ -3329,7 +3337,7 @@ public class ConnectivityManager {
                 // Only remove mapping if rpc was successful.
                 sCallbacks.remove(r);
             }
-            networkCallback.networkRequest = null;
+            networkCallback.networkRequest = ALREADY_UNREGISTERED;
         }
     }