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

Commit 6d0e74bf authored by Adam Bookatz's avatar Adam Bookatz
Browse files

Minimize the stopUser force parameter 

The "force" parameter in stopUser is confusing and misleading. It sounds
like "force stopping" a user is a powerful, generic action. In reality,
this parameter is extremely narrow in its meaning: when false, it means
that a profile won't be stopped if it is a profile of the current or
system user. Since, hitherto, profiles always belong to the system user,
this effectively means that
stopUser(force==false) just means "stop the user but only if it isn't a
profile user", which is basically never desired (since then the caller
shouldn't be calling the method in the first place).
For full users (e.g. when called as part of a user switch), it is
completely irrelevant.

This confusion has propagated, since callers seem to think it means
something more. Moreover, due to HSUM, going forward, profiles may
belong to other users, in which case the effective action of this
parameter will change, so clarity is important.

Here, we try to clarify and wean usage off the "force" parameter. It is almost
always set to true, except in the very rare instances where false is actually
desired (when stopping a user due to there being too many running users,
and when stopping a background user due to a change in the corresponding
user restriction).

Unfortunately, we cannot simply get rid of the force-parameter version
of AMS.stopUser, but we can at least make it clear that it's not normal
to use.

Test: atest UserControllerTest

Test: CarSystemUI-tests:com.android.systemui.car.qc.QCLogoutButtonControllerTest
Test: CtsCarBuiltinApiTestCases
Test: CarServiceUnitTest

Test: atest SystemUIGoogleTests:com.google.android.systemui.communal.dock.callbacks.UserSwitcherTest

Bug: 324647580
Change-Id: I28f30d5ac68330850691141be93bcad05b4c3210
parent fb810e65

apct-tests/perftests/multiuser/src/android/multiuser/UserLifecycleTests.java

+19 −19
Original line number Diff line number Diff line
@@ -366,7 +366,7 @@ public class UserLifecycleTests { 
            mRunner.pauseTiming();

            Log.i(TAG, "Stopping timer");



            stopUser(userId, /* force */true);

            stopUser(userId);

            mRunner.resumeTimingForNextIteration();

        }
@@ -429,7 +429,7 @@ public class UserLifecycleTests { 


            mRunner.pauseTiming();

            Log.i(TAG, "Stopping timer");

            stopUser(userId, /* force */true);

            stopUser(userId);

            mRunner.resumeTimingForNextIteration();

        }
@@ -545,7 +545,7 @@ public class UserLifecycleTests { 
            mRunner.pauseTiming();

            Log.d(TAG, "Stopping timer");

            switchUserNoCheck(currentUserId);

            stopUserAfterWaitingForBroadcastIdle(userId, /* force */true);

            stopUserAfterWaitingForBroadcastIdle(userId);

            attestFalse("Failed to stop user " + userId, mAm.isUserRunning(userId));

            mRunner.resumeTimingForNextIteration();

        }
@@ -571,7 +571,7 @@ public class UserLifecycleTests { 
            mRunner.pauseTiming();

            Log.d(TAG, "Stopping timer");

            switchUserNoCheck(startUser);

            stopUserAfterWaitingForBroadcastIdle(testUser, true);

            stopUserAfterWaitingForBroadcastIdle(testUser);

            attestFalse("Failed to stop user " + testUser, mAm.isUserRunning(testUser));

            mRunner.resumeTimingForNextIteration();

        }
@@ -660,7 +660,7 @@ public class UserLifecycleTests { 
            mRunner.resumeTiming();

            Log.i(TAG, "Starting timer");



            stopUser(userId, false);

            stopUser(userId);



            mRunner.pauseTiming();

            Log.i(TAG, "Stopping timer");
@@ -685,7 +685,7 @@ public class UserLifecycleTests { 
            Log.d(TAG, "Starting timer");

            mRunner.resumeTiming();



            stopUser(userId, false);

            stopUser(userId);



            mRunner.pauseTiming();

            Log.d(TAG, "Stopping timer");
@@ -883,7 +883,7 @@ public class UserLifecycleTests { 
            final int userId = createManagedProfile();

            // Start the profile initially, then stop it. Similar to setQuietModeEnabled.

            startUserInBackgroundAndWaitForUnlock(userId);

            stopUserAfterWaitingForBroadcastIdle(userId, true);

            stopUserAfterWaitingForBroadcastIdle(userId);

            mRunner.resumeTiming();

            Log.i(TAG, "Starting timer");
@@ -905,7 +905,7 @@ public class UserLifecycleTests { 
        startUserInBackgroundAndWaitForUnlock(userId);

        while (mRunner.keepRunning()) {

            mRunner.pauseTiming();

            stopUserAfterWaitingForBroadcastIdle(userId, true);

            stopUserAfterWaitingForBroadcastIdle(userId);

            mRunner.resumeTiming();

            Log.d(TAG, "Starting timer");
@@ -987,7 +987,7 @@ public class UserLifecycleTests { 
            installPreexistingApp(userId, DUMMY_PACKAGE_NAME);

            startUserInBackgroundAndWaitForUnlock(userId);

            startApp(userId, DUMMY_PACKAGE_NAME);

            stopUserAfterWaitingForBroadcastIdle(userId, true);

            stopUserAfterWaitingForBroadcastIdle(userId);

            SystemClock.sleep(1_000); // 1 second cool-down before re-starting profile.

            mRunner.resumeTiming();

            Log.i(TAG, "Starting timer");
@@ -1019,7 +1019,7 @@ public class UserLifecycleTests { 
            installPreexistingApp(userId, DUMMY_PACKAGE_NAME);

            startUserInBackgroundAndWaitForUnlock(userId);

            startApp(userId, DUMMY_PACKAGE_NAME);

            stopUserAfterWaitingForBroadcastIdle(userId, true);

            stopUserAfterWaitingForBroadcastIdle(userId);

            SystemClock.sleep(1_000); // 1 second cool-down before re-starting profile.

            mRunner.resumeTiming();

            Log.d(TAG, "Starting timer");
@@ -1144,7 +1144,7 @@ public class UserLifecycleTests { 
            mRunner.resumeTiming();

            Log.i(TAG, "Starting timer");



            stopUser(userId, true);

            stopUser(userId);



            mRunner.pauseTiming();

            Log.i(TAG, "Stopping timer");
@@ -1168,7 +1168,7 @@ public class UserLifecycleTests { 
            mRunner.resumeTiming();

            Log.d(TAG, "Starting timer");



            stopUser(userId, true);

            stopUser(userId);



            mRunner.pauseTiming();

            Log.d(TAG, "Stopping timer");
@@ -1294,15 +1294,15 @@ public class UserLifecycleTests { 
     * Do not call this method while timing is on. i.e. between mRunner.resumeTiming() and

     * mRunner.pauseTiming(). Otherwise it would cause the test results to be spiky.

     */

    private void stopUserAfterWaitingForBroadcastIdle(int userId, boolean force)

    private void stopUserAfterWaitingForBroadcastIdle(int userId)

            throws RemoteException {

        waitForBroadcastIdle();

        stopUser(userId, force);

        stopUser(userId);

    }



    private void stopUser(int userId, boolean force) throws RemoteException {

    private void stopUser(int userId) throws RemoteException {

        final CountDownLatch latch = new CountDownLatch(1);

        mIam.stopUser(userId, force /* force */, new IStopUserCallback.Stub() {

        mIam.stopUserWithCallback(userId, new IStopUserCallback.Stub() {

            @Override

            public void userStopped(int userId) throws RemoteException {

                latch.countDown();
@@ -1352,7 +1352,7 @@ public class UserLifecycleTests { 
        attestTrue("Didn't switch back to user, " + origUser, origUser == mAm.getCurrentUser());



        if (stopNewUser) {

            stopUserAfterWaitingForBroadcastIdle(testUser, true);

            stopUserAfterWaitingForBroadcastIdle(testUser);

            attestFalse("Failed to stop user " + testUser, mAm.isUserRunning(testUser));

        }
@@ -1471,7 +1471,7 @@ public class UserLifecycleTests { 
    }



    private void removeUser(int userId) throws RemoteException {

        stopUserAfterWaitingForBroadcastIdle(userId, true);

        stopUserAfterWaitingForBroadcastIdle(userId);

        try {

            ShellHelper.runShellCommandWithTimeout("pm remove-user -w " + userId,

                    TIMEOUT_IN_SECOND);
@@ -1512,7 +1512,7 @@ public class UserLifecycleTests { 


            final boolean preStartComplete = mIam.startUserInBackgroundWithListener(userId,

                    preWaiter) && preWaiter.waitForFinish(TIMEOUT_IN_SECOND * 1000);

            stopUserAfterWaitingForBroadcastIdle(userId, /* force */true);

            stopUserAfterWaitingForBroadcastIdle(userId);



            assertTrue("Pre start was not performed for user" + userId, preStartComplete);

        }

core/api/test-current.txt

+1 −1
Original line number Diff line number Diff line
@@ -157,7 +157,7 @@ package android.app { 
    method @RequiresPermission(android.Manifest.permission.CHANGE_CONFIGURATION) public void scheduleApplicationInfoChanged(java.util.List<java.lang.String>, int);

    method @RequiresPermission(anyOf={android.Manifest.permission.MANAGE_USERS, android.Manifest.permission.INTERACT_ACROSS_USERS}) public void setStopUserOnSwitch(int);

    method @RequiresPermission(anyOf={android.Manifest.permission.MANAGE_USERS, android.Manifest.permission.INTERACT_ACROSS_USERS}) public boolean startUserInBackgroundVisibleOnDisplay(int, int);

    method @RequiresPermission(android.Manifest.permission.INTERACT_ACROSS_USERS_FULL) public boolean stopUser(int, boolean);

    method @RequiresPermission(android.Manifest.permission.INTERACT_ACROSS_USERS_FULL) public boolean stopUser(int);

    method @RequiresPermission(android.Manifest.permission.PACKAGE_USAGE_STATS) public void unregisterUidFrozenStateChangedCallback(@NonNull android.app.ActivityManager.UidFrozenStateChangedCallback);

    method @RequiresPermission(android.Manifest.permission.CHANGE_CONFIGURATION) public boolean updateMccMncConfiguration(@NonNull String, @NonNull String);

    method @RequiresPermission(android.Manifest.permission.DUMP) public void waitForBroadcastIdle();

core/java/android/app/ActivityManager.java

+5 −4
Original line number Diff line number Diff line
@@ -5071,7 +5071,7 @@ public class ActivityManager { 
     * <p><b>NOTE:</b> differently from {@link #switchUser(int)}, which stops the current foreground

     * user before starting a new one, this method does not stop the previous user running in

     * background in the display, and it will return {@code false} in this case. It's up to the

     * caller to call {@link #stopUser(int, boolean)} before starting a new user.

     * caller to call {@link #stopUser(int)} before starting a new user.

     *

     * @param userId user to be started in the display. It will return {@code false} if the user is

     * a profile, the {@link #getCurrentUser()}, the {@link UserHandle#SYSTEM system user}, or
@@ -5281,15 +5281,16 @@ public class ActivityManager { 
     *

     * @hide

     */

    @SuppressLint("UnflaggedApi") // @TestApi without associated feature.

    @TestApi

    @RequiresPermission(android.Manifest.permission.INTERACT_ACROSS_USERS_FULL)

    public boolean stopUser(@UserIdInt int userId, boolean force) {

    public boolean stopUser(@UserIdInt int userId) {

        if (userId == UserHandle.USER_SYSTEM) {

            return false;

        }

        try {

            return USER_OP_SUCCESS == getService().stopUser(

                    userId, force, /* callback= */ null);

            return USER_OP_SUCCESS == getService().stopUserWithCallback(

                    userId, /* callback= */ null);

        } catch (RemoteException e) {

            throw e.rethrowFromSystemServer();

        }

core/java/android/app/IActivityManager.aidl

+5 −3
Original line number Diff line number Diff line
@@ -452,12 +452,14 @@ interface IActivityManager { 
            in IBinder resultTo, in String resultWho, int requestCode, int flags,

            in ProfilerInfo profilerInfo, in Bundle options, int userId);

    @UnsupportedAppUsage(maxTargetSdk = 30, trackingBug = 170729553)

    int stopUser(int userid, boolean force, in IStopUserCallback callback);

    int stopUser(int userid, boolean stopProfileRegardlessOfParent, in IStopUserCallback callback);

    int stopUserWithCallback(int userid, in IStopUserCallback callback);

    int stopUserExceptCertainProfiles(int userid, boolean stopProfileRegardlessOfParent, in IStopUserCallback callback);

    /**

     * Check {@link com.android.server.am.ActivityManagerService#stopUserWithDelayedLocking(int, boolean, IStopUserCallback)}

     * Check {@link com.android.server.am.ActivityManagerService#stopUserWithDelayedLocking(int, IStopUserCallback)}

     * for details.

     */

    int stopUserWithDelayedLocking(int userid, boolean force, in IStopUserCallback callback);

    int stopUserWithDelayedLocking(int userid, in IStopUserCallback callback);



    @UnsupportedAppUsage

    void registerUserSwitchObserver(in IUserSwitchObserver observer, in String name);

core/java/android/content/pm/UserProperties.java

+1 −1
Original line number Diff line number Diff line
@@ -887,7 +887,7 @@ public final class UserProperties implements Parcelable { 
     * <p> Setting this property to true will enable the user's CE storage to remain unlocked when

     * the user is stopped using

     * {@link com.android.server.am.ActivityManagerService#stopUserWithDelayedLocking(int,

     * boolean, IStopUserCallback)}.

     * IStopUserCallback)}.

     *

     * <p> When this property is false, delayed locking may still be applicable at a global

     * level for all users via the {@code config_multiuserDelayUserDataLocking}. That is, delayed