From 2b8e0378ba7eda2203a7152a689c100259ae52ef Mon Sep 17 00:00:00 2001 From: Bryce Lee Date: Thu, 5 Apr 2018 17:01:37 -0700 Subject: [PATCH] Properly remove activity from task on start failure. When we fail to start an activity, it is removed from its parent task by the starter. We currently directly ask the task to remove the child activity. This is incorrect as the stack is unaware of the movement and cannot cleanup the task if necessary. This changelist addresses the problem by asking the stack instead to finish the activity. Change-Id: I41faa2f4e81873601d45a8ccd1bf22fef9e57033 Fixes: 76679042 Test: atest FrameworksServicesTests:ActivityStarterTests#testTaskModeViolation --- .../server/am/ActivityManagerService.java | 6 +- .../com/android/server/am/ActivityRecord.java | 10 ++- .../com/android/server/am/ActivityStack.java | 11 +++- .../server/am/ActivityStackSupervisor.java | 11 ++-- .../android/server/am/ActivityStarter.java | 15 +++-- .../android/server/am/EventLogTags.logtags | 5 +- .../com/android/server/am/RecentTasks.java | 4 +- .../server/am/SafeActivityOptions.java | 2 +- .../com/android/server/am/TaskRecord.java | 7 +- .../com/android/server/am/UserController.java | 2 +- .../server/am/ActivityStarterTests.java | 49 ++++++++++---- .../android/server/am/ActivityTestsBase.java | 65 +++++++++++++++++-- 12 files changed, 141 insertions(+), 46 deletions(-) diff --git a/services/core/java/com/android/server/am/ActivityManagerService.java b/services/core/java/com/android/server/am/ActivityManagerService.java index 2e87a442a3f1d..0a72249803a70 100644 --- a/services/core/java/com/android/server/am/ActivityManagerService.java +++ b/services/core/java/com/android/server/am/ActivityManagerService.java @@ -764,7 +764,7 @@ public class ActivityManagerService extends IActivityManager.Stub /** * The controller for all operations related to locktask. */ - final LockTaskController mLockTaskController; + private final LockTaskController mLockTaskController; final UserController mUserController; @@ -12396,6 +12396,10 @@ public class ActivityManagerService extends IActivityManager.Stub return mActivityStartController; } + LockTaskController getLockTaskController() { + return mLockTaskController; + } + ClientLifecycleManager getLifecycleManager() { return mLifecycleManager; } diff --git a/services/core/java/com/android/server/am/ActivityRecord.java b/services/core/java/com/android/server/am/ActivityRecord.java index 1af4114441c36..a15967be75846 100644 --- a/services/core/java/com/android/server/am/ActivityRecord.java +++ b/services/core/java/com/android/server/am/ActivityRecord.java @@ -1590,14 +1590,20 @@ final class ActivityRecord extends ConfigurationContainer implements AppWindowCo void pauseKeyDispatchingLocked() { if (!keysPaused) { keysPaused = true; - mWindowContainerController.pauseKeyDispatching(); + + if (mWindowContainerController != null) { + mWindowContainerController.pauseKeyDispatching(); + } } } void resumeKeyDispatchingLocked() { if (keysPaused) { keysPaused = false; - mWindowContainerController.resumeKeyDispatching(); + + if (mWindowContainerController != null) { + mWindowContainerController.resumeKeyDispatching(); + } } } diff --git a/services/core/java/com/android/server/am/ActivityStack.java b/services/core/java/com/android/server/am/ActivityStack.java index 649d5770f6a9b..1e9edd9a1b449 100644 --- a/services/core/java/com/android/server/am/ActivityStack.java +++ b/services/core/java/com/android/server/am/ActivityStack.java @@ -3743,7 +3743,7 @@ class ActivityStack extends ConfigurationContai } if (endTask) { - mService.mLockTaskController.clearLockedTask(task); + mService.getLockTaskController().clearLockedTask(task); } } else if (!r.isState(PAUSING)) { // If the activity is PAUSING, we will complete the finish once @@ -4639,7 +4639,7 @@ class ActivityStack extends ConfigurationContai // In LockTask mode, moving a locked task to the back of the stack may expose unlocked // ones. Therefore we need to check if this operation is allowed. - if (!mService.mLockTaskController.canMoveTaskToBack(tr)) { + if (!mService.getLockTaskController().canMoveTaskToBack(tr)) { return false; } @@ -5084,7 +5084,12 @@ class ActivityStack extends ConfigurationContai onActivityRemovedFromStack(record); } - mTaskHistory.remove(task); + final boolean removed = mTaskHistory.remove(task); + + if (removed) { + EventLog.writeEvent(EventLogTags.AM_REMOVE_TASK, task.taskId, getStackId()); + } + removeActivitiesFromLRUListLocked(task); updateTaskMovement(task, true); diff --git a/services/core/java/com/android/server/am/ActivityStackSupervisor.java b/services/core/java/com/android/server/am/ActivityStackSupervisor.java index ea089e0ce2396..031ea25c364a3 100644 --- a/services/core/java/com/android/server/am/ActivityStackSupervisor.java +++ b/services/core/java/com/android/server/am/ActivityStackSupervisor.java @@ -1371,12 +1371,13 @@ public class ActivityStackSupervisor extends ConfigurationContainer implements D mService.updateLruProcessLocked(app, true, null); mService.updateOomAdjLocked(); + final LockTaskController lockTaskController = mService.getLockTaskController(); if (task.mLockTaskAuth == LOCK_TASK_AUTH_LAUNCHABLE || task.mLockTaskAuth == LOCK_TASK_AUTH_LAUNCHABLE_PRIV || (task.mLockTaskAuth == LOCK_TASK_AUTH_WHITELISTED - && mService.mLockTaskController.getLockTaskModeState() - == LOCK_TASK_MODE_LOCKED)) { - mService.mLockTaskController.startLockTaskMode(task, false, 0 /* blank UID */); + && lockTaskController.getLockTaskModeState() + == LOCK_TASK_MODE_LOCKED)) { + lockTaskController.startLockTaskMode(task, false, 0 /* blank UID */); } try { @@ -2899,7 +2900,7 @@ public class ActivityStackSupervisor extends ConfigurationContainer implements D if (tr != null) { tr.removeTaskActivitiesLocked(pauseImmediately, reason); cleanUpRemovedTaskLocked(tr, killProcess, removeFromRecents); - mService.mLockTaskController.clearLockedTask(tr); + mService.getLockTaskController().clearLockedTask(tr); if (tr.isPersistable) { mService.notifyTaskPersisterLocked(null, true); } @@ -3813,7 +3814,7 @@ public class ActivityStackSupervisor extends ConfigurationContainer implements D pw.print(mRecentTasks.isRecentsComponentHomeActivity(mCurrentUser)); getKeyguardController().dump(pw, prefix); - mService.mLockTaskController.dump(pw, prefix); + mService.getLockTaskController().dump(pw, prefix); } public void writeToProto(ProtoOutputStream proto, long fieldId) { diff --git a/services/core/java/com/android/server/am/ActivityStarter.java b/services/core/java/com/android/server/am/ActivityStarter.java index 1b7e1edeffe91..182a3f60d211a 100644 --- a/services/core/java/com/android/server/am/ActivityStarter.java +++ b/services/core/java/com/android/server/am/ActivityStarter.java @@ -1147,9 +1147,10 @@ class ActivityStarter { // If we are not able to proceed, disassociate the activity from the task. Leaving an // activity in an incomplete state can lead to issues, such as performing operations // without a window container. - if (!ActivityManager.isStartResultSuccessful(result) - && mStartActivity.getTask() != null) { - mStartActivity.getTask().removeActivity(mStartActivity); + final ActivityStack stack = mStartActivity.getStack(); + if (!ActivityManager.isStartResultSuccessful(result) && stack != null) { + stack.finishActivityLocked(mStartActivity, RESULT_CANCELED, + null /* intentResultData */, "startActivity", true /* oomAdj */); } mService.mWindowManager.continueSurfaceLayout(); } @@ -1199,7 +1200,7 @@ class ActivityStarter { // When the flags NEW_TASK and CLEAR_TASK are set, then the task gets reused but // still needs to be a lock task mode violation since the task gets cleared out and // the device would otherwise leave the locked task. - if (mService.mLockTaskController.isLockTaskModeViolation(reusedActivity.getTask(), + if (mService.getLockTaskController().isLockTaskModeViolation(reusedActivity.getTask(), (mLaunchFlags & (FLAG_ACTIVITY_NEW_TASK | FLAG_ACTIVITY_CLEAR_TASK)) == (FLAG_ACTIVITY_NEW_TASK | FLAG_ACTIVITY_CLEAR_TASK))) { Slog.e(TAG, "startActivityUnchecked: Attempt to violate Lock Task Mode"); @@ -2011,7 +2012,7 @@ class ActivityStarter { mStartActivity.setTaskToAffiliateWith(taskToAffiliate); } - if (mService.mLockTaskController.isLockTaskModeViolation(mStartActivity.getTask())) { + if (mService.getLockTaskController().isLockTaskModeViolation(mStartActivity.getTask())) { Slog.e(TAG, "Attempted Lock Task Mode violation mStartActivity=" + mStartActivity); return START_RETURN_LOCK_TASK_MODE_VIOLATION; } @@ -2034,7 +2035,7 @@ class ActivityStarter { } private int setTaskFromSourceRecord() { - if (mService.mLockTaskController.isLockTaskModeViolation(mSourceRecord.getTask())) { + if (mService.getLockTaskController().isLockTaskModeViolation(mSourceRecord.getTask())) { Slog.e(TAG, "Attempted Lock Task Mode violation mStartActivity=" + mStartActivity); return START_RETURN_LOCK_TASK_MODE_VIOLATION; } @@ -2128,7 +2129,7 @@ class ActivityStarter { private int setTaskFromInTask() { // The caller is asking that the new activity be started in an explicit // task it has provided to us. - if (mService.mLockTaskController.isLockTaskModeViolation(mInTask)) { + if (mService.getLockTaskController().isLockTaskModeViolation(mInTask)) { Slog.e(TAG, "Attempted Lock Task Mode violation mStartActivity=" + mStartActivity); return START_RETURN_LOCK_TASK_MODE_VIOLATION; } diff --git a/services/core/java/com/android/server/am/EventLogTags.logtags b/services/core/java/com/android/server/am/EventLogTags.logtags index 9caef4a001aef..40b9e4fde874b 100644 --- a/services/core/java/com/android/server/am/EventLogTags.logtags +++ b/services/core/java/com/android/server/am/EventLogTags.logtags @@ -133,4 +133,7 @@ option java_package com.android.server.am # The activity's onStart has been called. 30059 am_on_start_called (User|1|5),(Component Name|3),(Reason|3) # The activity's onDestroy has been called. -30060 am_on_destroy_called (User|1|5),(Component Name|3),(Reason|3) \ No newline at end of file +30060 am_on_destroy_called (User|1|5),(Component Name|3),(Reason|3) + +# The task is being removed from its parent stack +30061 am_remove_task (Task ID|1|5), (Stack ID|1|5) \ No newline at end of file diff --git a/services/core/java/com/android/server/am/RecentTasks.java b/services/core/java/com/android/server/am/RecentTasks.java index 1d305fb4248f1..f140af7ee2dee 100644 --- a/services/core/java/com/android/server/am/RecentTasks.java +++ b/services/core/java/com/android/server/am/RecentTasks.java @@ -523,7 +523,7 @@ class RecentTasks { } for (int i = mTasks.size() - 1; i >= 0; --i) { final TaskRecord tr = mTasks.get(i); - if (tr.userId == userId && !mService.mLockTaskController.isTaskWhitelisted(tr)) { + if (tr.userId == userId && !mService.getLockTaskController().isTaskWhitelisted(tr)) { remove(tr); } } @@ -1156,7 +1156,7 @@ class RecentTasks { } // If we're in lock task mode, ignore the root task - if (task == mService.mLockTaskController.getRootTask()) { + if (task == mService.getLockTaskController().getRootTask()) { return false; } diff --git a/services/core/java/com/android/server/am/SafeActivityOptions.java b/services/core/java/com/android/server/am/SafeActivityOptions.java index ac6f01fa855f3..2de752731ab6a 100644 --- a/services/core/java/com/android/server/am/SafeActivityOptions.java +++ b/services/core/java/com/android/server/am/SafeActivityOptions.java @@ -210,7 +210,7 @@ class SafeActivityOptions { // Check if someone tries to launch an unwhitelisted activity into LockTask mode. final boolean lockTaskMode = options.getLockTaskMode(); if (aInfo != null && lockTaskMode - && !supervisor.mService.mLockTaskController.isPackageWhitelisted( + && !supervisor.mService.getLockTaskController().isPackageWhitelisted( UserHandle.getUserId(callingUid), aInfo.packageName)) { final String msg = "Permission Denial: starting " + getIntentString(intent) + " from " + callerApp + " (pid=" + callingPid diff --git a/services/core/java/com/android/server/am/TaskRecord.java b/services/core/java/com/android/server/am/TaskRecord.java index 034cb2e373d5b..737105d8bdf11 100644 --- a/services/core/java/com/android/server/am/TaskRecord.java +++ b/services/core/java/com/android/server/am/TaskRecord.java @@ -451,7 +451,7 @@ class TaskRecord extends ConfigurationContainer implements TaskWindowContainerLi } void removeWindowContainer() { - mService.mLockTaskController.clearLockedTask(this); + mService.getLockTaskController().clearLockedTask(this); mWindowContainerController.removeContainer(); if (!getWindowConfiguration().persistTaskBounds()) { // Reset current bounds for task whose bounds shouldn't be persisted so it uses @@ -1446,9 +1446,10 @@ class TaskRecord extends ConfigurationContainer implements TaskWindowContainerLi } final String pkg = (realActivity != null) ? realActivity.getPackageName() : null; + final LockTaskController lockTaskController = mService.getLockTaskController(); switch (r.lockTaskLaunchMode) { case LOCK_TASK_LAUNCH_MODE_DEFAULT: - mLockTaskAuth = mService.mLockTaskController.isPackageWhitelisted(userId, pkg) + mLockTaskAuth = lockTaskController.isPackageWhitelisted(userId, pkg) ? LOCK_TASK_AUTH_WHITELISTED : LOCK_TASK_AUTH_PINNABLE; break; @@ -1461,7 +1462,7 @@ class TaskRecord extends ConfigurationContainer implements TaskWindowContainerLi break; case LOCK_TASK_LAUNCH_MODE_IF_WHITELISTED: - mLockTaskAuth = mService.mLockTaskController.isPackageWhitelisted(userId, pkg) + mLockTaskAuth = lockTaskController.isPackageWhitelisted(userId, pkg) ? LOCK_TASK_AUTH_LAUNCHABLE : LOCK_TASK_AUTH_PINNABLE; break; } diff --git a/services/core/java/com/android/server/am/UserController.java b/services/core/java/com/android/server/am/UserController.java index a2943346e0749..425a3210d7619 100644 --- a/services/core/java/com/android/server/am/UserController.java +++ b/services/core/java/com/android/server/am/UserController.java @@ -2220,7 +2220,7 @@ class UserController implements Handler.Callback { protected void clearAllLockedTasks(String reason) { synchronized (mService) { - mService.mLockTaskController.clearLockedTasks(reason); + mService.getLockTaskController().clearLockedTasks(reason); } } diff --git a/services/tests/servicestests/src/com/android/server/am/ActivityStarterTests.java b/services/tests/servicestests/src/com/android/server/am/ActivityStarterTests.java index d012bba375d3b..18e842f3e694f 100644 --- a/services/tests/servicestests/src/com/android/server/am/ActivityStarterTests.java +++ b/services/tests/servicestests/src/com/android/server/am/ActivityStarterTests.java @@ -23,6 +23,7 @@ import static android.app.ActivityManager.START_FORWARD_AND_REQUEST_CONFLICT; import static android.app.ActivityManager.START_INTENT_NOT_RESOLVED; import static android.app.ActivityManager.START_NOT_VOICE_COMPATIBLE; import static android.app.ActivityManager.START_PERMISSION_DENIED; +import static android.app.ActivityManager.START_RETURN_LOCK_TASK_MODE_VIOLATION; import static android.app.ActivityManager.START_SUCCESS; import static android.app.ActivityManager.START_SWITCHES_CANCELED; import static android.app.ActivityManager.START_TASK_TO_FRONT; @@ -34,6 +35,7 @@ import static android.app.WindowConfiguration.WINDOWING_MODE_SPLIT_SCREEN_SECOND import android.app.ActivityOptions; import android.app.IApplicationThread; +import android.content.ComponentName; import android.content.Intent; import android.content.pm.ActivityInfo; import android.content.pm.ActivityInfo.WindowLayout; @@ -74,6 +76,8 @@ import com.android.server.am.ActivityStarter.Factory; import com.android.server.am.LaunchParamsController.LaunchParamsModifier; import com.android.server.am.TaskRecord.TaskRecordFactory; +import java.util.ArrayList; + /** * Tests for the {@link ActivityStarter} class. * @@ -301,13 +305,14 @@ public class ActivityStarterTests extends ActivityTestsBase { anyBoolean(), any(), any(), any()); // instrument the stack and task used. - final ActivityStack stack = spy(mService.mStackSupervisor.getDefaultDisplay().createStack( - WINDOWING_MODE_FULLSCREEN, ACTIVITY_TYPE_STANDARD, true /* onTop */)); - final TaskRecord task = - spy(new TaskBuilder(mService.mStackSupervisor).setStack(stack).build()); + final ActivityStack stack = mService.mStackSupervisor.getDefaultDisplay().createStack( + WINDOWING_MODE_FULLSCREEN, ACTIVITY_TYPE_STANDARD, true /* onTop */); + final TaskRecord task = new TaskBuilder(mService.mStackSupervisor) + .setCreateStack(false) + .build(); // supervisor needs a focused stack. - mService.mStackSupervisor.mFocusedStack = task.getStack(); + mService.mStackSupervisor.mFocusedStack = stack; // use factory that only returns spy task. final TaskRecordFactory factory = mock(TaskRecordFactory.class); @@ -322,14 +327,6 @@ public class ActivityStarterTests extends ActivityTestsBase { doReturn(stack).when(mService.mStackSupervisor) .getLaunchStack(any(), any(), any(), anyBoolean(), anyInt()); - // ignore the start request. - doNothing().when(stack) - .startActivityLocked(any(), any(), anyBoolean(), anyBoolean(), any()); - - // ignore requests to create window container. - doNothing().when(task).createWindowContainer(anyBoolean(), anyBoolean()); - - final Intent intent = new Intent(); intent.addFlags(launchFlags); intent.setComponent(ActivityBuilder.getDefaultComponent()); @@ -448,4 +445,30 @@ public class ActivityStarterTests extends ActivityTestsBase { // Ensure result is moving task to front. assertEquals(result, START_TASK_TO_FRONT); } + + /** + * Tests activity is cleaned up properly in a task mode violation. + */ + @Test + public void testTaskModeViolation() { + final ActivityDisplay display = mService.mStackSupervisor.getDefaultDisplay(); + assertNoTasks(display); + + final ActivityStarter starter = prepareStarter(0); + + final LockTaskController lockTaskController = mService.getLockTaskController(); + doReturn(true).when(lockTaskController).isLockTaskModeViolation(any()); + + final int result = starter.setReason("testTaskModeViolation").execute(); + + assertEquals(START_RETURN_LOCK_TASK_MODE_VIOLATION, result); + assertNoTasks(display); + } + + private void assertNoTasks(ActivityDisplay display) { + for (int i = display.getChildCount() - 1; i >= 0; --i) { + final ActivityStack stack = display.getChildAt(i); + assertTrue(stack.getAllTasks().isEmpty()); + } + } } 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 741901d2e5dc0..f5e61a1db341b 100644 --- a/services/tests/servicestests/src/com/android/server/am/ActivityTestsBase.java +++ b/services/tests/servicestests/src/com/android/server/am/ActivityTestsBase.java @@ -30,6 +30,7 @@ import static org.mockito.Mockito.anyInt; import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.spy; +import android.app.ActivityOptions; import com.android.server.wm.DisplayWindowController; import org.junit.Rule; @@ -51,6 +52,9 @@ import android.service.voice.IVoiceInteractionSession; import android.support.test.InstrumentationRegistry; import android.testing.DexmakerShareClassLoaderRule; + +import com.android.internal.app.IVoiceInteractor; + import com.android.server.AttributeCache; import com.android.server.wm.AppWindowContainerController; import com.android.server.wm.PinnedStackWindowController; @@ -62,6 +66,7 @@ import org.junit.After; import org.junit.Before; import org.mockito.MockitoAnnotations; + /** * A base class to handle common operations in activity related unit tests. */ @@ -215,6 +220,7 @@ public class ActivityTestsBase { private int mTaskId = 0; private int mUserId = 0; private IVoiceInteractionSession mVoiceSession; + private boolean mCreateStack = true; private ActivityStack mStack; @@ -232,6 +238,15 @@ public class ActivityTestsBase { return this; } + /** + * Set to {@code true} by default, set to {@code false} to prevent the task from + * automatically creating a parent stack. + */ + TaskBuilder setCreateStack(boolean createStack) { + mCreateStack = createStack; + return this; + } + TaskBuilder setVoiceSession(IVoiceInteractionSession session) { mVoiceSession = session; return this; @@ -258,7 +273,7 @@ public class ActivityTestsBase { } TaskRecord build() { - if (mStack == null) { + if (mStack == null && mCreateStack) { mStack = mSupervisor.getDefaultDisplay().createStack( WINDOWING_MODE_FULLSCREEN, ACTIVITY_TYPE_STANDARD, true /* onTop */); } @@ -276,17 +291,38 @@ public class ActivityTestsBase { intent.setComponent(mComponent); intent.setFlags(mFlags); - final TaskRecord task = new TaskRecord(mSupervisor.mService, mTaskId, aInfo, + final TestTaskRecord task = new TestTaskRecord(mSupervisor.mService, mTaskId, aInfo, intent /*intent*/, mVoiceSession, null /*_voiceInteractor*/); task.userId = mUserId; - mSupervisor.setFocusStackUnchecked("test", mStack); - mStack.addTask(task, true, "creating test task"); - task.setStack(mStack); - task.setWindowContainerController(mock(TaskWindowContainerController.class)); + + if (mStack != null) { + mSupervisor.setFocusStackUnchecked("test", mStack); + mStack.addTask(task, true, "creating test task"); + task.setStack(mStack); + task.setWindowContainerController(); + } + task.touchActiveTime(); return task; } + + private static class TestTaskRecord extends TaskRecord { + TestTaskRecord(ActivityManagerService service, int _taskId, ActivityInfo info, + Intent _intent, IVoiceInteractionSession _voiceSession, + IVoiceInteractor _voiceInteractor) { + super(service, _taskId, info, _intent, _voiceSession, _voiceInteractor); + } + + @Override + void createWindowContainer(boolean onTop, boolean showForAllUsers) { + setWindowContainerController(); + } + + private void setWindowContainerController() { + setWindowContainerController(mock(TaskWindowContainerController.class)); + } + } } /** @@ -295,6 +331,7 @@ public class ActivityTestsBase { */ protected static class TestActivityManagerService extends ActivityManagerService { private ClientLifecycleManager mLifecycleManager; + private LockTaskController mLockTaskController; TestActivityManagerService(Context context) { super(context); @@ -314,6 +351,14 @@ public class ActivityTestsBase { return mLifecycleManager; } + public LockTaskController getLockTaskController() { + if (mLockTaskController == null) { + mLockTaskController = spy(super.getLockTaskController()); + } + + return mLockTaskController; + } + void setLifecycleManager(ClientLifecycleManager manager) { mLifecycleManager = manager; } @@ -444,7 +489,7 @@ public class ActivityTestsBase { } /** - * Overrided of {@link ActivityStack} that tracks test metrics, such as the number of times a + * Overridden {@link ActivityStack} that tracks test metrics, such as the number of times a * method is called. Note that its functionality depends on the implementations of the * construction arguments. */ @@ -530,5 +575,11 @@ public class ActivityTestsBase { return super.supportsSplitScreenWindowingMode(); } } + + @Override + void startActivityLocked(ActivityRecord r, ActivityRecord focusedTopActivity, + boolean newTask, boolean keepCurTransition, + ActivityOptions options) { + } } }