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

Commit 6c7e5619 authored by Jorim Jaggi's avatar Jorim Jaggi Committed by Winson Chung
Browse files

Fix possible race conditions when cancelling animations 

Since the surface is being released by the SurfaceAnimator, it was
possible that SurfaceAnimationRunner was still applying surface
changes on a released surface.

Solve this by introducing a cancel-lock on which all surface
operations are synchronized on.

Bug: 64674361
Test: SurfaceAnimationRunnerTest
Change-Id: I06ee9e8270f492faa1cbfd84a09a68c9a1a09ade
parent a5e10572

services/core/java/com/android/server/wm/SurfaceAnimationRunner.java

+51 −26
Original line number Diff line number Diff line
@@ -43,12 +43,19 @@ class SurfaceAnimationRunner { 


    private final Object mLock = new Object();



    /**

     * Lock for cancelling animations. Must be acquired on it's own, or after acquiring

     * {@link #mLock}

     */

    private final Object mCancelLock = new Object();



    @VisibleForTesting

    Choreographer mChoreographer;



    private final Runnable mApplyTransactionRunnable = this::applyTransaction;

    private final AnimationHandler mAnimationHandler;

    private final Transaction mFrameTransaction;

    private final AnimatorFactory mAnimatorFactory;

    private boolean mApplyScheduled;



    @GuardedBy("mLock")
@@ -57,15 +64,15 @@ class SurfaceAnimationRunner { 


    @GuardedBy("mLock")

    @VisibleForTesting

    final ArrayMap<SurfaceControl, ValueAnimator> mRunningAnimations = new ArrayMap<>();

    final ArrayMap<SurfaceControl, RunningAnimation> mRunningAnimations = new ArrayMap<>();



    SurfaceAnimationRunner() {

        this(null /* callbackProvider */, new Transaction());

        this(null /* callbackProvider */, null /* animatorFactory */, new Transaction());

    }



    @VisibleForTesting

    SurfaceAnimationRunner(@Nullable AnimationFrameCallbackProvider callbackProvider,

            Transaction frameTransaction) {

            AnimatorFactory animatorFactory, Transaction frameTransaction) {

        SurfaceAnimationThread.getHandler().runWithScissors(() -> mChoreographer = getSfInstance(),

                0 /* timeout */);

        mFrameTransaction = frameTransaction;
@@ -73,6 +80,9 @@ class SurfaceAnimationRunner { 
        mAnimationHandler.setProvider(callbackProvider != null

                ? callbackProvider

                : new SfVsyncFrameCallbackProvider(mChoreographer));

        mAnimatorFactory = animatorFactory != null

                ? animatorFactory

                : SfValueAnimator::new;

    }



    void startAnimation(AnimationSpec a, SurfaceControl animationLeash, Transaction t,
@@ -95,11 +105,14 @@ class SurfaceAnimationRunner { 
                mPendingAnimations.remove(leash);

                return;

            }

            final ValueAnimator anim = mRunningAnimations.get(leash);

            final RunningAnimation anim = mRunningAnimations.get(leash);

            if (anim != null) {

                mRunningAnimations.remove(leash);

                synchronized (mCancelLock) {

                    anim.mCancelled = true;

                }

                SurfaceAnimationThread.getHandler().post(() -> {

                    anim.cancel();

                    anim.mAnim.cancel();

                    applyTransaction();

                });

            }
@@ -114,44 +127,47 @@ class SurfaceAnimationRunner { 
    }



    private void startAnimationLocked(RunningAnimation a) {

        final ValueAnimator result = new SfValueAnimator();

        final ValueAnimator anim = mAnimatorFactory.makeAnimator();



        // Animation length is already expected to be scaled.

        result.overrideDurationScale(1.0f);

        result.setDuration(a.mAnimSpec.getDuration());

        result.addUpdateListener(animation -> {

            applyTransformation(a, mFrameTransaction, result.getCurrentPlayTime());

        anim.overrideDurationScale(1.0f);

        anim.setDuration(a.mAnimSpec.getDuration());

        anim.addUpdateListener(animation -> {

            synchronized (mCancelLock) {

                if (!a.mCancelled) {

                    applyTransformation(a, mFrameTransaction, anim.getCurrentPlayTime());

                }

            }



            // Transaction will be applied in the commit phase.

            scheduleApplyTransaction();

        });

        result.addListener(new AnimatorListenerAdapter() {



            private boolean mCancelled;



        anim.addListener(new AnimatorListenerAdapter() {

            @Override

            public void onAnimationStart(Animator animation) {

                synchronized (mCancelLock) {

                    if (!a.mCancelled) {

                        mFrameTransaction.show(a.mLeash);

                    }



            @Override

            public void onAnimationCancel(Animator animation) {

                mCancelled = true;

                }

            }



            @Override

            public void onAnimationEnd(Animator animation) {

                synchronized (mLock) {

                    mRunningAnimations.remove(a.mLeash);

                }

                if (!mCancelled) {

                    synchronized (mCancelLock) {

                        if (!a.mCancelled) {

                            // Post on other thread that we can push final state without jank.

                            AnimationThread.getHandler().post(a.mFinishCallback);

                        }

                    }

                }

            }

        });

        result.start();

        mRunningAnimations.put(a.mLeash, result);

        anim.start();

        a.mAnim = anim;

        mRunningAnimations.put(a.mLeash, a);

    }



    private void applyTransformation(RunningAnimation a, Transaction t, long currentPlayTime) {
@@ -181,6 +197,10 @@ class SurfaceAnimationRunner { 
        final AnimationSpec mAnimSpec;

        final SurfaceControl mLeash;

        final Runnable mFinishCallback;

        ValueAnimator mAnim;



        @GuardedBy("mCancelLock")

        private boolean mCancelled;



        RunningAnimation(AnimationSpec animSpec, SurfaceControl leash, Runnable finishCallback) {

            mAnimSpec = animSpec;
@@ -189,6 +209,11 @@ class SurfaceAnimationRunner { 
        }

    }



    @VisibleForTesting

    interface AnimatorFactory {

        ValueAnimator makeAnimator();

    }



    /**

     * Value animator that uses sf-vsync signal to tick.

     */

services/tests/servicestests/src/com/android/server/wm/SurfaceAnimationRunnerTest.java

+38 −6
Original line number Diff line number Diff line
@@ -24,9 +24,13 @@ import static org.mockito.Mockito.any; 
import static org.mockito.Mockito.atLeast;

import static org.mockito.Mockito.atLeastOnce;

import static org.mockito.Mockito.eq;

import static org.mockito.Mockito.times;

import static org.mockito.Mockito.verify;

import static org.mockito.Mockito.when;



import android.animation.AnimationHandler;

import android.animation.AnimationHandler.AnimationFrameCallbackProvider;

import android.animation.ValueAnimator;

import android.graphics.Matrix;

import android.graphics.Point;

import android.platform.test.annotations.Presubmit;
@@ -40,6 +44,7 @@ import android.view.animation.Animation; 
import android.view.animation.TranslateAnimation;



import com.android.server.wm.LocalAnimationAdapter.AnimationSpec;

import com.android.server.wm.SurfaceAnimationRunner.AnimatorFactory;



import org.junit.Before;

import org.junit.Rule;
@@ -63,6 +68,7 @@ public class SurfaceAnimationRunnerTest extends WindowTestsBase { 


    @Mock SurfaceControl mMockSurface;

    @Mock Transaction mMockTransaction;

    @Mock AnimationSpec mMockAnimationSpec;

    @Rule public MockitoRule mMockitoRule = MockitoJUnit.rule();



    private SurfaceAnimationRunner mSurfaceAnimationRunner;
@@ -72,7 +78,7 @@ public class SurfaceAnimationRunnerTest extends WindowTestsBase { 
    public void setUp() throws Exception {

        super.setUp();

        mFinishCallbackLatch = new CountDownLatch(1);

        mSurfaceAnimationRunner = new SurfaceAnimationRunner(null /* callbackProvider */,

        mSurfaceAnimationRunner = new SurfaceAnimationRunner(null /* callbackProvider */, null,

                mMockTransaction);

    }
@@ -104,7 +110,7 @@ public class SurfaceAnimationRunnerTest extends WindowTestsBase { 


    @Test

    public void testCancel_notStarted() throws Exception {

        mSurfaceAnimationRunner = new SurfaceAnimationRunner(new NoOpFrameCallbackProvider(),

        mSurfaceAnimationRunner = new SurfaceAnimationRunner(new NoOpFrameCallbackProvider(), null,

                mMockTransaction);

        mSurfaceAnimationRunner

                .startAnimation(createTranslateAnimation(), mMockSurface, mMockTransaction,
@@ -117,11 +123,10 @@ public class SurfaceAnimationRunnerTest extends WindowTestsBase { 


    @Test

    public void testCancel_running() throws Exception {

        mSurfaceAnimationRunner = new SurfaceAnimationRunner(new NoOpFrameCallbackProvider(),

        mSurfaceAnimationRunner = new SurfaceAnimationRunner(new NoOpFrameCallbackProvider(), null,

                mMockTransaction);

        mSurfaceAnimationRunner

                .startAnimation(createTranslateAnimation(), mMockSurface, mMockTransaction,

                this::finishedCallback);

        mSurfaceAnimationRunner.startAnimation(createTranslateAnimation(), mMockSurface,

                mMockTransaction, this::finishedCallback);

        waitUntilNextFrame();

        assertFalse(mSurfaceAnimationRunner.mRunningAnimations.isEmpty());

        mSurfaceAnimationRunner.onAnimationCancelled(mMockSurface);
@@ -130,6 +135,33 @@ public class SurfaceAnimationRunnerTest extends WindowTestsBase { 
        assertFinishCallbackNotCalled();

    }



    @Test

    public void testCancel_sneakyCancelBeforeUpdate() throws Exception {

        mSurfaceAnimationRunner = new SurfaceAnimationRunner(null, () -> new ValueAnimator() {

            {

                setFloatValues(0f, 1f);

            }



            @Override

            public void addUpdateListener(AnimatorUpdateListener listener) {

                super.addUpdateListener(animation -> {

                    // Sneaky test cancels animation just before applying frame to simulate

                    // interleaving of multiple threads. Muahahaha

                    if (animation.getCurrentPlayTime() > 0) {

                        mSurfaceAnimationRunner.onAnimationCancelled(mMockSurface);

                    }

                    listener.onAnimationUpdate(animation);

                });

            }

        }, mMockTransaction);

        when(mMockAnimationSpec.getDuration()).thenReturn(200L);

        mSurfaceAnimationRunner.startAnimation(mMockAnimationSpec, mMockSurface, mMockTransaction,

                this::finishedCallback);

        waitUntilNextFrame();

        assertFalse(mSurfaceAnimationRunner.mRunningAnimations.isEmpty());

        verify(mMockAnimationSpec, atLeastOnce()).apply(any(), any(), eq(0L));

    }



    private void waitUntilNextFrame() throws Exception {

        final CountDownLatch latch = new CountDownLatch(1);

        mSurfaceAnimationRunner.mChoreographer.postCallback(Choreographer.CALLBACK_COMMIT,