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

Commit 836dac49 authored by Jorim Jaggi's avatar Jorim Jaggi
Browse files

Fix out-of-order transactions (1/2) 

The following sequence of order may happen which cause wrong
surface positions:
- WA.animate updates surfaces properties to S
- WA.animate closes the surface transaction
- Since the previous animation transaction wasn't commited yet,
closeSurfaceTransaction blocks and updating the surface properties
on SF side is deferred.
- In the meantime, since we are not holding WM lock, we have
another thread updating surfaces properties to S'
- Closing the transaction in this thread completes immediately
because it's not a synchronous transaction or animation
transaction.
- After a frame has been processed S gets applied on SF side as
the other transaction is done waiting for the frame to complete.

The issue here is that properties are now set to S instead of S'.
Sad!

We originally started calling closeTransaction without the WM
lock being held because it lead to thread starvation (b/38192114).
However, that fix has this big flaw as described above.

To fix this, we create an empty animation transaction before
updating the animation properties to simulate the back-pressuring
behavior of animation transactions without the WM lock being held.
If that transaction arrives out of order, it doesn't matter at all
because it is empty.

After that, we perform the animation udpate in a transaction that
is not marked as an animation transaction, and thus will not
block, which avoids the starvation issue.

Part of this change is also a change in SF to allow executing
empty animation transactions.

Test: go/wm-smoke
Test: Open VideoPlayer from VRCore, close it, observe no wrong
positiioning of surfaces.
Test: Inspect traces while animating. Ensure back pressuring still
works.

Change-Id: Ie545463e71e0d1bc73439d14381077a290d2f959
Fixes: 63905190
Bug: 38192114
parent 4499576d

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

+42 −31
Original line number Diff line number Diff line
@@ -33,7 +33,6 @@ import android.view.Choreographer; 
import android.view.SurfaceControl;

import android.view.WindowManagerPolicy;



import com.android.internal.view.SurfaceFlingerVsyncChoreographer;

import com.android.server.AnimationThread;



import java.io.PrintWriter;
@@ -134,13 +133,27 @@ public class WindowAnimator { 
     * sure other threads can make progress if this happens.

     */

    private void animate(long frameTimeNs) {

        boolean transactionOpen = false;

        try {



        synchronized (mService.mWindowMap) {

            if (!mInitialized) {

                return;

            }



            // Schedule next frame already such that back-pressure happens continuously

            scheduleAnimation();

        }



        // Simulate back-pressure by opening and closing an empty animation transaction. This makes

        // sure that an animation frame is at least presented once on the screen. We do this outside

        // of the regular transaction such that we can avoid holding the window manager lock in case

        // we receive back-pressure from SurfaceFlinger. Since closing an animation transaction

        // without the window manager locks leads to ordering issues (as the transaction will be

        // processed only at the beginning of the next frame which may result in another transaction

        // that was executed later in WM side gets executed first on SF side), we don't update any

        // Surface properties here such that reordering doesn't cause issues.

        mService.executeEmptyAnimationTransaction();



        synchronized (mService.mWindowMap) {

            mCurrentTime = frameTimeNs / TimeUtils.NANOS_PER_MS;

            mBulkUpdateParams = SET_ORIENTATION_CHANGE_COMPLETE;

            mAnimating = false;
@@ -151,9 +164,7 @@ public class WindowAnimator { 


            if (SHOW_TRANSACTIONS) Slog.i(TAG, ">>> OPEN TRANSACTION animate");

            mService.openSurfaceTransaction();

                transactionOpen = true;

                SurfaceControl.setAnimationTransaction();



            try {

                final AccessibilityController accessibilityController =

                        mService.mAccessibilityController;

                final int numDisplays = mDisplayContentsAnimators.size();
@@ -216,27 +227,20 @@ public class WindowAnimator { 
                    mAnimating |= mService.mDragState.stepAnimationLocked(mCurrentTime);

                }



                if (mAnimating) {

                    mService.scheduleAnimationLocked();

                if (!mAnimating) {

                    cancelAnimation();

                }



                if (mService.mWatermark != null) {

                    mService.mWatermark.drawIfNeeded();

                }

            }

            } catch (RuntimeException e) {

                Slog.wtf(TAG, "Unhandled exception in Window Manager", e);

            } finally {

            if (transactionOpen) {



                // Do not hold window manager lock while closing the transaction, as this might be

                // blocking until the next frame, which can lead to total lock starvation.

                mService.closeSurfaceTransaction(false /* withLockHeld */);

                mService.closeSurfaceTransaction();

                if (SHOW_TRANSACTIONS) Slog.i(TAG, "<<< CLOSE TRANSACTION animate");

            }

        }



        synchronized (mService.mWindowMap) {

            boolean hasPendingLayoutChanges = mService.mRoot.hasPendingLayoutChanges(this);

            boolean doRequest = false;

            if (mBulkUpdateParams != 0) {
@@ -404,6 +408,13 @@ public class WindowAnimator { 
        }

    }



    private void cancelAnimation() {

        if (mAnimationFrameCallbackScheduled) {

            mAnimationFrameCallbackScheduled = false;

            mChoreographer.removeFrameCallback(mAnimationFrameCallback);

        }

    }



    private class DisplayContentsAnimator {

        ScreenRotationAnimation mScreenRotationAnimation = null;

    }

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

+29 −14
Original line number Diff line number Diff line
@@ -908,31 +908,46 @@ public class WindowManagerService extends IWindowManager.Stub 
        }

    }



    void closeSurfaceTransaction() {

        closeSurfaceTransaction(true /* withLockHeld */);

    }



    /**

     * Closes a surface transaction.

     *

     * @param withLockHeld Whether to acquire the window manager while doing so. In some cases

     *                     holding the lock my lead to starvation in WM in case closeTransaction

     *                     blocks and we call it repeatedly, like we do for animations.

     */

    void closeSurfaceTransaction(boolean withLockHeld) {

    void closeSurfaceTransaction() {

        try {

            Trace.traceBegin(TRACE_TAG_WINDOW_MANAGER, "closeSurfaceTransaction");

            synchronized (mWindowMap) {

                if (mRoot.mSurfaceTraceEnabled) {

                    mRoot.mRemoteEventTrace.closeSurfaceTransaction();

                }

                if (withLockHeld) {

                SurfaceControl.closeTransaction();

            }

        } finally {

            Trace.traceEnd(TRACE_TAG_WINDOW_MANAGER);

        }

    }

            if (!withLockHeld) {

                SurfaceControl.closeTransaction();



    /**

     * Executes an empty animation transaction without holding the WM lock to simulate

     * back-pressure. See {@link WindowAnimator#animate} why this is needed.

     */

    void executeEmptyAnimationTransaction() {

        try {

            Trace.traceBegin(TRACE_TAG_WINDOW_MANAGER, "openSurfaceTransaction");

            synchronized (mWindowMap) {

                if (mRoot.mSurfaceTraceEnabled) {

                    mRoot.mRemoteEventTrace.openSurfaceTransaction();

                }

                SurfaceControl.openTransaction();

                SurfaceControl.setAnimationTransaction();

                if (mRoot.mSurfaceTraceEnabled) {

                    mRoot.mRemoteEventTrace.closeSurfaceTransaction();

                }

            }

        } finally {

            Trace.traceEnd(TRACE_TAG_WINDOW_MANAGER);

        }

        try {

            Trace.traceBegin(TRACE_TAG_WINDOW_MANAGER, "closeSurfaceTransaction");

            SurfaceControl.closeTransaction();

        } finally {

            Trace.traceEnd(TRACE_TAG_WINDOW_MANAGER);

        }