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

Commit fbd233aa authored by Garfield Tan's avatar Garfield Tan
Browse files

Add DisplayContent to hierarchy after stubbing. 

doAnswer type of stubing doesn't completely eliminate race conditions.
The failure happens because when answers are commited to
InvocationContainerImpl it uses a local variable invocationForStubbing
to save the stubbing method, which could be changed from a different
thread by a natural call into the mock/spy before answers are added to
InvocationContainerImpl.

Sequence of events:
1) [testing thread] Commiting stubbing by calling method into the
mock/spy. In this case it could be
DisplayContent#supportsSystemDecoration() in line 159 in
TestDisplayContent in this case. The call was intercepted by
MockHandlerImpl#handle() and further handled by
InvocationContainerImpl#setMethodForStubbing(), in which it already set
invocationForTesting field and didn't clear doAnswerStyleStubbing yet.
2) Another thread calls into the same mock/spy regularly. In this case
it is DisplayContent#getDisplay() as shown in the stack trace in the
bug. It's intercepted by MockHandlerImpl#handle() as well and likely it
thinks we're in the process of doAnswers style procssing and calls into
setMethodForStubbing(). It then mistakenly think we're stubbing
DisplayContent#getDisplay() and found the answer we offered isn't
compatible in the return value.

This is a caveat in Mockito thread safety, though it can be fixed in
Mockito if they're willing to do so. However it's certainly a lot faster
to fix it in our test code.

Also check the caller holds global lock in lieu of synchronization
clause because caller is supposed to be in tests with
RunWith(WindowTestRunner.class) annotation.

Bug: 144611135
Test: atest com.android.server.wm.AnimatingActivityRegistryTest
Change-Id: I8cc4b6ce11ad6c205309476c9eeb4db315f558d6
parent 3c5d179a

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

+20 −20
Original line number Diff line number Diff line
@@ -1032,31 +1032,13 @@ class DisplayContent extends WindowContainer<DisplayContent.DisplayChildWindowCo 
        // Sets the display content for the children.

        onDisplayChanged(this);



        // Add itself as a child to the root container.

        mWmService.mRoot.addChild(this, POSITION_BOTTOM);



        // TODO(b/62541591): evaluate whether this is the best spot to declare the

        // {@link DisplayContent} ready for use.

        mDisplayReady = true;



        mWmService.mAnimator.addDisplayLocked(mDisplayId);

        mInputMonitor = new InputMonitor(mWmService, mDisplayId);

        mInputMonitor = new InputMonitor(mWmService, this);

        mInsetsStateController = new InsetsStateController(this);

        mInsetsPolicy = new InsetsPolicy(mInsetsStateController, this);



        if (DEBUG_DISPLAY) Slog.v(TAG_WM, "Adding display=" + display);

        if (DEBUG_DISPLAY) Slog.v(TAG_WM, "Creating display=" + display);



        mWmService.mDisplayWindowSettings.applySettingsToDisplayLocked(this);



        if (mWmService.mDisplayManagerInternal != null) {

            mWmService.mDisplayManagerInternal

                    .setDisplayInfoOverrideFromWindowManager(mDisplayId, getDisplayInfo());

            configureDisplayPolicy();

        }



        reconfigureDisplayLocked();

        onRequestedOverrideConfigurationChanged(getRequestedOverrideConfiguration());

        mWmService.mDisplayNotificationController.dispatchDisplayAdded(this);

    }



    boolean isReady() {
@@ -4994,6 +4976,24 @@ class DisplayContent extends WindowContainer<DisplayContent.DisplayChildWindowCo 
        // we create the root surfaces explicitly rather than chaining

        // up as the default implementation in onParentChanged does. So we

        // explicitly do NOT call super here.



        if (!isReady()) {

            // TODO(b/62541591): evaluate whether this is the best spot to declare the

            // {@link DisplayContent} ready for use.

            mDisplayReady = true;



            mWmService.mAnimator.addDisplayLocked(mDisplayId);



            if (mWmService.mDisplayManagerInternal != null) {

                mWmService.mDisplayManagerInternal

                        .setDisplayInfoOverrideFromWindowManager(mDisplayId, getDisplayInfo());

                configureDisplayPolicy();

            }



            reconfigureDisplayLocked();

            onRequestedOverrideConfigurationChanged(getRequestedOverrideConfiguration());

            mWmService.mDisplayNotificationController.dispatchDisplayAdded(this);

        }

    }



    @Override

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

+3 −3
Original line number Diff line number Diff line
@@ -151,10 +151,10 @@ final class InputMonitor { 


    private final UpdateInputWindows mUpdateInputWindows = new UpdateInputWindows();



    public InputMonitor(WindowManagerService service, int displayId) {

    InputMonitor(WindowManagerService service, DisplayContent displayContent) {

        mService = service;

        mDisplayContent = mService.mRoot.getDisplayContent(displayId);

        mDisplayId = displayId;

        mDisplayContent = displayContent;

        mDisplayId = displayContent.getDisplayId();

        mInputTransaction = mService.mTransactionFactory.get();

        mHandler = mService.mAnimationHandler;

        mUpdateInputForAllWindowsConsumer = new UpdateInputForAllWindowsConsumer();

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

+2 −0
Original line number Diff line number Diff line
@@ -1377,6 +1377,7 @@ class RootWindowContainer extends WindowContainer<DisplayContent> 
        for (int displayNdx = 0; displayNdx < displays.length; ++displayNdx) {

            final Display display = displays[displayNdx];

            final DisplayContent displayContent = new DisplayContent(display, this);

            addChild(displayContent, POSITION_BOTTOM);

            if (displayContent.mDisplayId == DEFAULT_DISPLAY) {

                mDefaultDisplay = displayContent;

            }
@@ -1445,6 +1446,7 @@ class RootWindowContainer extends WindowContainer<DisplayContent> 
        }

        // The display hasn't been added to ActivityManager yet, create a new record now.

        displayContent = new DisplayContent(display, this);

        addChild(displayContent, POSITION_BOTTOM);

        return displayContent;

    }

services/tests/wmtests/src/com/android/server/wm/SystemServicesTestRule.java

+10 −0
Original line number Diff line number Diff line
@@ -408,6 +408,16 @@ public class SystemServicesTestRule implements TestRule { 
        }

    }



    /**

     * Throws if caller doesn't hold the given lock.

     * @param lock the lock

     */

    static void checkHoldsLock(Object lock) {

        if (!Thread.holdsLock(lock)) {

            throw new IllegalStateException("Caller doesn't hold global lock.");

        }

    }



    protected class TestActivityTaskManagerService extends ActivityTaskManagerService {

        // ActivityStackSupervisor may be created more than once while setting up AMS and ATMS.

        // We keep the reference in order to prevent creating it twice.

services/tests/wmtests/src/com/android/server/wm/TestDisplayContent.java

+9 −5
Original line number Diff line number Diff line
@@ -72,7 +72,7 @@ class TestDisplayContent extends DisplayContent { 
        private final DisplayInfo mInfo;

        private boolean mCanRotate = true;

        private int mWindowingMode = WINDOWING_MODE_FULLSCREEN;

        private int mPosition = POSITION_TOP;

        private int mPosition = POSITION_BOTTOM;

        private final ActivityTaskManagerService mService;

        private boolean mSystemDecorations = false;
@@ -127,13 +127,13 @@ class TestDisplayContent extends DisplayContent { 
            return this;

        }

        TestDisplayContent build() {

            SystemServicesTestRule.checkHoldsLock(mService.mGlobalLock);



            final int displayId = SystemServicesTestRule.sNextDisplayId++;

            final Display display = new Display(DisplayManagerGlobal.getInstance(), displayId,

                    mInfo, DEFAULT_DISPLAY_ADJUSTMENTS);

            final TestDisplayContent newDisplay;

            synchronized (mService.mGlobalLock) {

                newDisplay = new TestDisplayContent(mService.mStackSupervisor, display);

            }

            final TestDisplayContent newDisplay =

                    new TestDisplayContent(mService.mStackSupervisor, display);

            // disable the normal system decorations

            final DisplayPolicy displayPolicy = newDisplay.mDisplayContent.getDisplayPolicy();

            spyOn(displayPolicy);
@@ -153,6 +153,10 @@ class TestDisplayContent extends DisplayContent { 
                doReturn(false).when(newDisplay.mDisplayContent)

                        .handlesOrientationChangeFromDescendant();

            }

            // Please add stubbing before this line. Services will start using this display in other

            // threads immediately after adding it to hierarchy. Calling doAnswer() type of stubbing

            // reduces chance of races, but still doesn't eliminate race conditions.

            mService.mRootWindowContainer.addChild(newDisplay, mPosition);

            return newDisplay;

        }

    }