From d61e267922a84edcfa890657fc401000af456576 Mon Sep 17 00:00:00 2001 From: Nicolo' Mazzucato Date: Fri, 15 Dec 2023 12:05:12 +0000 Subject: Fix race in ScopedUnfoldTransitionProgressProvider Callbacks were being added from the main thread, but the listener list was being read from the background thread used to generate unfold progress, causing a ConcurrentModificationException. Now the scoped provider creates an handler backed by the same looper as the received progress, and uses it to forward progresses. This makes it possible for the class to work even without explicitly injecting the handler to use to propagate progresses. + Adding many tests for ScopedUnfoldTransitionProgressProvider Flag: ACONFIG unfold_animation_background_progress DISABLED Bug: 316554882 Test: ScopedUnfoldTransitionProgressProviderTest Change-Id: I4d8ef19f0116e52b84e6689bddd096fa8fa468e4 --- ...atusBarMoveFromCenterAnimationControllerTest.kt | 2 + .../unfold/progress/TestUnfoldProgressListener.kt | 6 + .../NaturalRotationUnfoldProgressProviderTest.kt | 5 + .../util/ScaleAwareUnfoldProgressProviderTest.kt | 2 + .../ScopedUnfoldTransitionProgressProviderTest.kt | 161 +++++++++++++++++++++ .../unfold/util/UnfoldOnlyProgressProviderTest.kt | 10 +- .../util/ScopedUnfoldTransitionProgressProvider.kt | 48 ++++-- 7 files changed, 217 insertions(+), 17 deletions(-) create mode 100644 packages/SystemUI/tests/src/com/android/systemui/unfold/util/ScopedUnfoldTransitionProgressProviderTest.kt diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/phone/StatusBarMoveFromCenterAnimationControllerTest.kt b/packages/SystemUI/tests/src/com/android/systemui/statusbar/phone/StatusBarMoveFromCenterAnimationControllerTest.kt index 7594c90daa8b..feff046bb708 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/phone/StatusBarMoveFromCenterAnimationControllerTest.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/phone/StatusBarMoveFromCenterAnimationControllerTest.kt @@ -1,6 +1,7 @@ package com.android.systemui.statusbar.phone import android.graphics.Point +import android.testing.TestableLooper import android.view.Display import android.view.Surface import android.view.View @@ -19,6 +20,7 @@ import org.mockito.Mock import org.mockito.MockitoAnnotations @SmallTest +@TestableLooper.RunWithLooper class StatusBarMoveFromCenterAnimationControllerTest : SysuiTestCase() { @Mock diff --git a/packages/SystemUI/tests/src/com/android/systemui/unfold/progress/TestUnfoldProgressListener.kt b/packages/SystemUI/tests/src/com/android/systemui/unfold/progress/TestUnfoldProgressListener.kt index e461e3f7fb1a..bbc96f703738 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/unfold/progress/TestUnfoldProgressListener.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/unfold/progress/TestUnfoldProgressListener.kt @@ -26,8 +26,11 @@ class TestUnfoldProgressListener : UnfoldTransitionProgressProvider.TransitionPr private val recordings: MutableList = arrayListOf() private var currentRecording: UnfoldTransitionRecording? = null + var lastCallbackThread: Thread? = null + private set override fun onTransitionStarted() { + lastCallbackThread = Thread.currentThread() assertWithMessage("Trying to start a transition when it is already in progress") .that(currentRecording) .isNull() @@ -36,6 +39,7 @@ class TestUnfoldProgressListener : UnfoldTransitionProgressProvider.TransitionPr } override fun onTransitionProgress(progress: Float) { + lastCallbackThread = Thread.currentThread() assertWithMessage("Received transition progress event when it's not started") .that(currentRecording) .isNotNull() @@ -43,6 +47,7 @@ class TestUnfoldProgressListener : UnfoldTransitionProgressProvider.TransitionPr } override fun onTransitionFinishing() { + lastCallbackThread = Thread.currentThread() assertWithMessage("Received transition finishing event when it's not started") .that(currentRecording) .isNotNull() @@ -50,6 +55,7 @@ class TestUnfoldProgressListener : UnfoldTransitionProgressProvider.TransitionPr } override fun onTransitionFinished() { + lastCallbackThread = Thread.currentThread() assertWithMessage("Received transition finish event when it's not started") .that(currentRecording) .isNotNull() diff --git a/packages/SystemUI/tests/src/com/android/systemui/unfold/util/NaturalRotationUnfoldProgressProviderTest.kt b/packages/SystemUI/tests/src/com/android/systemui/unfold/util/NaturalRotationUnfoldProgressProviderTest.kt index a25469bfc09b..d864d53fea32 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/unfold/util/NaturalRotationUnfoldProgressProviderTest.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/unfold/util/NaturalRotationUnfoldProgressProviderTest.kt @@ -16,6 +16,7 @@ package com.android.systemui.unfold.util import android.testing.AndroidTestingRunner +import android.testing.TestableLooper import android.view.Surface import androidx.test.filters.SmallTest import com.android.systemui.SysuiTestCase @@ -37,6 +38,7 @@ import org.mockito.MockitoAnnotations @RunWith(AndroidTestingRunner::class) @SmallTest +@TestableLooper.RunWithLooper class NaturalRotationUnfoldProgressProviderTest : SysuiTestCase() { @Mock lateinit var rotationChangeProvider: RotationChangeProvider @@ -48,10 +50,12 @@ class NaturalRotationUnfoldProgressProviderTest : SysuiTestCase() { @Captor private lateinit var rotationListenerCaptor: ArgumentCaptor lateinit var progressProvider: NaturalRotationUnfoldProgressProvider + private lateinit var testableLooper : TestableLooper @Before fun setUp() { MockitoAnnotations.initMocks(this) + testableLooper = TestableLooper.get(this) progressProvider = NaturalRotationUnfoldProgressProvider(context, rotationChangeProvider, sourceProvider) @@ -123,5 +127,6 @@ class NaturalRotationUnfoldProgressProviderTest : SysuiTestCase() { private fun onRotationChanged(rotation: Int) { rotationListenerCaptor.value.onRotationChanged(rotation) + testableLooper.processAllMessages() } } diff --git a/packages/SystemUI/tests/src/com/android/systemui/unfold/util/ScaleAwareUnfoldProgressProviderTest.kt b/packages/SystemUI/tests/src/com/android/systemui/unfold/util/ScaleAwareUnfoldProgressProviderTest.kt index e1e54a903917..2f29b3bdd3b5 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/unfold/util/ScaleAwareUnfoldProgressProviderTest.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/unfold/util/ScaleAwareUnfoldProgressProviderTest.kt @@ -19,6 +19,7 @@ import android.content.ContentResolver import android.database.ContentObserver import android.provider.Settings import android.testing.AndroidTestingRunner +import android.testing.TestableLooper import androidx.test.filters.SmallTest import com.android.systemui.SysuiTestCase import com.android.systemui.unfold.TestUnfoldTransitionProvider @@ -36,6 +37,7 @@ import org.mockito.MockitoAnnotations @RunWith(AndroidTestingRunner::class) @SmallTest +@TestableLooper.RunWithLooper class ScaleAwareUnfoldProgressProviderTest : SysuiTestCase() { @Mock lateinit var sinkProvider: TransitionProgressListener diff --git a/packages/SystemUI/tests/src/com/android/systemui/unfold/util/ScopedUnfoldTransitionProgressProviderTest.kt b/packages/SystemUI/tests/src/com/android/systemui/unfold/util/ScopedUnfoldTransitionProgressProviderTest.kt new file mode 100644 index 000000000000..5b4f4d37214f --- /dev/null +++ b/packages/SystemUI/tests/src/com/android/systemui/unfold/util/ScopedUnfoldTransitionProgressProviderTest.kt @@ -0,0 +1,161 @@ +/* + * 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.unfold.util + +import android.os.Handler +import android.os.HandlerThread +import android.os.Process +import android.testing.AndroidTestingRunner +import android.testing.TestableLooper.RunWithLooper +import androidx.test.filters.SmallTest +import com.android.systemui.SysuiTestCase +import com.android.systemui.unfold.TestUnfoldTransitionProvider +import com.android.systemui.unfold.progress.TestUnfoldProgressListener +import com.google.common.truth.Truth.assertThat +import kotlin.time.Duration.Companion.seconds +import kotlinx.coroutines.CancellableContinuation +import kotlinx.coroutines.runBlocking +import kotlinx.coroutines.suspendCancellableCoroutine +import kotlinx.coroutines.test.TestScope +import kotlinx.coroutines.test.UnconfinedTestDispatcher +import kotlinx.coroutines.test.runTest +import kotlinx.coroutines.withTimeout +import org.junit.Assert.assertThrows +import org.junit.Test +import org.junit.runner.RunWith + +@RunWith(AndroidTestingRunner::class) +@SmallTest +@RunWithLooper +class ScopedUnfoldTransitionProgressProviderTest : SysuiTestCase() { + + private val rootProvider = TestUnfoldTransitionProvider() + private val listener = TestUnfoldProgressListener() + private val testScope = TestScope(UnconfinedTestDispatcher()) + private val bgThread = + HandlerThread("UnfoldBgTest", Process.THREAD_PRIORITY_FOREGROUND).apply { start() } + private val bgHandler = Handler(bgThread.looper) + private val scopedProvider = + ScopedUnfoldTransitionProgressProvider(rootProvider).apply { addCallback(listener) } + + @Test + fun setReadyToHandleTransition_whileTransitionRunning_propagatesCallbacks() = + testScope.runTest { + runBlockingInBg { rootProvider.onTransitionStarted() } + + scopedProvider.setReadyToHandleTransition(true) + + runBlockingInBg { /* sync barrier */} + + listener.assertStarted() + + runBlockingInBg { rootProvider.onTransitionProgress(1f) } + + listener.assertLastProgress(1f) + + runBlockingInBg { rootProvider.onTransitionFinished() } + + listener.assertNotStarted() + } + + @Test + fun setReadyToHandleTransition_whileTransitionNotRunning_callbacksInProgressThread() { + testScope.runTest { + scopedProvider.setReadyToHandleTransition(true) + + val bgThread = runBlockingInBg { Thread.currentThread() } + + runBlockingInBg { rootProvider.onTransitionStarted() } + + listener.assertStarted() + + assertThat(listener.lastCallbackThread).isEqualTo(bgThread) + } + } + + @Test + fun setReadyToHandleTransition_beforeAnyCallback_doesNotCrash() { + testScope.runTest { scopedProvider.setReadyToHandleTransition(true) } + } + + @Test + fun onTransitionStarted_whileNotReadyToHandleTransition_doesNotPropagate() { + testScope.runTest { + scopedProvider.setReadyToHandleTransition(false) + + rootProvider.onTransitionStarted() + + listener.assertNotStarted() + } + } + + @Test + fun onTransitionStarted_defaultReadiness_doesNotPropagate() { + testScope.runTest { + rootProvider.onTransitionStarted() + + listener.assertNotStarted() + } + } + + @Test + fun onTransitionStarted_fromDifferentThreads_throws() { + testScope.runTest { + runBlockingInBg { + rootProvider.onTransitionStarted() + rootProvider.onTransitionFinished() + } + assertThrows(IllegalStateException::class.java) { rootProvider.onTransitionStarted() } + } + } + + @Test + fun onTransitionProgress_fromDifferentThreads_throws() { + testScope.runTest { + runBlockingInBg { rootProvider.onTransitionStarted() } + assertThrows(IllegalStateException::class.java) { + rootProvider.onTransitionProgress(1f) + } + } + } + + @Test + fun onTransitionFinished_fromDifferentThreads_throws() { + testScope.runTest { + runBlockingInBg { rootProvider.onTransitionStarted() } + assertThrows(IllegalStateException::class.java) { rootProvider.onTransitionFinished() } + } + } + + @Test + fun onTransitionFinishing_fromDifferentThreads_throws() { + testScope.runTest { + runBlockingInBg { rootProvider.onTransitionStarted() } + assertThrows(IllegalStateException::class.java) { rootProvider.onTransitionFinishing() } + } + } + + private fun runBlockingInBg(f: () -> T): T { + return runBlocking { + withTimeout(5.seconds) { + suspendCancellableCoroutine { c: CancellableContinuation -> + bgHandler.post { c.resumeWith(Result.success(f())) } + } + } + } + } +} diff --git a/packages/SystemUI/tests/src/com/android/systemui/unfold/util/UnfoldOnlyProgressProviderTest.kt b/packages/SystemUI/tests/src/com/android/systemui/unfold/util/UnfoldOnlyProgressProviderTest.kt index 4a38fc069d9f..f484ea04bb4f 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/unfold/util/UnfoldOnlyProgressProviderTest.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/unfold/util/UnfoldOnlyProgressProviderTest.kt @@ -16,6 +16,7 @@ package com.android.systemui.unfold.util import android.testing.AndroidTestingRunner +import android.testing.TestableLooper import androidx.test.filters.SmallTest import com.android.systemui.SysuiTestCase import com.android.systemui.unfold.TestUnfoldTransitionProvider @@ -27,6 +28,7 @@ import org.junit.runner.RunWith @RunWith(AndroidTestingRunner::class) @SmallTest +@TestableLooper.RunWithLooper class UnfoldOnlyProgressProviderTest : SysuiTestCase() { private val listener = TestUnfoldProgressListener() @@ -54,9 +56,7 @@ class UnfoldOnlyProgressProviderTest : SysuiTestCase() { sourceProvider.onTransitionProgress(0.5f) sourceProvider.onTransitionFinished() - with(listener.ensureTransitionFinished()) { - assertLastProgress(0.5f) - } + with(listener.ensureTransitionFinished()) { assertLastProgress(0.5f) } } @Test @@ -121,8 +121,6 @@ class UnfoldOnlyProgressProviderTest : SysuiTestCase() { sourceProvider.onTransitionProgress(0.1f) sourceProvider.onTransitionFinished() - with(listener.ensureTransitionFinished()) { - assertLastProgress(0.1f) - } + with(listener.ensureTransitionFinished()) { assertLastProgress(0.1f) } } } diff --git a/packages/SystemUI/unfold/src/com/android/systemui/unfold/util/ScopedUnfoldTransitionProgressProvider.kt b/packages/SystemUI/unfold/src/com/android/systemui/unfold/util/ScopedUnfoldTransitionProgressProvider.kt index f9751d9c279c..2bca2722be6c 100644 --- a/packages/SystemUI/unfold/src/com/android/systemui/unfold/util/ScopedUnfoldTransitionProgressProvider.kt +++ b/packages/SystemUI/unfold/src/com/android/systemui/unfold/util/ScopedUnfoldTransitionProgressProvider.kt @@ -15,8 +15,11 @@ */ package com.android.systemui.unfold.util +import android.os.Handler +import android.os.Looper import com.android.systemui.unfold.UnfoldTransitionProgressProvider import com.android.systemui.unfold.UnfoldTransitionProgressProvider.TransitionProgressListener +import java.util.concurrent.CopyOnWriteArrayList /** * Manages progress listeners that can have smaller lifespan than the unfold animation. @@ -33,12 +36,13 @@ open class ScopedUnfoldTransitionProgressProvider constructor(source: UnfoldTransitionProgressProvider? = null) : UnfoldTransitionProgressProvider, TransitionProgressListener { + private var progressHandler: Handler? = null private var source: UnfoldTransitionProgressProvider? = null - private val listeners: MutableList = mutableListOf() + private val listeners = CopyOnWriteArrayList() - private var isReadyToHandleTransition = false - private var isTransitionRunning = false + @Volatile private var isReadyToHandleTransition = false + @Volatile private var isTransitionRunning = false private var lastTransitionProgress = PROGRESS_UNSET init { @@ -70,15 +74,18 @@ constructor(source: UnfoldTransitionProgressProvider? = null) : * Call it with readyToHandleTransition = false when listeners can't process the events. */ fun setReadyToHandleTransition(isReadyToHandleTransition: Boolean) { - if (isTransitionRunning) { - if (isReadyToHandleTransition) { - listeners.forEach { it.onTransitionStarted() } - if (lastTransitionProgress != PROGRESS_UNSET) { - listeners.forEach { it.onTransitionProgress(lastTransitionProgress) } + val progressHandler = this.progressHandler + if (isTransitionRunning && progressHandler != null) { + progressHandler.post { + if (isReadyToHandleTransition) { + listeners.forEach { it.onTransitionStarted() } + if (lastTransitionProgress != PROGRESS_UNSET) { + listeners.forEach { it.onTransitionProgress(lastTransitionProgress) } + } + } else { + isTransitionRunning = false + listeners.forEach { it.onTransitionFinished() } } - } else { - isTransitionRunning = false - listeners.forEach { it.onTransitionFinished() } } } this.isReadyToHandleTransition = isReadyToHandleTransition @@ -98,6 +105,7 @@ constructor(source: UnfoldTransitionProgressProvider? = null) : } override fun onTransitionStarted() { + assertInProgressThread() isTransitionRunning = true if (isReadyToHandleTransition) { listeners.forEach { it.onTransitionStarted() } @@ -105,6 +113,7 @@ constructor(source: UnfoldTransitionProgressProvider? = null) : } override fun onTransitionProgress(progress: Float) { + assertInProgressThread() if (isReadyToHandleTransition) { listeners.forEach { it.onTransitionProgress(progress) } } @@ -112,12 +121,14 @@ constructor(source: UnfoldTransitionProgressProvider? = null) : } override fun onTransitionFinishing() { + assertInProgressThread() if (isReadyToHandleTransition) { listeners.forEach { it.onTransitionFinishing() } } } override fun onTransitionFinished() { + assertInProgressThread() if (isReadyToHandleTransition) { listeners.forEach { it.onTransitionFinished() } } @@ -125,6 +136,21 @@ constructor(source: UnfoldTransitionProgressProvider? = null) : lastTransitionProgress = PROGRESS_UNSET } + private fun assertInProgressThread() { + val cachedProgressHandler = progressHandler + if (cachedProgressHandler == null) { + val thisLooper = Looper.myLooper() ?: error("This thread is expected to have a looper.") + progressHandler = Handler(thisLooper) + } else { + check(cachedProgressHandler.looper.isCurrentThread) { + """Receiving unfold transition callback from different threads. + |Current: ${Thread.currentThread()} + |expected: ${cachedProgressHandler.looper.thread}""" + .trimMargin() + } + } + } + companion object { private const val PROGRESS_UNSET = -1f } -- cgit v1.2.3-59-g8ed1b