From 2cfc95bf2d73ac4b23e5f4a2caac169639c4cd30 Mon Sep 17 00:00:00 2001 From: Julia Tuttle Date: Tue, 11 Oct 2022 18:05:32 -0400 Subject: [PATCH] HeadsUpViewBinder: Don't WTF if notif stops alerting after cleanup. Right now, a notification can go from heads up to not after it's removed from the NotifCollection and cleaned up, and HeadsUpCoordinator will happily ask HeadsUpViewVinder to unbind the heads up view, but then RowContentBindStage will complain that there are no bind stage params for the notification. Long-term, we'd like to improve the clarity of the interactions between a notification's lifetime in NotifCollection and in HeadsUpManager, but short-term, this change keeps HeadsUpViewBinder from triggering a WTF when a notification stops alerting after it was cleaned up. Bug: 245537746 Test: atest HeadsUpViewBinderTest Change-Id: I06a6a74c9946398cc4ed22544e5d205f6d74bd6a --- .../interruption/HeadsUpViewBinder.java | 13 +++- .../interruption/HeadsUpViewBinderLogger.kt | 8 +++ .../statusbar/notification/row/BindStage.java | 14 +++- .../interruption/HeadsUpViewBinderTest.java | 30 +++++++++ .../row/RowContentBindStageTest.java | 66 +++++++++++++++++++ 5 files changed, 129 insertions(+), 2 deletions(-) diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/interruption/HeadsUpViewBinder.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/interruption/HeadsUpViewBinder.java index 6f41425b506d2..9a7610ddd3540 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/interruption/HeadsUpViewBinder.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/interruption/HeadsUpViewBinder.java @@ -114,7 +114,18 @@ public class HeadsUpViewBinder { */ public void unbindHeadsUpView(NotificationEntry entry) { abortBindCallback(entry); - mStage.getStageParams(entry).markContentViewsFreeable(FLAG_CONTENT_VIEW_HEADS_UP); + + // params may be null if the notification was already removed from the collection but we let + // it stick around during a launch animation. In this case, the heads up view has already + // been unbound, so we don't need to unbind it. + // TODO(b/253081345): Change this back to getStageParams and remove null check. + RowContentBindParams params = mStage.tryGetStageParams(entry); + if (params == null) { + mLogger.entryBindStageParamsNullOnUnbind(entry); + return; + } + + params.markContentViewsFreeable(FLAG_CONTENT_VIEW_HEADS_UP); mLogger.entryContentViewMarkedFreeable(entry); mStage.requestRebind(entry, e -> mLogger.entryUnbound(e)); } diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/interruption/HeadsUpViewBinderLogger.kt b/packages/SystemUI/src/com/android/systemui/statusbar/notification/interruption/HeadsUpViewBinderLogger.kt index d1feaa05c653b..5dbec8dcba20f 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/interruption/HeadsUpViewBinderLogger.kt +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/interruption/HeadsUpViewBinderLogger.kt @@ -47,6 +47,14 @@ class HeadsUpViewBinderLogger @Inject constructor(@NotificationHeadsUpLog val bu "start unbinding heads up entry $str1 " }) } + + fun entryBindStageParamsNullOnUnbind(entry: NotificationEntry) { + buffer.log(TAG, INFO, { + str1 = entry.logKey + }, { + "heads up entry bind stage params null on unbind $str1 " + }) + } } private const val TAG = "HeadsUpViewBinder" diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/row/BindStage.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/row/BindStage.java index 7c41800d880dd..d626c18e46f56 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/row/BindStage.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/row/BindStage.java @@ -21,6 +21,7 @@ import android.util.ArrayMap; import android.util.Log; import androidx.annotation.NonNull; +import androidx.annotation.Nullable; import com.android.systemui.statusbar.notification.collection.NotificationEntry; @@ -64,7 +65,7 @@ public abstract class BindStage extends BindRequester { * Get the stage parameters for the entry. Clients should use this to modify how the stage * handles the notification content. */ - public final Params getStageParams(@NonNull NotificationEntry entry) { + public final @NonNull Params getStageParams(@NonNull NotificationEntry entry) { Params params = mContentParams.get(entry); if (params == null) { // TODO: This should throw an exception but there are some cases of re-entrant calls @@ -79,6 +80,17 @@ public abstract class BindStage extends BindRequester { return params; } + // TODO(b/253081345): Remove this method. + /** + * Get the stage parameters for the entry, or null if there are no stage parameters for the + * entry. + * + * @see #getStageParams(NotificationEntry) + */ + public final @Nullable Params tryGetStageParams(@NonNull NotificationEntry entry) { + return mContentParams.get(entry); + } + /** * Create a params entry for the notification for this stage. */ diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/interruption/HeadsUpViewBinderTest.java b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/interruption/HeadsUpViewBinderTest.java index 3f641df376edc..ca6598726a85c 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/interruption/HeadsUpViewBinderTest.java +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/interruption/HeadsUpViewBinderTest.java @@ -91,6 +91,8 @@ public class HeadsUpViewBinderTest extends SysuiTestCase { verifyNoMoreInteractions(mLogger); clearInvocations(mLogger); + when(mBindStage.tryGetStageParams(eq(mEntry))).thenReturn(new RowContentBindParams()); + mViewBinder.unbindHeadsUpView(mEntry); verify(mLogger).entryContentViewMarkedFreeable(eq(mEntry)); verifyNoMoreInteractions(mLogger); @@ -139,6 +141,8 @@ public class HeadsUpViewBinderTest extends SysuiTestCase { verifyNoMoreInteractions(mLogger); clearInvocations(mLogger); + when(mBindStage.tryGetStageParams(eq(mEntry))).thenReturn(new RowContentBindParams()); + mViewBinder.unbindHeadsUpView(mEntry); verify(mLogger).currentOngoingBindingAborted(eq(mEntry)); verify(mLogger).entryContentViewMarkedFreeable(eq(mEntry)); @@ -150,4 +154,30 @@ public class HeadsUpViewBinderTest extends SysuiTestCase { verifyNoMoreInteractions(mLogger); clearInvocations(mLogger); } + + @Test + public void testLoggingForLateUnbindFlow() { + AtomicReference callback = new AtomicReference<>(); + when(mBindStage.requestRebind(any(), any())).then(i -> { + callback.set(i.getArgument(1)); + return new CancellationSignal(); + }); + + mViewBinder.bindHeadsUpView(mEntry, null); + verify(mLogger).startBindingHun(eq(mEntry)); + verifyNoMoreInteractions(mLogger); + clearInvocations(mLogger); + + callback.get().onBindFinished(mEntry); + verify(mLogger).entryBoundSuccessfully(eq(mEntry)); + verifyNoMoreInteractions(mLogger); + clearInvocations(mLogger); + + when(mBindStage.tryGetStageParams(eq(mEntry))).thenReturn(null); + + mViewBinder.unbindHeadsUpView(mEntry); + verify(mLogger).entryBindStageParamsNullOnUnbind(eq(mEntry)); + verifyNoMoreInteractions(mLogger); + clearInvocations(mLogger); + } } diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/row/RowContentBindStageTest.java b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/row/RowContentBindStageTest.java index ad3bd711c23f1..7c99568ee75fc 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/row/RowContentBindStageTest.java +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/row/RowContentBindStageTest.java @@ -21,6 +21,10 @@ import static com.android.systemui.statusbar.notification.row.NotificationRowCon import static com.android.systemui.statusbar.notification.row.NotificationRowContentBinder.FLAG_CONTENT_VIEW_EXPANDED; import static com.android.systemui.statusbar.notification.row.NotificationRowContentBinder.FLAG_CONTENT_VIEW_HEADS_UP; +import static junit.framework.Assert.assertEquals; +import static junit.framework.Assert.assertNotNull; +import static junit.framework.Assert.assertNotSame; +import static junit.framework.Assert.assertNull; import static junit.framework.Assert.assertTrue; import static org.mockito.ArgumentMatchers.any; @@ -31,6 +35,7 @@ import static org.mockito.Mockito.verify; import android.testing.AndroidTestingRunner; import android.testing.TestableLooper; +import android.util.Log; import androidx.test.filters.SmallTest; @@ -100,6 +105,67 @@ public class RowContentBindStageTest extends SysuiTestCase { verify(mBinder).unbindContent(eq(mEntry), any(), eq(flags)); } + class CountingWtfHandler implements Log.TerribleFailureHandler { + private Log.TerribleFailureHandler mOldHandler = null; + private int mWtfCount = 0; + + public void register() { + mOldHandler = Log.setWtfHandler(this); + } + + public void unregister() { + Log.setWtfHandler(mOldHandler); + mOldHandler = null; + } + + @Override + public void onTerribleFailure(String tag, Log.TerribleFailure what, boolean system) { + mWtfCount++; + } + + public int getWtfCount() { + return mWtfCount; + } + } + + @Test + public void testGetStageParamsAfterCleanUp() { + // GIVEN an entry whose params have already been deleted. + RowContentBindParams originalParams = mRowContentBindStage.getStageParams(mEntry); + mRowContentBindStage.deleteStageParams(mEntry); + + // WHEN a caller calls getStageParams. + CountingWtfHandler countingWtfHandler = new CountingWtfHandler(); + countingWtfHandler.register(); + + RowContentBindParams blankParams = mRowContentBindStage.getStageParams(mEntry); + + countingWtfHandler.unregister(); + + // THEN getStageParams logs a WTF and returns blank params created to avoid a crash. + assertEquals(1, countingWtfHandler.getWtfCount()); + assertNotNull(blankParams); + assertNotSame(originalParams, blankParams); + } + + @Test + public void testTryGetStageParamsAfterCleanUp() { + // GIVEN an entry whose params have already been deleted. + mRowContentBindStage.deleteStageParams(mEntry); + + // WHEN a caller calls getStageParams. + CountingWtfHandler countingWtfHandler = new CountingWtfHandler(); + countingWtfHandler.register(); + + RowContentBindParams nullParams = mRowContentBindStage.tryGetStageParams(mEntry); + + countingWtfHandler.unregister(); + + // THEN getStageParams does NOT log a WTF and returns null to indicate missing params. + assertEquals(0, countingWtfHandler.getWtfCount()); + assertNull(nullParams); + } + @Test public void testRebindAllContentViews() { // GIVEN a view with content bound.