From 30bd1bb72d3de0efe3a2b460b927c52fc5b0f742 Mon Sep 17 00:00:00 2001 From: Makoto Onuki Date: Tue, 14 Sep 2021 17:31:45 -0700 Subject: [PATCH 1/2] Expose PendingIntent.addCancelListener Bug: 195146423 Test: atest android.app.cts.PendingIntentTest#testCancelListener Change-Id: I74e6ae49bfb2b31f0bc693d75e1f1433206011ce --- core/api/module-lib-current.txt | 6 + core/api/test-current.txt | 6 + core/java/android/app/IActivityManager.aidl | 9 +- core/java/android/app/PendingIntent.java | 113 ++++++++++++++---- .../server/am/ActivityManagerService.java | 5 +- .../server/am/PendingIntentController.java | 15 ++- 6 files changed, 122 insertions(+), 32 deletions(-) diff --git a/core/api/module-lib-current.txt b/core/api/module-lib-current.txt index 9c4d53a157aad..f72c3a7c8af15 100644 --- a/core/api/module-lib-current.txt +++ b/core/api/module-lib-current.txt @@ -35,8 +35,14 @@ package android.app { } public final class PendingIntent implements android.os.Parcelable { + method public boolean addCancelListener(@NonNull java.util.concurrent.Executor, @NonNull android.app.PendingIntent.CancelListener); method @RequiresPermission(android.Manifest.permission.GET_INTENT_SENDER_INTENT) public boolean intentFilterEquals(@Nullable android.app.PendingIntent); method @NonNull @RequiresPermission(android.Manifest.permission.GET_INTENT_SENDER_INTENT) public java.util.List queryIntentComponents(int); + method public void removeCancelListener(@NonNull android.app.PendingIntent.CancelListener); + } + + public static interface PendingIntent.CancelListener { + method public void onCancelled(@NonNull android.app.PendingIntent); } public class StatusBarManager { diff --git a/core/api/test-current.txt b/core/api/test-current.txt index ea6d0cecfd739..8a4c8db0cd7ea 100644 --- a/core/api/test-current.txt +++ b/core/api/test-current.txt @@ -313,11 +313,17 @@ package android.app { } public final class PendingIntent implements android.os.Parcelable { + method public boolean addCancelListener(@NonNull java.util.concurrent.Executor, @NonNull android.app.PendingIntent.CancelListener); method @RequiresPermission("android.permission.GET_INTENT_SENDER_INTENT") public boolean intentFilterEquals(@Nullable android.app.PendingIntent); method @NonNull @RequiresPermission("android.permission.GET_INTENT_SENDER_INTENT") public java.util.List queryIntentComponents(int); + method public void removeCancelListener(@NonNull android.app.PendingIntent.CancelListener); field @Deprecated public static final int FLAG_MUTABLE_UNAUDITED = 33554432; // 0x2000000 } + public static interface PendingIntent.CancelListener { + method public void onCancelled(@NonNull android.app.PendingIntent); + } + public final class PictureInPictureParams implements android.os.Parcelable { method public java.util.List getActions(); method public float getAspectRatio(); diff --git a/core/java/android/app/IActivityManager.aidl b/core/java/android/app/IActivityManager.aidl index b90b9a11611e9..dfc4a6aecabba 100644 --- a/core/java/android/app/IActivityManager.aidl +++ b/core/java/android/app/IActivityManager.aidl @@ -250,7 +250,14 @@ interface IActivityManager { in String[] resolvedTypes, int flags, in Bundle options, int userId); void cancelIntentSender(in IIntentSender sender); ActivityManager.PendingIntentInfo getInfoForIntentSender(in IIntentSender sender); - void registerIntentSenderCancelListener(in IIntentSender sender, in IResultReceiver receiver); + /** + This method used to be called registerIntentSenderCancelListener(), was void, and + would call `receiver` if the PI has already been canceled. + Now it returns false if the PI is cancelled, without calling `receiver`. + The method was renamed to catch calls to the original method. + */ + boolean registerIntentSenderCancelListenerEx(in IIntentSender sender, + in IResultReceiver receiver); void unregisterIntentSenderCancelListener(in IIntentSender sender, in IResultReceiver receiver); void enterSafeMode(); void noteWakeupAlarm(in IIntentSender sender, in WorkSource workSource, int sourceUid, diff --git a/core/java/android/app/PendingIntent.java b/core/java/android/app/PendingIntent.java index 0136a35e3975b..0863500844392 100644 --- a/core/java/android/app/PendingIntent.java +++ b/core/java/android/app/PendingIntent.java @@ -53,8 +53,10 @@ import android.os.RemoteException; import android.os.UserHandle; import android.util.AndroidException; import android.util.ArraySet; +import android.util.Pair; import android.util.proto.ProtoOutputStream; +import com.android.internal.annotations.GuardedBy; import com.android.internal.os.IResultReceiver; import java.lang.annotation.Retention; @@ -62,6 +64,7 @@ import java.lang.annotation.RetentionPolicy; import java.util.Collections; import java.util.List; import java.util.Objects; +import java.util.concurrent.Executor; /** * A description of an Intent and target action to perform with it. Instances @@ -127,7 +130,27 @@ public final class PendingIntent implements Parcelable { private final IIntentSender mTarget; private IResultReceiver mCancelReceiver; private IBinder mWhitelistToken; - private ArraySet mCancelListeners; + + /** + * To protect {@link #mCancelListeners}. We could stop lazy-initialization and synchronize + * on {@link #mCancelListeners} directly, and that wouldn't increase allocations + * (an empty ArraySet won't causew extra allocations), but + * because an empty ArraySet is slightly larger than an Object, and because + * {@link #addCancelListener} is rarely used, having a separate lock object would probably + * be a net win. + */ + private final Object mLock = new Object(); + + @GuardedBy("mLock") + private ArraySet> mCancelListeners; + + /** + * Whether the PI is canceld or not. Note this is essentially a "cache" that's updated + * only when the client uses {@link #addCancelListener}. Even if this is fase, that + * still doesn't know the PI is *not* cancled, but if it's true, this PI is definitely canceled. + */ + @GuardedBy("mLock") + private boolean mCanceled; // cached pending intent information private @Nullable PendingIntentInfo mCachedInfo; @@ -1048,19 +1071,38 @@ public final class PendingIntent implements Parcelable { } /** - * Register a listener to when this pendingIntent is cancelled. There are no guarantees on which - * thread a listener will be called and it's up to the caller to synchronize. This may - * trigger a synchronous binder call so should therefore usually be called on a background - * thread. + * @hide + * @deprecated use {@link #addCancelListener(Executor, CancelListener)} instead. + */ + @Deprecated + public void registerCancelListener(@NonNull CancelListener cancelListener) { + if (!addCancelListener(Runnable::run, cancelListener)) { + // Call the callback right away synchronously, if the PI has been canceled already. + cancelListener.onCancelled(this); + } + } + + /** + * Register a listener to when this pendingIntent is cancelled. + * + * @return true if the listener has been set successfully. false if the {@link PendingIntent} + * has already been canceled. * * @hide */ - public void registerCancelListener(CancelListener cancelListener) { - synchronized (this) { + @SystemApi(client = SystemApi.Client.MODULE_LIBRARIES) + @TestApi + public boolean addCancelListener(@NonNull Executor executor, + @NonNull CancelListener cancelListener) { + synchronized (mLock) { + if (mCanceled) { + return false; + } + if (mCancelReceiver == null) { mCancelReceiver = new IResultReceiver.Stub() { @Override - public void send(int resultCode, Bundle resultData) throws RemoteException { + public void send(int resultCode, Bundle resultData) { notifyCancelListeners(); } }; @@ -1069,42 +1111,69 @@ public final class PendingIntent implements Parcelable { mCancelListeners = new ArraySet<>(); } boolean wasEmpty = mCancelListeners.isEmpty(); - mCancelListeners.add(cancelListener); + mCancelListeners.add(Pair.create(executor, cancelListener)); if (wasEmpty) { + boolean success; try { - ActivityManager.getService().registerIntentSenderCancelListener(mTarget, - mCancelReceiver); + success = ActivityManager.getService().registerIntentSenderCancelListenerEx( + mTarget, mCancelReceiver); } catch (RemoteException e) { throw e.rethrowFromSystemServer(); } + if (!success) { + mCanceled = true; + } + return success; + } else { + return !mCanceled; } } } private void notifyCancelListeners() { - ArraySet cancelListeners; - synchronized (this) { + ArraySet> cancelListeners; + synchronized (mLock) { + if (mCancelListeners == null || mCancelListeners.size() == 0) { + return; + } + mCanceled = true; cancelListeners = new ArraySet<>(mCancelListeners); + mCancelListeners.clear(); } int size = cancelListeners.size(); for (int i = 0; i < size; i++) { - cancelListeners.valueAt(i).onCancelled(this); + final Pair pair = cancelListeners.valueAt(i); + pair.first.execute(() -> pair.second.onCancelled(this)); } } + /** + * @hide + * @deprecated use {@link #removeCancelListener(CancelListener)} instead. + */ + @Deprecated + public void unregisterCancelListener(CancelListener cancelListener) { + removeCancelListener(cancelListener); + } + /** * Un-register a listener to when this pendingIntent is cancelled. * * @hide */ - public void unregisterCancelListener(CancelListener cancelListener) { - synchronized (this) { - if (mCancelListeners == null) { + @SystemApi(client = SystemApi.Client.MODULE_LIBRARIES) + @TestApi + public void removeCancelListener(@NonNull CancelListener cancelListener) { + synchronized (mLock) { + if (mCancelListeners.size() == 0) { return; } - boolean wasEmpty = mCancelListeners.isEmpty(); - mCancelListeners.remove(cancelListener); - if (mCancelListeners.isEmpty() && !wasEmpty) { + for (int i = mCancelListeners.size() - 1; i >= 0; i--) { + if (mCancelListeners.valueAt(i).second == cancelListener) { + mCancelListeners.removeAt(i); + } + } + if (mCancelListeners.isEmpty()) { try { ActivityManager.getService().unregisterIntentSenderCancelListener(mTarget, mCancelReceiver); @@ -1401,13 +1470,15 @@ public final class PendingIntent implements Parcelable { * * @hide */ + @SystemApi(client = SystemApi.Client.MODULE_LIBRARIES) + @TestApi public interface CancelListener { /** * Called when a Pending Intent is cancelled. * * @param intent The intent that was cancelled. */ - void onCancelled(PendingIntent intent); + void onCancelled(@NonNull PendingIntent intent); } private PendingIntentInfo getCachedInfo() { diff --git a/services/core/java/com/android/server/am/ActivityManagerService.java b/services/core/java/com/android/server/am/ActivityManagerService.java index f74d7ac07f518..3f816c88ae515 100644 --- a/services/core/java/com/android/server/am/ActivityManagerService.java +++ b/services/core/java/com/android/server/am/ActivityManagerService.java @@ -4990,8 +4990,9 @@ public class ActivityManagerService extends IActivityManager.Stub } @Override - public void registerIntentSenderCancelListener(IIntentSender sender, IResultReceiver receiver) { - mPendingIntentController.registerIntentSenderCancelListener(sender, receiver); + public boolean registerIntentSenderCancelListenerEx( + IIntentSender sender, IResultReceiver receiver) { + return mPendingIntentController.registerIntentSenderCancelListener(sender, receiver); } @Override diff --git a/services/core/java/com/android/server/am/PendingIntentController.java b/services/core/java/com/android/server/am/PendingIntentController.java index 534bd84a91a38..c49e6969c2c5c 100644 --- a/services/core/java/com/android/server/am/PendingIntentController.java +++ b/services/core/java/com/android/server/am/PendingIntentController.java @@ -271,9 +271,11 @@ public class PendingIntentController { } } - void registerIntentSenderCancelListener(IIntentSender sender, IResultReceiver receiver) { + boolean registerIntentSenderCancelListener(IIntentSender sender, IResultReceiver receiver) { if (!(sender instanceof PendingIntentRecord)) { - return; + Slog.w(TAG, "registerIntentSenderCancelListener called on non-PendingIntentRecord"); + // In this case, it's not "success", but we don't know if it's canceld either. + return true; } boolean isCancelled; synchronized (mLock) { @@ -281,12 +283,9 @@ public class PendingIntentController { isCancelled = pendingIntent.canceled; if (!isCancelled) { pendingIntent.registerCancelListenerLocked(receiver); - } - } - if (isCancelled) { - try { - receiver.send(Activity.RESULT_CANCELED, null); - } catch (RemoteException e) { + return true; + } else { + return false; } } } From 41b8d147fd82ab6d21acd33040328b61d6af6e11 Mon Sep 17 00:00:00 2001 From: Makoto Onuki Date: Fri, 17 Sep 2021 11:27:43 -0700 Subject: [PATCH 2/2] Reduce PendingIntent memory allocation Fix: 195146423 Test: atest android.app.cts.PendingIntentTest Change-Id: I246d272c77337f77623f0c11d5f445895cdbf5e8 --- core/java/android/app/PendingIntent.java | 104 +++++++++++------------ 1 file changed, 50 insertions(+), 54 deletions(-) diff --git a/core/java/android/app/PendingIntent.java b/core/java/android/app/PendingIntent.java index 0863500844392..9b9eed46db02e 100644 --- a/core/java/android/app/PendingIntent.java +++ b/core/java/android/app/PendingIntent.java @@ -127,34 +127,37 @@ import java.util.concurrent.Executor; */ public final class PendingIntent implements Parcelable { private static final String TAG = "PendingIntent"; + @NonNull private final IIntentSender mTarget; - private IResultReceiver mCancelReceiver; private IBinder mWhitelistToken; - /** - * To protect {@link #mCancelListeners}. We could stop lazy-initialization and synchronize - * on {@link #mCancelListeners} directly, and that wouldn't increase allocations - * (an empty ArraySet won't causew extra allocations), but - * because an empty ArraySet is slightly larger than an Object, and because - * {@link #addCancelListener} is rarely used, having a separate lock object would probably - * be a net win. - */ - private final Object mLock = new Object(); - - @GuardedBy("mLock") - private ArraySet> mCancelListeners; - - /** - * Whether the PI is canceld or not. Note this is essentially a "cache" that's updated - * only when the client uses {@link #addCancelListener}. Even if this is fase, that - * still doesn't know the PI is *not* cancled, but if it's true, this PI is definitely canceled. - */ - @GuardedBy("mLock") - private boolean mCanceled; - // cached pending intent information private @Nullable PendingIntentInfo mCachedInfo; + /** + * Structure to store information related to {@link #addCancelListener}, which is rarely used, + * so we lazily allocate it to keep the PendingIntent class size small. + */ + private final class CancelListerInfo extends IResultReceiver.Stub { + private final ArraySet> mCancelListeners = new ArraySet<>(); + + /** + * Whether the PI is canceled or not. Note this is essentially a "cache" that's updated + * only when the client uses {@link #addCancelListener}. Even if this is false, that + * still doesn't know the PI is *not* canceled, but if it's true, this PI is definitely + * canceled. + */ + private boolean mCanceled; + + @Override + public void send(int resultCode, Bundle resultData) throws RemoteException { + notifyCancelListeners(); + } + } + + @GuardedBy("mTarget") + private @Nullable CancelListerInfo mCancelListerInfo; + /** * It is now required to specify either {@link #FLAG_IMMUTABLE} * or {@link #FLAG_MUTABLE} when creating a PendingIntent. @@ -1094,51 +1097,43 @@ public final class PendingIntent implements Parcelable { @TestApi public boolean addCancelListener(@NonNull Executor executor, @NonNull CancelListener cancelListener) { - synchronized (mLock) { - if (mCanceled) { + synchronized (mTarget) { + if (mCancelListerInfo != null && mCancelListerInfo.mCanceled) { return false; } + if (mCancelListerInfo == null) { + mCancelListerInfo = new CancelListerInfo(); + } + final CancelListerInfo cli = mCancelListerInfo; - if (mCancelReceiver == null) { - mCancelReceiver = new IResultReceiver.Stub() { - @Override - public void send(int resultCode, Bundle resultData) { - notifyCancelListeners(); - } - }; - } - if (mCancelListeners == null) { - mCancelListeners = new ArraySet<>(); - } - boolean wasEmpty = mCancelListeners.isEmpty(); - mCancelListeners.add(Pair.create(executor, cancelListener)); + boolean wasEmpty = cli.mCancelListeners.isEmpty(); + cli.mCancelListeners.add(Pair.create(executor, cancelListener)); if (wasEmpty) { boolean success; try { success = ActivityManager.getService().registerIntentSenderCancelListenerEx( - mTarget, mCancelReceiver); + mTarget, cli); } catch (RemoteException e) { throw e.rethrowFromSystemServer(); } if (!success) { - mCanceled = true; + cli.mCanceled = true; } return success; } else { - return !mCanceled; + return !cli.mCanceled; } } } private void notifyCancelListeners() { ArraySet> cancelListeners; - synchronized (mLock) { - if (mCancelListeners == null || mCancelListeners.size() == 0) { - return; - } - mCanceled = true; - cancelListeners = new ArraySet<>(mCancelListeners); - mCancelListeners.clear(); + synchronized (mTarget) { + // When notifyCancelListeners() is called, mCancelListerInfo must always be non-null. + final CancelListerInfo cli = mCancelListerInfo; + cli.mCanceled = true; + cancelListeners = new ArraySet<>(cli.mCancelListeners); + cli.mCancelListeners.clear(); } int size = cancelListeners.size(); for (int i = 0; i < size; i++) { @@ -1164,19 +1159,20 @@ public final class PendingIntent implements Parcelable { @SystemApi(client = SystemApi.Client.MODULE_LIBRARIES) @TestApi public void removeCancelListener(@NonNull CancelListener cancelListener) { - synchronized (mLock) { - if (mCancelListeners.size() == 0) { + synchronized (mTarget) { + final CancelListerInfo cli = mCancelListerInfo; + if (cli == null || cli.mCancelListeners.size() == 0) { return; } - for (int i = mCancelListeners.size() - 1; i >= 0; i--) { - if (mCancelListeners.valueAt(i).second == cancelListener) { - mCancelListeners.removeAt(i); + for (int i = cli.mCancelListeners.size() - 1; i >= 0; i--) { + if (cli.mCancelListeners.valueAt(i).second == cancelListener) { + cli.mCancelListeners.removeAt(i); } } - if (mCancelListeners.isEmpty()) { + if (cli.mCancelListeners.isEmpty()) { try { ActivityManager.getService().unregisterIntentSenderCancelListener(mTarget, - mCancelReceiver); + cli); } catch (RemoteException e) { throw e.rethrowFromSystemServer(); }