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

Commit a99411a8 authored by Matt Pietal's avatar Matt Pietal
Browse files

Ensure ordering in transition processing 

withContext does not provide that guarantee. Use a Mutex to
ensure that transitions do not run out of order. This was
first reported during a DOZING->LOCKSCREEN->OCCLUDED transition
on double-tap power press but was processed as
DOZING->OCCLUDED->LOCKSCREEN due to withContext.

Also, remove an unnecessary audit log

Fixes: 340041643
Test: atest KeyguardTransitionScenariosTest
KeyguardTransitionRepositoryTest
Flag: None
Change-Id: I0d4d35634f140658b384d06a16adbc6fca15aab0

Change-Id: I75ccc43c0d3b3775443486b62a312f82953cca3e
parent 6ba7f82b

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

+18 −0
Original line number Diff line number Diff line
@@ -34,6 +34,7 @@ import java.util.UUID 
import javax.inject.Inject

import kotlinx.coroutines.CoroutineDispatcher

import kotlinx.coroutines.channels.BufferOverflow

import kotlinx.coroutines.delay

import kotlinx.coroutines.flow.Flow

import kotlinx.coroutines.flow.MutableSharedFlow

import kotlinx.coroutines.flow.MutableStateFlow
@@ -42,6 +43,7 @@ import kotlinx.coroutines.flow.asSharedFlow 
import kotlinx.coroutines.flow.asStateFlow

import kotlinx.coroutines.flow.distinctUntilChanged

import kotlinx.coroutines.flow.filter

import kotlinx.coroutines.sync.Mutex



/**

 * The source of truth for all keyguard transitions.
@@ -129,6 +131,7 @@ constructor( 
    private var lastStep: TransitionStep = TransitionStep()

    private var lastAnimator: ValueAnimator? = null



    private val _currentTransitionMutex = Mutex()

    private val _currentTransitionInfo: MutableStateFlow<TransitionInfo> =

        MutableStateFlow(

            TransitionInfo(
@@ -146,6 +149,9 @@ constructor( 
     */

    private var updateTransitionId: UUID? = null



    // Only used in a test environment

    var forceDelayForRaceConditionTest = false



    init {

        // Start with a FINISHED transition in OFF. KeyguardBootInteractor will transition from OFF

        // to either GONE or LOCKSCREEN once we're booted up and can determine which state we should
@@ -162,9 +168,21 @@ constructor( 


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

        _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.

        _currentTransitionMutex.lock()



        // Only used in a test environment

        if (forceDelayForRaceConditionTest) {

            delay(50L)

        }



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

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

+0 −6
Original line number Diff line number Diff line
@@ -55,12 +55,6 @@ constructor( 
            }

        }



        scope.launch {

            sharedNotificationContainerViewModel

                .getMaxNotifications { height, useExtraShelfSpace -> height.toInt() }

                .collect { logger.log(TAG, VERBOSE, "Notif: max height in px", it) }

        }



        scope.launch {

            sharedNotificationContainerViewModel.isOnLockscreen.collect {

                logger.log(TAG, VERBOSE, "Notif: isOnLockscreen", it)

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

+1 −0
Original line number Diff line number Diff line
@@ -229,6 +229,7 @@ sealed class TransitionInteractor( 
                        startTransitionTo(

                            toState = KeyguardState.OCCLUDED,

                            modeOnCanceled = TransitionModeOnCanceled.RESET,

                            ownerReason = "keyguardInteractor.onCameraLaunchDetected",

                        )

                    }

                }

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

+42 −0
Original line number Diff line number Diff line
@@ -25,16 +25,19 @@ import androidx.test.filters.FlakyTest 
import androidx.test.filters.SmallTest

import com.android.app.animation.Interpolators

import com.android.systemui.SysuiTestCase

import com.android.systemui.coroutines.collectValues

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

import com.android.systemui.keyguard.shared.model.KeyguardState.AOD

import com.android.systemui.keyguard.shared.model.KeyguardState.DREAMING

import com.android.systemui.keyguard.shared.model.KeyguardState.LOCKSCREEN

import com.android.systemui.keyguard.shared.model.KeyguardState.OCCLUDED

import com.android.systemui.keyguard.shared.model.KeyguardState.OFF

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

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

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

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

import com.android.systemui.keyguard.util.KeyguardTransitionRunner

import com.android.systemui.kosmos.testDispatcher

import com.android.systemui.kosmos.testScope

import com.android.systemui.testKosmos

import com.google.common.truth.Truth.assertThat
@@ -45,6 +48,8 @@ import kotlinx.coroutines.Dispatchers 
import kotlinx.coroutines.flow.dropWhile

import kotlinx.coroutines.flow.launchIn

import kotlinx.coroutines.flow.onEach

import kotlinx.coroutines.launch

import kotlinx.coroutines.test.advanceTimeBy

import kotlinx.coroutines.test.runCurrent

import kotlinx.coroutines.test.runTest

import org.junit.After
@@ -372,6 +377,43 @@ class KeyguardTransitionRepositoryTest : SysuiTestCase() { 
            assertThat(wtfHandler.failed).isTrue()

        }



    @Test

    fun simulateRaceConditionIsProcessedInOrder() =

        testScope.runTest {

            val ktr = KeyguardTransitionRepositoryImpl(kosmos.testDispatcher)

            val steps by collectValues(ktr.transitions.dropWhile { step -> step.from == OFF })



            // Add a delay to the first transition in order to attempt to have the second transition

            // be processed first

            val info1 = TransitionInfo(OWNER_NAME, AOD, LOCKSCREEN, animator = null)

            launch {

                ktr.forceDelayForRaceConditionTest = true

                ktr.startTransition(info1)

            }

            val info2 = TransitionInfo(OWNER_NAME, LOCKSCREEN, OCCLUDED, animator = null)

            launch {

                ktr.forceDelayForRaceConditionTest = false

                ktr.startTransition(info2)

            }



            runCurrent()

            assertThat(steps.isEmpty()).isTrue()



            advanceTimeBy(60L)

            assertThat(steps[0])

                .isEqualTo(

                    TransitionStep(info1.from, info1.to, 0f, TransitionState.STARTED, OWNER_NAME)

                )

            assertThat(steps[1])

                .isEqualTo(

                    TransitionStep(info1.from, info1.to, 0f, TransitionState.CANCELED, OWNER_NAME)

                )

            assertThat(steps[2])

                .isEqualTo(

                    TransitionStep(info2.from, info2.to, 0f, TransitionState.STARTED, OWNER_NAME)

                )

        }



    private fun listWithStep(

        step: BigDecimal,

        start: BigDecimal = BigDecimal.ZERO,

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

+3 −1
Original line number Diff line number Diff line
@@ -1669,7 +1669,9 @@ class KeyguardTransitionScenariosTest(flags: FlagsParameterization?) : SysuiTest 
            // THEN a transition from DOZING => OCCLUDED should occur

            assertThat(transitionRepository)

                .startedTransition(

                    ownerName = "FromDozingTransitionInteractor",

                    ownerName =

                        "FromDozingTransitionInteractor" +

                            "(keyguardInteractor.onCameraLaunchDetected)",

                    from = KeyguardState.DOZING,

                    to = KeyguardState.OCCLUDED,

                    animatorAssertion = { it.isNotNull() },