Address destroying activities multiple times.

There are various paths which an activity can be destroyed. One
possibility is if we have already initiated destroying an activity
and then finish all activities in response to a display being
removed. In this case, a request will be made to destroy an activity
that could already be destroying. Additionally, the state is always
set to finishing, regardless of the current state.

With the activity lifecycler, multiple destroy requests are not
tolerated. The second request will lead to an exception due to not
being able to find the activity. Previously, redundant requests were
suppressed client side by simply ignoring them. The state change
leads to inconsistency as the Activity can come back from destroyed.

This changelist addresses these issues by not finishing a
destroying/destroyed activity. Additionally, the changelist now
enforces that state must move forward. Lastly, redundant destroy
requests are not ignored.

Bug: 71506345
Test: atest FrameworksServicesTests:com.android.server.am.ActivityRecordTests#testSetInvalidState
Test: atest FrameworksServicesTests:com.android.server.am.ActivityStackTests#testSuppressMultipleDestroy
Change-Id: I235fa8002f73ad20bb101551fe6ebd5bcc22fa6d

diff --git a/services/core/java/com/android/server/am/ActivityManagerService.java b/services/core/java/com/android/server/am/ActivityManagerService.java
index 0de4f86..1b24af5 100644 (file)
--- a/services/core/java/com/android/server/am/ActivityManagerService.java
+++ b/services/core/java/com/android/server/am/ActivityManagerService.java
@@ -685,7 +685,7 @@ public class ActivityManagerService extends IActivityManager.Stub
 
     private final ActivityStartController mActivityStartController;
 
-    final ClientLifecycleManager mLifecycleManager;
+    private final ClientLifecycleManager mLifecycleManager;
 
     final TaskChangeNotificationController mTaskChangeNotificationController;
 
@@ -12303,6 +12303,10 @@ public class ActivityManagerService extends IActivityManager.Stub
         return mActivityStartController;
     }
 
+    ClientLifecycleManager getLifecycleManager() {
+        return mLifecycleManager;
+    }
+
     PackageManagerInternal getPackageManagerInternalLocked() {
         if (mPackageManagerInt == null) {
             mPackageManagerInt = LocalServices.getService(PackageManagerInternal.class);

diff --git a/services/core/java/com/android/server/am/ActivityRecord.java b/services/core/java/com/android/server/am/ActivityRecord.java
index 50cc071..e6d6fdf 100644 (file)
--- a/services/core/java/com/android/server/am/ActivityRecord.java
+++ b/services/core/java/com/android/server/am/ActivityRecord.java
@@ -665,7 +665,7 @@ final class ActivityRecord extends ConfigurationContainer implements AppWindowCo
                     "Reporting activity moved to display" + ", activityRecord=" + this
                             + ", displayId=" + displayId + ", config=" + config);
 
-            service.mLifecycleManager.scheduleTransaction(app.thread, appToken,
+            service.getLifecycleManager().scheduleTransaction(app.thread, appToken,
                     MoveToDisplayItem.obtain(displayId, config));
         } catch (RemoteException e) {
             // If process died, whatever.
@@ -683,7 +683,7 @@ final class ActivityRecord extends ConfigurationContainer implements AppWindowCo
             if (DEBUG_CONFIGURATION) Slog.v(TAG, "Sending new config to " + this + ", config: "
                     + config);
 
-            service.mLifecycleManager.scheduleTransaction(app.thread, appToken,
+            service.getLifecycleManager().scheduleTransaction(app.thread, appToken,
                     ActivityConfigurationChangeItem.obtain(config));
         } catch (RemoteException e) {
             // If process died, whatever.
@@ -710,7 +710,7 @@ final class ActivityRecord extends ConfigurationContainer implements AppWindowCo
 
     private void scheduleMultiWindowModeChanged(Configuration overrideConfig) {
         try {
-            service.mLifecycleManager.scheduleTransaction(app.thread, appToken,
+            service.getLifecycleManager().scheduleTransaction(app.thread, appToken,
                     MultiWindowModeChangeItem.obtain(mLastReportedMultiWindowMode,
                             overrideConfig));
         } catch (Exception e) {
@@ -738,7 +738,7 @@ final class ActivityRecord extends ConfigurationContainer implements AppWindowCo
 
     private void schedulePictureInPictureModeChanged(Configuration overrideConfig) {
         try {
-            service.mLifecycleManager.scheduleTransaction(app.thread, appToken,
+            service.getLifecycleManager().scheduleTransaction(app.thread, appToken,
                     PipModeChangeItem.obtain(mLastReportedPictureInPictureMode,
                             overrideConfig));
         } catch (Exception e) {
@@ -1428,7 +1428,7 @@ final class ActivityRecord extends ConfigurationContainer implements AppWindowCo
             try {
                 ArrayList<ReferrerIntent> ar = new ArrayList<>(1);
                 ar.add(rintent);
-                service.mLifecycleManager.scheduleTransaction(app.thread, appToken,
+                service.getLifecycleManager().scheduleTransaction(app.thread, appToken,
                         NewIntentItem.obtain(ar, mState == PAUSED));
                 unsent = false;
             } catch (RemoteException e) {
@@ -1615,20 +1615,30 @@ final class ActivityRecord extends ConfigurationContainer implements AppWindowCo
         if (DEBUG_STATES) Slog.v(TAG_STATES, "State movement: " + this + " from:" + getState()
                         + " to:" + state + " reason:" + reason);
 
-        final ActivityState prev = mState;
-        final boolean stateChanged = prev != state;
+        if (state == mState) {
+            // No need to do anything if state doesn't change.
+            if (DEBUG_STATES) Slog.v(TAG_STATES, "State unchanged from:" + state);
+            return;
+        }
 
-        mState = state;
+        if (isState(DESTROYED) || (state != DESTROYED && isState(DESTROYING))) {
+            // We cannot move backwards from destroyed and destroying states.
+            throw new IllegalArgumentException("cannot move back states once destroying"
+                    + "current:" + mState + " requested:" + state);
+        }
 
-        if (stateChanged) {
-            if (mRecentTransitions.size() == MAX_STORED_STATE_TRANSITIONS) {
-                mRecentTransitions.remove(0);
-            }
+        final ActivityState prev = mState;
+        mState = state;
 
-            mRecentTransitions.add(new StateTransition(prev, state, reason));
+        if (mRecentTransitions.size() == MAX_STORED_STATE_TRANSITIONS) {
+            mRecentTransitions.remove(0);
         }
 
-        if (stateChanged && isState(DESTROYING, DESTROYED)) {
+        mRecentTransitions.add(new StateTransition(prev, state, reason));
+
+        mState = state;
+
+        if (isState(DESTROYING, DESTROYED)) {
             makeFinishingLocked();
 
             // When moving to the destroyed state, immediately destroy the activity in the
@@ -1726,7 +1736,7 @@ final class ActivityRecord extends ConfigurationContainer implements AppWindowCo
             setVisible(true);
             sleeping = false;
             app.pendingUiClean = true;
-            service.mLifecycleManager.scheduleTransaction(app.thread, appToken,
+            service.getLifecycleManager().scheduleTransaction(app.thread, appToken,
                     WindowVisibilityItem.obtain(true /* showWindow */));
             // The activity may be waiting for stop, but that is no longer appropriate for it.
             mStackSupervisor.mStoppingActivities.remove(this);
@@ -1744,7 +1754,7 @@ final class ActivityRecord extends ConfigurationContainer implements AppWindowCo
                 // An activity must be in the {@link PAUSING} state for the system to validate
                 // the move to {@link PAUSED}.
                 setState(PAUSING, "makeVisibleIfNeeded");
-                service.mLifecycleManager.scheduleTransaction(app.thread, appToken,
+                service.getLifecycleManager().scheduleTransaction(app.thread, appToken,
                         PauseActivityItem.obtain(finishing, false /* userLeaving */,
                                 configChangeFlags, false /* dontReport */)
                                 .setDescription(reason));
@@ -2712,7 +2722,7 @@ final class ActivityRecord extends ConfigurationContainer implements AppWindowCo
             final ClientTransaction transaction = ClientTransaction.obtain(app.thread, appToken);
             transaction.addCallback(callbackItem);
             transaction.setLifecycleStateRequest(lifecycleItem);
-            service.mLifecycleManager.scheduleTransaction(transaction);
+            service.getLifecycleManager().scheduleTransaction(transaction);
             // Note: don't need to call pauseIfSleepingLocked() here, because the caller will only
             // request resume if this activity is currently resumed, which implies we aren't
             // sleeping.

diff --git a/services/core/java/com/android/server/am/ActivityStack.java b/services/core/java/com/android/server/am/ActivityStack.java
index 9276abe..f4a4af2 100644 (file)
--- a/services/core/java/com/android/server/am/ActivityStack.java
+++ b/services/core/java/com/android/server/am/ActivityStack.java
@@ -1472,7 +1472,7 @@ class ActivityStack<T extends StackWindowController> extends ConfigurationContai
                         prev.shortComponentName, "userLeaving=" + userLeaving);
                 mService.updateUsageStats(prev, false);
 
-                mService.mLifecycleManager.scheduleTransaction(prev.app.thread, prev.appToken,
+                mService.getLifecycleManager().scheduleTransaction(prev.app.thread, prev.appToken,
                         PauseActivityItem.obtain(prev.finishing, userLeaving,
                                 prev.configChangeFlags, pauseImmediately).setDescription(
                                         prev.getLifecycleDescription("startPausingLocked")));
@@ -2109,7 +2109,7 @@ class ActivityStack<T extends StackWindowController> extends ConfigurationContai
                     if (r.app != null && r.app.thread != null) {
                         if (DEBUG_VISIBILITY) Slog.v(TAG_VISIBILITY,
                                 "Scheduling invisibility: " + r);
-                        mService.mLifecycleManager.scheduleTransaction(r.app.thread, r.appToken,
+                        mService.getLifecycleManager().scheduleTransaction(r.app.thread, r.appToken,
                                 WindowVisibilityItem.obtain(false /* showWindow */));
                     }
 
@@ -2661,13 +2661,12 @@ class ActivityStack<T extends StackWindowController> extends ConfigurationContai
                     next.app.pendingUiClean = true;
                     next.app.forceProcessStateUpTo(mService.mTopProcessState);
                     next.clearOptionsLocked();
-
                     transaction.setLifecycleStateRequest(
                             ResumeActivityItem.obtain(next.app.repProcState,
                                     mService.isNextTransitionForward())
                                     .setDescription(next.getLifecycleDescription(
                                             "resumeTopActivityInnerLocked")));
-                    mService.mLifecycleManager.scheduleTransaction(transaction);
+                    mService.getLifecycleManager().scheduleTransaction(transaction);
 
                     if (DEBUG_STATES) Slog.d(TAG_STATES, "resumeTopActivityLocked: Resumed "
                             + next);
@@ -3317,7 +3316,7 @@ class ActivityStack<T extends StackWindowController> extends ConfigurationContai
                 ArrayList<ResultInfo> list = new ArrayList<ResultInfo>();
                 list.add(new ResultInfo(resultWho, requestCode,
                         resultCode, data));
-                mService.mLifecycleManager.scheduleTransaction(r.app.thread, r.appToken,
+                mService.getLifecycleManager().scheduleTransaction(r.app.thread, r.appToken,
                         ActivityResultItem.obtain(list));
                 return;
             } catch (Exception e) {
@@ -3446,7 +3445,7 @@ class ActivityStack<T extends StackWindowController> extends ConfigurationContai
                 }
                 EventLogTags.writeAmStopActivity(
                         r.userId, System.identityHashCode(r), r.shortComponentName);
-                mService.mLifecycleManager.scheduleTransaction(r.app.thread, r.appToken,
+                mService.getLifecycleManager().scheduleTransaction(r.app.thread, r.appToken,
                         StopActivityItem.obtain(r.visible, r.configChangeFlags)
                                 .setDescription(r.getLifecycleDescription("stopActivityLocked")));
                 if (shouldSleepOrShutDownActivities()) {
@@ -3782,6 +3781,15 @@ class ActivityStack<T extends StackWindowController> extends ConfigurationContai
         }
         final ActivityState prevState = r.getState();
         if (DEBUG_STATES) Slog.v(TAG_STATES, "Moving to FINISHING: " + r);
+
+        // We are already destroying / have already destroyed the activity. Do not continue to
+        // modify it. Note that we do not use ActivityRecord#finishing here as finishing is not
+        // indicative of destruction (though destruction is indicative of finishing) as finishing
+        // can be delayed below.
+        if (r.isState(DESTROYING, DESTROYED)) {
+            return null;
+        }
+
         r.setState(FINISHING, "finishCurrentActivityLocked");
         final boolean finishingActivityInNonFocusedStack
                 = r.getStack() != mStackSupervisor.getFocusedStack()
@@ -4232,6 +4240,13 @@ class ActivityStack<T extends StackWindowController> extends ConfigurationContai
         if (DEBUG_SWITCH || DEBUG_CLEANUP) Slog.v(TAG_SWITCH,
                 "Removing activity from " + reason + ": token=" + r
                         + ", app=" + (r.app != null ? r.app.processName : "(null)"));
+
+        if (r.isState(DESTROYING, DESTROYED)) {
+            if (DEBUG_STATES) Slog.v(TAG_STATES, "activity " + r + " already finishing."
+                    + "skipping request with reason:" + reason);
+            return false;
+        }
+
         EventLog.writeEvent(EventLogTags.AM_DESTROY_ACTIVITY,
                 r.userId, System.identityHashCode(r),
                 r.getTask().taskId, r.shortComponentName, reason);
@@ -4265,7 +4280,7 @@ class ActivityStack<T extends StackWindowController> extends ConfigurationContai
 
             try {
                 if (DEBUG_SWITCH) Slog.i(TAG_SWITCH, "Destroying: " + r);
-                mService.mLifecycleManager.scheduleTransaction(r.app.thread, r.appToken,
+                mService.getLifecycleManager().scheduleTransaction(r.app.thread, r.appToken,
                         DestroyActivityItem.obtain(r.finishing, r.configChangeFlags)
                             .setDescription(
                                     r.getLifecycleDescription("destroyActivityLocked:" + reason)));

diff --git a/services/core/java/com/android/server/am/ActivityStackSupervisor.java b/services/core/java/com/android/server/am/ActivityStackSupervisor.java
index 26ffe95..740a312 100644 (file)
--- a/services/core/java/com/android/server/am/ActivityStackSupervisor.java
+++ b/services/core/java/com/android/server/am/ActivityStackSupervisor.java
@@ -1459,7 +1459,7 @@ public class ActivityStackSupervisor extends ConfigurationContainer implements D
                 clientTransaction.setLifecycleStateRequest(lifecycleItem);
 
                 // Schedule transaction.
-                mService.mLifecycleManager.scheduleTransaction(clientTransaction);
+                mService.getLifecycleManager().scheduleTransaction(clientTransaction);
 
 
                 if ((app.info.privateFlags & ApplicationInfo.PRIVATE_FLAG_CANT_SAVE_STATE) != 0) {

diff --git a/services/tests/servicestests/src/com/android/server/am/ActivityRecordTests.java b/services/tests/servicestests/src/com/android/server/am/ActivityRecordTests.java
index fad7180..5b1f5c1 100644 (file)
--- a/services/tests/servicestests/src/com/android/server/am/ActivityRecordTests.java
+++ b/services/tests/servicestests/src/com/android/server/am/ActivityRecordTests.java
@@ -24,6 +24,7 @@ import static android.view.Display.DEFAULT_DISPLAY;
 
 import static com.android.server.am.ActivityStack.ActivityState.DESTROYED;
 import static com.android.server.am.ActivityStack.ActivityState.DESTROYING;
+import static com.android.server.am.ActivityStack.ActivityState.FINISHING;
 import static com.android.server.am.ActivityStack.ActivityState.INITIALIZING;
 import static com.android.server.am.ActivityStack.ActivityState.PAUSED;
 import static com.android.server.am.ActivityStack.ActivityState.PAUSING;
@@ -229,4 +230,19 @@ public class ActivityRecordTests extends ActivityTestsBase {
         assertTrue(mActivity.isState(DESTROYED));
         assertTrue(mActivity.finishing);
     }
+
+    @Test
+    public void testSetInvalidState() throws Exception {
+        mActivity.setState(DESTROYED, "testInvalidState");
+
+        boolean exceptionEncountered = false;
+
+        try {
+            mActivity.setState(FINISHING, "testInvalidState");
+        } catch (IllegalArgumentException e) {
+            exceptionEncountered = true;
+        }
+
+        assertTrue(exceptionEncountered);
+    }
 }

diff --git a/services/tests/servicestests/src/com/android/server/am/ActivityStackTests.java b/services/tests/servicestests/src/com/android/server/am/ActivityStackTests.java
index a98d5a1..32a29a2 100644 (file)
--- a/services/tests/servicestests/src/com/android/server/am/ActivityStackTests.java
+++ b/services/tests/servicestests/src/com/android/server/am/ActivityStackTests.java
@@ -24,6 +24,8 @@ import static android.app.WindowConfiguration.WINDOWING_MODE_PINNED;
 import static android.app.WindowConfiguration.WINDOWING_MODE_SPLIT_SCREEN_PRIMARY;
 import static android.app.WindowConfiguration.WINDOWING_MODE_SPLIT_SCREEN_SECONDARY;
 
+import static com.android.server.am.ActivityStack.ActivityState.DESTROYED;
+import static com.android.server.am.ActivityStack.ActivityState.DESTROYING;
 import static com.android.server.am.ActivityStack.REMOVE_TASK_MODE_DESTROYING;
 
 import static org.junit.Assert.assertEquals;
@@ -31,8 +33,13 @@ import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertTrue;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
 
-import android.content.ComponentName;
+import android.app.servertransaction.DestroyActivityItem;
 import android.content.pm.ActivityInfo;
 import android.os.UserHandle;
 import android.platform.test.annotations.Presubmit;
@@ -457,4 +464,26 @@ public class ActivityStackTests extends ActivityTestsBase {
                 .setCreateTask(true).build();
         return stack;
     }
+
+    @Test
+    public void testSuppressMultipleDestroy() throws Exception {
+        final ActivityRecord r = new ActivityBuilder(mService).setTask(mTask).build();
+        final ClientLifecycleManager lifecycleManager = mock(ClientLifecycleManager.class);
+        final ProcessRecord app = r.app;
+
+        // The mocked lifecycle manager must be set on the ActivityStackSupervisor's reference to
+        // the service rather than mService as mService is a spy and setting the value will not
+        // propagate as ActivityManagerService hands its own reference to the
+        // ActivityStackSupervisor during construction.
+        ((TestActivityManagerService) mSupervisor.mService).setLifecycleManager(lifecycleManager);
+
+        mStack.destroyActivityLocked(r, true, "first invocation");
+        verify(lifecycleManager, times(1)).scheduleTransaction(eq(app.thread),
+                eq(r.appToken), any(DestroyActivityItem.class));
+        assertTrue(r.isState(DESTROYED, DESTROYING));
+
+        mStack.destroyActivityLocked(r, true, "second invocation");
+        verify(lifecycleManager, times(1)).scheduleTransaction(eq(app.thread),
+                eq(r.appToken), any(DestroyActivityItem.class));
+    }
 }

diff --git a/services/tests/servicestests/src/com/android/server/am/ActivityTestsBase.java b/services/tests/servicestests/src/com/android/server/am/ActivityTestsBase.java
index 10253c5..ff7b1d0 100644 (file)
--- a/services/tests/servicestests/src/com/android/server/am/ActivityTestsBase.java
+++ b/services/tests/servicestests/src/com/android/server/am/ActivityTestsBase.java
@@ -273,6 +273,8 @@ public class ActivityTestsBase {
      * {@link ActivityStackSupervisor}.
      */
     protected static class TestActivityManagerService extends ActivityManagerService {
+        private ClientLifecycleManager mLifecycleManager;
+
         TestActivityManagerService(Context context) {
             super(context);
             mSupportsMultiWindow = true;
@@ -284,6 +286,18 @@ public class ActivityTestsBase {
         }
 
         @Override
+        public ClientLifecycleManager getLifecycleManager() {
+            if (mLifecycleManager == null) {
+                return super.getLifecycleManager();
+            }
+            return mLifecycleManager;
+        }
+
+        void setLifecycleManager(ClientLifecycleManager manager) {
+            mLifecycleManager = manager;
+        }
+
+        @Override
         final protected ActivityStackSupervisor createStackSupervisor() {
             final ActivityStackSupervisor supervisor = spy(createTestSupervisor());