Fix SurfaceAnimator and SurfaceAnimationRunner tests

Since we marked mAnimator.mInitialized to true in the tests,
WM executed things from another thread during tests leading to
concurrency bugs.

Instead, we stub out addAfterPrepareSurfacesRunnable to a consumer
which executes the runnable directly during tests, avoiding the
need to let WM process animation frames.

Also attempts to fix flakyness in SurfaceAnimationRunner

Test: go/wm-smoke
Test: SurfaceAnimatorTest
Test: SurfaceAnimationRunnerTest

Change-Id: Ic9522e1afef6ce62667aefca80e58d6fb1db3424
Fixes: 71650763
Fixes: 71602314
Bug: 71719744

8 files changed, 27 insertions, 38 deletions

diff --git a/services/core/java/com/android/server/wm/AppWindowThumbnail.java b/services/core/java/com/android/server/wm/AppWindowThumbnail.java
index c16a5315060f..0d65790ce9e5 100644
--- a/services/core/java/com/android/server/wm/AppWindowThumbnail.java
+++ b/services/core/java/com/android/server/wm/AppWindowThumbnail.java
@@ -49,7 +49,8 @@ class AppWindowThumbnail implements Animatable {
 
     AppWindowThumbnail(Transaction t, AppWindowToken appToken, GraphicBuffer thumbnailHeader) {
         mAppToken = appToken;
-        mSurfaceAnimator = new SurfaceAnimator(this, this::onAnimationFinished, appToken.mService);
+        mSurfaceAnimator = new SurfaceAnimator(this, this::onAnimationFinished,
+                appToken.mService.mAnimator::addAfterPrepareSurfacesRunnable, appToken.mService);
         mWidth = thumbnailHeader.getWidth();
         mHeight = thumbnailHeader.getHeight();
 
diff --git a/services/core/java/com/android/server/wm/SurfaceAnimator.java b/services/core/java/com/android/server/wm/SurfaceAnimator.java
index a32e711df534..e67cdbaac35b 100644
--- a/services/core/java/com/android/server/wm/SurfaceAnimator.java
+++ b/services/core/java/com/android/server/wm/SurfaceAnimator.java
@@ -30,6 +30,7 @@ import android.view.SurfaceControl.Transaction;
 import com.android.internal.annotations.VisibleForTesting;
 
 import java.io.PrintWriter;
+import java.util.function.Consumer;
 
 /**
  * A class that can run animations on objects that have a set of child surfaces. We do this by
@@ -55,16 +56,20 @@ class SurfaceAnimator {
     /**
      * @param animatable The object to animate.
      * @param animationFinishedCallback Callback to invoke when an animation has finished running.
+     * @param addAfterPrepareSurfaces Consumer that takes a runnable and executes it after preparing
+     *                                surfaces in WM. Can be implemented differently during testing.
      */
     SurfaceAnimator(Animatable animatable, Runnable animationFinishedCallback,
-            WindowManagerService service) {
+            Consumer<Runnable> addAfterPrepareSurfaces, WindowManagerService service) {
         mAnimatable = animatable;
         mService = service;
         mAnimationFinishedCallback = animationFinishedCallback;
-        mInnerAnimationFinishedCallback = getFinishedCallback(animationFinishedCallback);
+        mInnerAnimationFinishedCallback = getFinishedCallback(animationFinishedCallback,
+                addAfterPrepareSurfaces);
     }
 
-    private OnAnimationFinishedCallback getFinishedCallback(Runnable animationFinishedCallback) {
+    private OnAnimationFinishedCallback getFinishedCallback(Runnable animationFinishedCallback,
+            Consumer<Runnable> addAfterPrepareSurfaces) {
         return anim -> {
             synchronized (mService.mWindowMap) {
                 final SurfaceAnimator target = mService.mAnimationTransferMap.remove(anim);
@@ -80,7 +85,7 @@ class SurfaceAnimator {
                 // 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(() -> {
+                addAfterPrepareSurfaces.accept(() -> {
                     if (anim != mAnimation) {
                         // Callback was from another animation - ignore.
                         return;
diff --git a/services/core/java/com/android/server/wm/WindowAnimator.java b/services/core/java/com/android/server/wm/WindowAnimator.java
index 729587375421..3efd6ac0afef 100644
--- a/services/core/java/com/android/server/wm/WindowAnimator.java
+++ b/services/core/java/com/android/server/wm/WindowAnimator.java
@@ -47,7 +47,6 @@ public class WindowAnimator {
     final WindowManagerService mService;
     final Context mContext;
     final WindowManagerPolicy mPolicy;
-    private final WindowSurfacePlacer mWindowPlacerLocked;
 
     /** Is any window animating? */
     private boolean mAnimating;
@@ -74,7 +73,7 @@ public class WindowAnimator {
 
     SparseArray<DisplayContentsAnimator> mDisplayContentsAnimators = new SparseArray<>(2);
 
-    boolean mInitialized = false;
+    private boolean mInitialized = false;
 
     // When set to true the animator will go over all windows after an animation frame is posted and
     // check if some got replaced and can be removed.
@@ -98,7 +97,6 @@ public class WindowAnimator {
         mService = service;
         mContext = service.mContext;
         mPolicy = service.mPolicy;
-        mWindowPlacerLocked = service.mWindowPlacerLocked;
         AnimationThread.getHandler().runWithScissors(
                 () -> mChoreographer = Choreographer.getSfInstance(), 0 /* timeout */);
 
@@ -241,7 +239,7 @@ public class WindowAnimator {
             }
 
             if (hasPendingLayoutChanges || doRequest) {
-                mWindowPlacerLocked.requestTraversal();
+                mService.mWindowPlacerLocked.requestTraversal();
             }
 
             final boolean rootAnimating = mService.mRoot.isSelfOrChildAnimating();
@@ -254,7 +252,7 @@ public class WindowAnimator {
                 Trace.asyncTraceBegin(Trace.TRACE_TAG_WINDOW_MANAGER, "animating", 0);
             }
             if (!rootAnimating && mLastRootAnimating) {
-                mWindowPlacerLocked.requestTraversal();
+                mService.mWindowPlacerLocked.requestTraversal();
                 mService.mTaskSnapshotController.setPersisterPaused(false);
                 Trace.asyncTraceEnd(Trace.TRACE_TAG_WINDOW_MANAGER, "animating", 0);
             }
diff --git a/services/core/java/com/android/server/wm/WindowContainer.java b/services/core/java/com/android/server/wm/WindowContainer.java
index 5d445eff0c92..5548f3d72786 100644
--- a/services/core/java/com/android/server/wm/WindowContainer.java
+++ b/services/core/java/com/android/server/wm/WindowContainer.java
@@ -102,7 +102,8 @@ class WindowContainer<E extends WindowContainer> extends ConfigurationContainer<
     WindowContainer(WindowManagerService service) {
         mService = service;
         mPendingTransaction = service.mTransactionFactory.make();
-        mSurfaceAnimator = new SurfaceAnimator(this, this::onAnimationFinished, service);
+        mSurfaceAnimator = new SurfaceAnimator(this, this::onAnimationFinished,
+                service.mAnimator::addAfterPrepareSurfacesRunnable, service);
     }
 
     @Override
diff --git a/services/core/java/com/android/server/wm/WindowManagerService.java b/services/core/java/com/android/server/wm/WindowManagerService.java
index 0a2ffbc96fe5..e91b16d013c6 100644
--- a/services/core/java/com/android/server/wm/WindowManagerService.java
+++ b/services/core/java/com/android/server/wm/WindowManagerService.java
@@ -928,7 +928,6 @@ public class WindowManagerService extends IWindowManager.Stub
             boolean haveInputMethods, boolean showBootMsgs, boolean onlyCore,
             WindowManagerPolicy policy) {
         installLock(this, INDEX_WINDOW);
-        mRoot = new RootWindowContainer(this);
         mContext = context;
         mHaveInputMethods = haveInputMethods;
         mAllowBootMessages = showBootMsgs;
@@ -952,8 +951,11 @@ public class WindowManagerService extends IWindowManager.Stub
         mDisplaySettings = new DisplaySettings();
         mDisplaySettings.readSettingsLocked();
 
-        mWindowPlacerLocked = new WindowSurfacePlacer(this);
         mPolicy = policy;
+        mAnimator = new WindowAnimator(this);
+        mRoot = new RootWindowContainer(this);
+
+        mWindowPlacerLocked = new WindowSurfacePlacer(this);
         mTaskSnapshotController = new TaskSnapshotController(this);
 
         mWindowTracing = WindowTracing.createDefaultAndStartLooper(context);
@@ -1051,7 +1053,6 @@ public class WindowManagerService extends IWindowManager.Stub
                 PowerManager.SCREEN_BRIGHT_WAKE_LOCK | PowerManager.ON_AFTER_RELEASE, TAG_WM);
         mHoldingScreenWakeLock.setReferenceCounted(false);
 
-        mAnimator = new WindowAnimator(this);
         mSurfaceAnimationRunner = new SurfaceAnimationRunner();
 
         mAllowTheaterModeWakeFromLayout = context.getResources().getBoolean(
diff --git a/services/tests/servicestests/src/com/android/server/wm/SurfaceAnimationRunnerTest.java b/services/tests/servicestests/src/com/android/server/wm/SurfaceAnimationRunnerTest.java
index 4831fcd67314..dad70a5872bc 100644
--- a/services/tests/servicestests/src/com/android/server/wm/SurfaceAnimationRunnerTest.java
+++ b/services/tests/servicestests/src/com/android/server/wm/SurfaceAnimationRunnerTest.java
@@ -36,6 +36,7 @@ import android.graphics.Point;
 import android.platform.test.annotations.Presubmit;
 import android.support.test.filters.SmallTest;
 import android.support.test.runner.AndroidJUnit4;
+import android.util.Log;
 import android.view.Choreographer;
 import android.view.Choreographer.FrameCallback;
 import android.view.SurfaceControl;
@@ -157,8 +158,12 @@ public class SurfaceAnimationRunnerTest extends WindowTestsBase {
         when(mMockAnimationSpec.getDuration()).thenReturn(200L);
         mSurfaceAnimationRunner.startAnimation(mMockAnimationSpec, mMockSurface, mMockTransaction,
                 this::finishedCallback);
+
+        // We need to wait for two frames: The first frame starts the animation, the second frame
+        // actually cancels the animation.
         waitUntilNextFrame();
-        assertFalse(mSurfaceAnimationRunner.mRunningAnimations.isEmpty());
+        waitUntilNextFrame();
+        assertTrue(mSurfaceAnimationRunner.mRunningAnimations.isEmpty());
         verify(mMockAnimationSpec, atLeastOnce()).apply(any(), any(), eq(0L));
     }
 
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 463ceeb9f2a2..64c303700639 100644
--- a/services/tests/servicestests/src/com/android/server/wm/SurfaceAnimatorTest.java
+++ b/services/tests/servicestests/src/com/android/server/wm/SurfaceAnimatorTest.java
@@ -75,10 +75,7 @@ public class SurfaceAnimatorTest extends WindowTestsBase {
         mAnimatable2 = new MyAnimatable();
     }
 
-    // TODO: Tests are flaky, and timeout after 5 minutes. Instead of wasting everybody's time we
-    // mark them as ignore.
     @Test
-    @Ignore
     public void testRunAnimation() throws Exception {
         mAnimatable.mSurfaceAnimator.startAnimation(mTransaction, mSpec, true /* hidden */);
         final ArgumentCaptor<OnAnimationFinishedCallback> callbackCaptor = ArgumentCaptor.forClass(
@@ -88,17 +85,13 @@ 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));
         // TODO: Verify reparenting once we use mPendingTransaction to reparent it back
     }
 
-    // TODO: Tests are flaky, and timeout after 5 minutes. Instead of wasting everybody's time we
-    // mark them as ignore.
     @Test
-    @Ignore
     public void testOverrideAnimation() throws Exception {
         mAnimatable.mSurfaceAnimator.startAnimation(mTransaction, mSpec, true /* hidden */);
         final SurfaceControl firstLeash = mAnimatable.mLeash;
@@ -114,13 +107,11 @@ 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);
     }
@@ -157,10 +148,7 @@ public class SurfaceAnimatorTest extends WindowTestsBase {
         assertTrue(mAnimatable.mPendingDestroySurfaces.contains(mAnimatable.mLeash));
     }
 
-    // TODO: Tests are flaky, and timeout after 5 minutes. Instead of wasting everybody's time we
-    // mark them as ignore.
     @Test
-    @Ignore
     public void testTransferAnimation() throws Exception {
         mAnimatable.mSurfaceAnimator.startAnimation(mTransaction, mSpec, true /* hidden */);
 
@@ -175,7 +163,6 @@ 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));
@@ -191,14 +178,6 @@ 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;
@@ -219,7 +198,7 @@ public class SurfaceAnimatorTest extends WindowTestsBase {
                     .build();
             mFinishedCallbackCalled = false;
             mLeash = null;
-            mSurfaceAnimator = new SurfaceAnimator(this, mFinishedCallback, sWm);
+            mSurfaceAnimator = new SurfaceAnimator(this, mFinishedCallback, Runnable::run, sWm);
         }
 
         @Override
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 ff840f3aeea9..c699a94db279 100644
--- a/services/tests/servicestests/src/com/android/server/wm/WindowTestsBase.java
+++ b/services/tests/servicestests/src/com/android/server/wm/WindowTestsBase.java
@@ -103,7 +103,6 @@ class WindowTestsBase {
 
         context.getDisplay().getDisplayInfo(mDisplayInfo);
         mDisplayContent = createNewDisplay();
-        sWm.mAnimator.mInitialized = true;
         sWm.mDisplayEnabled = true;
         sWm.mDisplayReady = true;