From 8c4eb2ed6109b067282d683f4c76d223cde2952c Mon Sep 17 00:00:00 2001 From: Steve Elliott Date: Fri, 18 Feb 2022 11:20:49 -0500 Subject: [PATCH] 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 --- .../coordinator/HeadsUpCoordinator.kt | 163 +++++++++++++----- 1 file changed, 118 insertions(+), 45 deletions(-) diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/HeadsUpCoordinator.kt b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/HeadsUpCoordinator.kt index 41b070635d4f7..0df2162d33387 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/HeadsUpCoordinator.kt +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/HeadsUpCoordinator.kt @@ -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() + private val mPostedEntries = LinkedHashMap() override fun attach(pipeline: NotifPipeline) { mNotifPipeline = pipeline @@ -101,10 +101,12 @@ class HeadsUpCoordinator @Inject constructor( return } // Process all non-group adds/updates - mPostedEntries.values.toList().forEach { posted -> - if (!posted.entry.sbn.isGroup) { - handlePostedEntry(posted, "non-group") - mPostedEntries.remove(posted.key) + mHeadsUpManager.modifyHuns { hunMutator -> + mPostedEntries.values.toList().forEach { posted -> + if (!posted.entry.sbn.isGroup) { + handlePostedEntry(posted, hunMutator, "non-group") + mPostedEntries.remove(posted.key) + } } } } @@ -114,10 +116,10 @@ class HeadsUpCoordinator @Inject constructor( * 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) { + fun onBeforeFinalizeFilter(list: List) = 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,51 +197,66 @@ class HeadsUpCoordinator @Inject constructor( if (!isSummaryAttached) { val summaryUpdateForRemoval = summaryUpdate?.also { it.shouldHeadsUpEver = false - } ?: PostedEntry(logicalSummary, - wasAdded = false, - wasUpdated = false, - shouldHeadsUpEver = false, - shouldHeadsUpAgain = false, - isAlerting = mHeadsUpManager.isAlerting(logicalSummary.key), - isBinding = isEntryBinding(logicalSummary), + } ?: PostedEntry( + logicalSummary, + wasAdded = false, + wasUpdated = false, + shouldHeadsUpEver = false, + shouldHeadsUpAgain = false, + isAlerting = mHeadsUpManager.isAlerting(logicalSummary.key), + isBinding = isEntryBinding(logicalSummary), ) // 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") - } else if (summaryUpdate!=null) { - mLogger.logPostedEntryWillNotEvaluate(summaryUpdate, "attached-summary-transferred") + handlePostedEntry( + summaryUpdateForRemoval, + hunMutator, + scenario = "detached-summary-remove-alert") + } else if (summaryUpdate != null) { + mLogger.logPostedEntryWillNotEvaluate( + summaryUpdate, + reason = "attached-summary-transferred") } // Handle all posted entries -- if the child receiving the parent's alert is in the // list, then set its flags to ensure it alerts. var didAlertChildToReceiveParentAlert = false postedEntries.asSequence() - .filter { it.key != logicalSummary.key } - .forEach { postedEntry -> - if (childToReceiveParentAlert.key == postedEntry.key) { - // Update the child's posted update so that it - postedEntry.shouldHeadsUpEver = true - postedEntry.shouldHeadsUpAgain = true - handlePostedEntry(postedEntry, "child-alert-transfer-target-$targetType") - didAlertChildToReceiveParentAlert = true - } else { - handlePostedEntry(postedEntry, "child-alert-non-target") + .filter { it.key != logicalSummary.key } + .forEach { postedEntry -> + if (childToReceiveParentAlert.key == postedEntry.key) { + // Update the child's posted update so that it + postedEntry.shouldHeadsUpEver = true + postedEntry.shouldHeadsUpAgain = true + handlePostedEntry( + postedEntry, + hunMutator, + scenario = "child-alert-transfer-target-$targetType") + didAlertChildToReceiveParentAlert = true + } else { + handlePostedEntry( + postedEntry, + hunMutator, + scenario = "child-alert-non-target") + } } - } // If the child receiving the alert was not updated on this tick (which can happen in a // standard alert transfer scenario), then construct an update so that we can apply it. if (!didAlertChildToReceiveParentAlert) { val posted = PostedEntry( - childToReceiveParentAlert, - wasAdded = false, - wasUpdated = false, - shouldHeadsUpEver = true, - shouldHeadsUpAgain = true, - isAlerting = mHeadsUpManager.isAlerting(childToReceiveParentAlert.key), - isBinding = isEntryBinding(childToReceiveParentAlert), + childToReceiveParentAlert, + wasAdded = false, + wasUpdated = false, + shouldHeadsUpEver = true, + shouldHeadsUpAgain = true, + 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() - - 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.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 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>() + + 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() + } +}