diff --git a/service/src/com/android/server/ConnectivityService.java b/service/src/com/android/server/ConnectivityService.java index adf593e488..2a66e4bff0 100644 --- a/service/src/com/android/server/ConnectivityService.java +++ b/service/src/com/android/server/ConnectivityService.java @@ -423,14 +423,14 @@ import java.util.TreeSet; 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 @@ public class ConnectivityService extends IConnectivityManager.Stub // 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 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 @@ public class ConnectivityService extends IConnectivityManager.Stub // 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 aa7d618eb8..3828dc80a3 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_TEST; 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.net.module.util.NetworkStackConstants.TEST_CAPTIVE_POR 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.net.wifi.WifiManager; 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 @@ public class ConnectivityManagerTest { * {@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 @@ public class ConnectivityManagerTest { } } + // 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));