From 7d40780e398a38a69a2325c3f091a61b04dc6206 Mon Sep 17 00:00:00 2001
From: Suprabh Shukla <suprabh@google.com>
Date: Sun, 26 Apr 2020 20:04:55 -0700
Subject: [PATCH] Don't remove OnAlarmListener mappings in the client

Removing the mapping is prone to races in keeping the state consistent
with the server. An inconsistent state can lead to multiple alarms on
the server for the same listener.

Test: atest CtsJobSchedulerTestCases doesn't fill the alarm queue with
more than one alarm.
atest CtsAlarmManagerTestCases

Bug: 154444435
Change-Id: Iaf11d5decb17fbf2366b49d9865231bf65dbdc41
---
 core/java/android/app/AlarmManager.java | 42 ++++++++++++-------------
 1 file changed, 20 insertions(+), 22 deletions(-)

diff --git a/core/java/android/app/AlarmManager.java b/core/java/android/app/AlarmManager.java
index 00627ef381ab3..ae8a2cbf51206 100644
--- a/core/java/android/app/AlarmManager.java
+++ b/core/java/android/app/AlarmManager.java
@@ -31,7 +31,6 @@ import android.os.Parcelable;
 import android.os.RemoteException;
 import android.os.WorkSource;
 import android.text.TextUtils;
-import android.util.ArrayMap;
 import android.util.Log;
 import android.util.proto.ProtoOutputStream;
 
@@ -40,6 +39,8 @@ import libcore.timezone.ZoneInfoDb;
 import java.io.IOException;
 import java.lang.annotation.Retention;
 import java.lang.annotation.RetentionPolicy;
+import java.lang.ref.WeakReference;
+import java.util.WeakHashMap;
 
 /**
  * This class provides access to the system alarm services.  These allow you
@@ -222,26 +223,12 @@ public class AlarmManager {
             } catch (RemoteException ex) {
                 throw ex.rethrowFromSystemServer();
             }
-
-            synchronized (AlarmManager.class) {
-                if (sWrappers != null) {
-                    sWrappers.remove(mListener);
-                }
-            }
         }
 
         @Override
         public void doAlarm(IAlarmCompleteListener alarmManager) {
             mCompletion = alarmManager;
 
-            // Remove this listener from the wrapper cache first; the server side
-            // already considers it gone
-            synchronized (AlarmManager.class) {
-                if (sWrappers != null) {
-                    sWrappers.remove(mListener);
-                }
-            }
-
             mHandler.post(this);
         }
 
@@ -263,9 +250,14 @@ public class AlarmManager {
         }
     }
 
-    // Tracking of the OnAlarmListener -> wrapper mapping, for cancel() support.
-    // Access is synchronized on the AlarmManager class object.
-    private static ArrayMap<OnAlarmListener, ListenerWrapper> sWrappers;
+    /**
+     * Tracking of the OnAlarmListener -> ListenerWrapper mapping, for cancel() support.
+     * An entry is guaranteed to stay in this map as long as its ListenerWrapper is held by the
+     * server.
+     *
+     * <p>Access is synchronized on the AlarmManager class object.
+     */
+    private static WeakHashMap<OnAlarmListener, WeakReference<ListenerWrapper>> sWrappers;
 
     /**
      * package private on purpose
@@ -682,14 +674,17 @@ public class AlarmManager {
         if (listener != null) {
             synchronized (AlarmManager.class) {
                 if (sWrappers == null) {
-                    sWrappers = new ArrayMap<OnAlarmListener, ListenerWrapper>();
+                    sWrappers = new WeakHashMap<>();
                 }
 
-                recipientWrapper = sWrappers.get(listener);
+                final WeakReference<ListenerWrapper> weakRef = sWrappers.get(listener);
+                if (weakRef != null) {
+                    recipientWrapper = weakRef.get();
+                }
                 // no existing wrapper => build a new one
                 if (recipientWrapper == null) {
                     recipientWrapper = new ListenerWrapper(listener);
-                    sWrappers.put(listener, recipientWrapper);
+                    sWrappers.put(listener, new WeakReference<>(recipientWrapper));
                 }
             }
 
@@ -948,7 +943,10 @@ public class AlarmManager {
         ListenerWrapper wrapper = null;
         synchronized (AlarmManager.class) {
             if (sWrappers != null) {
-                wrapper = sWrappers.get(listener);
+                final WeakReference<ListenerWrapper> weakRef = sWrappers.get(listener);
+                if (weakRef != null) {
+                    wrapper = weakRef.get();
+                }
             }
         }