Remove cancel listeners from pending intent alarms

The cancel listeners are created per PendingIntent instance and were
spamming the callback list stored inside PendingIntentRecord. In cases
where there is even a single live PendingIntent backed by this
PendingIntentRecord, all PendingIntent instances backed by this
PendingIntentRecord for which a callback was ever registered will leak.

Test: atest FrameworksMockingServicesTests:\
com.android.server.am.PendingIntentControllerTest
atest FrameworksMockingServicesTests:\
com.android.server.AlarmManagerServiceTest

Bug: 143091024
Change-Id: I65df12da0c437064e6e3719911926738c677c4eb

6 files changed, 175 insertions, 38 deletions

diff --git a/core/java/android/app/PendingIntent.java b/core/java/android/app/PendingIntent.java
index 4b4a071ecf72..a9cbfcead5c7 100644
--- a/core/java/android/app/PendingIntent.java
+++ b/core/java/android/app/PendingIntent.java
@@ -1268,7 +1268,12 @@ public final class PendingIntent implements Parcelable {
         return b != null ? new PendingIntent(b, in.getClassCookie(PendingIntent.class)) : null;
     }
 
-    /*package*/ PendingIntent(IIntentSender target) {
+    /**
+     * Creates a PendingIntent with the given target.
+     * @param target the backing IIntentSender
+     * @hide
+     */
+    public PendingIntent(IIntentSender target) {
         mTarget = target;
     }
 
diff --git a/services/core/java/com/android/server/AlarmManagerInternal.java b/services/core/java/com/android/server/AlarmManagerInternal.java
index 5b0de5e2aae0..0a735029eead 100644
--- a/services/core/java/com/android/server/AlarmManagerInternal.java
+++ b/services/core/java/com/android/server/AlarmManagerInternal.java
@@ -16,6 +16,8 @@
 
 package com.android.server;
 
+import android.app.PendingIntent;
+
 public interface AlarmManagerInternal {
     // Some other components in the system server need to know about
     // broadcast alarms currently in flight
@@ -30,4 +32,10 @@ public interface AlarmManagerInternal {
     boolean isIdling();
     public void removeAlarmsForUid(int uid);
     public void registerInFlightListener(InFlightListener callback);
+
+    /**
+     * Removes any alarm with the given pending intent with equality determined using
+     * {@link android.app.PendingIntent#equals(java.lang.Object) PendingIntent.equals}
+     */
+    void remove(PendingIntent rec);
 }
diff --git a/services/core/java/com/android/server/AlarmManagerService.java b/services/core/java/com/android/server/AlarmManagerService.java
index ff0044f6f1ad..be400a6afb7a 100644
--- a/services/core/java/com/android/server/AlarmManagerService.java
+++ b/services/core/java/com/android/server/AlarmManagerService.java
@@ -213,7 +213,6 @@ class AlarmManagerService extends SystemService {
     IAlarmListener mTimeTickTrigger;
     PendingIntent mDateChangeSender;
     Random mRandom;
-    PendingIntent.CancelListener mOperationCancelListener;
     boolean mInteractive = true;
     long mNonInteractiveStartTime;
     long mNonInteractiveTime;
@@ -1501,7 +1500,6 @@ class AlarmManagerService extends SystemService {
 
         synchronized (mLock) {
             mHandler = new AlarmHandler();
-            mOperationCancelListener = (intent) -> removeImpl(intent, null);
             mConstants = new Constants(mHandler);
             mAppWakeupHistory = new AppWakeupHistory(Constants.DEFAULT_APP_STANDBY_WINDOW);
 
@@ -1756,9 +1754,6 @@ class AlarmManagerService extends SystemService {
         } else {
             maxElapsed = triggerElapsed + windowLength;
         }
-        if (operation != null) {
-            operation.registerCancelListener(mOperationCancelListener);
-        }
         synchronized (mLock) {
             if (DEBUG_BATCH) {
                 Slog.v(TAG, "set(" + operation + ") : type=" + type
@@ -1771,8 +1766,6 @@ class AlarmManagerService extends SystemService {
                         "Maximum limit of concurrent alarms " + mConstants.MAX_ALARMS_PER_UID
                                 + " reached for uid: " + UserHandle.formatUid(callingUid)
                                 + ", callingPackage: " + callingPackage;
-                mHandler.obtainMessage(AlarmHandler.UNREGISTER_CANCEL_LISTENER,
-                        operation).sendToTarget();
                 Slog.w(TAG, errorMsg);
                 throw new IllegalStateException(errorMsg);
             }
@@ -1793,8 +1786,6 @@ class AlarmManagerService extends SystemService {
             if (ActivityManager.getService().isAppStartModeDisabled(callingUid, callingPackage)) {
                 Slog.w(TAG, "Not setting alarm from " + callingUid + ":" + a
                         + " -- package not allowed to start");
-                mHandler.obtainMessage(AlarmHandler.UNREGISTER_CANCEL_LISTENER,
-                        operation).sendToTarget();
                 return;
             }
         } catch (RemoteException e) {
@@ -2048,6 +2039,11 @@ class AlarmManagerService extends SystemService {
         }
 
         @Override
+        public void remove(PendingIntent pi) {
+            mHandler.obtainMessage(AlarmHandler.REMOVE_FOR_CANCELED, pi).sendToTarget();
+        }
+
+        @Override
         public void registerInFlightListener(InFlightListener callback) {
             synchronized (mLock) {
                 mInFlightListeners.add(callback);
@@ -2152,8 +2148,6 @@ class AlarmManagerService extends SystemService {
             synchronized (mLock) {
                 removeLocked(operation, listener);
             }
-            mHandler.obtainMessage(AlarmHandler.UNREGISTER_CANCEL_LISTENER,
-                    operation).sendToTarget();
         }
 
         @Override
@@ -4156,7 +4150,7 @@ class AlarmManagerService extends SystemService {
         public static final int APP_STANDBY_BUCKET_CHANGED = 5;
         public static final int CHARGING_STATUS_CHANGED = 6;
         public static final int REMOVE_FOR_STOPPED = 7;
-        public static final int UNREGISTER_CANCEL_LISTENER = 8;
+        public static final int REMOVE_FOR_CANCELED = 8;
 
         AlarmHandler() {
             super(Looper.myLooper());
@@ -4239,10 +4233,10 @@ class AlarmManagerService extends SystemService {
                     }
                     break;
 
-                case UNREGISTER_CANCEL_LISTENER:
-                    final PendingIntent pi = (PendingIntent) msg.obj;
-                    if (pi != null) {
-                        pi.unregisterCancelListener(mOperationCancelListener);
+                case REMOVE_FOR_CANCELED:
+                    final PendingIntent operation = (PendingIntent) msg.obj;
+                    synchronized (mLock) {
+                        removeLocked(operation, null);
                     }
                     break;
 
@@ -4721,11 +4715,6 @@ class AlarmManagerService extends SystemService {
                                         Intent.EXTRA_ALARM_COUNT, alarm.count),
                                 mDeliveryTracker, mHandler, null,
                                 allowWhileIdle ? mIdleOptions : null);
-                        if (alarm.repeatInterval == 0) {
-                            // Keep the listener for repeating alarms until they get cancelled
-                            mHandler.obtainMessage(AlarmHandler.UNREGISTER_CANCEL_LISTENER,
-                                    alarm.operation).sendToTarget();
-                        }
                     } catch (PendingIntent.CanceledException e) {
                         if (alarm.repeatInterval > 0) {
                             // This IntentSender is no longer valid, but this
diff --git a/services/core/java/com/android/server/am/PendingIntentController.java b/services/core/java/com/android/server/am/PendingIntentController.java
index d75591cc7432..df76713d58a6 100644
--- a/services/core/java/com/android/server/am/PendingIntentController.java
+++ b/services/core/java/com/android/server/am/PendingIntentController.java
@@ -43,6 +43,7 @@ import android.util.Slog;
 
 import com.android.internal.os.IResultReceiver;
 import com.android.internal.util.function.pooled.PooledLambda;
+import com.android.server.AlarmManagerInternal;
 import com.android.server.LocalServices;
 import com.android.server.wm.ActivityTaskManagerInternal;
 import com.android.server.wm.SafeActivityOptions;
@@ -293,6 +294,8 @@ public class PendingIntentController {
                     PendingIntentController::handlePendingIntentCancelled, this, callbacks);
             mH.sendMessage(m);
         }
+        final AlarmManagerInternal ami = LocalServices.getService(AlarmManagerInternal.class);
+        ami.remove(new PendingIntent(rec));
     }
 
     private void handlePendingIntentCancelled(RemoteCallbackList<IResultReceiver> callbacks) {
diff --git a/services/tests/mockingservicestests/src/com/android/server/AlarmManagerServiceTest.java b/services/tests/mockingservicestests/src/com/android/server/AlarmManagerServiceTest.java
index 9e7b80567263..c449bd665a54 100644
--- a/services/tests/mockingservicestests/src/com/android/server/AlarmManagerServiceTest.java
+++ b/services/tests/mockingservicestests/src/com/android/server/AlarmManagerServiceTest.java
@@ -24,6 +24,7 @@ import static android.app.usage.UsageStatsManager.STANDBY_BUCKET_FREQUENT;
 import static android.app.usage.UsageStatsManager.STANDBY_BUCKET_RARE;
 import static android.app.usage.UsageStatsManager.STANDBY_BUCKET_WORKING_SET;
 
+import static com.android.dx.mockito.inline.extended.ExtendedMockito.doCallRealMethod;
 import static com.android.dx.mockito.inline.extended.ExtendedMockito.doNothing;
 import static com.android.dx.mockito.inline.extended.ExtendedMockito.doReturn;
 import static com.android.dx.mockito.inline.extended.ExtendedMockito.doThrow;
@@ -35,6 +36,7 @@ import static com.android.dx.mockito.inline.extended.ExtendedMockito.when;
 import static com.android.server.AlarmManagerService.ACTIVE_INDEX;
 import static com.android.server.AlarmManagerService.AlarmHandler.APP_STANDBY_BUCKET_CHANGED;
 import static com.android.server.AlarmManagerService.AlarmHandler.CHARGING_STATUS_CHANGED;
+import static com.android.server.AlarmManagerService.AlarmHandler.REMOVE_FOR_CANCELED;
 import static com.android.server.AlarmManagerService.Constants.KEY_ALLOW_WHILE_IDLE_LONG_TIME;
 import static com.android.server.AlarmManagerService.Constants.KEY_ALLOW_WHILE_IDLE_SHORT_TIME;
 import static com.android.server.AlarmManagerService.Constants.KEY_ALLOW_WHILE_IDLE_WHITELIST_DURATION;
@@ -81,9 +83,9 @@ import android.provider.Settings;
 import android.util.Log;
 import android.util.SparseArray;
 
-import androidx.test.filters.FlakyTest;
 import androidx.test.runner.AndroidJUnit4;
 
+import com.android.dx.mockito.inline.extended.MockedVoidMethod;
 import com.android.internal.annotations.GuardedBy;
 import com.android.server.usage.AppStandbyInternal;
 
@@ -172,7 +174,6 @@ public class AlarmManagerServiceTest {
     }
 
     public class Injector extends AlarmManagerService.Injector {
-        boolean mIsAutomotiveOverride;
 
         Injector(Context context) {
             super(context);
@@ -258,12 +259,13 @@ public class AlarmManagerServiceTest {
                 .startMocking();
         doReturn(mIActivityManager).when(ActivityManager::getService);
         doReturn(mAppStateTracker).when(() -> LocalServices.getService(AppStateTracker.class));
-        doReturn(null)
-                .when(() -> LocalServices.getService(DeviceIdleInternal.class));
         doReturn(mAppStandbyInternal).when(
                 () -> LocalServices.getService(AppStandbyInternal.class));
         doReturn(mUsageStatsManagerInternal).when(
                 () -> LocalServices.getService(UsageStatsManagerInternal.class));
+        doCallRealMethod().when((MockedVoidMethod) () ->
+                LocalServices.addService(eq(AlarmManagerInternal.class), any()));
+        doCallRealMethod().when(() -> LocalServices.getService(AlarmManagerInternal.class));
         when(mUsageStatsManagerInternal.getAppStandbyBucket(eq(TEST_CALLING_PACKAGE),
                 eq(UserHandle.getUserId(TEST_CALLING_UID)), anyLong()))
                 .thenReturn(STANDBY_BUCKET_ACTIVE);
@@ -458,7 +460,6 @@ public class AlarmManagerServiceTest {
         assertEquals(expectedTriggerTime, mTestTimer.getElapsed());
     }
 
-    @FlakyTest(bugId = 130313408)
     @Test
     public void testEarliestAlarmSet() {
         final PendingIntent pi6 = getNewMockPendingIntent();
@@ -676,11 +677,15 @@ public class AlarmManagerServiceTest {
                 anyLong())).thenReturn(bucket);
         mAppStandbyListener.onAppIdleStateChanged(TEST_CALLING_PACKAGE,
                 UserHandle.getUserId(TEST_CALLING_UID), false, bucket, 0);
+        assertAndHandleMessageSync(APP_STANDBY_BUCKET_CHANGED);
+    }
+
+    private void assertAndHandleMessageSync(int what) {
         final ArgumentCaptor<Message> messageCaptor = ArgumentCaptor.forClass(Message.class);
         verify(mService.mHandler, atLeastOnce()).sendMessage(messageCaptor.capture());
         final Message lastMessage = messageCaptor.getValue();
         assertEquals("Unexpected message send to handler", lastMessage.what,
-                APP_STANDBY_BUCKET_CHANGED);
+                what);
         mService.mHandler.handleMessage(lastMessage);
     }
 
@@ -742,12 +747,7 @@ public class AlarmManagerServiceTest {
         mChargingReceiver.onReceive(mMockContext,
                 new Intent(parole ? BatteryManager.ACTION_CHARGING
                         : BatteryManager.ACTION_DISCHARGING));
-        final ArgumentCaptor<Message> messageCaptor = ArgumentCaptor.forClass(Message.class);
-        verify(mService.mHandler, atLeastOnce()).sendMessage(messageCaptor.capture());
-        final Message lastMessage = messageCaptor.getValue();
-        assertEquals("Unexpected message send to handler", lastMessage.what,
-                CHARGING_STATUS_CHANGED);
-        mService.mHandler.handleMessage(lastMessage);
+        assertAndHandleMessageSync(CHARGING_STATUS_CHANGED);
     }
 
     @Test
@@ -1050,12 +1050,13 @@ public class AlarmManagerServiceTest {
     }
 
     @Test
-    public void alarmCountOnPendingIntentCancel() {
+    public void alarmCountOnRemoveForCanceled() {
+        final AlarmManagerInternal ami = LocalServices.getService(AlarmManagerInternal.class);
         final PendingIntent pi = getNewMockPendingIntent();
-        setTestAlarm(ELAPSED_REALTIME_WAKEUP, mNowElapsedTest + 123, pi);
-        verify(pi).registerCancelListener(mService.mOperationCancelListener);
+        setTestAlarm(ELAPSED_REALTIME, mNowElapsedTest + 12345, pi);
         assertEquals(1, mService.mAlarmsPerUid.get(TEST_CALLING_UID));
-        mService.mOperationCancelListener.onCancelled(pi);
+        ami.remove(pi);
+        assertAndHandleMessageSync(REMOVE_FOR_CANCELED);
         assertEquals(0, mService.mAlarmsPerUid.get(TEST_CALLING_UID));
     }
 
@@ -1064,5 +1065,6 @@ public class AlarmManagerServiceTest {
         if (mMockingSession != null) {
             mMockingSession.finishMocking();
         }
+        LocalServices.removeServiceForTest(AlarmManagerInternal.class);
     }
 }
diff --git a/services/tests/mockingservicestests/src/com/android/server/am/PendingIntentControllerTest.java b/services/tests/mockingservicestests/src/com/android/server/am/PendingIntentControllerTest.java
new file mode 100644
index 000000000000..3975f0baa22a
--- /dev/null
+++ b/services/tests/mockingservicestests/src/com/android/server/am/PendingIntentControllerTest.java
@@ -0,0 +1,130 @@
+/*
+ * Copyright (C) 2019 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.android.server.am;
+
+import static com.android.dx.mockito.inline.extended.ExtendedMockito.doReturn;
+import static com.android.dx.mockito.inline.extended.ExtendedMockito.mockitoSession;
+import static com.android.dx.mockito.inline.extended.ExtendedMockito.verify;
+
+import static org.junit.Assert.assertEquals;
+import static org.mockito.ArgumentMatchers.anyInt;
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.when;
+
+import android.app.ActivityManager;
+import android.app.ActivityManagerInternal;
+import android.app.AppGlobals;
+import android.app.PendingIntent;
+import android.content.Intent;
+import android.content.pm.IPackageManager;
+import android.os.Looper;
+
+import androidx.test.runner.AndroidJUnit4;
+
+import com.android.server.AlarmManagerInternal;
+import com.android.server.LocalServices;
+
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.ArgumentCaptor;
+import org.mockito.Mock;
+import org.mockito.MockitoSession;
+import org.mockito.quality.Strictness;
+
+@RunWith(AndroidJUnit4.class)
+public class PendingIntentControllerTest {
+    private static final String TEST_PACKAGE_NAME = "test-package-1";
+    private static final int TEST_CALLING_UID = android.os.Process.myUid();
+    private static final Intent[] TEST_INTENTS = new Intent[]{new Intent("com.test.intent")};
+
+    @Mock
+    private UserController mUserController;
+    @Mock
+    private AlarmManagerInternal mAlarmManagerInternal;
+    @Mock
+    private ActivityManagerInternal mActivityManagerInternal;
+    @Mock
+    private IPackageManager mIPackageManager;
+
+    private MockitoSession mMockingSession;
+    private PendingIntentController mPendingIntentController;
+
+    @Before
+    public void setUp() throws Exception {
+        mMockingSession = mockitoSession()
+                .initMocks(this)
+                .mockStatic(LocalServices.class)
+                .mockStatic(AppGlobals.class)
+                .strictness(Strictness.LENIENT) // Needed to stub LocalServices.getService twice
+                .startMocking();
+        doReturn(mAlarmManagerInternal).when(
+                () -> LocalServices.getService(AlarmManagerInternal.class));
+        doReturn(mActivityManagerInternal).when(
+                () -> LocalServices.getService(ActivityManagerInternal.class));
+        doReturn(mIPackageManager).when(() -> AppGlobals.getPackageManager());
+        when(mIPackageManager.getPackageUid(eq(TEST_PACKAGE_NAME), anyInt(), anyInt())).thenReturn(
+                TEST_CALLING_UID);
+        mPendingIntentController = new PendingIntentController(Looper.getMainLooper(),
+                mUserController);
+        mPendingIntentController.onActivityManagerInternalAdded();
+    }
+
+    private PendingIntentRecord createPendingIntentRecord(int flags) {
+        return mPendingIntentController.getIntentSender(ActivityManager.INTENT_SENDER_BROADCAST,
+                TEST_PACKAGE_NAME, TEST_CALLING_UID, 0, null, null, 0, TEST_INTENTS, null, flags,
+                null);
+    }
+
+    @Test
+    public void alarmsRemovedOnCancel() {
+        final PendingIntentRecord pir = createPendingIntentRecord(0);
+        mPendingIntentController.cancelIntentSender(pir);
+        final ArgumentCaptor<PendingIntent> piCaptor = ArgumentCaptor.forClass(PendingIntent.class);
+        verify(mAlarmManagerInternal).remove(piCaptor.capture());
+        assertEquals("Wrong target for pending intent passed to alarm manager", pir,
+                piCaptor.getValue().getTarget());
+    }
+
+    @Test
+    public void alarmsRemovedOnRecreateWithCancelCurrent() {
+        final PendingIntentRecord pir = createPendingIntentRecord(0);
+        createPendingIntentRecord(PendingIntent.FLAG_CANCEL_CURRENT);
+        final ArgumentCaptor<PendingIntent> piCaptor = ArgumentCaptor.forClass(PendingIntent.class);
+        verify(mAlarmManagerInternal).remove(piCaptor.capture());
+        assertEquals("Wrong target for pending intent passed to alarm manager", pir,
+                piCaptor.getValue().getTarget());
+    }
+
+    @Test
+    public void alarmsRemovedOnSendingOneShot() {
+        final PendingIntentRecord pir = createPendingIntentRecord(PendingIntent.FLAG_ONE_SHOT);
+        pir.send(0, null, null, null, null, null, null);
+        final ArgumentCaptor<PendingIntent> piCaptor = ArgumentCaptor.forClass(PendingIntent.class);
+        verify(mAlarmManagerInternal).remove(piCaptor.capture());
+        assertEquals("Wrong target for pending intent passed to alarm manager", pir,
+                piCaptor.getValue().getTarget());
+    }
+
+    @After
+    public void tearDown() {
+        if (mMockingSession != null) {
+            mMockingSession.finishMocking();
+        }
+    }
+}