Merge "Fix potential crash when TaskFragmentOrganizer died" into tm-qpr-dev

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

@@ -425,7 +425,7 @@ public class TaskFragmentOrganizerController extends ITaskFragmentOrganizerContr
             ProtoLog.v(WM_DEBUG_WINDOW_ORGANIZER,
                     "Register task fragment organizer=%s uid=%d pid=%d",
                     organizer.asBinder(), uid, pid);
-            if (mTaskFragmentOrganizerState.containsKey(organizer.asBinder())) {
+            if (isOrganizerRegistered(organizer)) {
                 throw new IllegalStateException(
                         "Replacing existing organizer currently unsupported");
             }
@@ -502,19 +502,27 @@ public class TaskFragmentOrganizerController extends ITaskFragmentOrganizerContr
             @WindowManager.TransitionType int transitionType, boolean shouldApplyIndependently) {
         // Keep the calling identity to avoid unsecure change.
         synchronized (mGlobalLock) {
+            if (isValidTransaction(wct)) {
                 applyTransaction(wct, transitionType, shouldApplyIndependently);
-            final TaskFragmentOrganizerState state = validateAndGetState(
+            }
-                    wct.getTaskFragmentOrganizer());
+            // Even if the transaction is empty, we still need to invoke #onTransactionFinished
+            // unless the organizer has been unregistered.
+            final ITaskFragmentOrganizer organizer = wct.getTaskFragmentOrganizer();
+            final TaskFragmentOrganizerState state = organizer != null
+                    ? mTaskFragmentOrganizerState.get(organizer.asBinder())
+                    : null;
+            if (state != null) {
                 state.onTransactionFinished(transactionToken);
             }
         }
+    }
 
     @Override
     public void applyTransaction(@NonNull WindowContainerTransaction wct,
             @WindowManager.TransitionType int transitionType, boolean shouldApplyIndependently) {
         // Keep the calling identity to avoid unsecure change.
         synchronized (mGlobalLock) {
-            if (wct.isEmpty()) {
+            if (!isValidTransaction(wct)) {
                 return;
             }
             mWindowOrganizerController.applyTaskFragmentTransactionLocked(wct, transitionType,
@@ -655,7 +663,7 @@ public class TaskFragmentOrganizerController extends ITaskFragmentOrganizerContr
             }
             organizer = organizedTf[0].getTaskFragmentOrganizer();
         }
-        if (!mTaskFragmentOrganizerState.containsKey(organizer.asBinder())) {
+        if (!isOrganizerRegistered(organizer)) {
             Slog.w(TAG, "The last TaskFragmentOrganizer no longer exists");
             return;
         }
@@ -701,7 +709,7 @@ public class TaskFragmentOrganizerController extends ITaskFragmentOrganizerContr
         mPendingTaskFragmentEvents.get(event.mTaskFragmentOrg.asBinder()).remove(event);
     }
 
-    boolean isOrganizerRegistered(@NonNull ITaskFragmentOrganizer organizer) {
+    private boolean isOrganizerRegistered(@NonNull ITaskFragmentOrganizer organizer) {
         return mTaskFragmentOrganizerState.containsKey(organizer.asBinder());
     }
 
@@ -738,6 +746,20 @@ public class TaskFragmentOrganizerController extends ITaskFragmentOrganizerContr
         return state;
     }
 
+    boolean isValidTransaction(@NonNull WindowContainerTransaction t) {
+        if (t.isEmpty()) {
+            return false;
+        }
+        final ITaskFragmentOrganizer organizer = t.getTaskFragmentOrganizer();
+        if (t.getTaskFragmentOrganizer() == null || !isOrganizerRegistered(organizer)) {
+            // Transaction from an unregistered organizer should not be applied. This can happen
+            // when the organizer process died before the transaction is applied.
+            Slog.e(TAG, "Caller organizer=" + organizer + " is no longer registered");
+            return false;
+        }
+        return true;
+    }
+
     /**
      * A class to store {@link ITaskFragmentOrganizer} and its organized
      * {@link TaskFragment TaskFragments} with different pending event request.

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

@@ -403,9 +403,6 @@ class WindowOrganizerController extends IWindowOrganizerController.Stub
      */
     void applyTaskFragmentTransactionLocked(@NonNull WindowContainerTransaction wct,
             @WindowManager.TransitionType int type, boolean shouldApplyIndependently) {
-        if (!isValidTransaction(wct)) {
-            return;
-        }
         enforceTaskFragmentOrganizerPermission("applyTaskFragmentTransaction()",
                 Objects.requireNonNull(wct.getTaskFragmentOrganizer()),
                 Objects.requireNonNull(wct));
@@ -459,7 +456,7 @@ class WindowOrganizerController extends IWindowOrganizerController.Stub
                     // calls startSyncSet.
                     () -> mTransitionController.moveToCollecting(nextTransition),
                     () -> {
-                        if (isValidTransaction(wct)) {
+                        if (mTaskFragmentOrganizerController.isValidTransaction(wct)) {
                             applyTransaction(wct, -1 /*syncId*/, nextTransition, caller);
                             mTransitionController.requestStartTransition(nextTransition,
                                     null /* startTask */, null /* remoteTransition */,
@@ -1620,18 +1617,6 @@ class WindowOrganizerController extends IWindowOrganizerController.Stub
         return (cfgChanges & CONTROLLABLE_CONFIGS) == 0;
     }
 
-    private boolean isValidTransaction(@NonNull WindowContainerTransaction t) {
-        if (t.getTaskFragmentOrganizer() != null && !mTaskFragmentOrganizerController
-                .isOrganizerRegistered(t.getTaskFragmentOrganizer())) {
-            // Transaction from an unregistered organizer should not be applied. This can happen
-            // when the organizer process died before the transaction is applied.
-            Slog.e(TAG, "Caller organizer=" + t.getTaskFragmentOrganizer()
-                    + " is no longer registered");
-            return false;
-        }
-        return true;
-    }
-
     /**
      * Makes sure that the transaction only contains operations that are allowed for the
      * {@link WindowContainerTransaction#getTaskFragmentOrganizer()}.

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

@@ -821,6 +821,21 @@ public class TaskFragmentOrganizerControllerTest extends WindowTestsBase {
         assertNotNull(mWindowOrganizerController.getTaskFragment(fragmentToken));
     }
 
+    @Test
+    public void testOnTransactionHandled_skipTransactionForUnregisterOrganizer() {
+        mController.unregisterOrganizer(mIOrganizer);
+        final ActivityRecord ownerActivity = createActivityRecord(mDisplayContent);
+        final IBinder fragmentToken = new Binder();
+
+        // Allow organizer to create TaskFragment and start/reparent activity to TaskFragment.
+        createTaskFragmentFromOrganizer(mTransaction, ownerActivity, fragmentToken);
+        mController.onTransactionHandled(new Binder(), mTransaction,
+                getTransitionType(mTransaction), false /* shouldApplyIndependently */);
+
+        // Nothing should happen as the organizer is not registered.
+        assertNull(mWindowOrganizerController.getTaskFragment(fragmentToken));
+    }
+
     @Test
     public void testOrganizerRemovedWithPendingEvents() {
         final TaskFragment tf0 = new TaskFragmentBuilder(mAtm)