From fbd233aa869dbdb05a0014a003d159d3188d2375 Mon Sep 17 00:00:00 2001
From: Garfield Tan <xutan@google.com>
Date: Thu, 26 Dec 2019 12:39:25 -0800
Subject: [PATCH] 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
---
 .../com/android/server/wm/DisplayContent.java | 40 +++++++++----------
 .../com/android/server/wm/InputMonitor.java   |  6 +--
 .../server/wm/RootWindowContainer.java        |  2 +
 .../server/wm/SystemServicesTestRule.java     | 10 +++++
 .../android/server/wm/TestDisplayContent.java | 14 ++++---
 5 files changed, 44 insertions(+), 28 deletions(-)

diff --git a/services/core/java/com/android/server/wm/DisplayContent.java b/services/core/java/com/android/server/wm/DisplayContent.java
index 67fe1491c48be..6d3fabc3f93d4 100644
--- a/services/core/java/com/android/server/wm/DisplayContent.java
+++ b/services/core/java/com/android/server/wm/DisplayContent.java
@@ -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
diff --git a/services/core/java/com/android/server/wm/InputMonitor.java b/services/core/java/com/android/server/wm/InputMonitor.java
index a8ff5002bc584..c4b67d76607ee 100644
--- a/services/core/java/com/android/server/wm/InputMonitor.java
+++ b/services/core/java/com/android/server/wm/InputMonitor.java
@@ -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();
diff --git a/services/core/java/com/android/server/wm/RootWindowContainer.java b/services/core/java/com/android/server/wm/RootWindowContainer.java
index 704ab677f91d8..a7bf6600d7b56 100644
--- a/services/core/java/com/android/server/wm/RootWindowContainer.java
+++ b/services/core/java/com/android/server/wm/RootWindowContainer.java
@@ -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;
     }
 
diff --git a/services/tests/wmtests/src/com/android/server/wm/SystemServicesTestRule.java b/services/tests/wmtests/src/com/android/server/wm/SystemServicesTestRule.java
index 05d1c76fed8cc..e50750841e20f 100644
--- a/services/tests/wmtests/src/com/android/server/wm/SystemServicesTestRule.java
+++ b/services/tests/wmtests/src/com/android/server/wm/SystemServicesTestRule.java
@@ -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.
diff --git a/services/tests/wmtests/src/com/android/server/wm/TestDisplayContent.java b/services/tests/wmtests/src/com/android/server/wm/TestDisplayContent.java
index e5121b9297e61..77af5eec27963 100644
--- a/services/tests/wmtests/src/com/android/server/wm/TestDisplayContent.java
+++ b/services/tests/wmtests/src/com/android/server/wm/TestDisplayContent.java
@@ -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;
         }
     }