Android/frameworks_base
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

DO NOT MERGE Add stop method to backup handler thread.

Browse Source 
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
Test: blaze run -- //experimental/users/nathch/py/bug_repros:repro 136264323 -m acquire_quit  -log DEBUG
Test: blaze run -- //experimental/users/nathch/py/bug_repros:repro 136264323 -m quit_acquire  -log DEBUG
Test: blaze run -- //experimental/users/nathch/py/bug_repros:repro 136264323 -m acquire_quit_release  -log DEBUG
Bug: 136264323

Change-Id: I42dcf997fc44cde05695a563aa19c8e47f6f9f26
This commit is contained in:
nathch
2019-07-24 18:11:40 +01:00
committed by Chandan Nath
parent dc860bdfa8
commit 842369a82b
7 changed files with 102 additions and 26 deletions
Download Patch File Download Diff File Expand all files Collapse all files

63
services/backup/java/com/android/server/backup/UserBackupManagerService.java
View File

@@ -166,6 +166,52 @@ 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 +298,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 +308,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 +532,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 +632,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 +655,7 @@ public class UserBackupManagerService {
mContext.unregisterReceiver(mRunBackupReceiver);
mContext.unregisterReceiver(mRunInitReceiver);
mContext.unregisterReceiver(mPackageTrackingReceiver);
mUserBackupThread.quit();
mBackupHandler.stop();
}
public @UserIdInt int getUserId() {
@@ -668,7 +715,7 @@ public class UserBackupManagerService {
mSetupComplete = setupComplete;
}
public PowerManager.WakeLock getWakelock() {
public BackupWakeLock getWakelock() {
return mWakelock;
}
@@ -679,7 +726,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() {

34
services/backup/java/com/android/server/backup/internal/BackupHandler.java
View File

@@ -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) {

4
services/backup/java/com/android/server/backup/internal/RunInitializeReceiver.java
View File

@@ -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();

5
services/backup/java/com/android/server/backup/restore/ActiveRestoreSession.java
View File

@@ -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);

3
services/robotests/backup/src/com/android/server/backup/keyvalue/KeyValueBackupTaskTest.java
View File

@@ -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;

11
services/robotests/backup/src/com/android/server/backup/restore/ActiveRestoreSessionTest.java
View File

@@ -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);

8
services/robotests/backup/src/com/android/server/backup/testing/BackupManagerServiceTestUtils.java
View File

@@ -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*"));
}
/**