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
diff --git a/packages/SystemUI/src/com/android/systemui/keyguard/data/repository/KeyguardTransitionRepository.kt b/packages/SystemUI/src/com/android/systemui/keyguard/data/repository/KeyguardTransitionRepository.kt
index 7655d7a..f488d3b 100644
--- a/packages/SystemUI/src/com/android/systemui/keyguard/data/repository/KeyguardTransitionRepository.kt
+++ b/packages/SystemUI/src/com/android/systemui/keyguard/data/repository/KeyguardTransitionRepository.kt
@@ -34,6 +34,7 @@
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.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 @@
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 @@
*/
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 @@
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
diff --git a/packages/SystemUI/src/com/android/systemui/keyguard/domain/interactor/KeyguardTransitionAuditLogger.kt b/packages/SystemUI/src/com/android/systemui/keyguard/domain/interactor/KeyguardTransitionAuditLogger.kt
index 75c4d6f..cf6942e 100644
--- a/packages/SystemUI/src/com/android/systemui/keyguard/domain/interactor/KeyguardTransitionAuditLogger.kt
+++ b/packages/SystemUI/src/com/android/systemui/keyguard/domain/interactor/KeyguardTransitionAuditLogger.kt
@@ -56,12 +56,6 @@
}
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)
}
diff --git a/packages/SystemUI/src/com/android/systemui/keyguard/domain/interactor/TransitionInteractor.kt b/packages/SystemUI/src/com/android/systemui/keyguard/domain/interactor/TransitionInteractor.kt
index b2a24ca..323ceef 100644
--- a/packages/SystemUI/src/com/android/systemui/keyguard/domain/interactor/TransitionInteractor.kt
+++ b/packages/SystemUI/src/com/android/systemui/keyguard/domain/interactor/TransitionInteractor.kt
@@ -229,6 +229,7 @@
startTransitionTo(
toState = KeyguardState.OCCLUDED,
modeOnCanceled = TransitionModeOnCanceled.RESET,
+ ownerReason = "keyguardInteractor.onCameraLaunchDetected",
)
}
}
diff --git a/packages/SystemUI/tests/src/com/android/systemui/keyguard/data/repository/KeyguardTransitionRepositoryTest.kt b/packages/SystemUI/tests/src/com/android/systemui/keyguard/data/repository/KeyguardTransitionRepositoryTest.kt
index 53560d7..48a5df9 100644
--- a/packages/SystemUI/tests/src/com/android/systemui/keyguard/data/repository/KeyguardTransitionRepositoryTest.kt
+++ b/packages/SystemUI/tests/src/com/android/systemui/keyguard/data/repository/KeyguardTransitionRepositoryTest.kt
@@ -25,16 +25,19 @@
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.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 @@
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,
diff --git a/packages/SystemUI/tests/src/com/android/systemui/keyguard/domain/interactor/KeyguardTransitionScenariosTest.kt b/packages/SystemUI/tests/src/com/android/systemui/keyguard/domain/interactor/KeyguardTransitionScenariosTest.kt
index 00f94a5..fa3fe5c 100644
--- a/packages/SystemUI/tests/src/com/android/systemui/keyguard/domain/interactor/KeyguardTransitionScenariosTest.kt
+++ b/packages/SystemUI/tests/src/com/android/systemui/keyguard/domain/interactor/KeyguardTransitionScenariosTest.kt
@@ -1669,7 +1669,9 @@
// 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() },