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

Commit d61e2679 authored by Nicolo' Mazzucato's avatar Nicolo' Mazzucato
Browse files

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
parent e7e12e35

packages/SystemUI/tests/src/com/android/systemui/statusbar/phone/StatusBarMoveFromCenterAnimationControllerTest.kt

+2 −0
Original line number Diff line number Diff line 
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

packages/SystemUI/tests/src/com/android/systemui/unfold/progress/TestUnfoldProgressListener.kt

+6 −0
Original line number Diff line number Diff line
@@ -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()

packages/SystemUI/tests/src/com/android/systemui/unfold/util/NaturalRotationUnfoldProgressProviderTest.kt

+5 −0
Original line number Diff line number Diff line
@@ -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()

    }

}

packages/SystemUI/tests/src/com/android/systemui/unfold/util/ScaleAwareUnfoldProgressProviderTest.kt

+2 −0
Original line number Diff line number Diff line
@@ -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

packages/SystemUI/tests/src/com/android/systemui/unfold/util/ScopedUnfoldTransitionProgressProviderTest.kt

0 → 100644
+161 −0
Original line number Diff line number Diff line 
/*

 * 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())) }

                }

            }

        }

    }

}