Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 3c1f216e authored by Adam Bookatz's avatar Adam Bookatz
Browse files

Properly handle stopping related users 

Fixes UserController's mechanism for stopping users along with their
other profile group members, and improves what happens when exceeding
the maximum number of running users.

* The UsersLru will now always list profiles *before* their parent
  * This makes sense since parents must be "used" for the profile to run
  * This ensures that if we exceed maxRunningUsers, we'll stop profiles
    before their parents (i.e. priotize keeping full users alive)
  * This ensures that if we exceed maxRunningUsers, we'll count the
    number of users we plan on stopping correctly (i.e. no overkill)
* stopUsersLU, which stops a user as well as its "related users",
  formerly stopped a parent when called on its profile. This didn't make
  sense since stopping a profile shouldn't stop its parent. We change it
  to only stop a parent's profiles, not a profile's parent.
  This fixes the following bugs:
  * when exceeding maxRunningUsers, a user with profiles was much more
    likely to be killed, even if it was the main user
  * the stopProfile API would attempt to kill its parent too (!)
  * applying quiet mode to a profile of a background user would attempt
    to kill that background parent user too
* Adds more documentation explaining a bunch of this

Issues NOT tackled in this cl are:
* The system user's profiles are still not stopped when maxRunningUsers
  is exceeded
* When exceeding maxRunningUsers, we still don't prioritize keeping
  difficult-to-restart profiles (with separate challenges) alive
* The entire notion of "force==true" is left alone, even though it
  doesn't make much sense

Test: atest UserControllerTest#testRunningUsersListOrder_parentAfterProfile,testRunningUsersListOrder_currentAtEnd,testStopProfile_doesNotStopItsParent
Test: atest UserControllerTest

Bug: 324647580
Bug: 310249114
Change-Id: Ie08342bb77544117cab4423b543af74cbeb109e2
parent a0d8e5fb

services/core/java/com/android/server/am/UserController.java

+71 −28
Original line number Diff line number Diff line
@@ -276,7 +276,15 @@ class UserController implements Handler.Callback { 
    private final SparseArray<UserState> mStartedUsers = new SparseArray<>();



    /**

     * LRU list of history of current users.  Most recently current is at the end.

     * LRU list of history of running users, in order of when we last needed to start them.

     *

     * Switching to a user will move it towards the end. Attempting to start a user/profile (even

     * if it was already running) will move it towards the end.

     *

     * <p>Guarantees (by the end of startUser):

     * <li>The current user will always be at the end, even if background users were started

     * subsequently.

     * <li>Parents always come later than (but not necessarily adjacent to) their profiles.

     */

    @GuardedBy("mLock")

    private final ArrayList<Integer> mUserLru = new ArrayList<>();
@@ -299,6 +307,9 @@ class UserController implements Handler.Callback { 
    /**

     * Mapping from each known user ID to the profile group ID it is associated with.

     * <p>Users not present in this array have a profile group of NO_PROFILE_GROUP_ID.

     *

     * <p>For better or worse, this class sometimes assumes that the profileGroupId of a parent user

     * is always identical with its userId. If that ever becomes false, this class needs updating.

     */

    @GuardedBy("mLock")

    private final SparseIntArray mUserProfileGroupIds = new SparseIntArray();
@@ -499,6 +510,23 @@ class UserController implements Handler.Callback { 
        });

    }



    /** Adds a user to mUserLru, moving it to the end of the list if it was already present. */

    private void addUserToUserLru(@UserIdInt int userId) {

        synchronized (mLock) {

            final Integer userIdObj = userId;

            mUserLru.remove(userIdObj);

            mUserLru.add(userIdObj);



            // Now also move the user's parent to the end (if applicable).

            Integer parentIdObj = mUserProfileGroupIds.get(userId, UserInfo.NO_PROFILE_GROUP_ID);

            if (parentIdObj != UserInfo.NO_PROFILE_GROUP_ID && !parentIdObj.equals(userIdObj)

                    && mUserLru.remove(parentIdObj)) {

                mUserLru.add(parentIdObj);

            }

        }

    }



    /** Returns a list of running users, in order of when they were started (oldest first). */

    @GuardedBy("mLock")

    @VisibleForTesting

    List<Integer> getRunningUsersLU() {
@@ -536,9 +564,9 @@ class UserController implements Handler.Callback { 


    @GuardedBy("mLock")

    private void stopExcessRunningUsersLU(int maxRunningUsers, ArraySet<Integer> exemptedUsers) {

        List<Integer> currentlyRunning = getRunningUsersLU();

        Iterator<Integer> iterator = currentlyRunning.iterator();

        while (currentlyRunning.size() > maxRunningUsers && iterator.hasNext()) {

        List<Integer> currentlyRunningLru = getRunningUsersLU();

        Iterator<Integer> iterator = currentlyRunningLru.iterator();

        while (currentlyRunningLru.size() > maxRunningUsers && iterator.hasNext()) {

            Integer userId = iterator.next();

            if (userId == UserHandle.USER_SYSTEM

                    || userId == mCurrentUserId
@@ -551,6 +579,10 @@ class UserController implements Handler.Callback { 
            if (stopUsersLU(userId, /* force= */ false, /* allowDelayedLocking= */ true,

                    /* stopUserCallback= */ null, /* keyEvictedCallback= */ null)

                    == USER_OP_SUCCESS) {

                // Technically, stopUsersLU can remove more than one user when stopping a parent.

                // But mUserLru is designed so that profiles always precede their parent, so this

                // normally won't happen here, and therefore won't cause underestimating the number

                // removed.

                iterator.remove();

            }

        }
@@ -947,7 +979,7 @@ class UserController implements Handler.Callback { 
    }



    /**

     * Stops the user along with its related users. The method calls

     * Stops the user along with its profiles. The method calls

     * {@link #getUsersToStopLU(int)} to determine the list of users that should be stopped.

     */

    @GuardedBy("mLock")
@@ -992,7 +1024,12 @@ class UserController implements Handler.Callback { 
    }



    /**

     * Stops a single User. This can also trigger locking user data out depending on device's

     * Stops a single User.

     *

     * This should only ever be called by {@link #stopUsersLU},

     * which is responsible to making sure any associated users are appropriately stopped too.

     *

     * This can also trigger locking user data out depending on device's

     * config ({@code mDelayUserDataLocking}) and arguments.

     *

     * In the default configuration for most device and users, users will be locked when stopping.
@@ -1425,7 +1462,8 @@ class UserController implements Handler.Callback { 


    /**

     * Determines the list of users that should be stopped together with the specified

     * {@code userId}. The returned list includes {@code userId}.

     * {@code userId}, i.e. the user and its profiles (if the given user is a parent).

     * The returned list includes {@code userId}.

     */

    @GuardedBy("mLock")

    private @NonNull int[] getUsersToStopLU(@UserIdInt int userId) {
@@ -1433,6 +1471,8 @@ class UserController implements Handler.Callback { 
        IntArray userIds = new IntArray();

        userIds.add(userId);

        int userGroupId = mUserProfileGroupIds.get(userId, UserInfo.NO_PROFILE_GROUP_ID);

        if (userGroupId == userId) {

            // The user is the parent of the profile group. Stop its profiles too.

            for (int i = 0; i < startedUsersSize; i++) {

                UserState uss = mStartedUsers.valueAt(i);

                int startedUserId = uss.mHandle.getIdentifier();
@@ -1448,6 +1488,7 @@ class UserController implements Handler.Callback { 
                }

                userIds.add(startedUserId);

            }

        }

        return userIds.toArray();

    }
@@ -1519,6 +1560,7 @@ class UserController implements Handler.Callback { 
        });

    }



    /** Starts all applicable profiles of the current user. */

    private void startProfiles() {

        int currentUserId = getCurrentUserId();

        if (DEBUG_MU) Slogf.i(TAG, "startProfilesLocked");
@@ -1711,6 +1753,7 @@ class UserController implements Handler.Callback { 
            t.traceBegin("getStartedUserState");

            final int oldUserId = getCurrentUserId();

            if (oldUserId == userId) {

                // The user we're requested to start is already the current user.

                final UserState state = getStartedUserState(userId);

                if (state == null) {

                    Slogf.wtf(TAG, "Current user has no UserState");
@@ -1793,10 +1836,12 @@ class UserController implements Handler.Callback { 
                    t.traceEnd(); // updateStartedUserArrayStarting

                    return true;

                }

                final Integer userIdInt = userId;

                mUserLru.remove(userIdInt);

                mUserLru.add(userIdInt);

            }



            // No matter what, the fact that we're requested to start the user (even if it is

            // already running) puts it towards the end of the mUserLru list.

            addUserToUserLru(userId);



            if (unlockListener != null) {

                uss.mUnlockProgress.addListener(unlockListener);

            }
@@ -1835,12 +1880,10 @@ class UserController implements Handler.Callback { 
                }



            } else {

                final Integer currentUserIdInt = mCurrentUserId;

                updateProfileRelatedCaches();

                synchronized (mLock) {

                    mUserLru.remove(currentUserIdInt);

                    mUserLru.add(currentUserIdInt);

                }

                // We are starting a non-foreground user. They have already been added to the end

                // of mUserLru, so we need to ensure that the foreground user isn't displaced.

                addUserToUserLru(mCurrentUserId);

            }

            t.traceEnd();

services/tests/servicestests/src/com/android/server/am/UserControllerTest.java

+169 −15
Original line number Diff line number Diff line
@@ -111,6 +111,8 @@ import com.android.server.pm.UserTypeFactory; 
import com.android.server.wm.ActivityTaskManagerInternal;

import com.android.server.wm.WindowManagerService;



import com.google.common.collect.Range;



import org.junit.After;

import org.junit.Before;

import org.junit.Rule;
@@ -155,6 +157,7 @@ public class UserControllerTest { 
    private UserController mUserController;

    private TestInjector mInjector;

    private final HashMap<Integer, UserState> mUserStates = new HashMap<>();

    private final HashMap<Integer, UserInfo> mUserInfos = new HashMap<>();



    private final KeyEvictedCallback mKeyEvictedCallback = (userId) -> { /* ignore */ };
@@ -587,25 +590,25 @@ public class UserControllerTest { 


        setUpUser(TEST_USER_ID1, 0);

        setUpUser(TEST_USER_ID2, 0);

        int numerOfUserSwitches = 1;

        int numberOfUserSwitches = 1;

        addForegroundUserAndContinueUserSwitch(TEST_USER_ID, UserHandle.USER_SYSTEM,

                numerOfUserSwitches, false);

                numberOfUserSwitches, false);

        // running: user 0, USER_ID

        assertTrue(mUserController.canStartMoreUsers());

        assertEquals(Arrays.asList(new Integer[] {0, TEST_USER_ID}),

                mUserController.getRunningUsersLU());



        numerOfUserSwitches++;

        numberOfUserSwitches++;

        addForegroundUserAndContinueUserSwitch(TEST_USER_ID1, TEST_USER_ID,

                numerOfUserSwitches, false);

                numberOfUserSwitches, false);

        // running: user 0, USER_ID, USER_ID1

        assertFalse(mUserController.canStartMoreUsers());

        assertEquals(Arrays.asList(new Integer[] {0, TEST_USER_ID, TEST_USER_ID1}),

                mUserController.getRunningUsersLU());



        numerOfUserSwitches++;

        numberOfUserSwitches++;

        addForegroundUserAndContinueUserSwitch(TEST_USER_ID2, TEST_USER_ID1,

                numerOfUserSwitches, false);

                numberOfUserSwitches, false);

        UserState ussUser2 = mUserStates.get(TEST_USER_ID2);

        // skip middle step and call this directly.

        mUserController.finishUserSwitch(ussUser2);
@@ -631,20 +634,20 @@ public class UserControllerTest { 


        setUpUser(TEST_USER_ID1, 0);

        setUpUser(TEST_USER_ID2, 0);

        int numerOfUserSwitches = 1;

        int numberOfUserSwitches = 1;

        addForegroundUserAndContinueUserSwitch(TEST_USER_ID, UserHandle.USER_SYSTEM,

                numerOfUserSwitches, false);

                numberOfUserSwitches, false);

        // running: user 0, USER_ID

        assertTrue(mUserController.canStartMoreUsers());

        assertEquals(Arrays.asList(new Integer[] {0, TEST_USER_ID}),

                mUserController.getRunningUsersLU());

        numerOfUserSwitches++;

        numberOfUserSwitches++;



        addForegroundUserAndContinueUserSwitch(TEST_USER_ID1, TEST_USER_ID,

                numerOfUserSwitches, true);

                numberOfUserSwitches, true);

        // running: user 0, USER_ID1

        // stopped + unlocked: USER_ID

        numerOfUserSwitches++;

        numberOfUserSwitches++;

        assertTrue(mUserController.canStartMoreUsers());

        assertEquals(Arrays.asList(new Integer[] {0, TEST_USER_ID1}),

                mUserController.getRunningUsersLU());
@@ -659,7 +662,7 @@ public class UserControllerTest { 
                .lockCeStorage(anyInt());



        addForegroundUserAndContinueUserSwitch(TEST_USER_ID2, TEST_USER_ID1,

                numerOfUserSwitches, true);

                numberOfUserSwitches, true);

        // running: user 0, USER_ID2

        // stopped + unlocked: USER_ID1

        // stopped + locked: USER_ID
@@ -674,6 +677,105 @@ public class UserControllerTest { 
                .lockCeStorage(TEST_USER_ID);

    }



    /**

     * Test that, in getRunningUsersLU, parents come after their profile, even if the profile was

     * started afterwards.

     */

    @Test

    public void testRunningUsersListOrder_parentAfterProfile() {

        mUserController.setInitialConfig(/* userSwitchUiEnabled= */ true,

                /* maxRunningUsers= */ 7, /* delayUserDataLocking= */ false);



        final int PARENT_ID = 200;

        final int PROFILE1_ID = 201;

        final int PROFILE2_ID = 202;

        final int FG_USER_ID = 300;

        final int BG_USER_ID = 400;



        setUpUser(PARENT_ID, 0).profileGroupId = PARENT_ID;

        setUpUser(PROFILE1_ID, UserInfo.FLAG_PROFILE).profileGroupId = PARENT_ID;

        setUpUser(PROFILE2_ID, UserInfo.FLAG_PROFILE).profileGroupId = PARENT_ID;

        setUpUser(FG_USER_ID, 0).profileGroupId = FG_USER_ID;

        setUpUser(BG_USER_ID, 0).profileGroupId = UserInfo.NO_PROFILE_GROUP_ID;

        mUserController.onSystemReady(); // To set the profileGroupIds in UserController.



        assertEquals(Arrays.asList(

                new Integer[] {SYSTEM_USER_ID}),

                mUserController.getRunningUsersLU());



        int numberOfUserSwitches = 1;

        addForegroundUserAndContinueUserSwitch(PARENT_ID, UserHandle.USER_SYSTEM,

                numberOfUserSwitches, false);

        assertEquals(Arrays.asList(

                new Integer[] {SYSTEM_USER_ID, PARENT_ID}),

                mUserController.getRunningUsersLU());



        assertThat(mUserController.startProfile(PROFILE1_ID, true, null)).isTrue();

        assertEquals(Arrays.asList(

                new Integer[] {SYSTEM_USER_ID, PROFILE1_ID, PARENT_ID}),

                mUserController.getRunningUsersLU());



        numberOfUserSwitches++;

        addForegroundUserAndContinueUserSwitch(FG_USER_ID, PARENT_ID,

                numberOfUserSwitches, false);

        assertEquals(Arrays.asList(

                new Integer[] {SYSTEM_USER_ID, PROFILE1_ID, PARENT_ID, FG_USER_ID}),

                mUserController.getRunningUsersLU());



        mUserController.startUser(BG_USER_ID, USER_START_MODE_BACKGROUND);

        assertEquals(Arrays.asList(

                new Integer[] {SYSTEM_USER_ID, PROFILE1_ID, PARENT_ID, BG_USER_ID, FG_USER_ID}),

                mUserController.getRunningUsersLU());



        assertThat(mUserController.startProfile(PROFILE2_ID, true, null)).isTrue();

        // Note for the future:

        // It is not absolutely essential that PROFILE1 come before PROFILE2,

        // nor that PROFILE1 come before BG_USER. We can change that policy later if we'd like.

        // The important thing is that PROFILE1 and PROFILE2 precede PARENT,

        // and that everything precedes OTHER.

        assertEquals(Arrays.asList(new Integer[] {

                SYSTEM_USER_ID, PROFILE1_ID, BG_USER_ID, PROFILE2_ID, PARENT_ID, FG_USER_ID}),

                mUserController.getRunningUsersLU());

    }



    /**

     * Test that, in getRunningUsersLU, the current user is always at the end, even if background

     * users were started subsequently.

     */

    @Test

    public void testRunningUsersListOrder_currentAtEnd() {

        mUserController.setInitialConfig(/* userSwitchUiEnabled= */ true,

                /* maxRunningUsers= */ 7, /* delayUserDataLocking= */ false);



        final int CURRENT_ID = 200;

        final int PROFILE_ID = 201;

        final int BG_USER_ID = 400;



        setUpUser(CURRENT_ID, 0).profileGroupId = CURRENT_ID;

        setUpUser(PROFILE_ID, UserInfo.FLAG_PROFILE).profileGroupId = CURRENT_ID;

        setUpUser(BG_USER_ID, 0).profileGroupId = BG_USER_ID;

        mUserController.onSystemReady(); // To set the profileGroupIds in UserController.



        assertEquals(Arrays.asList(

                new Integer[] {SYSTEM_USER_ID}),

                mUserController.getRunningUsersLU());



        addForegroundUserAndContinueUserSwitch(CURRENT_ID, UserHandle.USER_SYSTEM, 1, false);

        assertEquals(Arrays.asList(

                new Integer[] {SYSTEM_USER_ID, CURRENT_ID}),

                mUserController.getRunningUsersLU());



        mUserController.startUser(BG_USER_ID, USER_START_MODE_BACKGROUND);

        assertEquals(Arrays.asList(

                new Integer[] {SYSTEM_USER_ID, BG_USER_ID, CURRENT_ID}),

                mUserController.getRunningUsersLU());



        assertThat(mUserController.startProfile(PROFILE_ID, true, null)).isTrue();

        assertEquals(Arrays.asList(

                new Integer[] {SYSTEM_USER_ID, BG_USER_ID, PROFILE_ID, CURRENT_ID}),

                mUserController.getRunningUsersLU());

    }



    /**

     * Test locking user with mDelayUserDataLocking false.

     */
@@ -785,6 +887,52 @@ public class UserControllerTest { 
        verifyUserUnassignedFromDisplayNeverCalled(TEST_USER_ID);

    }



    /** Test that stopping a profile doesn't also stop its parent, even if it's in background. */

    @Test

    public void testStopProfile_doesNotStopItsParent() throws Exception {

        mUserController.setInitialConfig(/* userSwitchUiEnabled= */ true,

                /* maxRunningUsers= */ 5, /* delayUserDataLocking= */ false);



        final Range<Integer> RUNNING_RANGE =

                Range.closed(UserState.STATE_BOOTING, UserState.STATE_RUNNING_UNLOCKED);



        final int PARENT_ID = TEST_USER_ID1;

        final int PROFILE_ID = TEST_USER_ID2;

        final int OTHER_ID = TEST_USER_ID3;



        setUpUser(PARENT_ID, 0).profileGroupId = PARENT_ID;

        setUpUser(PROFILE_ID, UserInfo.FLAG_PROFILE).profileGroupId = PARENT_ID;

        setUpUser(OTHER_ID, 0).profileGroupId = OTHER_ID;

        mUserController.onSystemReady(); // To set the profileGroupIds in UserController.



        // Start the parent in the background

        boolean started = mUserController.startUser(PARENT_ID, USER_START_MODE_BACKGROUND);

        assertWithMessage("startUser(%s)", PARENT_ID).that(started).isTrue();

        assertThat(mUserController.getStartedUserState(PARENT_ID).state).isIn(RUNNING_RANGE);



        // Start the profile

        started = mUserController.startProfile(PROFILE_ID, true, null);

        assertWithMessage("startProfile(%s)", PROFILE_ID).that(started).isTrue();

        assertThat(mUserController.getStartedUserState(PARENT_ID).state).isIn(RUNNING_RANGE);

        assertThat(mUserController.getStartedUserState(PROFILE_ID).state).isIn(RUNNING_RANGE);



        // Start an unrelated user

        started = mUserController.startUser(OTHER_ID, USER_START_MODE_FOREGROUND);

        assertWithMessage("startUser(%s)", OTHER_ID).that(started).isTrue();

        assertThat(mUserController.getStartedUserState(PARENT_ID).state).isIn(RUNNING_RANGE);

        assertThat(mUserController.getStartedUserState(PROFILE_ID).state).isIn(RUNNING_RANGE);

        assertThat(mUserController.getStartedUserState(OTHER_ID).state).isIn(RUNNING_RANGE);



        // Stop the profile and assert that its (background) parent didn't stop too

        boolean stopped = mUserController.stopProfile(PROFILE_ID);

        assertWithMessage("stopProfile(%s)", PROFILE_ID).that(stopped).isTrue();

        if (mUserController.getStartedUserState(PROFILE_ID) != null) {

            assertThat(mUserController.getStartedUserState(PROFILE_ID).state)

                    .isNotIn(RUNNING_RANGE);

        }

        assertThat(mUserController.getStartedUserState(PARENT_ID).state).isIn(RUNNING_RANGE);

    }



    @Test

    public void testStartProfile_disabledProfileFails() {

        setUpUser(TEST_USER_ID1, UserInfo.FLAG_PROFILE | UserInfo.FLAG_DISABLED, /* preCreated= */
@@ -1152,11 +1300,11 @@ public class UserControllerTest { 
        continueUserSwitchAssertions(oldUserId, newUserId, expectOldUserStopping);

    }



    private void setUpUser(@UserIdInt int userId, @UserInfoFlag int flags) {

        setUpUser(userId, flags, /* preCreated= */ false, /* userType */ null);

    private UserInfo setUpUser(@UserIdInt int userId, @UserInfoFlag int flags) {

        return setUpUser(userId, flags, /* preCreated= */ false, /* userType */ null);

    }



    private void setUpUser(@UserIdInt int userId, @UserInfoFlag int flags, boolean preCreated,

    private UserInfo setUpUser(@UserIdInt int userId, @UserInfoFlag int flags, boolean preCreated,

            @Nullable String userType) {

        if (userType == null) {

            userType = UserInfo.getDefaultUserType(flags);
@@ -1171,6 +1319,12 @@ public class UserControllerTest { 
        assertThat(userTypeDetails).isNotNull();

        when(mInjector.mUserManagerInternalMock.getUserProperties(eq(userId)))

                .thenReturn(userTypeDetails.getDefaultUserPropertiesReference());



        mUserInfos.put(userId, userInfo);

        when(mInjector.mUserManagerMock.getUsers(anyBoolean()))

                .thenReturn(mUserInfos.values().stream().toList());



        return userInfo;

    }



    private static List<String> getActions(List<Intent> intents) {