From 4130a68a92b2c5264917d9ac37b51b74aa7993b9 Mon Sep 17 00:00:00 2001 From: Jorim Jaggi Date: Tue, 9 Jan 2018 14:28:44 +0100 Subject: [PATCH] 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 --- .../android/server/wm/AppWindowThumbnail.java | 3 ++- .../android/server/wm/SurfaceAnimator.java | 13 +++++++---- .../com/android/server/wm/WindowAnimator.java | 8 +++---- .../android/server/wm/WindowContainer.java | 3 ++- .../server/wm/WindowManagerService.java | 7 +++--- .../server/wm/SurfaceAnimationRunnerTest.java | 7 +++++- .../server/wm/SurfaceAnimatorTest.java | 23 +------------------ .../android/server/wm/WindowTestsBase.java | 1 - 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 c16a5315060f7..0d65790ce9e56 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 a32e711df5342..e67cdbaac35bd 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 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 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 7295873754214..3efd6ac0afefb 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 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 5d445eff0c922..5548f3d727862 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 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 0a2ffbc96fe56..e91b16d013c63 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 4831fcd67314b..dad70a5872bc0 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 463ceeb9f2a27..64c3037006399 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 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 ff840f3aeea91..c699a94db279b 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;