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

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

@@ -1923,6 +1923,8 @@ class DisplayContent extends WindowContainer<DisplayContent.DisplayChildWindowCo
                     mService.unregisterPointerEventListener(mService.mMousePositionTracker);
                 }
             }
+            mService.mAnimator.removeDisplayLocked(mDisplayId);
+
             // The pending transaction won't be applied so we should
             // just clean up any surfaces pending destruction.
             onPendingTransactionApplied();

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

@@ -72,23 +72,29 @@ class SurfaceAnimator {
                     target.mInnerAnimationFinishedCallback.onAnimationFinished(anim);
                     return;
                 }
-                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 {
-                    // TODO: This should use pendingTransaction eventually, but right now things
-                    // happening on the animation finished callback are happening on the global
-                    // transaction.
-                    SurfaceControl.mergeToGlobalTransaction(t);
-                    SurfaceControl.closeTransaction();
-                }
+                // TODO: This should use pendingTransaction eventually, but right now things
+                // happening on the animation finished callback are happening on the global
+                // transaction.
+                // For now we need to run this after it's guaranteed that the transaction that
+                // reparents the surface onto the leash is executed already. Otherwise this may be
+                // executed first, leading to surface loss, as the reparent operations wouldn't
+                // be in order.
+                mService.mAnimator.addAfterPrepareSurfacesRunnable(() -> {
+                    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();
+                    }
+                });
             }
         };
     }

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<Runnable> 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();
+    }
 }

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;

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;