Merge "Fix : crash ConnectivityService when PendingIntent is canceled" into main
diff --git a/service/src/com/android/server/ConnectivityService.java b/service/src/com/android/server/ConnectivityService.java
index adf593e..2a66e4b 100644
--- a/service/src/com/android/server/ConnectivityService.java
+++ b/service/src/com/android/server/ConnectivityService.java
@@ -423,14 +423,14 @@
 import java.util.concurrent.CopyOnWriteArraySet;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicInteger;
+import java.util.concurrent.atomic.AtomicReference;
 import java.util.function.BiConsumer;
 import java.util.function.Consumer;
 
 /**
  * @hide
  */
-public class ConnectivityService extends IConnectivityManager.Stub
-        implements PendingIntent.OnFinished {
+public class ConnectivityService extends IConnectivityManager.Stub {
     private static final String TAG = ConnectivityService.class.getSimpleName();
 
     private static final String DIAG_ARG = "--diag";
@@ -10936,10 +10936,42 @@
         // else not handled
     }
 
+    /**
+     * A small class to manage releasing a lock exactly once even if releaseLock is called
+     * multiple times. See b/390043283
+     * PendingIntent#send throws CanceledException in various cases. In some of them it will
+     * still call onSendFinished, in others it won't and the client can't know. This class
+     * keeps a ref to the wakelock that it releases exactly once, thanks to Atomics semantics.
+     */
+    private class WakeLockOnFinishedReceiver implements PendingIntent.OnFinished {
+        private final AtomicReference<PowerManager.WakeLock> mLock;
+        WakeLockOnFinishedReceiver(@NonNull final PowerManager.WakeLock lock) {
+            mLock = new AtomicReference<>(lock);
+            lock.acquire();
+        }
+
+        public void releaseLock() {
+            final PowerManager.WakeLock lock = mLock.getAndSet(null);
+            if (null != lock) lock.release();
+        }
+
+        @Override
+        public void onSendFinished(PendingIntent pendingIntent, Intent intent, int resultCode,
+                String resultData, Bundle resultExtras) {
+            if (DBG) log("Finished sending " + pendingIntent);
+            releaseLock();
+            releasePendingNetworkRequestWithDelay(pendingIntent);
+        }
+    }
+
     // TODO(b/193460475): Remove when tooling supports SystemApi to public API.
     @SuppressLint("NewApi")
     private void sendIntent(PendingIntent pendingIntent, Intent intent) {
-        mPendingIntentWakeLock.acquire();
+        // Since the receiver will take the lock exactly once and release it exactly once, it
+        // is safe to pass the same wakelock to all receivers and avoid creating a new lock
+        // every time.
+        final WakeLockOnFinishedReceiver receiver =
+                new WakeLockOnFinishedReceiver(mPendingIntentWakeLock);
         try {
             if (DBG) log("Sending " + pendingIntent);
             final BroadcastOptions options = BroadcastOptions.makeBasic();
@@ -10948,25 +10980,14 @@
                 // utilizing the PendingIntent as a backdoor to do this.
                 options.setPendingIntentBackgroundActivityLaunchAllowed(false);
             }
-            pendingIntent.send(mContext, 0, intent, this /* onFinished */, null /* Handler */,
+            pendingIntent.send(mContext, 0, intent, receiver, null /* Handler */,
                     null /* requiredPermission */,
                     mDeps.isAtLeastT() ? options.toBundle() : null);
         } catch (PendingIntent.CanceledException e) {
             if (DBG) log(pendingIntent + " was not sent, it had been canceled.");
-            mPendingIntentWakeLock.release();
+            receiver.releaseLock();
             releasePendingNetworkRequest(pendingIntent);
         }
-        // ...otherwise, mPendingIntentWakeLock.release() gets called by onSendFinished()
-    }
-
-    @Override
-    public void onSendFinished(PendingIntent pendingIntent, Intent intent, int resultCode,
-            String resultData, Bundle resultExtras) {
-        if (DBG) log("Finished sending " + pendingIntent);
-        mPendingIntentWakeLock.release();
-        // Release with a delay so the receiving client has an opportunity to put in its
-        // own request.
-        releasePendingNetworkRequestWithDelay(pendingIntent);
     }
 
     @Nullable
diff --git a/tests/cts/net/src/android/net/cts/ConnectivityManagerTest.java b/tests/cts/net/src/android/net/cts/ConnectivityManagerTest.java
index aa7d618..3828dc8 100644
--- a/tests/cts/net/src/android/net/cts/ConnectivityManagerTest.java
+++ b/tests/cts/net/src/android/net/cts/ConnectivityManagerTest.java
@@ -90,7 +90,6 @@
 import static android.net.NetworkCapabilities.TRANSPORT_VPN;
 import static android.net.NetworkCapabilities.TRANSPORT_WIFI;
 import static android.net.NetworkStack.PERMISSION_MAINLINE_NETWORK_STACK;
-import static android.net.cts.util.CtsNetUtils.ConnectivityActionReceiver;
 import static android.net.cts.util.CtsNetUtils.HTTP_PORT;
 import static android.net.cts.util.CtsNetUtils.NETWORK_CALLBACK_ACTION;
 import static android.net.cts.util.CtsNetUtils.TEST_HOST;
@@ -111,6 +110,7 @@
 import static com.android.networkstack.apishim.ConstantsShim.BLOCKED_REASON_LOCKDOWN_VPN;
 import static com.android.networkstack.apishim.ConstantsShim.BLOCKED_REASON_NONE;
 import static com.android.networkstack.apishim.ConstantsShim.RECEIVER_EXPORTED;
+import static com.android.networkstack.apishim.ConstantsShim.RECEIVER_NOT_EXPORTED;
 import static com.android.testutils.Cleanup.testAndCleanup;
 import static com.android.testutils.DevSdkIgnoreRuleKt.SC_V2;
 import static com.android.testutils.MiscAsserts.assertEventuallyTrue;
@@ -178,6 +178,7 @@
 import android.os.Binder;
 import android.os.Build;
 import android.os.Bundle;
+import android.os.ConditionVariable;
 import android.os.Handler;
 import android.os.Looper;
 import android.os.MessageQueue;
@@ -1229,42 +1230,42 @@
      * {@link #testRegisterNetworkCallback} except that a {@code PendingIntent} is used instead
      * of a {@code NetworkCallback}.
      */
-    @AppModeFull(reason = "Cannot get WifiManager in instant app mode")
     @Test
     public void testRegisterNetworkCallback_withPendingIntent() {
-        assumeTrue(mPackageManager.hasSystemFeature(FEATURE_WIFI));
+        final ConditionVariable received = new ConditionVariable();
 
-        // Create a ConnectivityActionReceiver that has an IntentFilter for our locally defined
-        // action, NETWORK_CALLBACK_ACTION.
-        final IntentFilter filter = new IntentFilter();
-        filter.addAction(NETWORK_CALLBACK_ACTION);
+        // Register a callback with intent and a request for any Internet-providing network,
+        // which should match the currently connected network.
+        final BroadcastReceiver receiver = new BroadcastReceiver() {
+            @Override
+            public void onReceive(final Context context, final Intent intent) {
+                received.open();
+            }
+        };
 
-        final ConnectivityActionReceiver receiver = new ConnectivityActionReceiver(
-                mCm, ConnectivityManager.TYPE_WIFI, NetworkInfo.State.CONNECTED);
-        final int flags = SdkLevel.isAtLeastT() ? RECEIVER_EXPORTED : 0;
-        mContext.registerReceiver(receiver, filter, flags);
+        final int flags = SdkLevel.isAtLeastT() ? RECEIVER_NOT_EXPORTED : 0;
+        mContext.registerReceiver(receiver, new IntentFilter(NETWORK_CALLBACK_ACTION), flags);
 
         // Create a broadcast PendingIntent for NETWORK_CALLBACK_ACTION.
         final Intent intent = new Intent(NETWORK_CALLBACK_ACTION)
                 .setPackage(mContext.getPackageName());
-        // While ConnectivityService would put extra info such as network or request id before
-        // broadcasting the inner intent. The MUTABLE flag needs to be added accordingly.
         final PendingIntent pendingIntent = PendingIntent.getBroadcast(mContext, 0 /*requestCode*/,
                 intent, PendingIntent.FLAG_CANCEL_CURRENT | PendingIntent.FLAG_MUTABLE);
 
-        // We will register for a WIFI network being available or lost.
-        mCm.registerNetworkCallback(makeWifiNetworkRequest(), pendingIntent);
+        // Register for a network providing Internet being available or lost.
+        final NetworkRequest nr = new NetworkRequest.Builder()
+                .addCapability(NET_CAPABILITY_INTERNET)
+                .build();
+        mCm.registerNetworkCallback(nr, pendingIntent);
 
         try {
             mCtsNetUtils.ensureWifiConnected();
 
-            // Now we expect to get the Intent delivered notifying of the availability of the wifi
+            // Wait for delivery of the Intent notifying of the availability of the INTERNET
             // network even if it was already connected as a state-based action when the callback
             // is registered.
-            assertTrue("Did not receive expected Intent " + intent + " for TRANSPORT_WIFI",
-                    receiver.waitForState());
-        } catch (InterruptedException e) {
-            fail("Broadcast receiver or NetworkCallback wait was interrupted.");
+            assertTrue("Did not receive expected Intent " + intent + " for INTERNET",
+                    received.block(NETWORK_CALLBACK_TIMEOUT_MS));
         } finally {
             mCm.unregisterNetworkCallback(pendingIntent);
             pendingIntent.cancel();
@@ -1272,6 +1273,31 @@
         }
     }
 
+    // Up to R ConnectivityService can't be updated through mainline, and there was a bug
+    // where registering a callback with a canceled pending intent would crash the system.
+    @Test
+    @IgnoreUpTo(Build.VERSION_CODES.R)
+    public void testRegisterNetworkCallback_pendingIntent_classNotFound() {
+        final Intent intent = new Intent()
+                .setClassName(mContext.getPackageName(), "NonExistent");
+        final PendingIntent pi = PendingIntent.getActivity(mContext, /* requestCode */ 1,
+                intent, PendingIntent.FLAG_CANCEL_CURRENT | PendingIntent.FLAG_MUTABLE);
+
+        final NetworkRequest nr = new NetworkRequest.Builder()
+                .addCapability(NET_CAPABILITY_INTERNET)
+                .build();
+        try {
+            // Before the fix delivered through Mainline, this used to crash the system, because
+            // trying to send the pending intent would throw which would prompt ConnectivityService
+            // to release the wake lock, but it would still send a finished notification at which
+            // point CS would try to release the wake lock again and crash.
+            mCm.registerNetworkCallback(nr, pi);
+        } finally {
+            mCm.unregisterNetworkCallback(pi);
+            pi.cancel();
+        }
+    }
+
     private void runIdenticalPendingIntentsRequestTest(boolean useListen) throws Exception {
         assumeTrue(mPackageManager.hasSystemFeature(FEATURE_WIFI));