From 61ba70f1d199806984d9ded5cb330c8140e659c6 Mon Sep 17 00:00:00 2001 From: Chris Li Date: Wed, 19 Oct 2022 11:40:45 +0800 Subject: [PATCH] Fix system crash when ActivityEmbedding process died with pending events Before, when the organizer binder died, it will trigger TaskFragment removal that can cause surface placement. Now, we make sure to cleanup all pending events before removing organized TaskFragments. Bug: 251583070 Bug: 254322068 Test: atest WmTests:TaskFragmentOrganizerControllerTest Change-Id: Ifec6f763a7e7819c93cc28d71bd0314f3ac9ac0d --- .../wm/TaskFragmentOrganizerController.java | 38 ++++++++++----- .../TaskFragmentOrganizerControllerTest.java | 46 +++++++++++++++++++ 2 files changed, 73 insertions(+), 11 deletions(-) diff --git a/services/core/java/com/android/server/wm/TaskFragmentOrganizerController.java b/services/core/java/com/android/server/wm/TaskFragmentOrganizerController.java index d24ef75294947..509b1e6f41ca6 100644 --- a/services/core/java/com/android/server/wm/TaskFragmentOrganizerController.java +++ b/services/core/java/com/android/server/wm/TaskFragmentOrganizerController.java @@ -184,19 +184,30 @@ public class TaskFragmentOrganizerController extends ITaskFragmentOrganizerContr } void dispose() { - while (!mOrganizedTaskFragments.isEmpty()) { - final TaskFragment taskFragment = mOrganizedTaskFragments.get(0); - // Cleanup before remove to prevent it from sending any additional event, such as - // #onTaskFragmentVanished, to the removed organizer. + for (int i = mOrganizedTaskFragments.size() - 1; i >= 0; i--) { + // Cleanup the TaskFragmentOrganizer from all TaskFragments it organized before + // removing the windows to prevent it from adding any additional TaskFragment + // pending event. + final TaskFragment taskFragment = mOrganizedTaskFragments.get(i); taskFragment.onTaskFragmentOrganizerRemoved(); - taskFragment.removeImmediately(); - mOrganizedTaskFragments.remove(taskFragment); } + + // Defer to avoid unnecessary layout when there are multiple TaskFragments removal. + mAtmService.deferWindowLayout(); + try { + while (!mOrganizedTaskFragments.isEmpty()) { + final TaskFragment taskFragment = mOrganizedTaskFragments.remove(0); + taskFragment.removeImmediately(); + } + } finally { + mAtmService.continueWindowLayout(); + } + for (int i = mDeferredTransitions.size() - 1; i >= 0; i--) { // Cleanup any running transaction to unblock the current transition. onTransactionFinished(mDeferredTransitions.keyAt(i)); } - mOrganizer.asBinder().unlinkToDeath(this, 0 /*flags*/); + mOrganizer.asBinder().unlinkToDeath(this, 0 /* flags */); } @NonNull @@ -426,7 +437,6 @@ public class TaskFragmentOrganizerController extends ITaskFragmentOrganizerContr @Override public void unregisterOrganizer(@NonNull ITaskFragmentOrganizer organizer) { - validateAndGetState(organizer); final int pid = Binder.getCallingPid(); final long uid = Binder.getCallingUid(); final long origId = Binder.clearCallingIdentity(); @@ -697,11 +707,17 @@ public class TaskFragmentOrganizerController extends ITaskFragmentOrganizerContr } private void removeOrganizer(@NonNull ITaskFragmentOrganizer organizer) { - final TaskFragmentOrganizerState state = validateAndGetState(organizer); + final TaskFragmentOrganizerState state = mTaskFragmentOrganizerState.get( + organizer.asBinder()); + if (state == null) { + Slog.w(TAG, "The organizer has already been removed."); + return; + } + // Remove any pending event of this organizer first because state.dispose() may trigger + // event dispatch as result of surface placement. + mPendingTaskFragmentEvents.remove(organizer.asBinder()); // remove all of the children of the organized TaskFragment state.dispose(); - // Remove any pending event of this organizer. - mPendingTaskFragmentEvents.remove(organizer.asBinder()); mTaskFragmentOrganizerState.remove(organizer.asBinder()); } 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 ad0b9b2c2a839..4202f46c188c6 100644 --- a/services/tests/wmtests/src/com/android/server/wm/TaskFragmentOrganizerControllerTest.java +++ b/services/tests/wmtests/src/com/android/server/wm/TaskFragmentOrganizerControllerTest.java @@ -56,6 +56,7 @@ import static org.mockito.ArgumentMatchers.anyBoolean; import static org.mockito.ArgumentMatchers.anyInt; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.clearInvocations; +import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; import static org.mockito.Mockito.verify; @@ -91,6 +92,7 @@ import org.mockito.ArgumentCaptor; import org.mockito.Captor; import org.mockito.Mock; import org.mockito.MockitoAnnotations; +import org.mockito.stubbing.Answer; import java.util.List; @@ -761,6 +763,50 @@ public class TaskFragmentOrganizerControllerTest extends WindowTestsBase { assertNotNull(mWindowOrganizerController.getTaskFragment(fragmentToken)); } + @Test + public void testOrganizerRemovedWithPendingEvents() { + final TaskFragment tf0 = new TaskFragmentBuilder(mAtm) + .setCreateParentTask() + .setOrganizer(mOrganizer) + .setFragmentToken(mFragmentToken) + .build(); + final TaskFragment tf1 = new TaskFragmentBuilder(mAtm) + .setCreateParentTask() + .setOrganizer(mOrganizer) + .setFragmentToken(new Binder()) + .build(); + assertTrue(tf0.isOrganizedTaskFragment()); + assertTrue(tf1.isOrganizedTaskFragment()); + assertTrue(tf0.isAttached()); + assertTrue(tf0.isAttached()); + + // Mock the behavior that remove TaskFragment can trigger event dispatch. + final Answer removeImmediately = invocation -> { + invocation.callRealMethod(); + mController.dispatchPendingEvents(); + return null; + }; + doAnswer(removeImmediately).when(tf0).removeImmediately(); + doAnswer(removeImmediately).when(tf1).removeImmediately(); + + // Add pending events. + mController.onTaskFragmentAppeared(mIOrganizer, tf0); + mController.onTaskFragmentAppeared(mIOrganizer, tf1); + + // Remove organizer. + mController.unregisterOrganizer(mIOrganizer); + mController.dispatchPendingEvents(); + + // Nothing should happen after the organizer is removed. + verify(mOrganizer, never()).onTransactionReady(any()); + + // TaskFragments should be removed. + assertFalse(tf0.isOrganizedTaskFragment()); + assertFalse(tf1.isOrganizedTaskFragment()); + assertFalse(tf0.isAttached()); + assertFalse(tf0.isAttached()); + } + @Test public void testTaskFragmentInPip_startActivityInTaskFragment() { setupTaskFragmentInPip();