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

Commit c085f48b authored by Luca Zuccarini's avatar Luca Zuccarini
Browse files

Clean up the controller reference after execution. 

After making the controller a member val in ag/30280320, the dispose
function was cleaning up the delegate but views were still being leaked
by this controller reference.

This CL addresses this issue for instances that do leak (i.e. when using
the Runner directly in ActivityOptions) while keeping the transition
functional for instances where it is simply registered with the
RemoteTransitionHandler.

Bug: 379268680
Flag: com.android.systemui.shared.return_animation_framework_library
Flag: com.android.systemui.shared.return_animation_framework_long_lived
Test: atest ActivityTransitionAnimatorTest
Change-Id: I2e3d9889e8cd69bc29a5bc2891dd6ab6784f7869
parent 4aa2f758

packages/SystemUI/animation/src/com/android/systemui/animation/ActivityTransitionAnimator.kt

+28 −20
Original line number Diff line number Diff line
@@ -17,6 +17,7 @@ 
package com.android.systemui.animation



import android.app.ActivityManager

import android.app.ActivityOptions

import android.app.ActivityTaskManager

import android.app.PendingIntent

import android.app.TaskInfo
@@ -434,8 +435,7 @@ constructor( 
                    private fun cleanUp() {

                        cleanUpRunnable?.run()

                    }

                },

                initializeLazily = longLivedReturnAnimationsEnabled(),

                }

            )



        // mTypeSet and mModes match back signals only, and not home. This is on purpose, because
@@ -478,8 +478,8 @@ constructor( 
    /** Create a new animation [Runner] controlled by [controller]. */

    @VisibleForTesting

    @JvmOverloads

    fun createRunner(controller: Controller, initializeLazily: Boolean = false): Runner {

        if (initializeLazily) assertLongLivedReturnAnimations()

    fun createRunner(controller: Controller, longLived: Boolean = false): Runner {

        if (longLived) assertLongLivedReturnAnimations()



        // Make sure we use the modified timings when animating a dialog into an app.

        val transitionAnimator =
@@ -489,13 +489,7 @@ constructor( 
                transitionAnimator

            }



        return Runner(

            controller,

            callback!!,

            transitionAnimator,

            lifecycleListener,

            initializeLazily,

        )

        return Runner(controller, callback!!, transitionAnimator, lifecycleListener, longLived)

    }



    interface PendingIntentStarter {
@@ -699,7 +693,7 @@ constructor( 
            }

        val launchRemoteTransition =

            RemoteTransition(

                OriginTransition(createRunner(controller, initializeLazily = true)),

                OriginTransition(createRunner(controller, longLived = true)),

                "${cookie}_launchTransition",

            )

        transitionRegister.register(launchFilter, launchRemoteTransition, includeTakeover = true)
@@ -721,7 +715,7 @@ constructor( 
            }

        val returnRemoteTransition =

            RemoteTransition(

                OriginTransition(createRunner(returnController, initializeLazily = true)),

                OriginTransition(createRunner(returnController, longLived = true)),

                "${cookie}_returnTransition",

            )

        transitionRegister.register(returnFilter, returnRemoteTransition, includeTakeover = true)
@@ -910,14 +904,22 @@ constructor( 


    @VisibleForTesting

    inner class Runner(

        private val controller: Controller,

        /**

         * This can hold a reference to a view, so it needs to be cleaned up and can't be held on to

         * forever when ![longLived].

         */

        private var controller: Controller?,

        private val callback: Callback,

        /** The animator to use to animate the window transition. */

        private val transitionAnimator: TransitionAnimator,

        /** Listener for animation lifecycle events. */

        private val listener: Listener? = null,

        /** Whether the internal [delegate] should be initialized lazily. */

        private val initializeLazily: Boolean = false,

        /**

         * Whether the internal should be kept around after execution for later usage. IMPORTANT:

         * should always be false if this [Runner] is to be used directly with [ActivityOptions]

         * (i.e. for ephemeral launches), or the controller will leak its view.

         */

        private val longLived: Boolean = false,

    ) : IRemoteAnimationRunner.Stub() {

        // This is being passed across IPC boundaries and cycles (through PendingIntentRecords,

        // etc.) are possible. So we need to make sure we drop any references that might
@@ -926,7 +928,7 @@ constructor( 


        init {

            delegate = null

            if (!initializeLazily) {

            if (!longLived) {

                // Ephemeral launches bundle the runner with the launch request (instead of being

                // registered ahead of time for later use). This means that there could be a timeout

                // between creation and invocation, so the delegate needs to exist from the
@@ -1004,16 +1006,17 @@ constructor( 


        @AnyThread

        private fun maybeSetUp() {

            if (!initializeLazily || delegate != null) return

            if (!longLived || delegate != null) return

            createDelegate()

        }



        @AnyThread

        private fun createDelegate() {

            if (controller == null) return

            delegate =

                AnimationDelegate(

                    mainExecutor,

                    controller,

                    controller!!,

                    callback,

                    DelegatingAnimationCompletionListener(listener, this::dispose),

                    transitionAnimator,
@@ -1025,7 +1028,12 @@ constructor( 
        fun dispose() {

            // Drop references to animation controller once we're done with the animation

            // to avoid leaking.

            mainExecutor.execute { delegate = null }

            mainExecutor.execute {

                delegate = null

                // When long lived, the same Runner can be used more than once. In this case we need

                // to keep the controller around so we can rebuild the delegate on demand.

                if (!longLived) controller = null

            }

        }

    }

packages/SystemUI/tests/src/com/android/systemui/animation/ActivityTransitionAnimatorTest.kt

+5 −5
Original line number Diff line number Diff line
@@ -404,7 +404,7 @@ class ActivityTransitionAnimatorTest : SysuiTestCase() { 
    @Test

    fun creatingRunnerWithLazyInitializationThrows_whenTheFlagsAreDisabled() {

        assertThrows(IllegalStateException::class.java) {

            activityTransitionAnimator.createRunner(controller, initializeLazily = true)

            activityTransitionAnimator.createRunner(controller, longLived = true)

        }

    }
@@ -414,7 +414,7 @@ class ActivityTransitionAnimatorTest : SysuiTestCase() { 
    )

    @Test

    fun runnerCreatesDelegateLazily_whenPostingTimeouts() {

        val runner = activityTransitionAnimator.createRunner(controller, initializeLazily = true)

        val runner = activityTransitionAnimator.createRunner(controller, longLived = true)

        assertNull(runner.delegate)

        runner.postTimeouts()

        assertNotNull(runner.delegate)
@@ -426,7 +426,7 @@ class ActivityTransitionAnimatorTest : SysuiTestCase() { 
    )

    @Test

    fun runnerCreatesDelegateLazily_onAnimationStart() {

        val runner = activityTransitionAnimator.createRunner(controller, initializeLazily = true)

        val runner = activityTransitionAnimator.createRunner(controller, longLived = true)

        assertNull(runner.delegate)



        // The delegate is cleaned up after execution (which happens in another thread), so what we
@@ -458,7 +458,7 @@ class ActivityTransitionAnimatorTest : SysuiTestCase() { 
    )

    @Test

    fun runnerCreatesDelegateLazily_onAnimationTakeover() {

        val runner = activityTransitionAnimator.createRunner(controller, initializeLazily = true)

        val runner = activityTransitionAnimator.createRunner(controller, longLived = true)

        assertNull(runner.delegate)



        // The delegate is cleaned up after execution (which happens in another thread), so what we
@@ -489,7 +489,7 @@ class ActivityTransitionAnimatorTest : SysuiTestCase() { 
    )

    @Test

    fun animationTakeoverThrows_whenTheFlagsAreDisabled() {

        val runner = activityTransitionAnimator.createRunner(controller, initializeLazily = false)

        val runner = activityTransitionAnimator.createRunner(controller, longLived = false)

        assertThrows(IllegalStateException::class.java) {

            runner.takeOverAnimation(

                arrayOf(fakeWindow()),