diff options
| author | 2023-08-31 12:48:23 +0200 | |
|---|---|---|
| committer | 2023-09-27 14:55:33 +0000 | |
| commit | 43b4d613a3cc9132b4f93858112bc8ef4643cf63 (patch) | |
| tree | 4edf15335242c1ac53a6d642d6ac2678b890bcc0 | |
| parent | b14d7c70701916a71deae1f8f46a19b9ddfd883b (diff) | |
Don't store old pending intents in NotificationTemplateViewWrapper
In NotificationTemplateViewWrapper, we kept Action PendingIntents around so we could disable notification Actions correctly if those intents were cancelled and Actions were updated (there's a race there due to
use of multiple threads).
If application keeps updating their PendingIntents, we ended up keeping several 1000 of them, which causes problems for system_server due to Binder leaks.
This CL makes sure we only keep PendingIntent HashCodes (they're enough to disambiguate) and clean even those as Actions get updated. We now also only have a single listener instance to avoid leaks of those.
Bug:295847709
Bug:298028699
Test: Newly added unit tests.
(cherry picked from https://googleplex-android-review.googlesource.com/q/commit:af02615322671de23cb3bae55b1e24ee7f309ae0)
Merged-In: I67188ee73920b1fea3a785bf729342ffc0099f20
Change-Id: I67188ee73920b1fea3a785bf729342ffc0099f20
3 files changed, 452 insertions, 60 deletions
diff --git a/packages/SystemUI/res/values/ids.xml b/packages/SystemUI/res/values/ids.xml index 21696fefb80f..6cdd15e637bd 100644 --- a/packages/SystemUI/res/values/ids.xml +++ b/packages/SystemUI/res/values/ids.xml @@ -233,6 +233,11 @@ <item type="id" name="pin_pad"/> <!-- + Tag used to store pending intent registration listeners in NotificationTemplateViewWrapper + --> + <item type="id" name="pending_intent_listener_tag" /> + + <!-- Used to tag views programmatically added to the smartspace area so they can be more easily removed later. --> diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/row/wrapper/NotificationTemplateViewWrapper.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/row/wrapper/NotificationTemplateViewWrapper.java index 875a409c07f0..91b12ccf919a 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/row/wrapper/NotificationTemplateViewWrapper.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/row/wrapper/NotificationTemplateViewWrapper.java @@ -20,6 +20,9 @@ import static android.view.View.VISIBLE; import static com.android.systemui.statusbar.notification.row.ExpandableNotificationRow.DEFAULT_HEADER_VISIBLE_AMOUNT; +import android.annotation.MainThread; +import android.annotation.NonNull; +import android.annotation.Nullable; import android.app.Notification; import android.app.PendingIntent; import android.content.Context; @@ -34,8 +37,7 @@ import android.widget.ImageView; import android.widget.ProgressBar; import android.widget.TextView; -import androidx.annotation.Nullable; - +import com.android.internal.annotations.VisibleForTesting; import com.android.internal.util.ContrastColorUtil; import com.android.internal.widget.NotificationActionListLayout; import com.android.systemui.Dependency; @@ -49,6 +51,8 @@ import com.android.systemui.statusbar.notification.TransformState; import com.android.systemui.statusbar.notification.row.ExpandableNotificationRow; import com.android.systemui.statusbar.notification.row.HybridNotificationView; +import java.util.function.Consumer; + /** * Wraps a notification view inflated from a template. */ @@ -66,9 +70,13 @@ public class NotificationTemplateViewWrapper extends NotificationHeaderViewWrapp private int mContentHeight; private int mMinHeightHint; + @Nullable private NotificationActionListLayout mActions; - private ArraySet<PendingIntent> mCancelledPendingIntents = new ArraySet<>(); - private UiOffloadThread mUiOffloadThread; + // Holds list of pending intents that have been cancelled by now - we only keep hash codes + // to avoid holding full binder proxies for intents that may have been removed by now. + @NonNull + @VisibleForTesting + final ArraySet<Integer> mCancelledPendingIntents = new ArraySet<>(); private View mRemoteInputHistory; private boolean mCanHideHeader; private float mHeaderTranslation; @@ -147,6 +155,7 @@ public class NotificationTemplateViewWrapper extends NotificationHeaderViewWrapp com.android.internal.R.dimen.notification_content_margin_top); } + @MainThread private void resolveTemplateViews(StatusBarNotification sbn) { mRightIcon = mView.findViewById(com.android.internal.R.id.right_icon); if (mRightIcon != null) { @@ -195,34 +204,57 @@ public class NotificationTemplateViewWrapper extends NotificationHeaderViewWrapp return getLargeIcon(n); } + @MainThread private void updatePendingIntentCancellations() { if (mActions != null) { int numActions = mActions.getChildCount(); + final ArraySet<Integer> currentlyActivePendingIntents = new ArraySet<>(numActions); for (int i = 0; i < numActions; i++) { Button action = (Button) mActions.getChildAt(i); - performOnPendingIntentCancellation(action, () -> { - if (action.isEnabled()) { - action.setEnabled(false); - // The visual appearance doesn't look disabled enough yet, let's add the - // alpha as well. Since Alpha doesn't play nicely right now with the - // transformation, we rather blend it manually with the background color. - ColorStateList textColors = action.getTextColors(); - int[] colors = textColors.getColors(); - int[] newColors = new int[colors.length]; - float disabledAlpha = mView.getResources().getFloat( - com.android.internal.R.dimen.notification_action_disabled_alpha); - for (int j = 0; j < colors.length; j++) { - int color = colors[j]; - color = blendColorWithBackground(color, disabledAlpha); - newColors[j] = color; - } - ColorStateList newColorStateList = new ColorStateList( - textColors.getStates(), newColors); - action.setTextColor(newColorStateList); + PendingIntent pendingIntent = getPendingIntentForAction(action); + // Check if passed intent has already been cancelled in this class and immediately + // disable the action to avoid temporary race with enable/disable. + if (pendingIntent != null) { + int pendingIntentHashCode = getHashCodeForPendingIntent(pendingIntent); + currentlyActivePendingIntents.add(pendingIntentHashCode); + if (mCancelledPendingIntents.contains(pendingIntentHashCode)) { + disableActionView(action); } - }); + } + updatePendingIntentCancellationListener(action, pendingIntent); + } + + // This cleanup ensures that the size of this set doesn't grow into unreasonable sizes. + // There are scenarios where applications updated notifications with different + // PendingIntents which could cause this Set to grow to 1000+ elements. + mCancelledPendingIntents.retainAll(currentlyActivePendingIntents); + } + } + + @MainThread + private void updatePendingIntentCancellationListener(Button action, + @Nullable PendingIntent pendingIntent) { + ActionPendingIntentCancellationHandler cancellationHandler = null; + if (pendingIntent != null) { + // Attach listeners to handle intent cancellation to this view. + cancellationHandler = new ActionPendingIntentCancellationHandler(pendingIntent, action, + this::disableActionViewWithIntent); + action.addOnAttachStateChangeListener(cancellationHandler); + // Immediately fire the event if the view is already attached to register + // pending intent cancellation listener. + if (action.isAttachedToWindow()) { + cancellationHandler.onViewAttachedToWindow(action); } } + + // If the view has an old attached listener, remove it to avoid leaking intents. + ActionPendingIntentCancellationHandler previousHandler = + (ActionPendingIntentCancellationHandler) action.getTag( + R.id.pending_intent_listener_tag); + if (previousHandler != null) { + previousHandler.remove(); + } + action.setTag(R.id.pending_intent_listener_tag, cancellationHandler); } private int blendColorWithBackground(int color, float alpha) { @@ -231,42 +263,6 @@ public class NotificationTemplateViewWrapper extends NotificationHeaderViewWrapp Color.red(color), Color.green(color), Color.blue(color)), resolveBackgroundColor()); } - private void performOnPendingIntentCancellation(View view, Runnable cancellationRunnable) { - PendingIntent pendingIntent = (PendingIntent) view.getTag( - com.android.internal.R.id.pending_intent_tag); - if (pendingIntent == null) { - return; - } - if (mCancelledPendingIntents.contains(pendingIntent)) { - cancellationRunnable.run(); - } else { - PendingIntent.CancelListener listener = (PendingIntent intent) -> { - mView.post(() -> { - mCancelledPendingIntents.add(pendingIntent); - cancellationRunnable.run(); - }); - }; - if (mUiOffloadThread == null) { - mUiOffloadThread = Dependency.get(UiOffloadThread.class); - } - if (view.isAttachedToWindow()) { - mUiOffloadThread.execute(() -> pendingIntent.registerCancelListener(listener)); - } - view.addOnAttachStateChangeListener(new View.OnAttachStateChangeListener() { - @Override - public void onViewAttachedToWindow(View v) { - mUiOffloadThread.execute(() -> pendingIntent.registerCancelListener(listener)); - } - - @Override - public void onViewDetachedFromWindow(View v) { - mUiOffloadThread.execute( - () -> pendingIntent.unregisterCancelListener(listener)); - } - }); - } - } - @Override public void onContentUpdated(ExpandableNotificationRow row) { // Reinspect the notification. Before the super call, because the super call also updates @@ -364,4 +360,141 @@ public class NotificationTemplateViewWrapper extends NotificationHeaderViewWrapp } return extra + super.getExtraMeasureHeight(); } + + /** + * This finds Action view with a given intent and disables it. + * With maximum of 3 views, this is sufficiently fast to iterate on main thread every time. + */ + @MainThread + private void disableActionViewWithIntent(PendingIntent intent) { + mCancelledPendingIntents.add(getHashCodeForPendingIntent(intent)); + if (mActions != null) { + int numActions = mActions.getChildCount(); + for (int i = 0; i < numActions; i++) { + Button action = (Button) mActions.getChildAt(i); + PendingIntent pendingIntent = getPendingIntentForAction(action); + if (intent.equals(pendingIntent)) { + disableActionView(action); + } + } + } + } + + /** + * Disables Action view when, e.g., its PendingIntent is disabled. + */ + @MainThread + private void disableActionView(Button action) { + if (action.isEnabled()) { + action.setEnabled(false); + // The visual appearance doesn't look disabled enough yet, let's add the + // alpha as well. Since Alpha doesn't play nicely right now with the + // transformation, we rather blend it manually with the background color. + ColorStateList textColors = action.getTextColors(); + int[] colors = textColors.getColors(); + int[] newColors = new int[colors.length]; + float disabledAlpha = mView.getResources().getFloat( + com.android.internal.R.dimen.notification_action_disabled_alpha); + for (int j = 0; j < colors.length; j++) { + int color = colors[j]; + color = blendColorWithBackground(color, disabledAlpha); + newColors[j] = color; + } + ColorStateList newColorStateList = new ColorStateList( + textColors.getStates(), newColors); + action.setTextColor(newColorStateList); + } + } + + /** + * Returns the hashcode of underlying target of PendingIntent. We can get multiple + * Java PendingIntent wrapper objects pointing to the same cancelled PI in system_server. + * This makes sure we treat them equally. + */ + private static int getHashCodeForPendingIntent(PendingIntent pendingIntent) { + return System.identityHashCode(pendingIntent.getTarget().asBinder()); + } + + /** + * Returns PendingIntent contained in the action tag. May be null. + */ + @Nullable + private static PendingIntent getPendingIntentForAction(View action) { + return (PendingIntent) action.getTag(com.android.internal.R.id.pending_intent_tag); + } + + /** + * Registers listeners for pending intent cancellation when Action views are attached + * to window. + * It calls onCancelPendingIntentForActionView when a PendingIntent is cancelled. + */ + @VisibleForTesting + static final class ActionPendingIntentCancellationHandler + implements View.OnAttachStateChangeListener { + + @Nullable + private static UiOffloadThread sUiOffloadThread = null; + + @NonNull + private static UiOffloadThread getUiOffloadThread() { + if (sUiOffloadThread == null) { + sUiOffloadThread = Dependency.get(UiOffloadThread.class); + } + return sUiOffloadThread; + } + + private final View mView; + private final Consumer<PendingIntent> mOnCancelledCallback; + + private final PendingIntent mPendingIntent; + + ActionPendingIntentCancellationHandler(PendingIntent pendingIntent, View actionView, + Consumer<PendingIntent> onCancelled) { + this.mPendingIntent = pendingIntent; + this.mView = actionView; + this.mOnCancelledCallback = onCancelled; + } + + private final PendingIntent.CancelListener mCancelListener = + new PendingIntent.CancelListener() { + @Override + public void onCanceled(PendingIntent pendingIntent) { + mView.post(() -> { + mOnCancelledCallback.accept(pendingIntent); + // We don't need this listener anymore once the intent was cancelled. + remove(); + }); + } + }; + + @MainThread + @Override + public void onViewAttachedToWindow(View view) { + // This is safe to call multiple times with the same listener instance. + getUiOffloadThread().execute(() -> { + mPendingIntent.registerCancelListener(mCancelListener); + }); + } + + @MainThread + @Override + public void onViewDetachedFromWindow(View view) { + // This is safe to call multiple times with the same listener instance. + getUiOffloadThread().execute(() -> + mPendingIntent.unregisterCancelListener(mCancelListener)); + } + + /** + * Removes this listener from callbacks and releases the held PendingIntent. + */ + @MainThread + public void remove() { + mView.removeOnAttachStateChangeListener(this); + if (mView.getTag(R.id.pending_intent_listener_tag) == this) { + mView.setTag(R.id.pending_intent_listener_tag, null); + } + getUiOffloadThread().execute(() -> + mPendingIntent.unregisterCancelListener(mCancelListener)); + } + } } diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/row/wrapper/NotificationTemplateViewWrapperTest.kt b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/row/wrapper/NotificationTemplateViewWrapperTest.kt new file mode 100644 index 000000000000..8c3bfd55ecf1 --- /dev/null +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/row/wrapper/NotificationTemplateViewWrapperTest.kt @@ -0,0 +1,254 @@ +/* + * Copyright (C) 2023 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.systemui.statusbar.notification.row.wrapper + +import android.app.PendingIntent +import android.app.PendingIntent.CancelListener +import android.content.Intent +import android.testing.AndroidTestingRunner +import android.testing.TestableLooper +import android.testing.TestableLooper.RunWithLooper +import android.testing.ViewUtils +import android.view.LayoutInflater +import android.view.View +import android.view.ViewGroup +import android.widget.FrameLayout +import androidx.test.filters.SmallTest +import com.android.internal.R +import com.android.systemui.SysuiTestCase +import com.android.systemui.statusbar.notification.row.ExpandableNotificationRow +import com.android.systemui.statusbar.notification.row.NotificationTestHelper +import com.android.systemui.statusbar.notification.row.wrapper.NotificationTemplateViewWrapper.ActionPendingIntentCancellationHandler +import com.google.common.truth.Truth.assertThat +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.mockito.ArgumentCaptor +import org.mockito.Mockito +import org.mockito.Mockito.times +import org.mockito.Mockito.verify + +@SmallTest +@RunWith(AndroidTestingRunner::class) +@RunWithLooper +class NotificationTemplateViewWrapperTest : SysuiTestCase() { + + private lateinit var helper: NotificationTestHelper + + private lateinit var root: ViewGroup + private lateinit var view: ViewGroup + private lateinit var row: ExpandableNotificationRow + private lateinit var actions: ViewGroup + + private lateinit var looper: TestableLooper + + @Before + fun setUp() { + looper = TestableLooper.get(this) + allowTestableLooperAsMainThread() + helper = NotificationTestHelper(mContext, mDependency, looper) + row = helper.createRow() + // Some code in the view iterates through parents so we need some extra containers around + // it. + root = FrameLayout(mContext) + val root2 = FrameLayout(mContext) + root.addView(root2) + view = + (LayoutInflater.from(mContext) + .inflate(R.layout.notification_template_material_big_text, root2) as ViewGroup) + actions = view.findViewById(R.id.actions)!! + ViewUtils.attachView(root) + } + + @Test + fun noActionsPresent_noCrash() { + view.removeView(actions) + val wrapper = NotificationTemplateViewWrapper(mContext, view, row) + wrapper.onContentUpdated(row) + } + + @Test + fun actionPendingIntentCancelled_actionDisabled() { + val wrapper = NotificationTemplateViewWrapper(mContext, view, row) + val action1 = createActionWithPendingIntent() + val action2 = createActionWithPendingIntent() + val action3 = createActionWithPendingIntent() + wrapper.onContentUpdated(row) + waitForUiOffloadThread() // Wait for cancellation registration to execute. + + val pi3 = getPendingIntent(action3) + pi3.cancel() + looper.processAllMessages() // Wait for listener callbacks to execute + + assertThat(action1.isEnabled).isTrue() + assertThat(action2.isEnabled).isTrue() + assertThat(action3.isEnabled).isFalse() + assertThat(wrapper.mCancelledPendingIntents) + .doesNotContain(getPendingIntent(action1).hashCode()) + assertThat(wrapper.mCancelledPendingIntents) + .doesNotContain(getPendingIntent(action2).hashCode()) + assertThat(wrapper.mCancelledPendingIntents).contains(pi3.hashCode()) + } + + @Test + fun newActionWithSamePendingIntentPosted_actionDisabled() { + val wrapper = NotificationTemplateViewWrapper(mContext, view, row) + val action = createActionWithPendingIntent() + wrapper.onContentUpdated(row) + waitForUiOffloadThread() // Wait for cancellation registration to execute. + + // Cancel the intent and check action is now false. + val pi = getPendingIntent(action) + pi.cancel() + looper.processAllMessages() // Wait for listener callbacks to execute + assertThat(action.isEnabled).isFalse() + + // Create a NEW action and make sure that one will also be cancelled with same PI. + actions.removeView(action) + val newAction = createActionWithPendingIntent(pi) + wrapper.onContentUpdated(row) + looper.processAllMessages() // Wait for listener callbacks to execute + + assertThat(newAction.isEnabled).isFalse() + assertThat(wrapper.mCancelledPendingIntents).containsExactly(pi.hashCode()) + } + + @Test + fun twoActionsWithSameCancelledIntent_bothActionsDisabled() { + val wrapper = NotificationTemplateViewWrapper(mContext, view, row) + val action1 = createActionWithPendingIntent() + val action2 = createActionWithPendingIntent() + val action3 = createActionWithPendingIntent(getPendingIntent(action2)) + wrapper.onContentUpdated(row) + waitForUiOffloadThread() // Wait for cancellation registration to execute. + + val pi = getPendingIntent(action2) + pi.cancel() + looper.processAllMessages() // Wait for listener callbacks to execute + + assertThat(action1.isEnabled).isTrue() + assertThat(action2.isEnabled).isFalse() + assertThat(action3.isEnabled).isFalse() + } + + @Test + fun actionPendingIntentCancelled_whileDetached_actionDisabled() { + ViewUtils.detachView(root) + val wrapper = NotificationTemplateViewWrapper(mContext, view, row) + val action = createActionWithPendingIntent() + wrapper.onContentUpdated(row) + getPendingIntent(action).cancel() + ViewUtils.attachView(root) + waitForUiOffloadThread() + looper.processAllMessages() + + assertThat(action.isEnabled).isFalse() + } + + @Test + fun actionViewDetached_pendingIntentListenersDeregistered() { + val pi = + PendingIntent.getActivity( + mContext, + System.currentTimeMillis().toInt(), + Intent(Intent.ACTION_VIEW), + PendingIntent.FLAG_IMMUTABLE + ) + val spy = Mockito.spy(pi) + createActionWithPendingIntent(spy) + val wrapper = NotificationTemplateViewWrapper(mContext, view, row) + wrapper.onContentUpdated(row) + ViewUtils.detachView(root) + waitForUiOffloadThread() + looper.processAllMessages() + + val captor = ArgumentCaptor.forClass(CancelListener::class.java) + verify(spy, times(1)).registerCancelListener(captor.capture()) + verify(spy, times(1)).unregisterCancelListener(captor.value) + } + + @Test + fun actionViewUpdated_oldPendingIntentListenersRemoved() { + val pi = + PendingIntent.getActivity( + mContext, + System.currentTimeMillis().toInt(), + Intent(Intent.ACTION_VIEW), + PendingIntent.FLAG_IMMUTABLE + ) + val spy = Mockito.spy(pi) + val action = createActionWithPendingIntent(spy) + val wrapper = NotificationTemplateViewWrapper(mContext, view, row) + wrapper.onContentUpdated(row) + waitForUiOffloadThread() + looper.processAllMessages() + + // Grab set attach listener + val attachListener = + Mockito.spy(action.getTag(com.android.systemui.res.R.id.pending_intent_listener_tag)) + as ActionPendingIntentCancellationHandler + action.setTag(com.android.systemui.res.R.id.pending_intent_listener_tag, attachListener) + + // Update pending intent in the existing action + val newPi = + PendingIntent.getActivity( + mContext, + System.currentTimeMillis().toInt(), + Intent(Intent.ACTION_ALARM_CHANGED), + PendingIntent.FLAG_IMMUTABLE + ) + action.setTagInternal(R.id.pending_intent_tag, newPi) + wrapper.onContentUpdated(row) + waitForUiOffloadThread() + looper.processAllMessages() + + // Listeners for original pending intent need to be cleaned up now. + val captor = ArgumentCaptor.forClass(CancelListener::class.java) + verify(spy, times(1)).registerCancelListener(captor.capture()) + verify(spy, times(1)).unregisterCancelListener(captor.value) + // Attach listener has to be replaced with a new one. + assertThat(action.getTag(com.android.systemui.res.R.id.pending_intent_listener_tag)) + .isNotEqualTo(attachListener) + verify(attachListener).remove() + } + + private fun createActionWithPendingIntent(): View { + val pi = + PendingIntent.getActivity( + mContext, + System.currentTimeMillis().toInt(), + Intent(Intent.ACTION_VIEW), + PendingIntent.FLAG_IMMUTABLE + ) + return createActionWithPendingIntent(pi) + } + + private fun createActionWithPendingIntent(pi: PendingIntent): View { + val view = + LayoutInflater.from(mContext) + .inflate(R.layout.notification_material_action, null, false) + view.setTagInternal(R.id.pending_intent_tag, pi) + actions.addView(view) + return view + } + + private fun getPendingIntent(action: View): PendingIntent { + val pendingIntent = action.getTag(R.id.pending_intent_tag) as PendingIntent + assertThat(pendingIntent).isNotNull() + return pendingIntent + } +} |