From 15bee2d83dc37fc4197de64d3ff46cf8320611f2 Mon Sep 17 00:00:00 2001 From: nathch Date: Wed, 24 Jul 2019 18:11:40 +0100 Subject: Add stop method to backup handler thread. This manually cherry picks ag/8713360 into master. I had to do a manual cherry pick as the cherry pick from gerrit fails with: "Could not perform action: Cannot create new patch set of change 8677635 because it is abandoned" Currently we call .quit() on the underlying thread which will cause all messages to stop being processed. This has the side effect that, because the backup system is a state machine where the state transitions are messages, the message to transition into a state where the WakeLock is released may not occur when a user is torn down. This change adds a stop method we can call instead of .quit() on the thread which drops any remaining messages and then releases the WakeLock. We also wrap the wakelock acquire/release calls to prevent any acquire/release on the underlying wakelock after a quit. For the acquire, this avoids a non-released wakelock and for the release, this avoids a runtime exception which can happen when we release a released wakelock Test: atest CtsBackupTestCases CtsBackupHostTestCases Test: m RunBackupFrameworksServicesRoboTests && atest RunBackupFrameworksServicesRoboTests Bug: 136264323 Change-Id: Ic8742bf01a0ff71bd57dd73b01a423d3432bf7b2 --- .../server/backup/UserBackupManagerService.java | 64 +++++++++++++++++++--- .../server/backup/internal/BackupHandler.java | 34 +++++++++++- .../backup/internal/RunInitializeReceiver.java | 4 +- .../backup/restore/ActiveRestoreSession.java | 5 +- .../backup/keyvalue/KeyValueBackupTaskTest.java | 3 +- .../backup/restore/ActiveRestoreSessionTest.java | 11 ++-- .../testing/BackupManagerServiceTestUtils.java | 8 ++- 7 files changed, 103 insertions(+), 26 deletions(-) diff --git a/services/backup/java/com/android/server/backup/UserBackupManagerService.java b/services/backup/java/com/android/server/backup/UserBackupManagerService.java index ed4e59611c22..d599aabbaa28 100644 --- a/services/backup/java/com/android/server/backup/UserBackupManagerService.java +++ b/services/backup/java/com/android/server/backup/UserBackupManagerService.java @@ -166,6 +166,53 @@ import java.util.concurrent.atomic.AtomicInteger; /** System service that performs backup/restore operations. */ public class UserBackupManagerService { + /** + * Wrapper over {@link PowerManager.WakeLock} to prevent double-free exceptions on release() + * after quit(). + */ + public static class BackupWakeLock { + private final PowerManager.WakeLock mPowerManagerWakeLock; + private boolean mHasQuit = false; + + public BackupWakeLock(PowerManager.WakeLock powerManagerWakeLock) { + mPowerManagerWakeLock = powerManagerWakeLock; + } + + /** Acquires the {@link PowerManager.WakeLock} if hasn't been quit. */ + public synchronized void acquire() { + if (mHasQuit) { + Slog.v(TAG, "Ignore wakelock acquire after quit: " + mPowerManagerWakeLock.getTag()); + return; + } + mPowerManagerWakeLock.acquire(); + } + + /** Releases the {@link PowerManager.WakeLock} if hasn't been quit. */ + public synchronized void release() { + if (mHasQuit) { + Slog.v(TAG, "Ignore wakelock release after quit: " + mPowerManagerWakeLock.getTag()); + return; + } + mPowerManagerWakeLock.release(); + } + + /** + * Returns true if the {@link PowerManager.WakeLock} has been acquired but not yet released. + */ + public synchronized boolean isHeld() { + return mPowerManagerWakeLock.isHeld(); + } + + /** Release the {@link PowerManager.WakeLock} till it isn't held. */ + public synchronized void quit() { + while (mPowerManagerWakeLock.isHeld()) { + Slog.v(TAG, "Releasing wakelock: " + mPowerManagerWakeLock.getTag()); + mPowerManagerWakeLock.release(); + } + mHasQuit = true; + } + } + // Persistently track the need to do a full init. private static final String INIT_SENTINEL_FILE_NAME = "_need_init_"; @@ -252,7 +299,6 @@ public class UserBackupManagerService { private final @UserIdInt int mUserId; private final BackupAgentTimeoutParameters mAgentTimeoutParameters; private final TransportManager mTransportManager; - private final HandlerThread mUserBackupThread; private final Context mContext; private final PackageManager mPackageManager; @@ -263,7 +309,7 @@ public class UserBackupManagerService { private final AlarmManager mAlarmManager; private final IStorageManager mStorageManager; private final BackupManagerConstants mConstants; - private final PowerManager.WakeLock mWakelock; + private final BackupWakeLock mWakelock; private final BackupHandler mBackupHandler; private final IBackupManager mBackupManagerBinder; @@ -487,8 +533,7 @@ public class UserBackupManagerService { mAgentTimeoutParameters.start(); checkNotNull(userBackupThread, "userBackupThread cannot be null"); - mUserBackupThread = userBackupThread; - mBackupHandler = new BackupHandler(this, userBackupThread.getLooper()); + mBackupHandler = new BackupHandler(this, userBackupThread); // Set up our bookkeeping final ContentResolver resolver = context.getContentResolver(); @@ -588,7 +633,10 @@ public class UserBackupManagerService { mBackupHandler.postDelayed(this::parseLeftoverJournals, INITIALIZATION_DELAY_MILLIS); // Power management - mWakelock = mPowerManager.newWakeLock(PowerManager.PARTIAL_WAKE_LOCK, "*backup*-" + userId); + mWakelock = new BackupWakeLock( + mPowerManager.newWakeLock( + PowerManager.PARTIAL_WAKE_LOCK, + "*backup*-" + userId + "-" + userBackupThread.getThreadId())); // Set up the various sorts of package tracking we do mFullBackupScheduleFile = new File(mBaseStateDir, "fb-schedule"); @@ -608,7 +656,7 @@ public class UserBackupManagerService { mContext.unregisterReceiver(mRunBackupReceiver); mContext.unregisterReceiver(mRunInitReceiver); mContext.unregisterReceiver(mPackageTrackingReceiver); - mUserBackupThread.quit(); + mBackupHandler.stop(); } public @UserIdInt int getUserId() { @@ -668,7 +716,7 @@ public class UserBackupManagerService { mSetupComplete = setupComplete; } - public PowerManager.WakeLock getWakelock() { + public BackupWakeLock getWakelock() { return mWakelock; } @@ -679,7 +727,7 @@ public class UserBackupManagerService { @VisibleForTesting public void setWorkSource(@Nullable WorkSource workSource) { // TODO: This is for testing, unfortunately WakeLock is final and WorkSource is not exposed - mWakelock.setWorkSource(workSource); + mWakelock.mPowerManagerWakeLock.setWorkSource(workSource); } public Handler getBackupHandler() { diff --git a/services/backup/java/com/android/server/backup/internal/BackupHandler.java b/services/backup/java/com/android/server/backup/internal/BackupHandler.java index ba153bf90ebe..059b1b9379af 100644 --- a/services/backup/java/com/android/server/backup/internal/BackupHandler.java +++ b/services/backup/java/com/android/server/backup/internal/BackupHandler.java @@ -23,7 +23,7 @@ import static com.android.server.backup.BackupManagerService.TAG; import android.app.backup.RestoreSet; import android.content.Intent; import android.os.Handler; -import android.os.Looper; +import android.os.HandlerThread; import android.os.Message; import android.os.RemoteException; import android.os.UserHandle; @@ -83,19 +83,47 @@ public class BackupHandler extends Handler { // backup task state machine tick public static final int MSG_BACKUP_RESTORE_STEP = 20; public static final int MSG_OP_COMPLETE = 21; + // Release the wakelock. This is used to ensure we don't hold it after + // a user is removed. This will also terminate the looper thread. + public static final int MSG_STOP = 22; private final UserBackupManagerService backupManagerService; private final BackupAgentTimeoutParameters mAgentTimeoutParameters; - public BackupHandler(UserBackupManagerService backupManagerService, Looper looper) { - super(looper); + private final HandlerThread mBackupThread; + private volatile boolean mIsStopping = false; + + public BackupHandler( + UserBackupManagerService backupManagerService, HandlerThread backupThread) { + super(backupThread.getLooper()); + mBackupThread = backupThread; this.backupManagerService = backupManagerService; mAgentTimeoutParameters = Preconditions.checkNotNull( backupManagerService.getAgentTimeoutParameters(), "Timeout parameters cannot be null"); } + /** + * Put the BackupHandler into a stopping state where the remaining messages on the queue will be + * silently dropped and the {@link WakeLock} held by the {@link UserBackupManagerService} will + * then be released. + */ + public void stop() { + mIsStopping = true; + sendMessage(obtainMessage(BackupHandler.MSG_STOP)); + } + public void handleMessage(Message msg) { + if (msg.what == MSG_STOP) { + Slog.v(TAG, "Stopping backup handler"); + backupManagerService.getWakelock().quit(); + mBackupThread.quitSafely(); + } + + if (mIsStopping) { + // If we're finishing all other types of messages should be ignored + return; + } TransportManager transportManager = backupManagerService.getTransportManager(); switch (msg.what) { diff --git a/services/backup/java/com/android/server/backup/internal/RunInitializeReceiver.java b/services/backup/java/com/android/server/backup/internal/RunInitializeReceiver.java index 97711e3c27ed..96d61e5755c8 100644 --- a/services/backup/java/com/android/server/backup/internal/RunInitializeReceiver.java +++ b/services/backup/java/com/android/server/backup/internal/RunInitializeReceiver.java @@ -23,7 +23,6 @@ import static com.android.server.backup.UserBackupManagerService.RUN_INITIALIZE_ import android.content.BroadcastReceiver; import android.content.Context; import android.content.Intent; -import android.os.PowerManager; import android.util.Slog; import com.android.server.backup.UserBackupManagerService; @@ -57,7 +56,8 @@ public class RunInitializeReceiver extends BroadcastReceiver { mUserBackupManagerService.clearPendingInits(); - PowerManager.WakeLock wakelock = mUserBackupManagerService.getWakelock(); + UserBackupManagerService.BackupWakeLock wakelock = + mUserBackupManagerService.getWakelock(); wakelock.acquire(); OnTaskFinishedListener listener = caller -> wakelock.release(); diff --git a/services/backup/java/com/android/server/backup/restore/ActiveRestoreSession.java b/services/backup/java/com/android/server/backup/restore/ActiveRestoreSession.java index 10304c39f021..5a57cdc39402 100644 --- a/services/backup/java/com/android/server/backup/restore/ActiveRestoreSession.java +++ b/services/backup/java/com/android/server/backup/restore/ActiveRestoreSession.java @@ -34,7 +34,6 @@ import android.content.pm.PackageManager.NameNotFoundException; import android.os.Binder; import android.os.Handler; import android.os.Message; -import android.os.PowerManager; import android.util.Slog; import com.android.server.backup.TransportManager; @@ -110,7 +109,7 @@ public class ActiveRestoreSession extends IRestoreSession.Stub { // comes in. mBackupManagerService.getBackupHandler().removeMessages(MSG_RESTORE_SESSION_TIMEOUT); - PowerManager.WakeLock wakelock = mBackupManagerService.getWakelock(); + UserBackupManagerService.BackupWakeLock wakelock = mBackupManagerService.getWakelock(); wakelock.acquire(); // Prevent lambda from leaking 'this' @@ -392,7 +391,7 @@ public class ActiveRestoreSession extends IRestoreSession.Stub { Handler backupHandler = mBackupManagerService.getBackupHandler(); backupHandler.removeMessages(MSG_RESTORE_SESSION_TIMEOUT); - PowerManager.WakeLock wakelock = mBackupManagerService.getWakelock(); + UserBackupManagerService.BackupWakeLock wakelock = mBackupManagerService.getWakelock(); wakelock.acquire(); if (MORE_DEBUG) { Slog.d(TAG, callerLogString); diff --git a/services/robotests/backup/src/com/android/server/backup/keyvalue/KeyValueBackupTaskTest.java b/services/robotests/backup/src/com/android/server/backup/keyvalue/KeyValueBackupTaskTest.java index 43691e0e6163..4aba1f487f49 100644 --- a/services/robotests/backup/src/com/android/server/backup/keyvalue/KeyValueBackupTaskTest.java +++ b/services/robotests/backup/src/com/android/server/backup/keyvalue/KeyValueBackupTaskTest.java @@ -94,7 +94,6 @@ import android.os.Handler; import android.os.Looper; import android.os.Message; import android.os.ParcelFileDescriptor; -import android.os.PowerManager; import android.os.RemoteException; import android.platform.test.annotations.Presubmit; import android.util.Pair; @@ -184,7 +183,7 @@ public class KeyValueBackupTaskTest { private TransportData mTransport; private ShadowLooper mShadowBackupLooper; private Handler mBackupHandler; - private PowerManager.WakeLock mWakeLock; + private UserBackupManagerService.BackupWakeLock mWakeLock; private KeyValueBackupReporter mReporter; private PackageManager mPackageManager; private ShadowPackageManager mShadowPackageManager; diff --git a/services/robotests/backup/src/com/android/server/backup/restore/ActiveRestoreSessionTest.java b/services/robotests/backup/src/com/android/server/backup/restore/ActiveRestoreSessionTest.java index f4cea7ae86a6..3fc421dfb6e9 100644 --- a/services/robotests/backup/src/com/android/server/backup/restore/ActiveRestoreSessionTest.java +++ b/services/robotests/backup/src/com/android/server/backup/restore/ActiveRestoreSessionTest.java @@ -18,7 +18,7 @@ package com.android.server.backup.restore; import static com.android.server.backup.testing.BackupManagerServiceTestUtils.createBackupWakeLock; import static com.android.server.backup.testing.BackupManagerServiceTestUtils.setUpBackupManagerServiceBasics; -import static com.android.server.backup.testing.BackupManagerServiceTestUtils.startBackupThreadAndGetLooper; +import static com.android.server.backup.testing.BackupManagerServiceTestUtils.startBackupThread; import static com.android.server.backup.testing.TestUtils.assertEventLogged; import static com.android.server.backup.testing.TestUtils.assertEventNotLogged; import static com.android.server.backup.testing.TransportData.backupTransport; @@ -44,8 +44,8 @@ import android.app.backup.RestoreSet; import android.content.pm.ApplicationInfo; import android.content.pm.PackageInfo; import android.os.Handler; +import android.os.HandlerThread; import android.os.Looper; -import android.os.PowerManager; import android.os.RemoteException; import android.platform.test.annotations.Presubmit; @@ -98,7 +98,7 @@ public class ActiveRestoreSessionTest { @Mock private IBackupManagerMonitor mMonitor; private ShadowLooper mShadowBackupLooper; private ShadowApplication mShadowApplication; - private PowerManager.WakeLock mWakeLock; + private UserBackupManagerService.BackupWakeLock mWakeLock; private TransportData mTransport; private RestoreSet mRestoreSet1; private RestoreSet mRestoreSet2; @@ -118,7 +118,8 @@ public class ActiveRestoreSessionTest { mShadowPackageManager = shadowOf(application.getPackageManager()); - Looper backupLooper = startBackupThreadAndGetLooper(); + HandlerThread handlerThread = startBackupThread(null); + Looper backupLooper = handlerThread.getLooper(); mShadowBackupLooper = shadowOf(backupLooper); Handler mainHandler = new Handler(Looper.getMainLooper()); @@ -129,7 +130,7 @@ public class ActiveRestoreSessionTest { // We need to mock BMS timeout parameters before initializing the BackupHandler since // the constructor of BackupHandler relies on it. when(mBackupManagerService.getAgentTimeoutParameters()).thenReturn(agentTimeoutParameters); - BackupHandler backupHandler = new BackupHandler(mBackupManagerService, backupLooper); + BackupHandler backupHandler = new BackupHandler(mBackupManagerService, handlerThread); mWakeLock = createBackupWakeLock(application); diff --git a/services/robotests/backup/src/com/android/server/backup/testing/BackupManagerServiceTestUtils.java b/services/robotests/backup/src/com/android/server/backup/testing/BackupManagerServiceTestUtils.java index 47abcc5a2dd1..392d182328a5 100644 --- a/services/robotests/backup/src/com/android/server/backup/testing/BackupManagerServiceTestUtils.java +++ b/services/robotests/backup/src/com/android/server/backup/testing/BackupManagerServiceTestUtils.java @@ -113,7 +113,7 @@ public class BackupManagerServiceTestUtils { TransportManager transportManager, PackageManager packageManager, Handler backupHandler, - PowerManager.WakeLock wakeLock, + UserBackupManagerService.BackupWakeLock wakeLock, BackupAgentTimeoutParameters agentTimeoutParameters) { when(backupManagerService.getContext()).thenReturn(application); @@ -161,10 +161,12 @@ public class BackupManagerServiceTestUtils { }); } - public static PowerManager.WakeLock createBackupWakeLock(Application application) { + public static UserBackupManagerService.BackupWakeLock createBackupWakeLock( + Application application) { PowerManager powerManager = (PowerManager) application.getSystemService(Context.POWER_SERVICE); - return powerManager.newWakeLock(PowerManager.PARTIAL_WAKE_LOCK, "*backup*"); + return new UserBackupManagerService.BackupWakeLock( + powerManager.newWakeLock(PowerManager.PARTIAL_WAKE_LOCK, "*backup*")); } /** -- cgit v1.2.3-59-g8ed1b