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

Commit 93678d42 authored by Matt Pietal's avatar Matt Pietal
Browse files

Remove MutableStateFlow for transition current info 

When multiple threads are attempting to begin transitions,
the MutableStateFlow can get stuck on a background thread
while emitting the update the consumers. This leaves a
small chance for other transitions to jump the queue, ones
that are scheduled onto a more favorable thread.

Avoid MutableStateFlow completely. There's no need, just
use a volatile to store state.

Fixes: 358533338
Test: atest KeyguardTransitionRepositoryTest
Flag: com.android.systemui.transition_race_condition
Change-Id: I1daf9d5074c966445a1ed54acc10fb91193d6c85
parent 43776a28

packages/SystemUI/aconfig/systemui.aconfig

+10 −0
Original line number Diff line number Diff line
@@ -1545,6 +1545,16 @@ flag { 
  bug: "362719719"

}



flag {

  name: "transition_race_condition"

  namespace: "systemui"

  description: "Thread-safe keyguard transitions"

  bug: "358533338"

  metadata {

       purpose: PURPOSE_BUGFIX

  }

}



flag {

   name: "media_projection_request_attribution_fix"

   namespace: "systemui"

packages/SystemUI/src/com/android/systemui/communal/domain/interactor/CommunalSceneTransitionInteractor.kt

+3 −3
Original line number Diff line number Diff line
@@ -73,7 +73,7 @@ constructor( 
    private var progressJob: Job? = null



    private val currentToState: KeyguardState

        get() = internalTransitionInteractor.currentTransitionInfoInternal.value.to

        get() = internalTransitionInteractor.currentTransitionInfoInternal().to



    /**

     * The next keyguard state to trigger when exiting [CommunalScenes.Communal]. This is only used
@@ -197,7 +197,7 @@ constructor( 
        val newTransition =

            TransitionInfo(

                ownerName = this::class.java.simpleName,

                from = internalTransitionInteractor.currentTransitionInfoInternal.value.to,

                from = internalTransitionInteractor.currentTransitionInfoInternal().to,

                to = state,

                animator = null,

                modeOnCanceled = TransitionModeOnCanceled.REVERSE,
@@ -273,7 +273,7 @@ constructor( 
    }



    private suspend fun startTransitionToGlanceableHub() {

        val currentState = internalTransitionInteractor.currentTransitionInfoInternal.value.to

        val currentState = internalTransitionInteractor.currentTransitionInfoInternal().to

        val newTransition =

            TransitionInfo(

                ownerName = this::class.java.simpleName,

packages/SystemUI/src/com/android/systemui/keyguard/data/repository/KeyguardTransitionRepository.kt

+40 −13
Original line number Diff line number Diff line
@@ -25,6 +25,7 @@ import android.os.Trace 
import android.util.Log

import com.android.app.animation.Interpolators

import com.android.app.tracing.coroutines.withContextTraced as withContext

import com.android.systemui.Flags.transitionRaceCondition

import com.android.systemui.dagger.SysUISingleton

import com.android.systemui.dagger.qualifiers.Main

import com.android.systemui.keyguard.shared.model.KeyguardState
@@ -77,6 +78,8 @@ interface KeyguardTransitionRepository { 


    /** The [TransitionInfo] of the most recent call to [startTransition]. */

    val currentTransitionInfoInternal: StateFlow<TransitionInfo>

    /** The [TransitionInfo] of the most recent call to [startTransition]. */

    val currentTransitionInfo: TransitionInfo



    /**

     * Interactors that require information about changes between [KeyguardState]s will call this to
@@ -132,7 +135,7 @@ constructor(@Main val mainDispatcher: CoroutineDispatcher) : KeyguardTransitionR 
    private var lastStep: TransitionStep = TransitionStep()

    private var lastAnimator: ValueAnimator? = null



    private val _currentTransitionMutex = Mutex()

    private val withContextMutex = Mutex()

    private val _currentTransitionInfo: MutableStateFlow<TransitionInfo> =

        MutableStateFlow(

            TransitionInfo(
@@ -144,6 +147,16 @@ constructor(@Main val mainDispatcher: CoroutineDispatcher) : KeyguardTransitionR 
        )

    override var currentTransitionInfoInternal = _currentTransitionInfo.asStateFlow()



    @Volatile

    override var currentTransitionInfo: TransitionInfo =

        TransitionInfo(

            ownerName = "",

            from = KeyguardState.OFF,

            to = KeyguardState.OFF,

            animator = null,

        )

        private set



    /*

     * When manual control of the transition is requested, a unique [UUID] is used as the handle

     * to permit calls to [updateTransition]
@@ -163,13 +176,17 @@ constructor(@Main val mainDispatcher: CoroutineDispatcher) : KeyguardTransitionR 
    }



    override suspend fun startTransition(info: TransitionInfo): UUID? {

        if (transitionRaceCondition()) {

            currentTransitionInfo = info

        } else {

            _currentTransitionInfo.value = info

        }

        Log.d(TAG, "(Internal) Setting current transition info: $info")



        // There is no fairness guarantee with 'withContext', which means that transitions could

        // be processed out of order. Use a Mutex to guarantee ordering. [updateTransition]

        // requires the same lock

        _currentTransitionMutex.lock()

        withContextMutex.lock()

        // Only used in a test environment

        if (forceDelayForRaceConditionTest) {

            delay(50L)
@@ -177,7 +194,7 @@ constructor(@Main val mainDispatcher: CoroutineDispatcher) : KeyguardTransitionR 


        // Animators must be started on the main thread.

        return withContext("$TAG#startTransition", mainDispatcher) {

            _currentTransitionMutex.unlock()

            withContextMutex.unlock()

            if (lastStep.from == info.from && lastStep.to == info.to) {

                Log.i(TAG, "Duplicate call to start the transition, rejecting: $info")

                return@withContext null
@@ -265,9 +282,9 @@ constructor(@Main val mainDispatcher: CoroutineDispatcher) : KeyguardTransitionR 
        // There is no fairness guarantee with 'withContext', which means that transitions could

        // be processed out of order. Use a Mutex to guarantee ordering. [startTransition]

        // requires the same lock

        _currentTransitionMutex.lock()

        withContextMutex.lock()

        withContext("$TAG#updateTransition", mainDispatcher) {

            _currentTransitionMutex.unlock()

            withContextMutex.unlock()



            updateTransitionInternal(transitionId, value, state)

        }
@@ -302,6 +319,15 @@ constructor(@Main val mainDispatcher: CoroutineDispatcher) : KeyguardTransitionR 
        // Tests runs on testDispatcher, which is not the main thread, causing the animator thread

        // check to fail

        if (testSetup) {

            if (transitionRaceCondition()) {

                currentTransitionInfo =

                    TransitionInfo(

                        ownerName = ownerName,

                        from = KeyguardState.OFF,

                        to = to,

                        animator = null,

                    )

            } else {

                _currentTransitionInfo.value =

                    TransitionInfo(

                        ownerName = ownerName,
@@ -309,6 +335,7 @@ constructor(@Main val mainDispatcher: CoroutineDispatcher) : KeyguardTransitionR 
                        to = to,

                        animator = null,

                    )

            }

            emitTransition(

                TransitionStep(

                    KeyguardState.OFF,

packages/SystemUI/src/com/android/systemui/keyguard/domain/interactor/FromLockscreenTransitionInteractor.kt

+7 −7
Original line number Diff line number Diff line
@@ -40,6 +40,7 @@ import com.android.systemui.scene.shared.flag.SceneContainerFlag 
import com.android.systemui.scene.shared.model.Scenes

import com.android.systemui.shade.data.repository.ShadeRepository

import com.android.systemui.util.kotlin.Utils.Companion.sample as sampleCombine

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

import java.util.UUID

import javax.inject.Inject

import kotlin.time.Duration.Companion.milliseconds
@@ -132,11 +133,10 @@ constructor( 
        scope.launch("$TAG#listenForLockscreenToDreaming") {

            keyguardInteractor.isAbleToDream

                .filterRelevantKeyguardState()

                .sampleCombine(

                    internalTransitionInteractor.currentTransitionInfoInternal,

                    transitionInteractor.isFinishedIn(KeyguardState.LOCKSCREEN),

                )

                .collect { (isAbleToDream, transitionInfo, isOnLockscreen) ->

                .sample(transitionInteractor.isFinishedIn(KeyguardState.LOCKSCREEN), ::Pair)

                .collect { (isAbleToDream, isOnLockscreen) ->

                    val transitionInfo =

                        internalTransitionInteractor.currentTransitionInfoInternal()

                    val isTransitionInterruptible =

                        transitionInfo.to == KeyguardState.LOCKSCREEN &&

                            !invalidFromStates.contains(transitionInfo.from)
@@ -179,7 +179,6 @@ constructor( 
            shadeRepository.legacyShadeExpansion

                .sampleCombine(

                    transitionInteractor.startedKeyguardTransitionStep,

                    internalTransitionInteractor.currentTransitionInfoInternal,

                    keyguardInteractor.statusBarState,

                    keyguardInteractor.isKeyguardDismissible,

                    keyguardInteractor.isKeyguardOccluded,
@@ -188,11 +187,12 @@ constructor( 
                    (

                        shadeExpansion,

                        startedStep,

                        currentTransitionInfo,

                        statusBarState,

                        isKeyguardUnlocked,

                        isKeyguardOccluded) ->

                    val id = transitionId

                    val currentTransitionInfo =

                        internalTransitionInteractor.currentTransitionInfoInternal()

                    if (id != null) {

                        if (startedStep.to == KeyguardState.PRIMARY_BOUNCER) {

                            // An existing `id` means a transition is started, and calls to

packages/SystemUI/src/com/android/systemui/keyguard/domain/interactor/InternalKeyguardTransitionInteractor.kt

+10 −7
Original line number Diff line number Diff line
@@ -17,13 +17,13 @@ 
package com.android.systemui.keyguard.domain.interactor



import android.annotation.FloatRange

import com.android.systemui.Flags.transitionRaceCondition

import com.android.systemui.dagger.SysUISingleton

import com.android.systemui.keyguard.data.repository.KeyguardTransitionRepository

import com.android.systemui.keyguard.shared.model.TransitionInfo

import com.android.systemui.keyguard.shared.model.TransitionState

import java.util.UUID

import javax.inject.Inject

import kotlinx.coroutines.flow.StateFlow



/**

 * This interactor provides direct access to [KeyguardTransitionRepository] internals and exposes
@@ -32,9 +32,7 @@ import kotlinx.coroutines.flow.StateFlow 
@SysUISingleton

class InternalKeyguardTransitionInteractor

@Inject

constructor(

    private val repository: KeyguardTransitionRepository,

) {

constructor(private val repository: KeyguardTransitionRepository) {



    /**

     * The [TransitionInfo] of the most recent call to
@@ -58,14 +56,19 @@ constructor( 
     * *will* be emitted, and therefore that it can safely request an AOD -> LOCKSCREEN transition

     * which will subsequently cancel GONE -> AOD.

     */

    internal val currentTransitionInfoInternal: StateFlow<TransitionInfo> =

        repository.currentTransitionInfoInternal

    internal fun currentTransitionInfoInternal(): TransitionInfo {

        return if (transitionRaceCondition()) {

            repository.currentTransitionInfo

        } else {

            repository.currentTransitionInfoInternal.value

        }

    }



    suspend fun startTransition(info: TransitionInfo) = repository.startTransition(info)



    suspend fun updateTransition(

        transitionId: UUID,

        @FloatRange(from = 0.0, to = 1.0) value: Float,

        state: TransitionState

        state: TransitionState,

    ) = repository.updateTransition(transitionId, value, state)

}