From 84a4c971c484f05f2a2494d6353f36f4d954a5e0 Mon Sep 17 00:00:00 2001 From: Jeff Sharkey Date: Mon, 18 Apr 2016 15:36:39 -0600 Subject: [PATCH] Unlock should always wait for pending PRE_BOOT. While processing an unlock request, we might go async to handle long-running operations like dispatching PRE_BOOT_COMPLETED. This change ensures that all unlock requests for a particular user wait in line behind any pending async operations. Without this CL, any subsequent unlock requests would immediately return successful, even though PRE_BOOT_COMPLETED events were still being processed. Bug: 28240584 Change-Id: I307d6aaebfb8f38028f3666a2e19e4399b7cf3a7 --- .../android/internal/util/ProgressReporter.java | 119 +++++++++++++++++---- .../internal/util/ProgressReporterTest.java | 2 +- .../android/server/am/ActivityManagerService.java | 2 +- .../java/com/android/server/am/UserController.java | 62 ++++++----- .../core/java/com/android/server/am/UserState.java | 10 +- 5 files changed, 140 insertions(+), 55 deletions(-) diff --git a/core/java/com/android/internal/util/ProgressReporter.java b/core/java/com/android/internal/util/ProgressReporter.java index 796f8acccdcb..7a8efba8a637 100644 --- a/core/java/com/android/internal/util/ProgressReporter.java +++ b/core/java/com/android/internal/util/ProgressReporter.java @@ -20,9 +20,12 @@ import android.annotation.Nullable; import android.content.Intent; import android.os.Bundle; import android.os.IProgressListener; +import android.os.RemoteCallbackList; import android.os.RemoteException; import android.util.MathUtils; +import com.android.internal.annotations.GuardedBy; + /** * Tracks and reports progress of a single task to a {@link IProgressListener}. * The reported progress of a task ranges from 0-100, but the task can be @@ -44,33 +47,67 @@ import android.util.MathUtils; * } * * - * This class is not thread safe. - * * @hide */ public class ProgressReporter { - public static final ProgressReporter NO_OP = new ProgressReporter(0, null); + private static final int STATE_INIT = 0; + private static final int STATE_STARTED = 1; + private static final int STATE_FINISHED = 2; private final int mId; - private final IProgressListener mListener; - private Bundle mExtras = new Bundle(); + @GuardedBy("this") + private final RemoteCallbackList mListeners = new RemoteCallbackList<>(); + @GuardedBy("this") + private int mState = STATE_INIT; + @GuardedBy("this") private int mProgress = 0; + @GuardedBy("this") + private Bundle mExtras = new Bundle(); /** * Current segment range: first element is starting progress of this * segment, second element is length of segment. */ + @GuardedBy("this") private int[] mSegmentRange = new int[] { 0, 100 }; /** * Create a new task with the given identifier whose progress will be * reported to the given listener. */ - public ProgressReporter(int id, @Nullable IProgressListener listener) { + public ProgressReporter(int id) { mId = id; - mListener = listener; + } + + /** + * Add given listener to watch for progress events. The current state will + * be immediately dispatched to the given listener. + */ + public void addListener(@Nullable IProgressListener listener) { + if (listener == null) return; + synchronized (this) { + mListeners.register(listener); + switch (mState) { + case STATE_INIT: + // Nothing has happened yet + break; + case STATE_STARTED: + try { + listener.onStarted(mId, null); + listener.onProgress(mId, mProgress, mExtras); + } catch (RemoteException ignored) { + } + break; + case STATE_FINISHED: + try { + listener.onFinished(mId, null); + } catch (RemoteException ignored) { + } + break; + } + } } /** @@ -102,12 +139,17 @@ public class ProgressReporter { * Set the fractional progress of the currently active segment. */ public void setProgress(int n, int m, @Nullable CharSequence title) { - mProgress = mSegmentRange[0] - + MathUtils.constrain((n * mSegmentRange[1]) / m, 0, mSegmentRange[1]); - if (title != null) { - mExtras.putCharSequence(Intent.EXTRA_TITLE, title); + synchronized (this) { + if (mState != STATE_STARTED) { + throw new IllegalStateException("Must be started to change progress"); + } + mProgress = mSegmentRange[0] + + MathUtils.constrain((n * mSegmentRange[1]) / m, 0, mSegmentRange[1]); + if (title != null) { + mExtras.putCharSequence(Intent.EXTRA_TITLE, title); + } + notifyProgress(mId, mProgress, mExtras); } - notifyProgress(mId, mProgress, mExtras); } /** @@ -116,17 +158,21 @@ public class ProgressReporter { * {@link #endSegment(int[])} when finished. */ public int[] startSegment(int size) { - final int[] lastRange = mSegmentRange; - mSegmentRange = new int[] { mProgress, (size * mSegmentRange[1] / 100) }; - return lastRange; + synchronized (this) { + final int[] lastRange = mSegmentRange; + mSegmentRange = new int[] { mProgress, (size * mSegmentRange[1] / 100) }; + return lastRange; + } } /** * End the current segment. */ public void endSegment(int[] lastRange) { - mProgress = mSegmentRange[0] + mSegmentRange[1]; - mSegmentRange = lastRange; + synchronized (this) { + mProgress = mSegmentRange[0] + mSegmentRange[1]; + mSegmentRange = lastRange; + } } int getProgress() { @@ -138,27 +184,54 @@ public class ProgressReporter { } /** + * Report this entire task as being started. + */ + public void start() { + synchronized (this) { + mState = STATE_STARTED; + notifyStarted(mId, null); + notifyProgress(mId, mProgress, mExtras); + } + } + + /** * Report this entire task as being finished. */ public void finish() { - notifyFinished(mId, null); + synchronized (this) { + mState = STATE_FINISHED; + notifyFinished(mId, null); + mListeners.kill(); + } + } + + private void notifyStarted(int id, Bundle extras) { + for (int i = mListeners.beginBroadcast() - 1; i >= 0; i--) { + try { + mListeners.getBroadcastItem(i).onStarted(id, extras); + } catch (RemoteException ignored) { + } + } + mListeners.finishBroadcast(); } private void notifyProgress(int id, int progress, Bundle extras) { - if (mListener != null) { + for (int i = mListeners.beginBroadcast() - 1; i >= 0; i--) { try { - mListener.onProgress(id, progress, extras); + mListeners.getBroadcastItem(i).onProgress(id, progress, extras); } catch (RemoteException ignored) { } } + mListeners.finishBroadcast(); } - public void notifyFinished(int id, Bundle extras) { - if (mListener != null) { + private void notifyFinished(int id, Bundle extras) { + for (int i = mListeners.beginBroadcast() - 1; i >= 0; i--) { try { - mListener.onFinished(id, extras); + mListeners.getBroadcastItem(i).onFinished(id, extras); } catch (RemoteException ignored) { } } + mListeners.finishBroadcast(); } } diff --git a/core/tests/coretests/src/com/android/internal/util/ProgressReporterTest.java b/core/tests/coretests/src/com/android/internal/util/ProgressReporterTest.java index fbf552322451..87f2a8a67947 100644 --- a/core/tests/coretests/src/com/android/internal/util/ProgressReporterTest.java +++ b/core/tests/coretests/src/com/android/internal/util/ProgressReporterTest.java @@ -24,7 +24,7 @@ public class ProgressReporterTest extends TestCase { @Override protected void setUp() throws Exception { super.setUp(); - r = new ProgressReporter(0, null); + r = new ProgressReporter(0); } private void assertProgress(int expected) { diff --git a/services/core/java/com/android/server/am/ActivityManagerService.java b/services/core/java/com/android/server/am/ActivityManagerService.java index 8653f1ad0000..5fd8741b77b3 100644 --- a/services/core/java/com/android/server/am/ActivityManagerService.java +++ b/services/core/java/com/android/server/am/ActivityManagerService.java @@ -20669,7 +20669,7 @@ public final class ActivityManagerService extends ActivityManagerNative @Override public boolean unlockUser(int userId, byte[] token, byte[] secret, IProgressListener listener) { - return mUserController.unlockUser(userId, token, secret, new ProgressReporter(0, listener)); + return mUserController.unlockUser(userId, token, secret, listener); } @Override diff --git a/services/core/java/com/android/server/am/UserController.java b/services/core/java/com/android/server/am/UserController.java index 4292fcf406b0..4c050c461d9d 100644 --- a/services/core/java/com/android/server/am/UserController.java +++ b/services/core/java/com/android/server/am/UserController.java @@ -63,6 +63,7 @@ import android.os.Bundle; import android.os.Debug; import android.os.Handler; import android.os.IBinder; +import android.os.IProgressListener; import android.os.IRemoteCallback; import android.os.IUserManager; import android.os.Process; @@ -83,7 +84,6 @@ import android.util.SparseIntArray; import com.android.internal.R; import com.android.internal.annotations.GuardedBy; import com.android.internal.util.ArrayUtils; -import com.android.internal.util.ProgressReporter; import com.android.internal.widget.LockPatternUtils; import com.android.server.LocalServices; import com.android.server.pm.UserManagerService; @@ -260,7 +260,7 @@ final class UserController { * Step from {@link UserState#STATE_RUNNING_LOCKED} to * {@link UserState#STATE_RUNNING_UNLOCKING}. */ - void finishUserUnlocking(final UserState uss, final ProgressReporter progress) { + private void finishUserUnlocking(final UserState uss) { final int userId = uss.mHandle.getIdentifier(); synchronized (mService) { // Bail if we ended up with a stale user @@ -270,10 +270,13 @@ final class UserController { if (!isUserKeyUnlocked(userId)) return; if (uss.setState(STATE_RUNNING_LOCKED, STATE_RUNNING_UNLOCKING)) { + uss.mUnlockProgress.start(); + // Prepare app storage before we go any further - progress.setProgress(5, mService.mContext.getString(R.string.android_start_title)); + uss.mUnlockProgress.setProgress(5, + mService.mContext.getString(R.string.android_start_title)); mUserManager.onBeforeUnlockUser(userId); - progress.setProgress(20); + uss.mUnlockProgress.setProgress(20); // Dispatch unlocked to system services mHandler.sendMessage(mHandler.obtainMessage(SYSTEM_USER_UNLOCK_MSG, userId, 0)); @@ -306,15 +309,15 @@ final class UserController { // Send PRE_BOOT broadcasts if fingerprint changed final UserInfo info = getUserInfo(userId); if (!Objects.equals(info.lastLoggedInFingerprint, Build.FINGERPRINT)) { - progress.startSegment(80); - new PreBootBroadcaster(mService, userId, progress) { + uss.mUnlockProgress.startSegment(80); + new PreBootBroadcaster(mService, userId, uss.mUnlockProgress) { @Override public void onFinished() { - finishUserUnlocked(uss, progress); + finishUserUnlocked(uss); } }.sendNext(); } else { - finishUserUnlocked(uss, progress); + finishUserUnlocked(uss); } } } @@ -324,15 +327,15 @@ final class UserController { * Step from {@link UserState#STATE_RUNNING_UNLOCKING} to * {@link UserState#STATE_RUNNING_UNLOCKED}. */ - void finishUserUnlocked(UserState uss, ProgressReporter progress) { + private void finishUserUnlocked(UserState uss) { try { finishUserUnlockedInternal(uss); } finally { - progress.finish(); + uss.mUnlockProgress.finish(); } } - void finishUserUnlockedInternal(UserState uss) { + private void finishUserUnlockedInternal(UserState uss) { final int userId = uss.mHandle.getIdentifier(); synchronized (mService) { // Bail if we ended up with a stale user @@ -860,7 +863,7 @@ final class UserController { return result; } - boolean unlockUser(final int userId, byte[] token, byte[] secret, ProgressReporter progress) { + boolean unlockUser(final int userId, byte[] token, byte[] secret, IProgressListener listener) { if (mService.checkCallingPermission(INTERACT_ACROSS_USERS_FULL) != PackageManager.PERMISSION_GRANTED) { String msg = "Permission Denial: unlockUser() from pid=" @@ -873,7 +876,7 @@ final class UserController { final long binderToken = Binder.clearCallingIdentity(); try { - return unlockUserCleared(userId, token, secret, progress); + return unlockUserCleared(userId, token, secret, listener); } finally { Binder.restoreCallingIdentity(binderToken); } @@ -887,23 +890,29 @@ final class UserController { */ boolean maybeUnlockUser(final int userId) { // Try unlocking storage using empty token - return unlockUserCleared(userId, null, null, ProgressReporter.NO_OP); + return unlockUserCleared(userId, null, null, null); + } + + private static void notifyFinished(int userId, IProgressListener listener) { + if (listener == null) return; + try { + listener.onFinished(userId, null); + } catch (RemoteException ignored) { + } } boolean unlockUserCleared(final int userId, byte[] token, byte[] secret, - ProgressReporter progress) { + IProgressListener listener) { + final UserState uss; synchronized (mService) { - // Bail if already running unlocked, or if not running at all - final UserState uss = mStartedUsers.get(userId); + // Bail if user isn't actually running, otherwise register the given + // listener to watch for unlock progress + uss = mStartedUsers.get(userId); if (uss == null) { - progress.finish(); + notifyFinished(userId, listener); return false; - } - switch (uss.state) { - case STATE_RUNNING_UNLOCKING: - case STATE_RUNNING_UNLOCKED: - progress.finish(); - return true; + } else { + uss.mUnlockProgress.addListener(listener); } } @@ -914,14 +923,13 @@ final class UserController { mountService.unlockUserKey(userId, userInfo.serialNumber, token, secret); } catch (RemoteException | RuntimeException e) { Slog.w(TAG, "Failed to unlock: " + e.getMessage()); - progress.finish(); + notifyFinished(userId, listener); return false; } } synchronized (mService) { - final UserState uss = mStartedUsers.get(userId); - finishUserUnlocking(uss, progress); + finishUserUnlocking(uss); // We just unlocked a user, so let's now attempt to unlock any // managed profiles under that user. diff --git a/services/core/java/com/android/server/am/UserState.java b/services/core/java/com/android/server/am/UserState.java index 6e2342b2556b..56abd95179d1 100644 --- a/services/core/java/com/android/server/am/UserState.java +++ b/services/core/java/com/android/server/am/UserState.java @@ -16,9 +16,6 @@ package com.android.server.am; -import java.io.PrintWriter; -import java.util.ArrayList; - import static com.android.server.am.ActivityManagerDebugConfig.DEBUG_MU; import static com.android.server.am.ActivityManagerDebugConfig.TAG_AM; import static com.android.server.am.ActivityManagerDebugConfig.TAG_WITH_CLASS_NAME; @@ -28,6 +25,11 @@ import android.os.UserHandle; import android.util.ArrayMap; import android.util.Slog; +import com.android.internal.util.ProgressReporter; + +import java.io.PrintWriter; +import java.util.ArrayList; + public final class UserState { private static final String TAG = TAG_WITH_CLASS_NAME ? "UserState" : TAG_AM; @@ -47,6 +49,7 @@ public final class UserState { public final UserHandle mHandle; public final ArrayList mStopCallbacks = new ArrayList(); + public final ProgressReporter mUnlockProgress; public int state = STATE_BOOTING; public int lastState = STATE_BOOTING; @@ -61,6 +64,7 @@ public final class UserState { public UserState(UserHandle handle) { mHandle = handle; + mUnlockProgress = new ProgressReporter(handle.getIdentifier()); } public boolean setState(int oldState, int newState) { -- 2.11.0