[KV] Refactor clean-up
Only clean-up after possibly calling quota exceeded to avoid bug linked.
Small refactors and tests.
Test: atest KeyValueBackupTaskTest
Test: atest KeyValueQuotaTest
Bug: 114002519
Bug: 110082831
Change-Id: If53aaa59287cf9f2ac8d9ad47afc0a62161373c5
diff --git a/services/backup/java/com/android/server/backup/keyvalue/KeyValueBackupTask.java b/services/backup/java/com/android/server/backup/keyvalue/KeyValueBackupTask.java
index a4cd629..e915ce1 100644
--- a/services/backup/java/com/android/server/backup/keyvalue/KeyValueBackupTask.java
+++ b/services/backup/java/com/android/server/backup/keyvalue/KeyValueBackupTask.java
@@ -249,10 +249,10 @@
private final List<String> mPendingFullBackups;
private final Object mQueueLock;
@Nullable private final DataChangedJournal mJournal;
- @Nullable private PerformFullTransportBackupTask mFullBackupTask;
private int mStatus;
- @Nullable private IBackupAgent mAgentBinder;
+ @Nullable private PerformFullTransportBackupTask mFullBackupTask;
+ @Nullable private IBackupAgent mAgent;
@Nullable private PackageInfo mCurrentPackage;
@Nullable private File mSavedStateFile;
@Nullable private File mBackupDataFile;
@@ -349,24 +349,24 @@
// Not an explicit cancel, we need to flag it.
mCancelled = true;
mReporter.onAgentCancelled(packageInfo);
- errorCleanup();
+ cleanUpAgentForAgentError();
return false;
}
if (result == RemoteResult.FAILED_CANCELLED) {
mReporter.onAgentCancelled(packageInfo);
- errorCleanup();
+ cleanUpAgentForAgentError();
return false;
}
if (result == RemoteResult.FAILED_TIMED_OUT) {
mReporter.onAgentTimedOut(packageInfo);
- errorCleanup();
+ cleanUpAgentForAgentError();
return true;
}
Preconditions.checkState(result.isPresent());
long agentResult = result.get();
if (agentResult == BackupAgent.RESULT_ERROR) {
mReporter.onAgentResultError(packageInfo);
- errorCleanup();
+ cleanUpAgentForAgentError();
return true;
}
return sendDataToTransport();
@@ -380,37 +380,22 @@
/** Returns whether to consume next queue package. */
private boolean startTask() {
- synchronized (mBackupManagerService.getCurrentOpLock()) {
- if (mBackupManagerService.isBackupOperationInProgress()) {
- mReporter.onSkipBackup();
- return false;
- }
+ if (mBackupManagerService.isBackupOperationInProgress()) {
+ mReporter.onSkipBackup();
+ return false;
}
- String[] fullBackups = mPendingFullBackups.toArray(new String[mPendingFullBackups.size()]);
- mFullBackupTask =
- new PerformFullTransportBackupTask(
- mBackupManagerService,
- mTransportClient,
- /* fullBackupRestoreObserver */ null,
- fullBackups,
- /* updateSchedule */ false,
- /* runningJob */ null,
- new CountDownLatch(1),
- mReporter.getObserver(),
- mReporter.getMonitor(),
- mTaskFinishedListener,
- mUserInitiated);
+ // Unfortunately full backup task constructor registers the task with BMS, so we have to
+ // create it here instead of in our constructor.
+ mFullBackupTask = createFullBackupTask(mPendingFullBackups);
registerTask();
- mAgentBinder = null;
mStatus = BackupTransport.TRANSPORT_OK;
if (mQueue.isEmpty() && mPendingFullBackups.isEmpty()) {
mReporter.onEmptyQueueAtStart();
return false;
}
-
// We only backup PM if it was explicitly in the queue or if it's incremental.
boolean backupPm = mQueue.remove(PM_PACKAGE) || !mNonIncremental;
if (backupPm) {
@@ -446,6 +431,21 @@
return true;
}
+ private PerformFullTransportBackupTask createFullBackupTask(List<String> packages) {
+ return new PerformFullTransportBackupTask(
+ mBackupManagerService,
+ mTransportClient,
+ /* fullBackupRestoreObserver */ null,
+ packages.toArray(new String[packages.size()]),
+ /* updateSchedule */ false,
+ /* runningJob */ null,
+ new CountDownLatch(1),
+ mReporter.getObserver(),
+ mReporter.getMonitor(),
+ mTaskFinishedListener,
+ mUserInitiated);
+ }
+
/** Returns whether to consume next queue package. */
private boolean backupPm() {
RemoteResult agentResult = null;
@@ -455,10 +455,9 @@
// Since PM is running in the system process we can set up its agent directly.
BackupAgent pmAgent = mBackupManagerService.makeMetadataAgent();
- Pair<Integer, RemoteResult> statusAndResult =
- extractAgentData(
- PM_PACKAGE,
- IBackupAgent.Stub.asInterface(pmAgent.onBind()));
+ mAgent = IBackupAgent.Stub.asInterface(pmAgent.onBind());
+
+ Pair<Integer, RemoteResult> statusAndResult = extractAgentData(PM_PACKAGE, mAgent);
mStatus = statusAndResult.first;
agentResult = statusAndResult.second;
} catch (Exception e) {
@@ -467,6 +466,8 @@
}
if (mStatus != BackupTransport.TRANSPORT_OK) {
+ // In this case either extractAgentData() already made the agent clean-up or we haven't
+ // prepared the state for calling the agent, in either case we don't need to clean-up.
mBackupManagerService.resetBackupState(mStateDirectory);
return false;
}
@@ -512,7 +513,7 @@
applicationInfo,
ApplicationThreadConstants.BACKUP_MODE_INCREMENTAL);
if (agent != null) {
- mAgentBinder = agent;
+ mAgent = agent;
Pair<Integer, RemoteResult> statusAndResult =
extractAgentData(packageName, agent);
mStatus = statusAndResult.first;
@@ -532,7 +533,9 @@
}
if (mStatus != BackupTransport.TRANSPORT_OK) {
- mAgentBinder = null;
+ // In this case either extractAgentData() already made the agent clean-up or we haven't
+ // prepared the state for calling the agent, in either case we don't need to clean-up.
+ Preconditions.checkState(mAgent == null);
if (mStatus == BackupTransport.AGENT_ERROR) {
mReporter.onAgentError(packageName);
@@ -710,7 +713,7 @@
"doBackup()");
} catch (Exception e) {
mReporter.onCallAgentDoBackupError(packageName, callingAgent, e);
- errorCleanup();
+ cleanUpAgentForAgentError();
// TODO: Remove the check on callingAgent when RemoteCall supports local agent calls.
int status =
callingAgent ? BackupTransport.AGENT_ERROR : BackupTransport.TRANSPORT_ERROR;
@@ -808,7 +811,7 @@
boolean writingWidgetData = false;
try {
if (!validateBackupData(applicationInfo, mBackupDataFile)) {
- errorCleanup();
+ cleanUpAgentForAgentError();
return true;
}
writingWidgetData = true;
@@ -819,11 +822,11 @@
} else {
mReporter.onReadAgentDataError(packageName, e);
}
+ cleanUpAgentForAgentError();
revertTask();
return false;
}
- clearAgentState();
boolean nonIncremental = mSavedStateFile.length() == 0;
long size = mBackupDataFile.length();
if (size > 0) {
@@ -853,31 +856,12 @@
mStatus = BackupTransport.TRANSPORT_ERROR;
}
- updateFiles(mStatus);
- return handleTransportStatus(mStatus, packageName, size);
- }
- private void updateFiles(int status) {
- switch (status) {
- case BackupTransport.TRANSPORT_OK:
- mBackupDataFile.delete();
- mNewStateFile.renameTo(mSavedStateFile);
- break;
- case BackupTransport.TRANSPORT_NON_INCREMENTAL_BACKUP_REQUIRED:
- mSavedStateFile.delete();
- mBackupDataFile.delete();
- mNewStateFile.delete();
- break;
- default:
- // Includes:
- // * BackupTransport.TRANSPORT_PACKAGE_REJECTED
- // * BackupTransport.TRANSPORT_QUOTA_EXCEEDED
- // * BackupTransport.TRANSPORT_ERROR
- mBackupDataFile.delete();
- mNewStateFile.delete();
- break;
-
- }
+ boolean processQueue = handleTransportStatus(mStatus, packageName, size);
+ // We might report quota exceeded to the agent in handleTransportStatus() above, so we
+ // only clean-up after it.
+ cleanUpAgentForTransportStatus(mStatus);
+ return processQueue;
}
/** Returns whether to consume next queue package. */
@@ -898,7 +882,7 @@
}
if (status == BackupTransport.TRANSPORT_QUOTA_EXCEEDED) {
mReporter.onPackageBackupQuotaExceeded(packageName);
- agentDoQuotaExceeded(mAgentBinder, packageName, size);
+ agentDoQuotaExceeded(mAgent, packageName, size);
return true;
}
// Any other error here indicates a transport-level failure.
@@ -943,7 +927,7 @@
if (key != null && key.charAt(0) >= 0xff00) {
mReporter.onAgentIllegalKey(mCurrentPackage, key);
// Crash them if they wrote any protected keys.
- agentFail(mAgentBinder, "Illegal backup key: " + key);
+ agentFail(mAgent, "Illegal backup key: " + key);
return false;
}
backupDataInput.skipEntityData();
@@ -1025,21 +1009,50 @@
}
}
- private void errorCleanup() {
- mBackupDataFile.delete();
- mNewStateFile.delete();
- clearAgentState();
+ /** Cleans-up after having called the agent. */
+ private void cleanUpAgentForTransportStatus(int status) {
+ updateFiles(status);
+ cleanUpAgent();
}
- private void clearAgentState() {
- // Cleanup common to both success and failure cases.
+ /** Cleans-up if we failed to call the agent. */
+ private void cleanUpAgentForAgentError() {
+ mBackupDataFile.delete();
+ mNewStateFile.delete();
+ cleanUpAgent();
+ }
+
+ private void updateFiles(int status) {
+ switch (status) {
+ case BackupTransport.TRANSPORT_OK:
+ mBackupDataFile.delete();
+ mNewStateFile.renameTo(mSavedStateFile);
+ break;
+ case BackupTransport.TRANSPORT_NON_INCREMENTAL_BACKUP_REQUIRED:
+ mSavedStateFile.delete();
+ mBackupDataFile.delete();
+ mNewStateFile.delete();
+ break;
+ default:
+ // Includes:
+ // * BackupTransport.TRANSPORT_PACKAGE_REJECTED
+ // * BackupTransport.TRANSPORT_QUOTA_EXCEEDED
+ // * BackupTransport.TRANSPORT_ERROR
+ mBackupDataFile.delete();
+ mNewStateFile.delete();
+ break;
+ }
+ }
+
+ /** Cleans-up file-descriptors and unbinds agent. */
+ private void cleanUpAgent() {
+ mAgent = null;
tryCloseFileDescriptor(mSavedState, "old state");
tryCloseFileDescriptor(mBackupData, "backup data");
tryCloseFileDescriptor(mNewState, "new state");
- synchronized (mBackupManagerService.getCurrentOpLock()) {
- // TODO: Do we still need this?
- mSavedState = mBackupData = mNewState = null;
- }
+ mSavedState = null;
+ mBackupData = null;
+ mNewState = null;
// For PM metadata (for which applicationInfo is null) there is no agent-bound state.
if (mCurrentPackage.applicationInfo != null) {
diff --git a/services/robotests/src/com/android/server/backup/keyvalue/KeyValueBackupTaskTest.java b/services/robotests/src/com/android/server/backup/keyvalue/KeyValueBackupTaskTest.java
index 82d7ab8..b4bc9d1 100644
--- a/services/robotests/src/com/android/server/backup/keyvalue/KeyValueBackupTaskTest.java
+++ b/services/robotests/src/com/android/server/backup/keyvalue/KeyValueBackupTaskTest.java
@@ -819,7 +819,7 @@
}
@Test
- public void testRunTask_whenAgentOnBackupThrows_updatesAndCleansUpFiles() throws Exception {
+ public void testRunTask_whenAgentOnBackupThrows_updatesFilesAndCleansUp() throws Exception {
TransportMock transportMock = setUpInitializedTransport(mTransport);
AgentMock agentMock = setUpAgent(PACKAGE_1);
remoteAgentOnBackupThrows(
@@ -834,8 +834,7 @@
assertThat(Files.readAllBytes(getStateFile(mTransport, PACKAGE_1)))
.isEqualTo("oldState".getBytes());
- assertThat(Files.exists(getTemporaryStateFile(mTransport, PACKAGE_1))).isFalse();
- assertThat(Files.exists(getStagingFile(PACKAGE_1))).isFalse();
+ assertCleansUpFilesAndAgent(mTransport, PACKAGE_1);
}
@Test
@@ -914,7 +913,7 @@
}
@Test
- public void testRunTask_whenAgentUsesProhibitedKey_updatesAndCleansUpFiles() throws Exception {
+ public void testRunTask_whenAgentUsesProhibitedKey_updatesFilesAndCleansUp() throws Exception {
TransportMock transportMock = setUpInitializedTransport(mTransport);
AgentMock agentMock = setUpAgent(PACKAGE_1);
agentOnBackupDo(
@@ -931,8 +930,7 @@
assertThat(Files.readAllBytes(getStateFile(mTransport, PACKAGE_1)))
.isEqualTo("oldState".getBytes());
- assertThat(Files.exists(getTemporaryStateFile(mTransport, PACKAGE_1))).isFalse();
- assertThat(Files.exists(getStagingFile(PACKAGE_1))).isFalse();
+ assertCleansUpFilesAndAgent(mTransport, PACKAGE_1);
}
@Test
@@ -1094,7 +1092,7 @@
}
@Test
- public void testRunTask_whenAgentDoesNotWriteData_updatesAndCleansUpFiles() throws Exception {
+ public void testRunTask_whenAgentDoesNotWriteData_updatesFilesAndCleansUp() throws Exception {
TransportMock transportMock = setUpInitializedTransport(mTransport);
AgentMock agentMock = setUpAgent(PACKAGE_1);
agentOnBackupDo(
@@ -1107,8 +1105,7 @@
runTask(task);
assertThat(Files.readAllBytes(getStateFile(mTransport, PACKAGE_1))).isEqualTo(new byte[0]);
- assertThat(Files.exists(getTemporaryStateFile(mTransport, PACKAGE_1))).isFalse();
- assertThat(Files.exists(getStagingFile(PACKAGE_1))).isFalse();
+ assertCleansUpFilesAndAgent(mTransport, PACKAGE_1);
}
@Test
@@ -1159,7 +1156,7 @@
}
@Test
- public void testRunTask_whenFinishBackupSucceeds_updatesAndCleansUpFiles() throws Exception {
+ public void testRunTask_whenFinishBackupSucceeds_updatesFilesAndCleansUp() throws Exception {
TransportMock transportMock = setUpInitializedTransport(mTransport);
when(transportMock.transport.finishBackup()).thenReturn(BackupTransport.TRANSPORT_OK);
AgentMock agentMock = setUpAgent(PACKAGE_1);
@@ -1175,8 +1172,7 @@
assertThat(Files.readAllBytes(getStateFile(mTransport, PACKAGE_1)))
.isEqualTo("newState".getBytes());
- assertThat(Files.exists(getTemporaryStateFile(mTransport, PACKAGE_1))).isFalse();
- assertThat(Files.exists(getStagingFile(PACKAGE_1))).isFalse();
+ assertCleansUpFilesAndAgent(mTransport, PACKAGE_1);
}
@Test
@@ -1236,7 +1232,7 @@
}
@Test
- public void testRunTask_whenTransportRejectsPackage_updatesAndCleansUpFiles() throws Exception {
+ public void testRunTask_whenTransportRejectsPackage_updatesFilesAndCleansUp() throws Exception {
TransportMock transportMock = setUpInitializedTransport(mTransport);
when(transportMock.transport.performBackup(
argThat(packageInfo(PACKAGE_1)), any(), anyInt()))
@@ -1249,8 +1245,7 @@
assertThat(Files.readAllBytes(getStateFile(mTransport, PACKAGE_1)))
.isEqualTo("oldState".getBytes());
- assertThat(Files.exists(getTemporaryStateFile(mTransport, PACKAGE_1))).isFalse();
- assertThat(Files.exists(getStagingFile(PACKAGE_1))).isFalse();
+ assertCleansUpFilesAndAgent(mTransport, PACKAGE_1);
}
@Test
@@ -1350,7 +1345,9 @@
runTask(task);
- verify(agentMock.agent).onQuotaExceeded(anyLong(), eq(1234L));
+ InOrder inOrder = inOrder(agentMock.agent, mBackupManagerService);
+ inOrder.verify(agentMock.agent).onQuotaExceeded(anyLong(), eq(1234L));
+ inOrder.verify(mBackupManagerService).unbindAgent(argThat(applicationInfo(PACKAGE_1)));
}
@Test
@@ -1397,8 +1394,7 @@
runTask(task);
- assertThat(Files.exists(getTemporaryStateFile(mTransport, PACKAGE_1))).isFalse();
- assertThat(Files.exists(getStagingFile(PACKAGE_1))).isFalse();
+ assertCleansUpFilesAndAgent(mTransport, PACKAGE_1);
}
@Test
@@ -1413,8 +1409,7 @@
runTask(task);
assertThat(isFileNonEmpty(getStateFile(mTransport, PACKAGE_1))).isFalse();
- assertThat(Files.exists(getTemporaryStateFile(mTransport, PACKAGE_1))).isFalse();
- assertThat(Files.exists(getStagingFile(PACKAGE_1))).isFalse();
+ assertCleansUpFilesAndAgent(mTransport, PACKAGE_1);
}
@Test
@@ -1671,7 +1666,7 @@
}
@Test
- public void testRunTask_whenTransportReturnsError_updatesAndCleansUpFiles() throws Exception {
+ public void testRunTask_whenTransportReturnsError_updatesFilesAndCleansUp() throws Exception {
TransportMock transportMock = setUpInitializedTransport(mTransport);
when(transportMock.transport.performBackup(
argThat(packageInfo(PACKAGE_1)), any(), anyInt()))
@@ -1684,8 +1679,7 @@
assertThat(Files.readAllBytes(getStateFile(mTransport, PACKAGE_1)))
.isEqualTo("oldState".getBytes());
- assertThat(Files.exists(getTemporaryStateFile(mTransport, PACKAGE_1))).isFalse();
- assertThat(Files.exists(getStagingFile(PACKAGE_1))).isFalse();
+ assertCleansUpFilesAndAgent(mTransport, PACKAGE_1);
}
@Test
@@ -1733,20 +1727,54 @@
}
@Test
- public void testRunTask_whenReadingBackupDataThrows() throws Exception {
+ public void testRunTask_whenReadingBackupDataThrows_reportsCorrectly() throws Exception {
TransportMock transportMock = setUpInitializedTransport(mTransport);
setUpAgentWithData(PACKAGE_1);
- AgentMock agentMock = setUpAgentWithData(PACKAGE_2);
- KeyValueBackupTask task = createKeyValueBackupTask(transportMock, PACKAGE_1, PACKAGE_2);
+ KeyValueBackupTask task = createKeyValueBackupTask(transportMock, PACKAGE_1);
// We don't validate PM's data, so it will only throw in PACKAGE_1
ShadowBackupDataInput.throwInNextHeaderRead();
runTask(task);
verify(mReporter).onReadAgentDataError(eq(PACKAGE_1.packageName), any());
+ }
+
+ @Test
+ public void testRunTask_whenReadingBackupDataThrows_doesNotCallTransport() throws Exception {
+ TransportMock transportMock = setUpInitializedTransport(mTransport);
+ setUpAgentWithData(PACKAGE_1);
+ KeyValueBackupTask task = createKeyValueBackupTask(transportMock, PACKAGE_1);
+ ShadowBackupDataInput.throwInNextHeaderRead();
+
+ runTask(task);
+
verify(transportMock.transport, never())
.performBackup(argThat(packageInfo(PACKAGE_1)), any(), anyInt());
+ }
+
+ @Test
+ public void testRunTask_whenReadingBackupDataThrows_doesNotCallSecondAgent() throws Exception {
+ TransportMock transportMock = setUpInitializedTransport(mTransport);
+ setUpAgentWithData(PACKAGE_1);
+ AgentMock agentMock = setUpAgentWithData(PACKAGE_2);
+ KeyValueBackupTask task = createKeyValueBackupTask(transportMock, PACKAGE_1, PACKAGE_2);
+ ShadowBackupDataInput.throwInNextHeaderRead();
+
+ runTask(task);
+
verify(agentMock.agent, never()).onBackup(any(), any(), any());
+ }
+
+ @Test
+ public void testRunTask_whenReadingBackupDataThrows_cleansUpAndRevertsTask() throws Exception {
+ TransportMock transportMock = setUpInitializedTransport(mTransport);
+ setUpAgentsWithData(PACKAGE_1, PACKAGE_2);
+ KeyValueBackupTask task = createKeyValueBackupTask(transportMock, PACKAGE_1, PACKAGE_2);
+ ShadowBackupDataInput.throwInNextHeaderRead();
+
+ runTask(task);
+
+ assertCleansUpFiles(mTransport, PACKAGE_2);
assertTaskReverted(transportMock, PACKAGE_1, PACKAGE_2);
}
@@ -2385,6 +2413,16 @@
assertThat(data1).isEqualTo(value);
}
+ private void assertCleansUpFilesAndAgent(TransportData transport, PackageData packageData) {
+ assertCleansUpFiles(transport, packageData);
+ verify(mBackupManagerService).unbindAgent(argThat(applicationInfo(packageData)));
+ }
+
+ private void assertCleansUpFiles(TransportData transport, PackageData packageData) {
+ assertThat(Files.exists(getTemporaryStateFile(transport, packageData))).isFalse();
+ assertThat(Files.exists(getStagingFile(packageData))).isFalse();
+ }
+
/**
* Put conditions that should *always* be true after task execution.
*