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

Commit 8c4eb2ed authored by Steve Elliott's avatar Steve Elliott
Browse files

Fix re-entrant invalidation w/ HUNs + VisStability 

Inside of an OnBeforeFinalizeListener, HeadsUpCoordinator processes all
of the notifications in the Shade (up to finalization), and determines
whether or not they should be alerting, and whether or not they are
already alerting. Any differences are then reported to the
HeadsUpManager.

The problem: When HeadsUpManager#removeAlertingNotification is called,
VisualStability can be flipped from enabled->disabled, triggering an
invalidation. In the associated bug, this manifests when the device is
pulsing; removeAltertingNotification internally updates the
StatusBarStateController so that it's no longer pulsing, which notifies
the VisualStabilityManager of this change, which then invalidates.

The solution, in two parts:

1. If we can determine that a notification should stop alerting in the
NotifCollectionListener#onEntryUpdated callback, do so that that time.
This happens before the ShadeListBuilder is run, and so won't cause the
re-entrant call.

2. For group alert transfer, any removal of an alert is because that
alert is being transfered to another notification (via "group alert
transfer" behavior). To work around this, we simply ensure that the
HeadsUpManager is only alerted of removals *after* it is alerted of any
additions.

Fixes: 219684141
Test: Using Notify.apk:
      1. Post a group summary
      2. Post a child, with GROUP_ALERT_SUMMARY flag, while the first
      notification is still HUNNING.
      3. Observe no crash
Change-Id: I6df66af5fb2434815b94659a4da6ade79dd64d69
parent 180fe308

packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/HeadsUpCoordinator.kt

+118 −45
Original line number Diff line number Diff line
@@ -72,9 +72,9 @@ class HeadsUpCoordinator @Inject constructor( 
    private var mEndLifetimeExtension: OnEndLifetimeExtensionCallback? = null

    private lateinit var mNotifPipeline: NotifPipeline

    private var mNow: Long = -1



    // notifs we've extended the lifetime for

    private val mNotifsExtendingLifetime = ArraySet<NotificationEntry>()

    private val mPostedEntries = LinkedHashMap<String, PostedEntry>()



    override fun attach(pipeline: NotifPipeline) {

        mNotifPipeline = pipeline
@@ -101,23 +101,25 @@ class HeadsUpCoordinator @Inject constructor( 
            return

        }

        // Process all non-group adds/updates

        mHeadsUpManager.modifyHuns { hunMutator ->

            mPostedEntries.values.toList().forEach { posted ->

                if (!posted.entry.sbn.isGroup) {

                handlePostedEntry(posted, "non-group")

                    handlePostedEntry(posted, hunMutator, "non-group")

                    mPostedEntries.remove(posted.key)

                }

            }

        }

    }



    /**

     * Once we have a nearly final shade list (not including what's pruned for inflation reasons),

     * we know that stability and [NotifPromoter]s have been applied, so we can use the location of

     * notifications in this list to determine what kind of group alert behavior should happen.

     */

    fun onBeforeFinalizeFilter(list: List<ListEntry>) {

    fun onBeforeFinalizeFilter(list: List<ListEntry>) = mHeadsUpManager.modifyHuns { hunMutator ->

        // Nothing to do if there are no other adds/updates

        if (mPostedEntries.isEmpty()) {

            return

            return@modifyHuns

        }

        // Calculate a bunch of information about the logical group and the locations of group

        // entries in the nearly-finalized shade list.  These may be used in the per-group loop.
@@ -138,13 +140,17 @@ class HeadsUpCoordinator @Inject constructor( 


            // If there is no logical summary, then there is no alert to transfer

            if (logicalSummary == null) {

                postedEntries.forEach { handlePostedEntry(it, "logical-summary-missing") }

                postedEntries.forEach {

                    handlePostedEntry(it, hunMutator, scenario = "logical-summary-missing")

                }

                return@forEach

            }



            // If summary isn't wanted to be heads up, then there is no alert to transfer

            if (!isGoingToShowHunStrict(logicalSummary)) {

                postedEntries.forEach { handlePostedEntry(it, "logical-summary-not-alerting") }

                postedEntries.forEach {

                    handlePostedEntry(it, hunMutator, scenario = "logical-summary-not-alerting")

                }

                return@forEach

            }
@@ -177,7 +183,9 @@ class HeadsUpCoordinator @Inject constructor( 
            // If there is no child to receive the parent alert, then just handle the posted entries

            // and return.

            if (childToReceiveParentAlert == null) {

                postedEntries.forEach { handlePostedEntry(it, "no-transfer-target") }

                postedEntries.forEach {

                    handlePostedEntry(it, hunMutator, scenario = "no-transfer-target")

                }

                return@forEach

            }
@@ -189,7 +197,8 @@ class HeadsUpCoordinator @Inject constructor( 
            if (!isSummaryAttached) {

                val summaryUpdateForRemoval = summaryUpdate?.also {

                    it.shouldHeadsUpEver = false

                } ?: PostedEntry(logicalSummary,

                } ?: PostedEntry(

                        logicalSummary,

                        wasAdded = false,

                        wasUpdated = false,

                        shouldHeadsUpEver = false,
@@ -199,9 +208,14 @@ class HeadsUpCoordinator @Inject constructor( 
                )

                // If we transfer the alert and the summary isn't even attached, that means we

                // should ensure the summary is no longer alerting, so we remove it here.

                handlePostedEntry(summaryUpdateForRemoval, "detached-summary-remove-alert")

                handlePostedEntry(

                        summaryUpdateForRemoval,

                        hunMutator,

                        scenario = "detached-summary-remove-alert")

            } else if (summaryUpdate != null) {

                mLogger.logPostedEntryWillNotEvaluate(summaryUpdate, "attached-summary-transferred")

                mLogger.logPostedEntryWillNotEvaluate(

                        summaryUpdate,

                        reason = "attached-summary-transferred")

            }



            // Handle all posted entries -- if the child receiving the parent's alert is in the
@@ -214,10 +228,16 @@ class HeadsUpCoordinator @Inject constructor( 
                            // Update the child's posted update so that it

                            postedEntry.shouldHeadsUpEver = true

                            postedEntry.shouldHeadsUpAgain = true

                        handlePostedEntry(postedEntry, "child-alert-transfer-target-$targetType")

                            handlePostedEntry(

                                    postedEntry,

                                    hunMutator,

                                    scenario = "child-alert-transfer-target-$targetType")

                            didAlertChildToReceiveParentAlert = true

                        } else {

                        handlePostedEntry(postedEntry, "child-alert-non-target")

                            handlePostedEntry(

                                    postedEntry,

                                    hunMutator,

                                    scenario = "child-alert-non-target")

                        }

                    }
@@ -233,7 +253,10 @@ class HeadsUpCoordinator @Inject constructor( 
                        isAlerting = mHeadsUpManager.isAlerting(childToReceiveParentAlert.key),

                        isBinding = isEntryBinding(childToReceiveParentAlert),

                )

                handlePostedEntry(posted, "non-posted-child-alert-transfer-target-$targetType")

                handlePostedEntry(

                        posted,

                        hunMutator,

                        scenario = "non-posted-child-alert-transfer-target-$targetType")

            }

        }

        // After this method runs, all posted entries should have been handled (or skipped).
@@ -292,9 +315,7 @@ class HeadsUpCoordinator @Inject constructor( 
            }

        }



    private val mPostedEntries = LinkedHashMap<String, PostedEntry>()



    fun handlePostedEntry(posted: PostedEntry, scenario: String) {

    private fun handlePostedEntry(posted: PostedEntry, hunMutator: HunMutator, scenario: String) {

        mLogger.logPostedEntryWillEvaluate(posted, scenario)

        if (posted.wasAdded) {

            if (posted.shouldHeadsUpEver) {
@@ -308,12 +329,12 @@ class HeadsUpCoordinator @Inject constructor( 
                    // If alerting, we need to post an update.  Otherwise we're still binding,

                    // and we can just let that finish.

                    if (posted.isAlerting) {

                        mHeadsUpManager.updateNotification(posted.key, posted.shouldHeadsUpAgain)

                        hunMutator.updateNotification(posted.key, posted.shouldHeadsUpAgain)

                    }

                } else {

                    if (posted.isAlerting) {

                        // We don't want this to be interrupting anymore, let's remove it

                        mHeadsUpManager.removeNotification(posted.key, false /*removeImmediately*/)

                        hunMutator.removeNotification(posted.key, false /*removeImmediately*/)

                    } else {

                        // Don't let the bind finish

                        cancelHeadsUpBind(posted.entry)
@@ -366,7 +387,7 @@ class HeadsUpCoordinator @Inject constructor( 
            val shouldHeadsUpAgain = shouldHunAgain(entry)

            val isAlerting = mHeadsUpManager.isAlerting(entry.key)

            val isBinding = isEntryBinding(entry)

            mPostedEntries.compute(entry.key) { _, value ->

            val posted = mPostedEntries.compute(entry.key) { _, value ->

                value?.also { update ->

                    update.wasUpdated = true

                    update.shouldHeadsUpEver = update.shouldHeadsUpEver || shouldHeadsUpEver
@@ -383,6 +404,18 @@ class HeadsUpCoordinator @Inject constructor( 
                    isBinding = isBinding,

                )

            }

            // Handle cancelling alerts here, rather than in the OnBeforeFinalizeFilter, so that

            // work can be done before the ShadeListBuilder is run. This prevents re-entrant

            // behavior between this Coordinator, HeadsUpManager, and VisualStabilityManager.

            if (posted?.shouldHeadsUpEver == false) {

                if (posted.isAlerting) {

                    // We don't want this to be interrupting anymore, let's remove it

                    mHeadsUpManager.removeNotification(posted.key, false /*removeImmediately*/)

                } else if (posted.isBinding) {

                    // Don't let the bind finish

                    cancelHeadsUpBind(posted.entry)

                }

            }

        }



        /**
@@ -543,3 +576,43 @@ private enum class GroupLocation { Detached, Isolated, Summary, Child } 


private fun Map<String, GroupLocation>.getLocation(key: String): GroupLocation =

    getOrDefault(key, GroupLocation.Detached)



/**

 * Invokes the given block with a [HunMutator] that defers all HUN removals. This ensures that the

 * HeadsUpManager is notified of additions before removals, which prevents a glitch where the

 * HeadsUpManager temporarily believes that nothing is alerting, causing bad re-entrant behavior.

 */

private fun <R> HeadsUpManager.modifyHuns(block: (HunMutator) -> R): R {

    val mutator = HunMutatorImpl(this)

    return block(mutator).also { mutator.commitModifications() }

}



/** Mutates the HeadsUp state of notifications. */

private interface HunMutator {

    fun updateNotification(key: String, alert: Boolean)

    fun removeNotification(key: String, releaseImmediately: Boolean)

}



/**

 * [HunMutator] implementation that defers removing notifications from the HeadsUpManager until

 * after additions/updates.

 */

private class HunMutatorImpl(private val headsUpManager: HeadsUpManager) : HunMutator {

    private val deferred = mutableListOf<Pair<String, Boolean>>()



    override fun updateNotification(key: String, alert: Boolean) {

        headsUpManager.updateNotification(key, alert)

    }



    override fun removeNotification(key: String, releaseImmediately: Boolean) {

        val args = Pair(key, releaseImmediately)

        deferred.add(args)

    }



    fun commitModifications() {

        deferred.forEach { (key, releaseImmediately) ->

            headsUpManager.removeNotification(key, releaseImmediately)

        }

        deferred.clear()

    }

}