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

7 files changed, 217 insertions, 17 deletions

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<UnfoldTransitionRecording> = 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<RotationListener>
 
     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 <T> runBlockingInBg(f: () -> T): T {
+        return runBlocking {
+            withTimeout(5.seconds) {
+                suspendCancellableCoroutine { c: CancellableContinuation<T> ->
+                    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<TransitionProgressListener> = mutableListOf()
+    private val listeners = CopyOnWriteArrayList<TransitionProgressListener>()
 
-    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
     }