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 420f21db2c73a..14cc6bf1ea415 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, PipelineDumpable { 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, PipelineDumpable { mLogger.logOnBuildList(reason); mAllEntries = entries; - mChoreographer.schedule(); + scheduleRebuild(/* reentrant = */ false); } }; @@ -1332,11 +1336,64 @@ public class ShadeListBuilder implements Dumpable, PipelineDumpable { 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(