Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 22740d0f authored by Julia Tuttle's avatar Julia Tuttle
Browse files

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
parent 7c093004

packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/ShadeListBuilder.java

+61 −4
Original line number Diff line number Diff line
@@ -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<ListEntry> 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)) {

    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<ListEntry> entries) {

packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/ShadeListBuilderTest.java

+175 −27
Original line number Diff line number Diff line
@@ -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());

    }



    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 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 NOT thrown.

    }



    @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);

    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

        // 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);

        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 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 testOutOfOrderPrompterInvalidationThrows() {

        // GIVEN a NotifPromoter that gets invalidated during the sorting stage

    public void testOutOfOrderPrompterInvalidationThrowsAfterTooManyRuns() {

        // 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 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 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 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 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

        addNotif(0, PACKAGE_1);

        // 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

        // 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 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 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 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

        addNotif(0, PACKAGE_1);

        // 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

        // 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(