From a384403e1297bd61c76907631c31bf29d33fd6da Mon Sep 17 00:00:00 2001 From: Jorim Jaggi Date: Wed, 3 Jan 2018 15:54:43 +0100 Subject: [PATCH] Fix issue with 0 duration animations If the animation length was 0, it was possible that the finish runnable is run before applying the pending transaction to reparent the surface onto the leash. In that case, the reparent to the leash will be executed after, taking precedence. Then, the leash gets destroyed, and we loose the surface, leading to all kinds of crashes. Test: Disable animation duration scale, open a couple of apps, observe no crash. Test: go/wm-smoke Change-Id: I04db7b7c1c3295779b8afead97d7850f808f9081 Fixes: 71499373 --- .../com/android/server/wm/DisplayContent.java | 2 + .../android/server/wm/SurfaceAnimator.java | 38 +++++++++++-------- .../com/android/server/wm/WindowAnimator.java | 27 +++++++++++++ .../server/wm/SurfaceAnimatorTest.java | 13 +++++++ .../android/server/wm/WindowTestsBase.java | 1 + 5 files changed, 65 insertions(+), 16 deletions(-) diff --git a/services/core/java/com/android/server/wm/DisplayContent.java b/services/core/java/com/android/server/wm/DisplayContent.java index d053015cea821..63dfbc2c25b65 100644 --- a/services/core/java/com/android/server/wm/DisplayContent.java +++ b/services/core/java/com/android/server/wm/DisplayContent.java @@ -1923,6 +1923,8 @@ class DisplayContent extends WindowContainer { + if (anim != mAnimation) { + // Callback was from another animation - ignore. + return; + } + final Transaction t = new Transaction(); + SurfaceControl.openTransaction(); + try { + reset(t, true /* destroyLeash */); + animationFinishedCallback.run(); + } finally { + SurfaceControl.mergeToGlobalTransaction(t); + SurfaceControl.closeTransaction(); + } + }); } }; } diff --git a/services/core/java/com/android/server/wm/WindowAnimator.java b/services/core/java/com/android/server/wm/WindowAnimator.java index 8bceb640aa500..7295873754214 100644 --- a/services/core/java/com/android/server/wm/WindowAnimator.java +++ b/services/core/java/com/android/server/wm/WindowAnimator.java @@ -35,6 +35,7 @@ import com.android.server.AnimationThread; import com.android.server.policy.WindowManagerPolicy; import java.io.PrintWriter; +import java.util.ArrayList; /** * Singleton class that carries out the animations and Surface operations in a separate task @@ -87,6 +88,12 @@ public class WindowAnimator { */ private boolean mAnimationFrameCallbackScheduled; + /** + * A list of runnable that need to be run after {@link WindowContainer#prepareSurfaces} is + * executed and the corresponding transaction is closed and applied. + */ + private final ArrayList mAfterPrepareSurfacesRunnables = new ArrayList<>(); + WindowAnimator(final WindowManagerService service) { mService = service; mContext = service.mContext; @@ -262,6 +269,7 @@ public class WindowAnimator { mService.destroyPreservedSurfaceLocked(); mService.mWindowPlacerLocked.destroyPendingSurfaces(); + executeAfterPrepareSurfacesRunnables(); if (DEBUG_WINDOW_TRACE) { Slog.i(TAG, "!!! animate: exit mAnimating=" + mAnimating @@ -425,4 +433,23 @@ public class WindowAnimator { void orAnimating(boolean animating) { mAnimating |= animating; } + + /** + * Adds a runnable to be executed after {@link WindowContainer#prepareSurfaces} is called and + * the corresponding transaction is closed and applied. + */ + void addAfterPrepareSurfacesRunnable(Runnable r) { + mAfterPrepareSurfacesRunnables.add(r); + scheduleAnimation(); + } + + private void executeAfterPrepareSurfacesRunnables() { + + // Traverse in order they were added. + final int size = mAfterPrepareSurfacesRunnables.size(); + for (int i = 0; i < size; i++) { + mAfterPrepareSurfacesRunnables.get(i).run(); + } + mAfterPrepareSurfacesRunnables.clear(); + } } diff --git a/services/tests/servicestests/src/com/android/server/wm/SurfaceAnimatorTest.java b/services/tests/servicestests/src/com/android/server/wm/SurfaceAnimatorTest.java index 96ff461950025..3da560705ef45 100644 --- a/services/tests/servicestests/src/com/android/server/wm/SurfaceAnimatorTest.java +++ b/services/tests/servicestests/src/com/android/server/wm/SurfaceAnimatorTest.java @@ -45,6 +45,7 @@ import org.mockito.Mock; import org.mockito.MockitoAnnotations; import java.util.ArrayList; +import java.util.concurrent.CountDownLatch; /** * Test class for {@link SurfaceAnimatorTest}. @@ -82,6 +83,7 @@ public class SurfaceAnimatorTest extends WindowTestsBase { verify(mSpec).startAnimation(any(), any(), callbackCaptor.capture()); callbackCaptor.getValue().onAnimationFinished(mSpec); + waitUntilPrepareSurfaces(); assertNotAnimating(mAnimatable); assertTrue(mAnimatable.mFinishedCallbackCalled); assertTrue(mAnimatable.mPendingDestroySurfaces.contains(mAnimatable.mLeash)); @@ -104,11 +106,13 @@ public class SurfaceAnimatorTest extends WindowTestsBase { // First animation was finished, but this shouldn't cancel the second animation callbackCaptor.getValue().onAnimationFinished(mSpec); + waitUntilPrepareSurfaces(); assertTrue(mAnimatable.mSurfaceAnimator.isAnimating()); // Second animation was finished verify(mSpec2).startAnimation(any(), any(), callbackCaptor.capture()); callbackCaptor.getValue().onAnimationFinished(mSpec2); + waitUntilPrepareSurfaces(); assertNotAnimating(mAnimatable); assertTrue(mAnimatable.mFinishedCallbackCalled); } @@ -160,6 +164,7 @@ public class SurfaceAnimatorTest extends WindowTestsBase { assertEquals(leash, mAnimatable2.mSurfaceAnimator.mLeash); assertFalse(mAnimatable.mPendingDestroySurfaces.contains(leash)); callbackCaptor.getValue().onAnimationFinished(mSpec); + waitUntilPrepareSurfaces(); assertNotAnimating(mAnimatable2); assertTrue(mAnimatable2.mFinishedCallbackCalled); assertTrue(mAnimatable2.mPendingDestroySurfaces.contains(leash)); @@ -175,6 +180,14 @@ public class SurfaceAnimatorTest extends WindowTestsBase { assertNull(animatable.mSurfaceAnimator.getAnimation()); } + private void waitUntilPrepareSurfaces() throws Exception { + final CountDownLatch latch = new CountDownLatch(1); + synchronized (sWm.mWindowMap) { + sWm.mAnimator.addAfterPrepareSurfacesRunnable(latch::countDown); + } + latch.await(); + } + private class MyAnimatable implements Animatable { final SurfaceControl mParent; diff --git a/services/tests/servicestests/src/com/android/server/wm/WindowTestsBase.java b/services/tests/servicestests/src/com/android/server/wm/WindowTestsBase.java index c699a94db279b..ff840f3aeea91 100644 --- a/services/tests/servicestests/src/com/android/server/wm/WindowTestsBase.java +++ b/services/tests/servicestests/src/com/android/server/wm/WindowTestsBase.java @@ -103,6 +103,7 @@ class WindowTestsBase { context.getDisplay().getDisplayInfo(mDisplayInfo); mDisplayContent = createNewDisplay(); + sWm.mAnimator.mInitialized = true; sWm.mDisplayEnabled = true; sWm.mDisplayReady = true;