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

Commit da21dd7f authored by Nicolo' Mazzucato's avatar Nicolo' Mazzucato
Browse files

Fix CUJ UiThread detection when only a SurfaceControl is provided 

Most methods annotated with @UiThread were not actually running on the UI thread related to the CUJ (when its ui thread was different than the app main thread).

When deferred monitoring was true (the default case), everything was fine, as the "begin" method eventually goes to the choreographer, and is scheduled on the correct UI thread.

When deferred monitoring was true and only a surface was provided though, the "begin" method was being executed

Bug: 363884280
Test: FrameTrackerTest and Checked ui thread associated with splashscreen CUJs and fold animation
Flag: NONE recent bugfix
Change-Id: I3aa1e67b50d47f1eb6ae5c896e53f6955298e750
parent 45e22250

core/java/android/view/inputmethod/ImeTracker.java

+2 −1
Original line number Diff line number Diff line
@@ -920,7 +920,8 @@ public interface ImeTracker { 
            final Configuration.Builder builder = Configuration.Builder.withSurface(

                            cujType,

                            jankContext.getDisplayContext(),

                            jankContext.getTargetSurfaceControl())

                            jankContext.getTargetSurfaceControl(),

                            jankContext.getDisplayContext().getMainThreadHandler())

                    .setTag(String.format(Locale.US, "%d@%d@%s", animType,

                            useSeparatedThread ? 0 : 1, jankContext.getHostPackageName()));

            InteractionJankMonitor.getInstance().begin(builder);

core/java/com/android/internal/jank/InteractionJankMonitor.java

+40 −9
Original line number Diff line number Diff line
@@ -330,9 +330,10 @@ public class InteractionJankMonitor { 
     * @param cujType the specific {@link Cuj.CujType}.

     * @return boolean true if the tracker is started successfully, false otherwise.

     */

    public boolean begin(SurfaceControl surface, Context context, @Cuj.CujType int cujType) {

    public boolean begin(SurfaceControl surface, Context context, Handler handler,

            @Cuj.CujType int cujType) {

        try {

            return begin(Configuration.Builder.withSurface(cujType, context, surface));

            return begin(Configuration.Builder.withSurface(cujType, context, surface, handler));

        } catch (IllegalArgumentException ex) {

            Log.d(TAG, "Build configuration failed!", ex);

            return false;
@@ -348,11 +349,12 @@ public class InteractionJankMonitor { 
     * @param tag a tag containing extra information about the interaction.

     * @return boolean true if the tracker is started successfully, false otherwise.

     */

    public boolean begin(SurfaceControl surface, Context context, @Cuj.CujType int cujType,

    public boolean begin(SurfaceControl surface, Context context, Handler handler,

            @Cuj.CujType int cujType,

            String tag) {

        try {

            final Configuration.Builder builder =

                    Configuration.Builder.withSurface(cujType, context, surface);

                    Configuration.Builder.withSurface(cujType, context, surface, handler);

            if (!TextUtils.isEmpty(tag)) {

                builder.setTag(tag);

            }
@@ -689,20 +691,23 @@ public class InteractionJankMonitor { 
            private SurfaceControl mAttrSurfaceControl;

            private final @Cuj.CujType int mAttrCujType;

            private boolean mAttrDeferMonitor = true;

            private Handler mHandler = null;



            /**

             * Creates a builder which instruments only surface.

             * @param cuj The enum defined in {@link Cuj.CujType}.

             * @param context context

             * @param surfaceControl surface control

             * @param uiThreadHandler UI thread for that surface

             * @return builder

             */

            public static Builder withSurface(@Cuj.CujType int cuj, @NonNull Context context,

                    @NonNull SurfaceControl surfaceControl) {

                    @NonNull SurfaceControl surfaceControl, @NonNull Handler uiThreadHandler) {

                return new Builder(cuj)

                        .setContext(context)

                        .setSurfaceControl(surfaceControl)

                        .setSurfaceOnly(true);

                        .setSurfaceOnly(true)

                        .setHandler(uiThreadHandler);

            }



            /**
@@ -721,6 +726,18 @@ public class InteractionJankMonitor { 
                mAttrCujType = cuj;

            }



            /**

             * Specifies the UI thread handler. If not provided, the View's one will be used.

             * If only a surface is provided without handler, the app main thread will be used.

             *

             * @param uiThreadHandler handler associated to the cuj UI thread

             * @return builder

             */

            public Builder setHandler(Handler uiThreadHandler) {

                mHandler = uiThreadHandler;

                return this;

            }



            /**

             * Specifies a view, must be set if {@link #setSurfaceOnly(boolean)} is set to false.

             * @param view an attached view
@@ -798,13 +815,13 @@ public class InteractionJankMonitor { 
                return new Configuration(

                        mAttrCujType, mAttrView, mAttrTag, mAttrTimeout,

                        mAttrSurfaceOnly, mAttrContext, mAttrSurfaceControl,

                        mAttrDeferMonitor);

                        mAttrDeferMonitor, mHandler);

            }

        }



        private Configuration(@Cuj.CujType int cuj, View view, @NonNull String tag, long timeout,

                boolean surfaceOnly, Context context, SurfaceControl surfaceControl,

                boolean deferMonitor) {

                boolean deferMonitor, Handler handler) {

            mCujType = cuj;

            mTag = tag;

            mSessionName = generateSessionName(Cuj.getNameOfCuj(cuj), tag);
@@ -816,8 +833,16 @@ public class InteractionJankMonitor { 
                    : (view != null ? view.getContext().getApplicationContext() : null);

            mSurfaceControl = surfaceControl;

            mDeferMonitor = deferMonitor;

            if (handler != null) {

                mHandler = handler;

            } else if (mSurfaceOnly) {

                Log.w(TAG, "No UIThread provided for " + mSessionName

                        + " (surface only). Defaulting to app main thread.");

                mHandler = mContext.getMainThreadHandler();

            } else {

                mHandler = mView.getHandler();

            }

            validate();

            mHandler = mSurfaceOnly ? mContext.getMainThreadHandler() : mView.getHandler();

        }



        @VisibleForTesting
@@ -858,6 +883,12 @@ public class InteractionJankMonitor { 
                    shouldThrow = true;

                    msg.append("Must pass in a valid surface control if only instrument surface; ");

                }

                if (mHandler == null) {

                    shouldThrow = true;

                    msg.append(

                            "Must pass a UI thread handler when only a surface control is "

                                    + "provided.");

                }

            } else {

                if (!hasValidView()) {

                    shouldThrow = true;

core/tests/coretests/src/com/android/internal/jank/InteractionJankMonitorTest.java

+21 −0
Original line number Diff line number Diff line
@@ -24,6 +24,7 @@ import static com.android.internal.jank.InteractionJankMonitor.Configuration.gen 


import static com.google.common.truth.Truth.assertThat;



import static org.junit.Assert.assertThrows;

import static org.mockito.ArgumentMatchers.any;

import static org.mockito.ArgumentMatchers.anyInt;

import static org.mockito.ArgumentMatchers.anyLong;
@@ -117,6 +118,7 @@ public class InteractionJankMonitorTest { 


        // Simulate a trace session and see if begin / end are invoked.

        assertThat(monitor.begin(mSurfaceControl, mActivity.getApplicationContext(),

                mActivity.getMainThreadHandler(),

                Cuj.CUJ_NOTIFICATION_SHADE_EXPAND_COLLAPSE)).isTrue();

        verify(tracker).begin();

        assertThat(monitor.end(Cuj.CUJ_NOTIFICATION_SHADE_EXPAND_COLLAPSE)).isTrue();
@@ -188,6 +190,25 @@ public class InteractionJankMonitorTest { 
        assertThat(generateSessionName(cujName, tooLongTag)).isEqualTo(expectedTrimmedName);

    }



    @Test

    public void validateConfiguration_surfaceOnlyAndNotDeferMonitor_throwsError() {

        Configuration.Builder builder = Configuration.Builder.withSurface(1,

                mActivity.getApplicationContext(), mSurfaceControl,

                mActivity.getMainThreadHandler()).setDeferMonitorForAnimationStart(false);



        assertThrows(IllegalStateException.class, builder::build);

    }



    @Test

    public void validateConfiguration_surfaceOnlyAndDeferMonitor_doesNotThrowError() {

        Configuration.Builder builder = Configuration.Builder.withSurface(1,

                mActivity.getApplicationContext(),

                mSurfaceControl, mActivity.getMainThreadHandler()).setDeferMonitorForAnimationStart(

                true);



        builder.build(); // no exception.

    }



    private InteractionJankMonitor createMockedInteractionJankMonitor() {

        InteractionJankMonitor monitor = spy(new InteractionJankMonitor(mWorker));

        doReturn(true).when(monitor).shouldMonitor();

libs/WindowManager/Shell/src/com/android/wm/shell/back/BackAnimationController.java

+11 −5
Original line number Diff line number Diff line
@@ -117,6 +117,7 @@ public class BackAnimationController implements RemoteCallable<BackAnimationCont 
     */

    private static final long MAX_ANIMATION_DURATION = 2000;

    private final LatencyTracker mLatencyTracker;

    @ShellMainThread private final Handler mHandler;



    /** True when a back gesture is ongoing */

    private boolean mBackGestureStarted = false;
@@ -218,7 +219,8 @@ public class BackAnimationController implements RemoteCallable<BackAnimationCont 
            @NonNull BackAnimationBackground backAnimationBackground,

            ShellBackAnimationRegistry shellBackAnimationRegistry,

            ShellCommandHandler shellCommandHandler,

            Transitions transitions) {

            Transitions transitions,

            @ShellMainThread Handler handler) {

        this(

                shellInit,

                shellController,
@@ -230,7 +232,8 @@ public class BackAnimationController implements RemoteCallable<BackAnimationCont 
                backAnimationBackground,

                shellBackAnimationRegistry,

                shellCommandHandler,

                transitions);

                transitions,

                handler);

    }



    @VisibleForTesting
@@ -245,7 +248,8 @@ public class BackAnimationController implements RemoteCallable<BackAnimationCont 
            @NonNull BackAnimationBackground backAnimationBackground,

            ShellBackAnimationRegistry shellBackAnimationRegistry,

            ShellCommandHandler shellCommandHandler,

            Transitions transitions) {

            Transitions transitions,

            @NonNull @ShellMainThread Handler handler) {

        mShellController = shellController;

        mShellExecutor = shellExecutor;

        mActivityTaskManager = activityTaskManager;
@@ -263,6 +267,7 @@ public class BackAnimationController implements RemoteCallable<BackAnimationCont 
        mTransitions = transitions;

        mBackTransitionHandler = new BackTransitionHandler();

        mTransitions.addHandler(mBackTransitionHandler);

        mHandler = handler;

        updateTouchableArea();

    }
@@ -399,7 +404,7 @@ public class BackAnimationController implements RemoteCallable<BackAnimationCont 
        }

    }



    private static class IBackAnimationImpl extends IBackAnimation.Stub

    private class IBackAnimationImpl extends IBackAnimation.Stub

            implements ExternalInterfaceBinder {

        private BackAnimationController mController;
@@ -417,7 +422,8 @@ public class BackAnimationController implements RemoteCallable<BackAnimationCont 
                                    callback,

                                    runner,

                                    controller.mContext,

                                    CUJ_PREDICTIVE_BACK_HOME)));

                                    CUJ_PREDICTIVE_BACK_HOME,

                                    mHandler)));

        }



        @Override

libs/WindowManager/Shell/src/com/android/wm/shell/back/BackAnimationRunner.java

+25 −4
Original line number Diff line number Diff line
@@ -20,6 +20,7 @@ import static android.view.WindowManager.TRANSIT_OLD_UNSET; 


import android.annotation.NonNull;

import android.content.Context;

import android.os.Handler;

import android.os.RemoteException;

import android.util.Log;

import android.view.IRemoteAnimationFinishedCallback;
@@ -31,6 +32,7 @@ import android.window.IOnBackInvokedCallback; 
import com.android.internal.annotations.VisibleForTesting;

import com.android.internal.jank.Cuj.CujType;

import com.android.internal.jank.InteractionJankMonitor;

import com.android.wm.shell.shared.annotations.ShellMainThread;



/**

 * Used to register the animation callback and runner, it will trigger result if gesture was finish
@@ -45,6 +47,8 @@ public class BackAnimationRunner { 
    private final IRemoteAnimationRunner mRunner;

    private final @CujType int mCujType;

    private final Context mContext;

    @ShellMainThread

    private final Handler mHandler;



    // Whether we are waiting to receive onAnimationStart

    private boolean mWaitingAnimation;
@@ -56,18 +60,35 @@ public class BackAnimationRunner { 
            @NonNull IOnBackInvokedCallback callback,

            @NonNull IRemoteAnimationRunner runner,

            @NonNull Context context,

            @CujType int cujType) {

            @CujType int cujType,

            @ShellMainThread Handler handler) {

        mCallback = callback;

        mRunner = runner;

        mCujType = cujType;

        mContext = context;

        mHandler = handler;

    }



    public BackAnimationRunner(

            @NonNull IOnBackInvokedCallback callback,

            @NonNull IRemoteAnimationRunner runner,

            @NonNull Context context) {

        this(callback, runner, context, NO_CUJ);

            @NonNull Context context,

            @ShellMainThread Handler handler

    ) {

        this(callback, runner, context, NO_CUJ, handler);

    }



    /**

     * @deprecated Use {@link BackAnimationRunner} constructor providing an handler for the ui

     * thread of the animation.

     */

    @Deprecated

    public BackAnimationRunner(

            @NonNull IOnBackInvokedCallback callback,

            @NonNull IRemoteAnimationRunner runner,

            @NonNull Context context

    ) {

        this(callback, runner, context, NO_CUJ, context.getMainThreadHandler());

    }



    /** Returns the registered animation runner */
@@ -100,7 +121,7 @@ public class BackAnimationRunner { 
        mWaitingAnimation = false;

        if (shouldMonitorCUJ(apps)) {

            interactionJankMonitor.begin(

                    apps[0].leash, mContext, mCujType);

                    apps[0].leash, mContext, mHandler, mCujType);

        }

        try {

            getRunner().onAnimationStart(TRANSIT_OLD_UNSET, apps, wallpapers,