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

Commit 2235a04e authored by Matt Pietal's avatar Matt Pietal
Browse files

Fix race condition with manual transitions 

Transitions are triggered on background scope. Calls to
startTransition were transfered to the main dispatcher, but,
manual transitions that used updateTransition were not. This
could lead to a very rare race condition where a RUNNING step
could be sent after the corresponding CANCEL.

Test: atest KeyguardTransitionRepositoryTest
Fixes: 353698157
Flag: com.android.systemui.migrate_clocks_to_blueprint
Change-Id: I515358f0a9230d660fba4b0703eaad38477edd46
parent 6dd8be5e

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

+0 −24
Original line number Diff line number Diff line
@@ -135,30 +135,6 @@ class KeyguardTransitionInteractorTest : SysuiTestCase() { 
            assertThat(startedStates).isEqualTo(listOf(LOCKSCREEN, PRIMARY_BOUNCER, AOD, GONE))

        }



    @Test

    fun finishedKeyguardTransitionStepTests() =

        testScope.runTest {

            val finishedSteps by collectValues(underTest.finishedKeyguardTransitionStep)

            val steps = mutableListOf<TransitionStep>()



            steps.add(TransitionStep(LOCKSCREEN, AOD, 0f, STARTED))

            steps.add(TransitionStep(LOCKSCREEN, AOD, 0.9f, RUNNING))

            steps.add(TransitionStep(LOCKSCREEN, AOD, 1f, FINISHED))

            steps.add(TransitionStep(AOD, LOCKSCREEN, 0f, STARTED))

            steps.add(TransitionStep(AOD, LOCKSCREEN, 0.5f, RUNNING))

            steps.add(TransitionStep(AOD, LOCKSCREEN, 1f, FINISHED))

            steps.add(TransitionStep(AOD, GONE, 1f, STARTED))



            steps.forEach {

                repository.sendTransitionStep(it)

                runCurrent()

            }



            // Ignore the default state.

            assertThat(finishedSteps.subList(1, finishedSteps.size))

                .isEqualTo(listOf(steps[2], steps[5]))

        }



    @Test

    fun startedKeyguardTransitionStepTests() =

        testScope.runTest {

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

+2 −2
Original line number Diff line number Diff line
@@ -175,7 +175,7 @@ constructor( 
        }

    }



    private fun finishCurrentTransition() {

    private suspend fun finishCurrentTransition() {

        internalTransitionInteractor.updateTransition(

            currentTransitionId!!,

            1f,
@@ -285,7 +285,7 @@ constructor( 
        currentTransitionId = internalTransitionInteractor.startTransition(transitionInfo)

    }



    private fun updateProgress(progress: Float) {

    private suspend fun updateProgress(progress: Float) {

        if (currentTransitionId == null) return

        internalTransitionInteractor.updateTransition(

            currentTransitionId!!,

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

+21 −6
Original line number Diff line number Diff line
@@ -105,7 +105,7 @@ interface KeyguardTransitionRepository { 
     * When the transition is over, TransitionState.FINISHED must be passed into the [state]

     * parameter.

     */

    fun updateTransition(

    suspend fun updateTransition(

        transitionId: UUID,

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

        state: TransitionState
@@ -173,9 +173,9 @@ constructor( 
        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.

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

        // requires the same lock

        _currentTransitionMutex.lock()



        // Only used in a test environment

        if (forceDelayForRaceConditionTest) {

            delay(50L)
@@ -184,7 +184,6 @@ constructor( 
        // Animators must be started on the main thread.

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

            _currentTransitionMutex.unlock()



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

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

                return@withContext null
@@ -206,7 +205,7 @@ constructor( 


            // Cancel any existing manual transitions

            updateTransitionId?.let { uuid ->

                updateTransition(uuid, lastStep.value, TransitionState.CANCELED)

                updateTransitionInternal(uuid, lastStep.value, TransitionState.CANCELED)

            }



            info.animator?.let { animator ->
@@ -264,7 +263,23 @@ constructor( 
        }

    }



    override fun updateTransition(

    override suspend fun updateTransition(

        transitionId: UUID,

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

        state: TransitionState

    ) {

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

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

            _currentTransitionMutex.unlock()



            updateTransitionInternal(transitionId, value, state)

        }

    }



    private suspend fun updateTransitionInternal(

        transitionId: UUID,

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

        state: TransitionState

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

+1 −1
Original line number Diff line number Diff line
@@ -63,7 +63,7 @@ constructor( 


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



    fun updateTransition(

    suspend fun updateTransition(

        transitionId: UUID,

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

        state: TransitionState

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

+17 −6
Original line number Diff line number Diff line
@@ -47,6 +47,7 @@ import kotlinx.coroutines.flow.MutableSharedFlow 
import kotlinx.coroutines.flow.SharedFlow

import kotlinx.coroutines.flow.SharingStarted

import kotlinx.coroutines.flow.StateFlow

import kotlinx.coroutines.flow.buffer

import kotlinx.coroutines.flow.combine

import kotlinx.coroutines.flow.distinctUntilChanged

import kotlinx.coroutines.flow.filter
@@ -55,6 +56,7 @@ import kotlinx.coroutines.flow.mapLatest 
import kotlinx.coroutines.flow.onStart

import kotlinx.coroutines.flow.shareIn

import kotlinx.coroutines.flow.stateIn

import kotlinx.coroutines.flow.transform

import kotlinx.coroutines.launch



/** Encapsulates business-logic related to the keyguard transitions. */
@@ -150,6 +152,12 @@ constructor( 
                        startedStep.to != prevStep.from

                ) {

                    getTransitionValueFlow(prevStep.from).emit(0f)

                } else if (prevStep.transitionState == TransitionState.RUNNING) {

                    Log.e(

                        TAG,

                        "STARTED step ($startedStep) was preceded by a RUNNING step " +

                            "($prevStep), which should never happen. Things could go badly here."

                    )

                }

            }

        }
@@ -252,15 +260,12 @@ constructor( 
    val startedKeyguardTransitionStep: Flow<TransitionStep> =

        repository.transitions.filter { step -> step.transitionState == TransitionState.STARTED }



    /** The last [TransitionStep] with a [TransitionState] of FINISHED */

    val finishedKeyguardTransitionStep: Flow<TransitionStep> =

        repository.transitions.filter { step -> step.transitionState == TransitionState.FINISHED }



    /** The destination state of the last [TransitionState.STARTED] transition. */

    @SuppressLint("SharedFlowCreation")

    val startedKeyguardState: SharedFlow<KeyguardState> =

        startedKeyguardTransitionStep

            .map { step -> step.to }

            .buffer(2, BufferOverflow.DROP_OLDEST)

            .shareIn(scope, SharingStarted.Eagerly, replay = 1)



    /** The from state of the last [TransitionState.STARTED] transition. */
@@ -269,6 +274,7 @@ constructor( 
    val startedKeyguardFromState: SharedFlow<KeyguardState> =

        startedKeyguardTransitionStep

            .map { step -> step.from }

            .buffer(2, BufferOverflow.DROP_OLDEST)

            .shareIn(scope, SharingStarted.Eagerly, replay = 1)



    /** Which keyguard state to use when the device goes to sleep. */
@@ -310,8 +316,13 @@ constructor( 
     */

    @SuppressLint("SharedFlowCreation")

    val finishedKeyguardState: SharedFlow<KeyguardState> =

        finishedKeyguardTransitionStep

            .map { step -> step.to }

        repository.transitions

            .transform { step ->

                if (step.transitionState == TransitionState.FINISHED) {

                    emit(step.to)

                }

            }

            .buffer(2, BufferOverflow.DROP_OLDEST)

            .shareIn(scope, SharingStarted.Eagerly, replay = 1)



    /**