From bc9f4bb3efadd61189238a89eec8cda0cd77fa29 Mon Sep 17 00:00:00 2001
From: Evan Rosky <erosky@google.com>
Date: Tue, 7 Feb 2023 14:56:35 -0800
Subject: [PATCH] Fix race between startNew and Core-request transition

If WMCore requests a transition while a startNew from Shell
is "in flight", Shell could be expecting startNew to be
processed first while WM is actually processing its own
requested transition first. This can lead to incorrect
behavior or an NPE.

To resolve this, make sure the WM request transition is
the first non-started transition in Shell's records (basically
syncronizing Shell to Core's transition order).

Bug: 265861362
Test: atest ShellTransitionTests#testTransitionOrderMatchesCore
Change-Id: I62a25335f270a021a06dd4309e692e8812d43332
---
 .../wm/shell/transition/Transitions.java      | 10 +++++-
 .../transition/ShellTransitionTests.java      | 31 +++++++++++++++++--
 2 files changed, 38 insertions(+), 3 deletions(-)

diff --git a/libs/WindowManager/Shell/src/com/android/wm/shell/transition/Transitions.java b/libs/WindowManager/Shell/src/com/android/wm/shell/transition/Transitions.java
index b2f61c2319117..5275e90b931f7 100644
--- a/libs/WindowManager/Shell/src/com/android/wm/shell/transition/Transitions.java
+++ b/libs/WindowManager/Shell/src/com/android/wm/shell/transition/Transitions.java
@@ -829,7 +829,15 @@ public class Transitions implements RemoteCallable<Transitions> {
         }
         mOrganizer.startTransition(transitionToken, wct != null && wct.isEmpty() ? null : wct);
         active.mToken = transitionToken;
-        mActiveTransitions.add(active);
+        int insertIdx = 0;
+        for (; insertIdx < mActiveTransitions.size(); ++insertIdx) {
+            if (mActiveTransitions.get(insertIdx).mInfo == null) {
+                // A `startNewTransition` was sent to WMCore, but wasn't acknowledged before WMCore
+                // made this request, so insert this request beforehand to keep order in sync.
+                break;
+            }
+        }
+        mActiveTransitions.add(insertIdx, active);
     }
 
     /** Start a new transition directly. */
diff --git a/libs/WindowManager/Shell/tests/unittest/src/com/android/wm/shell/transition/ShellTransitionTests.java b/libs/WindowManager/Shell/tests/unittest/src/com/android/wm/shell/transition/ShellTransitionTests.java
index 595c3b4880df5..a9061aeb03147 100644
--- a/libs/WindowManager/Shell/tests/unittest/src/com/android/wm/shell/transition/ShellTransitionTests.java
+++ b/libs/WindowManager/Shell/tests/unittest/src/com/android/wm/shell/transition/ShellTransitionTests.java
@@ -45,6 +45,7 @@ import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertTrue;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.anyBoolean;
+import static org.mockito.ArgumentMatchers.anyInt;
 import static org.mockito.ArgumentMatchers.eq;
 import static org.mockito.ArgumentMatchers.isNull;
 import static org.mockito.Mockito.clearInvocations;
@@ -119,8 +120,8 @@ public class ShellTransitionTests extends ShellTestCase {
 
     @Before
     public void setUp() {
-        doAnswer(invocation -> invocation.getArguments()[1])
-                .when(mOrganizer).startTransition(any(), any());
+        doAnswer(invocation -> new Binder())
+                .when(mOrganizer).startNewTransition(anyInt(), any());
     }
 
     @Test
@@ -561,6 +562,32 @@ public class ShellTransitionTests extends ShellTestCase {
         assertEquals(0, mDefaultHandler.activeCount());
     }
 
+    @Test
+    public void testTransitionOrderMatchesCore() {
+        Transitions transitions = createTestTransitions();
+        transitions.replaceDefaultHandlerForTest(mDefaultHandler);
+
+        IBinder transitToken = new Binder();
+        IBinder shellInit = transitions.startTransition(TRANSIT_CLOSE,
+                new WindowContainerTransaction(), null /* handler */);
+        // make sure we are testing the "New" API.
+        verify(mOrganizer, times(1)).startNewTransition(eq(TRANSIT_CLOSE), any());
+        // WMCore may not receive the new transition before requesting its own.
+        transitions.requestStartTransition(transitToken,
+                new TransitionRequestInfo(TRANSIT_OPEN, null /* trigger */, null /* remote */));
+        verify(mOrganizer, times(1)).startTransition(eq(transitToken), any());
+
+        // At this point, WM is working on its transition (the shell-initialized one is still
+        // queued), so continue the transition lifecycle for that.
+        TransitionInfo info = new TransitionInfoBuilder(TRANSIT_OPEN)
+                .addChange(TRANSIT_OPEN).addChange(TRANSIT_CLOSE).build();
+        transitions.onTransitionReady(transitToken, info, mock(SurfaceControl.Transaction.class),
+                mock(SurfaceControl.Transaction.class));
+        // At this point, if things are not working, we'd get an NPE due to attempting to merge
+        // into the shellInit transition which hasn't started yet.
+        assertEquals(1, mDefaultHandler.activeCount());
+    }
+
     @Test
     public void testShouldRotateSeamlessly() throws Exception {
         final RunningTaskInfo taskInfo =