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

Commit 0234597b authored by Steve Elliott's avatar Steve Elliott
Browse files

Prevent desync of NICViewBinder + NotifCollection 

There is a potential race condition where the NotifCollection removes a
notification before the NICViewBinder attempts to bind its icon. In this
state, the NICViewBinder believes that the icon should be present, but
in reality the icon is about to be removed by the next pipeline run.

The fix tracks icons that "fail" to bind in this way. The expectation is
that this tracking is ephemeral, and will be cleared by the next update
from the ViewModel, after the pipeline has run again. This tracking is
dumped in a bug report to identify situations where there are repeated
failures to bind, indicating a more significant logic issue.

Flag: ACONFIG com.android.systemui.notifications_icon_container_refactor DEVELOPMENT
Fixes: 309555139
Bug: 278765923
Test: manual - repeatedly unplug and replug the phone into USB
      debugging, observe no crash.
Change-Id: I49e5cf068bf5156a2d6df6a6870d062a773d9ff5
parent 1b433c6a

packages/SystemUI/src/com/android/keyguard/KeyguardClockSwitchController.java

+5 −0
Original line number Diff line number Diff line
@@ -62,6 +62,7 @@ import com.android.systemui.statusbar.notification.AnimatableProperty; 
import com.android.systemui.statusbar.notification.PropertyAnimator;

import com.android.systemui.statusbar.notification.icon.ui.viewbinder.AlwaysOnDisplayNotificationIconViewStore;

import com.android.systemui.statusbar.notification.icon.ui.viewbinder.NotificationIconContainerViewBinder;

import com.android.systemui.statusbar.notification.icon.ui.viewbinder.StatusBarIconViewBindingFailureTracker;

import com.android.systemui.statusbar.notification.icon.ui.viewmodel.NotificationIconContainerAlwaysOnDisplayViewModel;

import com.android.systemui.statusbar.notification.shared.NotificationIconContainerRefactor;

import com.android.systemui.statusbar.notification.stack.AnimationProperties;
@@ -105,6 +106,7 @@ public class KeyguardClockSwitchController extends ViewController<KeyguardClockS 
    private final DozeParameters mDozeParameters;

    private final ScreenOffAnimationController mScreenOffAnimationController;

    private final AlwaysOnDisplayNotificationIconViewStore mAodIconViewStore;

    private final StatusBarIconViewBindingFailureTracker mIconViewBindingFailureTracker;

    private FrameLayout mSmallClockFrame; // top aligned clock

    private FrameLayout mLargeClockFrame; // centered clock
@@ -179,6 +181,7 @@ public class KeyguardClockSwitchController extends ViewController<KeyguardClockS 
            LockscreenSmartspaceController smartspaceController,

            ConfigurationController configurationController,

            ScreenOffAnimationController screenOffAnimationController,

            StatusBarIconViewBindingFailureTracker iconViewBindingFailureTracker,

            KeyguardUnlockAnimationController keyguardUnlockAnimationController,

            SecureSettings secureSettings,

            @Main DelayableExecutor uiExecutor,
@@ -202,6 +205,7 @@ public class KeyguardClockSwitchController extends ViewController<KeyguardClockS 
        mSmartspaceController = smartspaceController;

        mConfigurationController = configurationController;

        mScreenOffAnimationController = screenOffAnimationController;

        mIconViewBindingFailureTracker = iconViewBindingFailureTracker;

        mSecureSettings = secureSettings;

        mUiExecutor = uiExecutor;

        mKeyguardUnlockAnimationController = keyguardUnlockAnimationController;
@@ -605,6 +609,7 @@ public class KeyguardClockSwitchController extends ViewController<KeyguardClockS 
                            mAodIconsViewModel,

                            mConfigurationState,

                            mConfigurationController,

                            mIconViewBindingFailureTracker,

                            mAodIconViewStore);

                    final DisposableHandle visHandle = KeyguardRootViewBinder.bindAodIconVisibility(

                            nic,

packages/SystemUI/src/com/android/systemui/dagger/SystemUIModule.java

+2 −0
Original line number Diff line number Diff line
@@ -119,6 +119,7 @@ import com.android.systemui.statusbar.policy.PolicyModule; 
import com.android.systemui.statusbar.policy.ZenModeController;

import com.android.systemui.statusbar.policy.dagger.SmartRepliesInflationModule;

import com.android.systemui.statusbar.policy.dagger.StatusBarPolicyModule;

import com.android.systemui.statusbar.ui.binder.StatusBarViewBinderModule;

import com.android.systemui.statusbar.window.StatusBarWindowModule;

import com.android.systemui.telephony.data.repository.TelephonyRepositoryModule;

import com.android.systemui.temporarydisplay.dagger.TemporaryDisplayModule;
@@ -211,6 +212,7 @@ import javax.inject.Named; 
        StatusBarModule.class,

        StatusBarPipelineModule.class,

        StatusBarPolicyModule.class,

        StatusBarViewBinderModule.class,

        StatusBarWindowModule.class,

        SystemPropertiesFlagsModule.class,

        SysUIConcurrencyModule.class,

packages/SystemUI/src/com/android/systemui/keyguard/ui/view/layout/sections/AodNotificationIconsSection.kt

+3 −2
Original line number Diff line number Diff line
@@ -34,9 +34,9 @@ import com.android.systemui.keyguard.ui.viewmodel.KeyguardSmartspaceViewModel 
import com.android.systemui.res.R

import com.android.systemui.statusbar.notification.icon.ui.viewbinder.AlwaysOnDisplayNotificationIconViewStore

import com.android.systemui.statusbar.notification.icon.ui.viewbinder.NotificationIconContainerViewBinder

import com.android.systemui.statusbar.notification.icon.ui.viewbinder.StatusBarIconViewBindingFailureTracker

import com.android.systemui.statusbar.notification.icon.ui.viewmodel.NotificationIconContainerAlwaysOnDisplayViewModel

import com.android.systemui.statusbar.notification.shared.NotificationIconContainerRefactor

import com.android.systemui.statusbar.phone.DozeParameters

import com.android.systemui.statusbar.phone.NotificationIconAreaController

import com.android.systemui.statusbar.phone.NotificationIconContainer

import com.android.systemui.statusbar.policy.ConfigurationController
@@ -49,8 +49,8 @@ constructor( 
    private val context: Context,

    private val configurationState: ConfigurationState,

    private val configurationController: ConfigurationController,

    private val dozeParameters: DozeParameters,

    private val featureFlags: FeatureFlagsClassic,

    private val iconBindingFailureTracker: StatusBarIconViewBindingFailureTracker,

    private val nicAodViewModel: NotificationIconContainerAlwaysOnDisplayViewModel,

    private val nicAodIconViewStore: AlwaysOnDisplayNotificationIconViewStore,

    private val notificationIconAreaController: NotificationIconAreaController,
@@ -94,6 +94,7 @@ constructor( 
                    nicAodViewModel,

                    configurationState,

                    configurationController,

                    iconBindingFailureTracker,

                    nicAodIconViewStore,

                )

        } else {

packages/SystemUI/src/com/android/systemui/statusbar/notification/icon/ui/viewbinder/NotificationIconContainerViewBinder.kt

+74 −67
Original line number Diff line number Diff line
@@ -23,27 +23,37 @@ import androidx.annotation.ColorInt 
import androidx.collection.ArrayMap

import androidx.lifecycle.lifecycleScope

import com.android.internal.policy.SystemBarUtils

import com.android.internal.statusbar.StatusBarIcon

import com.android.internal.util.ContrastColorUtil

import com.android.systemui.CoreStartable

import com.android.systemui.common.ui.ConfigurationState

import com.android.systemui.dagger.SysUISingleton

import com.android.systemui.lifecycle.repeatWhenAttached

import com.android.systemui.res.R

import com.android.systemui.statusbar.StatusBarIconView

import com.android.systemui.statusbar.notification.collection.NotifCollection

import com.android.systemui.statusbar.notification.icon.IconPack

import com.android.systemui.statusbar.notification.icon.ui.viewbinder.NotificationIconContainerViewBinder.IconViewStore

import com.android.systemui.statusbar.notification.icon.ui.viewmodel.NotificationIconColors

import com.android.systemui.statusbar.notification.icon.ui.viewmodel.NotificationIconContainerAlwaysOnDisplayViewModel

import com.android.systemui.statusbar.notification.icon.ui.viewmodel.NotificationIconContainerShelfViewModel

import com.android.systemui.statusbar.notification.icon.ui.viewmodel.NotificationIconContainerStatusBarViewModel

import com.android.systemui.statusbar.notification.icon.ui.viewmodel.NotificationIconsViewData

import com.android.systemui.statusbar.notification.shared.NotificationIconContainerRefactor

import com.android.systemui.statusbar.phone.NotificationIconContainer

import com.android.systemui.statusbar.policy.ConfigurationController

import com.android.systemui.statusbar.policy.onConfigChanged

import com.android.systemui.util.children

import com.android.systemui.util.asIndenting

import com.android.systemui.util.kotlin.mapValuesNotNullTo

import com.android.systemui.util.kotlin.sample

import com.android.systemui.util.kotlin.stateFlow

import com.android.systemui.util.printCollection

import com.android.systemui.util.ui.isAnimating

import com.android.systemui.util.ui.stopAnimating

import com.android.systemui.util.ui.value

import dagger.Binds

import dagger.multibindings.ClassKey

import dagger.multibindings.IntoMap

import java.io.PrintWriter

import javax.inject.Inject

import kotlinx.coroutines.DisposableHandle

import kotlinx.coroutines.Job
@@ -62,11 +72,18 @@ object NotificationIconContainerViewBinder { 
        viewModel: NotificationIconContainerShelfViewModel,

        configuration: ConfigurationState,

        configurationController: ConfigurationController,

        failureTracker: StatusBarIconViewBindingFailureTracker,

        viewStore: ShelfNotificationIconViewStore,

    ): DisposableHandle {

        return view.repeatWhenAttached {

            lifecycleScope.launch {

                viewModel.icons.bindIcons(view, configuration, configurationController, viewStore)

                viewModel.icons.bindIcons(

                    view,

                    configuration,

                    configurationController,

                    notifyBindingFailures = { failureTracker.shelfFailures = it },

                    viewStore,

                )

            }

        }

    }
@@ -77,18 +94,20 @@ object NotificationIconContainerViewBinder { 
        viewModel: NotificationIconContainerStatusBarViewModel,

        configuration: ConfigurationState,

        configurationController: ConfigurationController,

        failureTracker: StatusBarIconViewBindingFailureTracker,

        viewStore: StatusBarNotificationIconViewStore,

    ): DisposableHandle {

        val contrastColorUtil = ContrastColorUtil.getInstance(view.context)

        return view.repeatWhenAttached {

            lifecycleScope.run {

                launch {

                    val iconColors =

                    val iconColors: Flow<NotificationIconColors> =

                        viewModel.iconColors.mapNotNull { it.iconColors(view.viewBounds) }

                    viewModel.icons.bindIcons(

                        view,

                        configuration,

                        configurationController,

                        notifyBindingFailures = { failureTracker.statusBarFailures = it },

                        viewStore,

                    ) { _, sbiv ->

                        StatusBarIconViewBinder.bindIconColors(
@@ -110,6 +129,7 @@ object NotificationIconContainerViewBinder { 
        viewModel: NotificationIconContainerAlwaysOnDisplayViewModel,

        configuration: ConfigurationState,

        configurationController: ConfigurationController,

        failureTracker: StatusBarIconViewBindingFailureTracker,

        viewStore: IconViewStore,

    ): DisposableHandle {

        return view.repeatWhenAttached {
@@ -119,6 +139,7 @@ object NotificationIconContainerViewBinder { 
                        view,

                        configuration,

                        configurationController,

                        notifyBindingFailures = { failureTracker.aodFailures = it },

                        viewStore,

                    ) { _, sbiv ->

                        viewModel.bindAodStatusBarIconView(sbiv, configuration)
@@ -176,7 +197,7 @@ object NotificationIconContainerViewBinder { 
    }



    /**

     * Binds [NotificationIconsViewData] to a [NotificationIconContainer]'s [children].

     * Binds [NotificationIconsViewData] to a [NotificationIconContainer]'s children.

     *

     * [bindIcon] will be invoked to bind a child [StatusBarIconView] to an icon associated with the

     * given `iconKey`. The parent [Job] of this coroutine will be cancelled automatically when the
@@ -186,6 +207,7 @@ object NotificationIconContainerViewBinder { 
        view: NotificationIconContainer,

        configuration: ConfigurationState,

        configurationController: ConfigurationController,

        notifyBindingFailures: (Collection<String>) -> Unit,

        viewStore: IconViewStore,

        bindIcon: suspend (iconKey: String, view: StatusBarIconView) -> Unit = { _, _ -> },

    ): Unit = coroutineScope {
@@ -208,57 +230,59 @@ object NotificationIconContainerViewBinder { 
                FrameLayout.LayoutParams(iconSize + 2 * iconHPadding, statusBarHeight)

            }



        launch {

            layoutParams.collect { params: FrameLayout.LayoutParams ->

                for (child in view.children) {

                    child.layoutParams = params

                }

            }

        }



        val iconBindings = mutableMapOf<String, Job>()

        val failedBindings = mutableSetOf<String>()

        val boundViewsByNotifKey = ArrayMap<String, Pair<StatusBarIconView, Job>>()

        var prevIcons = NotificationIconsViewData()

        sample(layoutParams, ::Pair).collect {

            (iconsData: NotificationIconsViewData, layoutParams: FrameLayout.LayoutParams),

            ->

        collect { iconsData: NotificationIconsViewData ->

            val iconsDiff = NotificationIconsViewData.computeDifference(iconsData, prevIcons)

            prevIcons = iconsData



            val replacingIcons =

                iconsDiff.groupReplacements.mapValuesNotNullTo(ArrayMap()) { (_, v) ->

                    viewStore.iconView(v.notifKey).statusBarIcon

            val replacingIcons: ArrayMap<String, StatusBarIcon> =

                iconsDiff.groupReplacements.mapValuesNotNullTo(ArrayMap()) { (_, info) ->

                    boundViewsByNotifKey[info.notifKey]?.first?.statusBarIcon

                }

            view.setReplacingIcons(replacingIcons)



            val childrenByNotifKey: Map<String, StatusBarIconView> =

                view.children.filterIsInstance<StatusBarIconView>().associateByTo(ArrayMap()) {

                    it.notification.key

                }



            iconsDiff.removed

                .mapNotNull { key -> childrenByNotifKey[key]?.let { key to it } }

                .forEach { (key, child) ->

            for (notifKey in iconsDiff.removed) {

                failedBindings.remove(notifKey)

                val (child, job) = boundViewsByNotifKey.remove(notifKey) ?: continue

                view.removeView(child)

                    iconBindings.remove(key)?.cancel()

                job.cancel()

            }



            val toAdd = iconsDiff.added.map { it.notifKey to viewStore.iconView(it.notifKey) }

            for ((i, keyAndView) in toAdd.withIndex()) {

                val (key, sbiv) = keyAndView

                // The view might still be transiently added if it was just removed

                // and added again

            val toAdd: Sequence<String> =

                iconsDiff.added.asSequence().map { it.notifKey } + failedBindings

            for ((idx, notifKey) in toAdd.withIndex()) {

                val sbiv = viewStore.iconView(notifKey)

                if (sbiv == null) {

                    failedBindings.add(notifKey)

                    continue

                }

                // The view might still be transiently added if it was just removed and added again

                view.removeTransientView(sbiv)

                view.addView(sbiv, i, layoutParams)

                iconBindings.remove(key)?.cancel()

                iconBindings[key] = launch { bindIcon(key, sbiv) }

                view.addView(sbiv, idx)

                boundViewsByNotifKey.remove(notifKey)?.second?.cancel()

                boundViewsByNotifKey[notifKey] =

                    Pair(

                        sbiv,

                        launch {

                            launch { layoutParams.collect { sbiv.layoutParams = it } }

                            bindIcon(notifKey, sbiv)

                        },

                    )

            }



            notifyBindingFailures(failedBindings)



            view.setChangingViewPositions(true)



            // Re-sort notification icons

            val expectedChildren =

                iconsData.visibleKeys.mapNotNull { boundViewsByNotifKey[it.notifKey]?.first }

            val childCount = view.childCount

            for (i in 0 until childCount) {

                val actual = view.getChildAt(i)

                val expected = viewStore.iconView(iconsData.visibleKeys[i].notifKey)

                val expected = expectedChildren[i]

                if (actual === expected) {

                    continue

                }
@@ -273,46 +297,29 @@ object NotificationIconContainerViewBinder { 


    /** External storage for [StatusBarIconView] instances. */

    fun interface IconViewStore {

        fun iconView(key: String): StatusBarIconView

        fun iconView(key: String): StatusBarIconView?

    }



    @ColorInt private val DEFAULT_AOD_ICON_COLOR = Color.WHITE

}



/** [IconViewStore] for the [com.android.systemui.statusbar.NotificationShelf] */

class ShelfNotificationIconViewStore

@Inject

constructor(

    private val notifCollection: NotifCollection,

) : IconViewStore {

    override fun iconView(key: String): StatusBarIconView {

        val entry = notifCollection.getEntry(key) ?: error("No entry found for key: $key")

        return entry.icons.shelfIcon ?: error("No shelf IconView found for key: $key")

    }

}

class ShelfNotificationIconViewStore @Inject constructor(notifCollection: NotifCollection) :

    IconViewStore by (notifCollection.iconViewStoreBy { it.shelfIcon })



/** [IconViewStore] for the always-on display. */

class AlwaysOnDisplayNotificationIconViewStore

@Inject

constructor(

    private val notifCollection: NotifCollection,

) : IconViewStore {

    override fun iconView(key: String): StatusBarIconView {

        val entry = notifCollection.getEntry(key) ?: error("No entry found for key: $key")

        return entry.icons.aodIcon ?: error("No AOD IconView found for key: $key")

    }

}

constructor(notifCollection: NotifCollection) :

    IconViewStore by (notifCollection.iconViewStoreBy { it.aodIcon })



/** [IconViewStore] for the status bar. */

class StatusBarNotificationIconViewStore

@Inject

constructor(

    private val notifCollection: NotifCollection,

) : IconViewStore {

    override fun iconView(key: String): StatusBarIconView {

        val entry = notifCollection.getEntry(key) ?: error("No entry found for key: $key")

        return entry.icons.statusBarIcon ?: error("No status bar IconView found for key: $key")

    }

class StatusBarNotificationIconViewStore @Inject constructor(notifCollection: NotifCollection) :

    IconViewStore by (notifCollection.iconViewStoreBy { it.statusBarIcon })



private fun NotifCollection.iconViewStoreBy(block: (IconPack) -> StatusBarIconView?) =

    IconViewStore { key ->

        getEntry(key)?.icons?.let(block)

    }



private val View.viewBounds: Rect

packages/SystemUI/src/com/android/systemui/statusbar/notification/icon/ui/viewbinder/StatusBarIconViewBindingFailureTracker.kt

0 → 100644
+58 −0
Original line number Diff line number Diff line 
/*

 * Copyright (C) 2023 The Android Open Source Project

 *

 * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file

 * except in compliance with the License. You may obtain a copy of the License at

 *

 *      http://www.apache.org/licenses/LICENSE-2.0

 *

 * Unless required by applicable law or agreed to in writing, software distributed under the

 * License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY

 * KIND, either express or implied. See the License for the specific language governing

 * permissions and limitations under the License.

 *

 */



package com.android.systemui.statusbar.notification.icon.ui.viewbinder



import com.android.systemui.CoreStartable

import com.android.systemui.dagger.SysUISingleton

import com.android.systemui.statusbar.notification.shared.NotificationIconContainerRefactor

import com.android.systemui.util.asIndenting

import com.android.systemui.util.printCollection

import dagger.Binds

import dagger.multibindings.ClassKey

import dagger.multibindings.IntoMap

import java.io.PrintWriter

import javax.inject.Inject



@SysUISingleton

class StatusBarIconViewBindingFailureTracker @Inject constructor() : CoreStartable {



    var aodFailures: Collection<String> = emptyList()

    var statusBarFailures: Collection<String> = emptyList()

    var shelfFailures: Collection<String> = emptyList()



    // TODO(b/310681665): Ideally we wouldn't need to implement CoreStartable at all, and could just

    //  @Binds @IntoSet the Dumpable.

    override fun start() {

        // no-op, we're just using this as a dumpable

    }



    override fun dump(pw: PrintWriter, args: Array<out String>) {

        if (!NotificationIconContainerRefactor.isEnabled) return

        pw.asIndenting().run {

            printCollection("AOD Icon binding failures:", aodFailures)

            printCollection("Status Bar Icon binding failures:", statusBarFailures)

            printCollection("Shelf Icon binding failures:", shelfFailures)

        }

    }



    @dagger.Module

    interface Module {

        @Binds

        @IntoMap

        @ClassKey(StatusBarIconViewBindingFailureTracker::class)

        fun bindStartable(impl: StatusBarIconViewBindingFailureTracker): CoreStartable

    }

}