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
--- a/services/core/java/com/android/server/am/ActivityManagerService.java
+++ b/services/core/java/com/android/server/am/ActivityManagerService.java
@@ -685,7 +685,7 @@
private final ActivityStartController mActivityStartController;
- final ClientLifecycleManager mLifecycleManager;
+ private final ClientLifecycleManager mLifecycleManager;
final TaskChangeNotificationController mTaskChangeNotificationController;
@@ -12303,6 +12303,10 @@
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
--- a/services/core/java/com/android/server/am/ActivityRecord.java
+++ b/services/core/java/com/android/server/am/ActivityRecord.java
@@ -665,7 +665,7 @@
"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 @@
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 @@
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 @@
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 @@
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 @@
if (DEBUG_STATES) Slog.v(TAG_STATES, "State movement: " + this + " from:" + getState()
+ " to:" + state + " reason:" + reason);
+ 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;
+ }
+
+ 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);
+ }
+
final ActivityState prev = mState;
- final boolean stateChanged = prev != state;
+ mState = state;
+
+ if (mRecentTransitions.size() == MAX_STORED_STATE_TRANSITIONS) {
+ mRecentTransitions.remove(0);
+ }
+
+ mRecentTransitions.add(new StateTransition(prev, state, reason));
mState = state;
- if (stateChanged) {
- if (mRecentTransitions.size() == MAX_STORED_STATE_TRANSITIONS) {
- mRecentTransitions.remove(0);
- }
-
- mRecentTransitions.add(new StateTransition(prev, state, reason));
- }
-
- if (stateChanged && isState(DESTROYING, DESTROYED)) {
+ if (isState(DESTROYING, DESTROYED)) {
makeFinishingLocked();
// When moving to the destroyed state, immediately destroy the activity in the
@@ -1726,7 +1736,7 @@
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 @@
// 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 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
--- a/services/core/java/com/android/server/am/ActivityStack.java
+++ b/services/core/java/com/android/server/am/ActivityStack.java
@@ -1472,7 +1472,7 @@
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 @@
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 @@
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 @@
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 @@
}
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 @@
}
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 @@
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 @@
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
--- a/services/core/java/com/android/server/am/ActivityStackSupervisor.java
+++ b/services/core/java/com/android/server/am/ActivityStackSupervisor.java
@@ -1459,7 +1459,7 @@
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
--- 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 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 @@
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
--- 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_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.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 @@
.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
--- 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 @@
* {@link ActivityStackSupervisor}.
*/
protected static class TestActivityManagerService extends ActivityManagerService {
+ private ClientLifecycleManager mLifecycleManager;
+
TestActivityManagerService(Context context) {
super(context);
mSupportsMultiWindow = true;
@@ -284,6 +286,18 @@
}
@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());