From 22740d0f9f3ed963b598de80a4f083fa48169130 Mon Sep 17 00:00:00 2001 From: Julia Tuttle Date: Tue, 2 Aug 2022 16:12:42 -0400 Subject: [PATCH] New Pipeline: begrudgingly tolerate a few reentrant runs Ideally, later pipeline components would never invalidate earlier ones, and as such, we fatally assert that invalidations aren't going "backwards" to earlier stages. Unfortunately, we have at least one bug left that trips that assertion. t's rare (I think you have to get a notification with remote input, activate the remote input from the lockscreen, get the keyguard prompt to unlock your device, and then dismiss (or have the app dismiss?) the notification while still on the keyguard screen), but it still happens. Therefore, modify ShadeListBuilder to complain loudly (Log.e) on reentrant runs, but to tolerate a few (currently three) in a row without crashing. More than that will crash as before, and the count will reset on any non-reentrant run. Bug: 231388160 Test: atest ShadeListBuilderTest # new test cases added Change-Id: If944b9c1f13206a9ccba7a5b22fc4d0f96dc1eae --- .../collection/ShadeListBuilder.java | 67 +++++- .../collection/ShadeListBuilderTest.java | 206 +++++++++++++++--- 2 files changed, 239 insertions(+), 34 deletions(-) diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/ShadeListBuilder.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/ShadeListBuilder.java index 075a0dc7555e5..6ba711ee5a260 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/ShadeListBuilder.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/ShadeListBuilder.java @@ -38,6 +38,7 @@ import android.os.Trace; import android.service.notification.StatusBarNotification; import android.util.ArrayMap; import android.util.ArraySet; +import android.util.Log; import androidx.annotation.NonNull; import androidx.annotation.VisibleForTesting; @@ -126,6 +127,9 @@ public class ShadeListBuilder implements Dumpable { private List mReadOnlyNewNotifList = Collections.unmodifiableList(mNewNotifList); private final NotifPipelineChoreographer mChoreographer; + private int mConsecutiveReentrantRebuilds = 0; + @VisibleForTesting public static final int MAX_CONSECUTIVE_REENTRANT_REBUILDS = 3; + @Inject public ShadeListBuilder( DumpManager dumpManager, @@ -310,7 +314,7 @@ public class ShadeListBuilder implements Dumpable { mLogger.logOnBuildList(reason); mAllEntries = entries; - mChoreographer.schedule(); + scheduleRebuild(/* reentrant = */ false); } }; @@ -1332,11 +1336,64 @@ public class ShadeListBuilder implements Dumpable { throw new RuntimeException("Missing default sectioner!"); } - private void rebuildListIfBefore(@PipelineState.StateName int state) { - mPipelineState.requireIsBefore(state); - if (mPipelineState.is(STATE_IDLE)) { - mChoreographer.schedule(); + private void rebuildListIfBefore(@PipelineState.StateName int rebuildState) { + final @PipelineState.StateName int currentState = mPipelineState.getState(); + + // If the pipeline is idle, requesting an invalidation is always okay, and starts a new run. + if (currentState == STATE_IDLE) { + scheduleRebuild(/* reentrant = */ false, rebuildState); + return; } + + // If the pipeline is running, it is okay to request an invalidation of a *later* stage. + // Since the current pipeline run hasn't run it yet, no new pipeline run is needed. + if (rebuildState > currentState) { + return; + } + + // If the pipeline is running, it is bad to request an invalidation of *earlier* stages or + // the *current* stage; this will run the pipeline more often than needed, and may even + // cause an infinite loop of pipeline runs. + // + // Unfortunately, there are some unfixed bugs that cause reentrant pipeline runs, so we keep + // a counter and allow a few reentrant runs in a row between any two non-reentrant runs. + // + // It is technically possible for a *pair* of invalidations, one reentrant and one not, to + // trigger *each other*, alternating responsibility for pipeline runs in an infinite loop + // but constantly resetting the reentrant run counter. Hopefully that doesn't happen. + scheduleRebuild(/* reentrant = */ true, rebuildState); + } + + private void scheduleRebuild(boolean reentrant) { + scheduleRebuild(reentrant, STATE_IDLE); + } + + private void scheduleRebuild(boolean reentrant, @PipelineState.StateName int rebuildState) { + if (!reentrant) { + mConsecutiveReentrantRebuilds = 0; + mChoreographer.schedule(); + return; + } + + final @PipelineState.StateName int currentState = mPipelineState.getState(); + + final String rebuildStateName = PipelineState.getStateName(rebuildState); + final String currentStateName = PipelineState.getStateName(currentState); + final IllegalStateException exception = new IllegalStateException( + "Reentrant notification pipeline rebuild of state " + rebuildStateName + + " while pipeline in state " + currentStateName + "."); + + mConsecutiveReentrantRebuilds++; + + if (mConsecutiveReentrantRebuilds > MAX_CONSECUTIVE_REENTRANT_REBUILDS) { + Log.e(TAG, "Crashing after more than " + MAX_CONSECUTIVE_REENTRANT_REBUILDS + + " consecutive reentrant notification pipeline rebuilds.", exception); + throw exception; + } + + Log.e(TAG, "Allowing " + mConsecutiveReentrantRebuilds + + " consecutive reentrant notification pipeline rebuild(s).", exception); + mChoreographer.schedule(); } private static int countChildren(List entries) { diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/ShadeListBuilderTest.java b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/ShadeListBuilderTest.java index dfa38abc1ff89..9f214097ea552 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/ShadeListBuilderTest.java +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/ShadeListBuilderTest.java @@ -68,6 +68,7 @@ import com.android.systemui.statusbar.notification.collection.listbuilder.plugga import com.android.systemui.statusbar.notification.collection.listbuilder.pluggable.NotifPromoter; import com.android.systemui.statusbar.notification.collection.listbuilder.pluggable.NotifSectioner; import com.android.systemui.statusbar.notification.collection.listbuilder.pluggable.NotifStabilityManager; +import com.android.systemui.statusbar.notification.collection.listbuilder.pluggable.Pluggable; import com.android.systemui.statusbar.notification.collection.notifcollection.CollectionReadyForBuildListener; import com.android.systemui.util.time.FakeSystemClock; @@ -1715,66 +1716,201 @@ public class ShadeListBuilderTest extends SysuiTestCase { assertEquals(GroupEntry.ROOT_ENTRY, group.getPreviousParent()); } - @Test(expected = IllegalStateException.class) - public void testOutOfOrderPreGroupFilterInvalidationThrows() { - // GIVEN a PreGroupNotifFilter that gets invalidated during the grouping stage - NotifFilter filter = new PackageFilter(PACKAGE_5); - OnBeforeTransformGroupsListener listener = (list) -> filter.invalidateList(null); + static class CountingInvalidator { + CountingInvalidator(Pluggable pluggableToInvalidate) { + mPluggableToInvalidate = pluggableToInvalidate; + mInvalidationCount = 0; + } + + public void setInvalidationCount(int invalidationCount) { + mInvalidationCount = invalidationCount; + } + + public void maybeInvalidate() { + if (mInvalidationCount > 0) { + mPluggableToInvalidate.invalidateList("test invalidation"); + mInvalidationCount--; + } + } + + private Pluggable mPluggableToInvalidate; + private int mInvalidationCount; + + private static final String TAG = "ShadeListBuilderTestCountingInvalidator"; + } + + @Test + public void testOutOfOrderPreGroupFilterInvalidationDoesNotThrowBeforeTooManyRuns() { + // GIVEN a PreGroupNotifFilter that gets invalidated during the grouping stage, + NotifFilter filter = new PackageFilter(PACKAGE_1); + CountingInvalidator invalidator = new CountingInvalidator(filter); + OnBeforeTransformGroupsListener listener = (list) -> invalidator.maybeInvalidate(); mListBuilder.addPreGroupFilter(filter); mListBuilder.addOnBeforeTransformGroupsListener(listener); - // WHEN we try to run the pipeline and the filter is invalidated - addNotif(0, PACKAGE_1); + // WHEN we try to run the pipeline and the filter is invalidated exactly + // MAX_CONSECUTIVE_REENTRANT_REBUILDS times, + addNotif(0, PACKAGE_2); + invalidator.setInvalidationCount(ShadeListBuilder.MAX_CONSECUTIVE_REENTRANT_REBUILDS); dispatchBuild(); + runWhileScheduledUpTo(ShadeListBuilder.MAX_CONSECUTIVE_REENTRANT_REBUILDS + 2); - // THEN an exception is thrown + // THEN an exception is NOT thrown. } @Test(expected = IllegalStateException.class) - public void testOutOfOrderPrompterInvalidationThrows() { - // GIVEN a NotifPromoter that gets invalidated during the sorting stage + public void testOutOfOrderPreGroupFilterInvalidationThrowsAfterTooManyRuns() { + // GIVEN a PreGroupNotifFilter that gets invalidated during the grouping stage, + NotifFilter filter = new PackageFilter(PACKAGE_1); + CountingInvalidator invalidator = new CountingInvalidator(filter); + OnBeforeTransformGroupsListener listener = (list) -> invalidator.maybeInvalidate(); + mListBuilder.addPreGroupFilter(filter); + mListBuilder.addOnBeforeTransformGroupsListener(listener); + + // WHEN we try to run the pipeline and the filter is invalidated more than + // MAX_CONSECUTIVE_REENTRANT_REBUILDS times, + addNotif(0, PACKAGE_2); + invalidator.setInvalidationCount(ShadeListBuilder.MAX_CONSECUTIVE_REENTRANT_REBUILDS + 1); + dispatchBuild(); + runWhileScheduledUpTo(ShadeListBuilder.MAX_CONSECUTIVE_REENTRANT_REBUILDS + 2); + + // THEN an exception IS thrown. + } + + @Test + public void testNonConsecutiveOutOfOrderInvalidationDontThrowAfterTooManyRuns() { + // GIVEN a PreGroupNotifFilter that gets invalidated during the grouping stage, + NotifFilter filter = new PackageFilter(PACKAGE_1); + CountingInvalidator invalidator = new CountingInvalidator(filter); + OnBeforeTransformGroupsListener listener = (list) -> invalidator.maybeInvalidate(); + mListBuilder.addPreGroupFilter(filter); + mListBuilder.addOnBeforeTransformGroupsListener(listener); + + // WHEN we try to run the pipeline and the filter is invalidated at least + // MAX_CONSECUTIVE_REENTRANT_REBUILDS times, + addNotif(0, PACKAGE_2); + invalidator.setInvalidationCount(ShadeListBuilder.MAX_CONSECUTIVE_REENTRANT_REBUILDS); + dispatchBuild(); + runWhileScheduledUpTo(ShadeListBuilder.MAX_CONSECUTIVE_REENTRANT_REBUILDS + 2); + invalidator.setInvalidationCount(ShadeListBuilder.MAX_CONSECUTIVE_REENTRANT_REBUILDS); + dispatchBuild(); + runWhileScheduledUpTo(ShadeListBuilder.MAX_CONSECUTIVE_REENTRANT_REBUILDS + 2); + + // THEN an exception is NOT thrown. + } + + @Test + public void testOutOfOrderPrompterInvalidationDoesNotThrowBeforeTooManyRuns() { + // GIVEN a NotifPromoter that gets invalidated during the sorting stage, NotifPromoter promoter = new IdPromoter(47); - OnBeforeSortListener listener = - (list) -> promoter.invalidateList(null); + CountingInvalidator invalidator = new CountingInvalidator(promoter); + OnBeforeSortListener listener = (list) -> invalidator.maybeInvalidate(); mListBuilder.addPromoter(promoter); mListBuilder.addOnBeforeSortListener(listener); - // WHEN we try to run the pipeline and the promoter is invalidated + // WHEN we try to run the pipeline and the promoter is invalidated exactly + // MAX_CONSECUTIVE_REENTRANT_REBUILDS times, addNotif(0, PACKAGE_1); + invalidator.setInvalidationCount(ShadeListBuilder.MAX_CONSECUTIVE_REENTRANT_REBUILDS); dispatchBuild(); + runWhileScheduledUpTo(ShadeListBuilder.MAX_CONSECUTIVE_REENTRANT_REBUILDS + 2); - // THEN an exception is thrown + // THEN an exception is NOT thrown. } @Test(expected = IllegalStateException.class) - public void testOutOfOrderComparatorInvalidationThrows() { - // GIVEN a NotifComparator that gets invalidated during the finalizing stage - NotifComparator comparator = new HypeComparator(PACKAGE_5); - OnBeforeRenderListListener listener = - (list) -> comparator.invalidateList(null); + public void testOutOfOrderPrompterInvalidationThrowsAfterTooManyRuns() { + // GIVEN a NotifPromoter that gets invalidated during the sorting stage, + NotifPromoter promoter = new IdPromoter(47); + CountingInvalidator invalidator = new CountingInvalidator(promoter); + OnBeforeSortListener listener = (list) -> invalidator.maybeInvalidate(); + mListBuilder.addPromoter(promoter); + mListBuilder.addOnBeforeSortListener(listener); + + // WHEN we try to run the pipeline and the promoter is invalidated more than + // MAX_CONSECUTIVE_REENTRANT_REBUILDS times, + addNotif(0, PACKAGE_1); + invalidator.setInvalidationCount(ShadeListBuilder.MAX_CONSECUTIVE_REENTRANT_REBUILDS + 1); + dispatchBuild(); + runWhileScheduledUpTo(ShadeListBuilder.MAX_CONSECUTIVE_REENTRANT_REBUILDS + 2); + + // THEN an exception IS thrown. + } + + @Test + public void testOutOfOrderComparatorInvalidationDoesNotThrowBeforeTooManyRuns() { + // GIVEN a NotifComparator that gets invalidated during the finalizing stage, + NotifComparator comparator = new HypeComparator(PACKAGE_1); + CountingInvalidator invalidator = new CountingInvalidator(comparator); + OnBeforeRenderListListener listener = (list) -> invalidator.maybeInvalidate(); mListBuilder.setComparators(singletonList(comparator)); mListBuilder.addOnBeforeRenderListListener(listener); - // WHEN we try to run the pipeline and the comparator is invalidated - addNotif(0, PACKAGE_1); + // WHEN we try to run the pipeline and the comparator is invalidated exactly + // MAX_CONSECUTIVE_REENTRANT_REBUILDS times, + addNotif(0, PACKAGE_2); + invalidator.setInvalidationCount(ShadeListBuilder.MAX_CONSECUTIVE_REENTRANT_REBUILDS); dispatchBuild(); + runWhileScheduledUpTo(ShadeListBuilder.MAX_CONSECUTIVE_REENTRANT_REBUILDS + 2); - // THEN an exception is thrown + // THEN an exception is NOT thrown. } @Test(expected = IllegalStateException.class) - public void testOutOfOrderPreRenderFilterInvalidationThrows() { - // GIVEN a PreRenderNotifFilter that gets invalidated during the finalizing stage - NotifFilter filter = new PackageFilter(PACKAGE_5); - OnBeforeRenderListListener listener = (list) -> filter.invalidateList(null); + public void testOutOfOrderComparatorInvalidationThrowsAfterTooManyRuns() { + // GIVEN a NotifComparator that gets invalidated during the finalizing stage, + NotifComparator comparator = new HypeComparator(PACKAGE_1); + CountingInvalidator invalidator = new CountingInvalidator(comparator); + OnBeforeRenderListListener listener = (list) -> invalidator.maybeInvalidate(); + mListBuilder.setComparators(singletonList(comparator)); + mListBuilder.addOnBeforeRenderListListener(listener); + + // WHEN we try to run the pipeline and the comparator is invalidated more than + // MAX_CONSECUTIVE_REENTRANT_REBUILDS times, + addNotif(0, PACKAGE_2); + invalidator.setInvalidationCount(ShadeListBuilder.MAX_CONSECUTIVE_REENTRANT_REBUILDS + 1); + dispatchBuild(); + runWhileScheduledUpTo(ShadeListBuilder.MAX_CONSECUTIVE_REENTRANT_REBUILDS + 2); + + // THEN an exception IS thrown. + } + + @Test + public void testOutOfOrderPreRenderFilterInvalidationDoesNotThrowBeforeTooManyRuns() { + // GIVEN a PreRenderNotifFilter that gets invalidated during the finalizing stage, + NotifFilter filter = new PackageFilter(PACKAGE_1); + CountingInvalidator invalidator = new CountingInvalidator(filter); + OnBeforeRenderListListener listener = (list) -> invalidator.maybeInvalidate(); mListBuilder.addFinalizeFilter(filter); mListBuilder.addOnBeforeRenderListListener(listener); - // WHEN we try to run the pipeline and the PreRenderFilter is invalidated - addNotif(0, PACKAGE_1); + // WHEN we try to run the pipeline and the PreRenderFilter is invalidated exactly + // MAX_CONSECUTIVE_REENTRANT_REBUILDS times, + addNotif(0, PACKAGE_2); + invalidator.setInvalidationCount(ShadeListBuilder.MAX_CONSECUTIVE_REENTRANT_REBUILDS); dispatchBuild(); + runWhileScheduledUpTo(ShadeListBuilder.MAX_CONSECUTIVE_REENTRANT_REBUILDS + 2); - // THEN an exception is thrown + // THEN an exception is NOT thrown. + } + + @Test(expected = IllegalStateException.class) + public void testOutOfOrderPreRenderFilterInvalidationThrowsAfterTooManyRuns() { + // GIVEN a PreRenderNotifFilter that gets invalidated during the finalizing stage, + NotifFilter filter = new PackageFilter(PACKAGE_1); + CountingInvalidator invalidator = new CountingInvalidator(filter); + OnBeforeRenderListListener listener = (list) -> invalidator.maybeInvalidate(); + mListBuilder.addFinalizeFilter(filter); + mListBuilder.addOnBeforeRenderListListener(listener); + + // WHEN we try to run the pipeline and the PreRenderFilter is invalidated more than + // MAX_CONSECUTIVE_REENTRANT_REBUILDS times, + addNotif(0, PACKAGE_2); + invalidator.setInvalidationCount(ShadeListBuilder.MAX_CONSECUTIVE_REENTRANT_REBUILDS + 1); + dispatchBuild(); + runWhileScheduledUpTo(ShadeListBuilder.MAX_CONSECUTIVE_REENTRANT_REBUILDS + 2); + + // THEN an exception IS thrown. } @Test @@ -2096,6 +2232,18 @@ public class ShadeListBuilderTest extends SysuiTestCase { mPipelineChoreographer.runIfScheduled(); } + private void runWhileScheduledUpTo(int maxRuns) { + int runs = 0; + while (mPipelineChoreographer.isScheduled()) { + if (runs > maxRuns) { + throw new IndexOutOfBoundsException( + "Pipeline scheduled itself more than " + maxRuns + "times"); + } + runs++; + mPipelineChoreographer.runIfScheduled(); + } + } + private void verifyBuiltList(ExpectedEntry ...expectedEntries) { try { assertEquals(