diff options
7 files changed, 124 insertions, 70 deletions
diff --git a/services/core/java/com/android/server/am/ActivityManagerService.java b/services/core/java/com/android/server/am/ActivityManagerService.java index 26899f196126..e4271ddb5195 100644 --- a/services/core/java/com/android/server/am/ActivityManagerService.java +++ b/services/core/java/com/android/server/am/ActivityManagerService.java @@ -16145,8 +16145,7 @@ public class ActivityManagerService extends IActivityManager.Stub @Override public boolean startUserInBackgroundOnSecondaryDisplay(int userId, int displayId) { // Permission check done inside UserController. - return mUserController.startUserOnSecondaryDisplay(userId, displayId, - /* unlockListener= */ null); + return mUserController.startUserOnSecondaryDisplay(userId, displayId); } /** diff --git a/services/core/java/com/android/server/am/UserController.java b/services/core/java/com/android/server/am/UserController.java index fa5cd2841342..1954f76825cd 100644 --- a/services/core/java/com/android/server/am/UserController.java +++ b/services/core/java/com/android/server/am/UserController.java @@ -1375,8 +1375,9 @@ class UserController implements Handler.Callback { } final int profilesToStartSize = profilesToStart.size(); int i = 0; - // TODO(b/239982558): pass displayId for (; i < profilesToStartSize && i < (getMaxRunningUsers() - 1); ++i) { + // NOTE: this method is setting the profiles of the current user - which is always + // assigned to the default display - so there's no need to pass PARENT_DISPLAY startUser(profilesToStart.get(i).id, /* foreground= */ false); } if (i < profilesToStartSize) { @@ -1419,8 +1420,10 @@ class UserController implements Handler.Callback { return false; } - // TODO(b/239982558): pass proper displayId - return startUserNoChecks(userId, Display.DEFAULT_DISPLAY, /* foreground= */ false, + int displayId = mInjector.isUsersOnSecondaryDisplaysEnabled() + ? UserManagerInternal.PARENT_DISPLAY + : Display.DEFAULT_DISPLAY; + return startUserNoChecks(userId, displayId, /* foreground= */ false, /* unlockListener= */ null); } @@ -1472,8 +1475,7 @@ class UserController implements Handler.Callback { // TODO(b/239982558): add javadoc (need to wait until the intents / SystemService callbacks are // defined - boolean startUserOnSecondaryDisplay(@UserIdInt int userId, int displayId, - @Nullable IProgressListener unlockListener) { + boolean startUserOnSecondaryDisplay(@UserIdInt int userId, int displayId) { checkCallingHasOneOfThosePermissions("startUserOnSecondaryDisplay", MANAGE_USERS, CREATE_USERS); @@ -1482,7 +1484,8 @@ class UserController implements Handler.Callback { "Cannot use DEFAULT_DISPLAY"); try { - return startUserNoChecks(userId, displayId, /* foreground= */ false, unlockListener); + return startUserNoChecks(userId, displayId, /* foreground= */ false, + /* unlockListener= */ null); } catch (RuntimeException e) { Slogf.w(TAG, "startUserOnSecondaryDisplay(%d, %d) failed: %s", userId, displayId, e); return false; @@ -3484,5 +3487,9 @@ class UserController implements Handler.Callback { } }, reason); } + + boolean isUsersOnSecondaryDisplaysEnabled() { + return UserManager.isUsersOnSecondaryDisplaysEnabled(); + } } } diff --git a/services/core/java/com/android/server/pm/UserManagerInternal.java b/services/core/java/com/android/server/pm/UserManagerInternal.java index b9a119504d1f..4f5fd023d470 100644 --- a/services/core/java/com/android/server/pm/UserManagerInternal.java +++ b/services/core/java/com/android/server/pm/UserManagerInternal.java @@ -46,6 +46,15 @@ public abstract class UserManagerInternal { public @interface OwnerType { } + // TODO(b/245963156): move to Display.java (and @hide) if we decide to support profiles on MUMD + /** + * Used only when starting a profile (on systems that + * {@link android.os.UserManager#isUsersOnSecondaryDisplaysSupported() support users running on + * secondary displays}), to indicate the profile should be started in the same display as its + * parent user. + */ + public static final int PARENT_DISPLAY = -2; + public interface UserRestrictionsListener { /** * Called when a user restriction changes. diff --git a/services/core/java/com/android/server/pm/UserManagerService.java b/services/core/java/com/android/server/pm/UserManagerService.java index bd1bd469d402..827b9863254b 100644 --- a/services/core/java/com/android/server/pm/UserManagerService.java +++ b/services/core/java/com/android/server/pm/UserManagerService.java @@ -6820,53 +6820,44 @@ public class UserManagerService extends IUserManager.Stub { int currentUserId = getCurrentUserId(); Preconditions.checkArgument(userId != currentUserId, - "Cannot assign current user to other displays"); - - boolean isProfile = isProfileUnchecked(userId); - - Preconditions.checkArgument(userId != currentUserId, - "Cannot assign current user to other displays"); - - Preconditions.checkArgument( - !isProfile || getProfileParentIdUnchecked(userId) != currentUserId, - "Cannot assign profile user %d to display %d when its parent is the current " - + "user (%d)", userId, displayId, currentUserId); + "Cannot assign current user (%d) to other displays", currentUserId); synchronized (mUsersOnSecondaryDisplays) { - if (DBG_MUMD) { - Slogf.d(LOG_TAG, "Adding %d->%d to mUsersOnSecondaryDisplays", - userId, displayId); - } - - if (isProfile) { + if (isProfileUnchecked(userId)) { // Profile can only start in the same display as parent + Preconditions.checkArgument(displayId == UserManagerInternal.PARENT_DISPLAY, + "Profile user can only be started in the same display as parent"); int parentUserId = getProfileParentId(userId); int parentDisplayId = mUsersOnSecondaryDisplays.get(parentUserId); - if (displayId != parentDisplayId) { - throw new IllegalStateException("Cannot assign profile " + userId + " to " - + "display " + displayId + " as its parent (user " + parentUserId - + ") is assigned to display " + parentDisplayId); + if (DBG_MUMD) { + Slogf.d(LOG_TAG, "Adding profile user %d -> display %d", userId, + parentDisplayId); } - } else { - // Check if display is available - for (int i = 0; i < mUsersOnSecondaryDisplays.size(); i++) { - // Make sure display is not used by other users... - // TODO(b/240736142); currently, if a user was started in a display, it - // would need to be stopped first, so "switching" a user on secondary - // diplay requires 2 non-atomic operations (stop and start). Once this logic - // is refactored, it should be atomic. - if (mUsersOnSecondaryDisplays.valueAt(i) == displayId) { - throw new IllegalStateException("Cannot assign " + userId + " to " - + "display " + displayId + " as it's already assigned to " - + "user " + mUsersOnSecondaryDisplays.keyAt(i)); - } - // TODO(b/239982558) also check that user is not already assigned to other - // display (including 0). That would be harder to tested under CTS though - // (for example, would need to add a new AM method to start user in bg on - // main display), so it's better to test on unit tests + mUsersOnSecondaryDisplays.put(userId, parentDisplayId); + return; + } + + // Check if display is available + for (int i = 0; i < mUsersOnSecondaryDisplays.size(); i++) { + // Make sure display is not used by other users... + // TODO(b/240736142); currently, if a user was started in a display, it + // would need to be stopped first, so "switching" a user on secondary + // diplay requires 2 non-atomic operations (stop and start). Once this logic + // is refactored, it should be atomic. + if (mUsersOnSecondaryDisplays.valueAt(i) == displayId) { + throw new IllegalStateException("Cannot assign " + userId + " to " + + "display " + displayId + " as it's already assigned to " + + "user " + mUsersOnSecondaryDisplays.keyAt(i)); } + // TODO(b/239982558) also check that user is not already assigned to other + // display (including 0). That would be harder to tested under CTS though + // (for example, would need to add a new AM method to start user in bg on + // main display), so it's better to test on unit tests } + if (DBG_MUMD) { + Slogf.d(LOG_TAG, "Adding full user %d -> display %d", userId, displayId); + } mUsersOnSecondaryDisplays.put(userId, displayId); } } diff --git a/services/tests/mockingservicestests/src/com/android/server/pm/UserManagerInternalTest.java b/services/tests/mockingservicestests/src/com/android/server/pm/UserManagerInternalTest.java index 86a5c90596ff..574bab2d9962 100644 --- a/services/tests/mockingservicestests/src/com/android/server/pm/UserManagerInternalTest.java +++ b/services/tests/mockingservicestests/src/com/android/server/pm/UserManagerInternalTest.java @@ -19,6 +19,8 @@ import static android.os.UserHandle.USER_SYSTEM; import static android.view.Display.DEFAULT_DISPLAY; import static android.view.Display.INVALID_DISPLAY; +import static com.android.server.pm.UserManagerInternal.PARENT_DISPLAY; + import static com.google.common.truth.Truth.assertWithMessage; import static org.junit.Assert.assertThrows; @@ -119,9 +121,6 @@ public final class UserManagerInternalTest extends UserManagerServiceOrInternalT () -> mUmi.assignUserToDisplay(PROFILE_USER_ID, SECONDARY_DISPLAY_ID)); Log.v(TAG, "Exception: " + e); - assertWithMessage("exception (%s) message", e).that(e).hasMessageThat() - .matches("Cannot.*" + PROFILE_USER_ID + ".*" + SECONDARY_DISPLAY_ID - + ".*parent.*current.*" + PARENT_USER_ID + ".*"); assertNoUserAssignedToDisplay(); } @@ -136,10 +135,6 @@ public final class UserManagerInternalTest extends UserManagerServiceOrInternalT () -> mUmi.assignUserToDisplay(PROFILE_USER_ID, SECONDARY_DISPLAY_ID)); Log.v(TAG, "Exception: " + e); - assertWithMessage("exception (%s) message", e).that(e).hasMessageThat() - .matches("Cannot.*" + PROFILE_USER_ID + ".*" + SECONDARY_DISPLAY_ID - + ".*parent.*current.*" + PARENT_USER_ID + ".*"); - assertNoUserAssignedToDisplay(); } @@ -173,7 +168,7 @@ public final class UserManagerInternalTest extends UserManagerServiceOrInternalT addDefaultProfileAndParent(); mUmi.assignUserToDisplay(PARENT_USER_ID, SECONDARY_DISPLAY_ID); - mUmi.assignUserToDisplay(PROFILE_USER_ID, SECONDARY_DISPLAY_ID); + mUmi.assignUserToDisplay(PROFILE_USER_ID, PARENT_DISPLAY); assertUsersAssignedToDisplays(PARENT_USER_ID, SECONDARY_DISPLAY_ID, pair(PROFILE_USER_ID, SECONDARY_DISPLAY_ID)); @@ -185,13 +180,10 @@ public final class UserManagerInternalTest extends UserManagerServiceOrInternalT addDefaultProfileAndParent(); mUmi.assignUserToDisplay(PARENT_USER_ID, SECONDARY_DISPLAY_ID); - IllegalStateException e = assertThrows(IllegalStateException.class, - () -> mUmi.assignUserToDisplay(PROFILE_USER_ID, OTHER_SECONDARY_DISPLAY_ID)); + IllegalArgumentException e = assertThrows(IllegalArgumentException.class, + () -> mUmi.assignUserToDisplay(PROFILE_USER_ID, SECONDARY_DISPLAY_ID)); Log.v(TAG, "Exception: " + e); - assertWithMessage("exception (%s) message", e).that(e).hasMessageThat() - .matches("Cannot.*" + PROFILE_USER_ID + ".*" + OTHER_SECONDARY_DISPLAY_ID - + ".*parent.*" + PARENT_USER_ID + ".*" + SECONDARY_DISPLAY_ID + ".*"); assertUserAssignedToDisplay(PARENT_USER_ID, SECONDARY_DISPLAY_ID); } diff --git a/services/tests/mockingservicestests/src/com/android/server/pm/UserManagerServiceOrInternalTestCase.java b/services/tests/mockingservicestests/src/com/android/server/pm/UserManagerServiceOrInternalTestCase.java index e2ffd839030f..42e9f86ef65e 100644 --- a/services/tests/mockingservicestests/src/com/android/server/pm/UserManagerServiceOrInternalTestCase.java +++ b/services/tests/mockingservicestests/src/com/android/server/pm/UserManagerServiceOrInternalTestCase.java @@ -101,11 +101,6 @@ abstract class UserManagerServiceOrInternalTestCase extends ExtendedMockitoTestC */ protected static final int OTHER_SECONDARY_DISPLAY_ID = 108; - /** - * Id of another secondary display (i.e, not {@link android.view.Display.DEFAULT_DISPLAY}). - */ - private static final int ANOTHER_SECONDARY_DISPLAY_ID = 108; - private final Object mPackagesLock = new Object(); private final Context mRealContext = androidx.test.InstrumentationRegistry.getInstrumentation() .getTargetContext(); @@ -422,10 +417,9 @@ abstract class UserManagerServiceOrInternalTestCase extends ExtendedMockitoTestC assignUserToDisplay(USER_ID, SECONDARY_DISPLAY_ID); assertWithMessage("isUserVisibleOnDisplay(%s, %s)", USER_ID, SECONDARY_DISPLAY_ID) - .that(isUserVisibleOnDisplay(USER_ID, ANOTHER_SECONDARY_DISPLAY_ID)).isFalse(); + .that(isUserVisibleOnDisplay(USER_ID, OTHER_SECONDARY_DISPLAY_ID)).isFalse(); } - // NOTE: we don't need to add tests for profiles (started / stopped profiles of bg user), as // isUserVisibleOnDisplay() for bg users relies only on the user / display assignments diff --git a/services/tests/servicestests/src/com/android/server/am/UserControllerTest.java b/services/tests/servicestests/src/com/android/server/am/UserControllerTest.java index db1209224bd5..fe079f423094 100644 --- a/services/tests/servicestests/src/com/android/server/am/UserControllerTest.java +++ b/services/tests/servicestests/src/com/android/server/am/UserControllerTest.java @@ -48,6 +48,7 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertThrows; import static org.junit.Assert.assertTrue; import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.Matchers.any; @@ -64,7 +65,6 @@ import static org.mockito.Mockito.times; import static org.mockito.Mockito.validateMockitoUsage; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; -import static org.testng.Assert.assertThrows; import android.annotation.Nullable; import android.annotation.UserIdInt; @@ -90,6 +90,7 @@ import android.os.UserManager; import android.os.storage.IStorageManager; import android.platform.test.annotations.Presubmit; import android.util.Log; +import android.view.Display; import androidx.test.filters.SmallTest; @@ -177,10 +178,11 @@ public class UserControllerTest { doNothing().when(mInjector).activityManagerOnUserStopped(anyInt()); doNothing().when(mInjector).clearBroadcastQueueForUser(anyInt()); doNothing().when(mInjector).taskSupervisorRemoveUser(anyInt()); + mockIsUsersOnSecondaryDisplaysEnabled(false); // All UserController params are set to default. mUserController = new UserController(mInjector); setUpUser(TEST_USER_ID, NO_USERINFO_FLAGS); - setUpUser(TEST_PRE_CREATED_USER_ID, NO_USERINFO_FLAGS, /* preCreated=*/ true, null); + setUpUser(TEST_PRE_CREATED_USER_ID, NO_USERINFO_FLAGS, /* preCreated= */ true, null); }); } @@ -199,11 +201,37 @@ public class UserControllerTest { verify(mInjector.getWindowManager()).setSwitchingUser(true); verify(mInjector).clearAllLockedTasks(anyString()); startForegroundUserAssertions(); + verifyUserAssignedToDisplay(TEST_USER_ID, Display.DEFAULT_DISPLAY); } @Test public void testStartUser_background() { - mUserController.startUser(TEST_USER_ID, false /* foreground */); + boolean started = mUserController.startUser(TEST_USER_ID, /* foreground= */ false); + assertWithMessage("startUser(%s, foreground=false)", TEST_USER_ID).that(started).isTrue(); + verify(mInjector.getWindowManager(), never()).startFreezingScreen(anyInt(), anyInt()); + verify(mInjector.getWindowManager(), never()).setSwitchingUser(anyBoolean()); + verify(mInjector, never()).clearAllLockedTasks(anyString()); + startBackgroundUserAssertions(); + verifyUserAssignedToDisplay(TEST_USER_ID, Display.DEFAULT_DISPLAY); + } + + @Test + public void testStartUserOnSecondaryDisplay_defaultDisplay() { + assertThrows(IllegalArgumentException.class, () -> mUserController + .startUserOnSecondaryDisplay(TEST_USER_ID, Display.DEFAULT_DISPLAY)); + + verifyUserNeverAssignedToDisplay(); + } + + @Test + public void testStartUserOnSecondaryDisplay() { + boolean started = mUserController.startUserOnSecondaryDisplay(TEST_USER_ID, 42); + + assertWithMessage("startUserOnSecondaryDisplay(%s, %s)", TEST_USER_ID, 42).that(started) + .isTrue(); + verifyUserAssignedToDisplay(TEST_USER_ID, 42); + + // TODO(b/239982558): might need to change assertions verify(mInjector.getWindowManager(), never()).startFreezingScreen(anyInt(), anyInt()); verify(mInjector.getWindowManager(), never()).setSwitchingUser(anyBoolean()); verify(mInjector, never()).clearAllLockedTasks(anyString()); @@ -611,6 +639,7 @@ public class UserControllerTest { setUpAndStartUserInBackground(TEST_USER_ID1); assertThrows(IllegalArgumentException.class, () -> mUserController.stopProfile(TEST_USER_ID1)); + verifyUserUnassignedFromDisplayNeverCalled(TEST_USER_ID); } @Test @@ -623,13 +652,26 @@ public class UserControllerTest { @Test public void testStartProfile() throws Exception { setUpAndStartProfileInBackground(TEST_USER_ID1); + + startBackgroundUserAssertions(); + verifyUserAssignedToDisplay(TEST_USER_ID1, Display.DEFAULT_DISPLAY); + } + + @Test + public void testStartProfile_whenUsersOnSecondaryDisplaysIsEnabled() throws Exception { + mockIsUsersOnSecondaryDisplaysEnabled(true); + + setUpAndStartProfileInBackground(TEST_USER_ID1); + startBackgroundUserAssertions(); + verifyUserAssignedToDisplay(TEST_USER_ID1, UserManagerInternal.PARENT_DISPLAY); } @Test public void testStopProfile() throws Exception { setUpAndStartProfileInBackground(TEST_USER_ID1); assertProfileLockedOrUnlockedAfterStopping(TEST_USER_ID1, /* expectLocking= */ true); + verifyUserUnassignedFromDisplay(TEST_USER_ID1); } /** Tests handleIncomingUser() for a variety of permissions and situations. */ @@ -680,7 +722,7 @@ public class UserControllerTest { eq(INTERACT_ACROSS_PROFILES), anyInt(), anyInt(), any())).thenReturn(false); checkHandleIncomingUser(user1a.id, user2.id, ALLOW_NON_FULL, true); - checkHandleIncomingUser(user1a.id, user2.id, ALLOW_NON_FULL_IN_PROFILE, false); + checkHandleIncomingUser(user1a.id, user2.id, ALLOW_NON_FULL_IN_PROFILE, false); checkHandleIncomingUser(user1a.id, user2.id, ALLOW_FULL_ONLY, false); checkHandleIncomingUser(user1a.id, user2.id, ALLOW_PROFILES_OR_NON_FULL, true); @@ -876,6 +918,26 @@ public class UserControllerTest { } } + private void mockIsUsersOnSecondaryDisplaysEnabled(boolean value) { + when(mInjector.isUsersOnSecondaryDisplaysEnabled()).thenReturn(value); + } + + private void verifyUserAssignedToDisplay(@UserIdInt int userId, int displayId) { + verify(mInjector.getUserManagerInternal()).assignUserToDisplay(userId, displayId); + } + + private void verifyUserNeverAssignedToDisplay() { + verify(mInjector.getUserManagerInternal(), never()).assignUserToDisplay(anyInt(), anyInt()); + } + + private void verifyUserUnassignedFromDisplay(@UserIdInt int userId) { + verify(mInjector.getUserManagerInternal()).unassignUserFromDisplay(userId); + } + + private void verifyUserUnassignedFromDisplayNeverCalled(@UserIdInt int userId) { + verify(mInjector.getUserManagerInternal(), never()).unassignUserFromDisplay(userId); + } + // Should be public to allow mocking private static class TestInjector extends UserController.Injector { public final TestHandler mHandler; |