From bed37ac501e0b44f8ddc972c2930c5695dafe27c Mon Sep 17 00:00:00 2001 From: Chris Li Date: Thu, 17 Nov 2022 19:45:43 +0800 Subject: [PATCH] Fix potential crash when TaskFragmentOrganizer died Before, when TaskFragmentOrganizerController#onTransactionHandled() is called, we will always call #validateAndGetState(), which call throw an exception for racing condition like organizer process died. Bug: 207070762 Test: atest WmTests:TaskFragmentOrganizerControllerTest Change-Id: I04d37ecab4a37a616167ce4e6ca98c668f1254da --- .../wm/TaskFragmentOrganizerController.java | 38 +++++++++++++++---- .../server/wm/WindowOrganizerController.java | 17 +-------- .../TaskFragmentOrganizerControllerTest.java | 15 ++++++++ 3 files changed, 46 insertions(+), 24 deletions(-) diff --git a/services/core/java/com/android/server/wm/TaskFragmentOrganizerController.java b/services/core/java/com/android/server/wm/TaskFragmentOrganizerController.java index 509b1e6f41ca6..21234d477a42d 100644 --- a/services/core/java/com/android/server/wm/TaskFragmentOrganizerController.java +++ b/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"); } @@ -503,10 +503,18 @@ public class TaskFragmentOrganizerController extends ITaskFragmentOrganizerContr @WindowManager.TransitionType int transitionType, boolean shouldApplyIndependently) { // Keep the calling identity to avoid unsecure change. synchronized (mGlobalLock) { - applyTransaction(wct, transitionType, shouldApplyIndependently); - final TaskFragmentOrganizerState state = validateAndGetState( - wct.getTaskFragmentOrganizer()); - state.onTransactionFinished(transactionToken); + if (isValidTransaction(wct)) { + applyTransaction(wct, transitionType, shouldApplyIndependently); + } + // 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); + } } } @@ -515,7 +523,7 @@ public class TaskFragmentOrganizerController extends ITaskFragmentOrganizerContr @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, @@ -656,7 +664,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; } @@ -702,7 +710,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()); } @@ -739,6 +747,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. diff --git a/services/core/java/com/android/server/wm/WindowOrganizerController.java b/services/core/java/com/android/server/wm/WindowOrganizerController.java index 557c0ef0be18f..b30fd07295252 100644 --- a/services/core/java/com/android/server/wm/WindowOrganizerController.java +++ b/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()}. diff --git a/services/tests/wmtests/src/com/android/server/wm/TaskFragmentOrganizerControllerTest.java b/services/tests/wmtests/src/com/android/server/wm/TaskFragmentOrganizerControllerTest.java index 4202f46c188c6..8569448dd1bd9 100644 --- a/services/tests/wmtests/src/com/android/server/wm/TaskFragmentOrganizerControllerTest.java +++ b/services/tests/wmtests/src/com/android/server/wm/TaskFragmentOrganizerControllerTest.java @@ -763,6 +763,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)