From eb6dabcdde47d20ba87a792e3349931f834a3348 Mon Sep 17 00:00:00 2001 From: Ruslan Tkhakokhov Date: Mon, 12 Nov 2018 11:09:04 +0000 Subject: [PATCH] KV] Don't set current token if no data was moved Bug: 119299848 Test: 1) atest KeyValueBackupTaskTest 2) Manual: - Run 'adb shell bmgr init' for the active transport to wipe all backup data - Run 'adb shell dumpsys backup' and verify string 'Current: 0', i.e. the current token is set to 0 - Create a test app implementing a BackupAgent that writes no data in onBackup() and install it on the device - Run 'adb shell bmgr backupnow --non-incremental ' to initiate a backup, where --non-incremental flag makes sure PM is not added to the backup queue - Run 'adb shell dumpsys backup' and verify string 'Current: 0' again Change-Id: I595bea9874fd84d0c81b32a509c970a1b142872c --- .../backup/keyvalue/KeyValueBackupTask.java | 14 ++++-- .../keyvalue/KeyValueBackupTaskTest.java | 45 +++++++++++++++++-- 2 files changed, 52 insertions(+), 7 deletions(-) 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 3a5232a6b97a8..d6f2a8775518b 100644 --- a/services/backup/java/com/android/server/backup/keyvalue/KeyValueBackupTask.java +++ b/services/backup/java/com/android/server/backup/keyvalue/KeyValueBackupTask.java @@ -260,6 +260,10 @@ public class KeyValueBackupTask implements BackupRestoreTask, Runnable { @Nullable private ParcelFileDescriptor mSavedState; @Nullable private ParcelFileDescriptor mBackupData; @Nullable private ParcelFileDescriptor mNewState; + // Indicates whether there was any data to be backed up, i.e. the queue was not empty + // and at least one of the packages had data. Used to avoid updating current token for + // empty backups. + private boolean mHasDataToBackup; /** * This {@link ConditionVariable} is used to signal that the cancel operation has been @@ -332,6 +336,8 @@ public class KeyValueBackupTask implements BackupRestoreTask, Runnable { public void run() { Process.setThreadPriority(THREAD_PRIORITY); + mHasDataToBackup = false; + int status = BackupTransport.TRANSPORT_OK; try { startTask(); @@ -529,10 +535,10 @@ public class KeyValueBackupTask implements BackupRestoreTask, Runnable { String callerLogString = "KVBT.finishTask()"; - // If we succeeded and this is the first time we've done a backup, we can record the current - // backup dataset token. + // If the backup data was not empty, we succeeded and this is the first time + // we've done a backup, we can record the current backup dataset token. long currentToken = mBackupManagerService.getCurrentToken(); - if ((status == BackupTransport.TRANSPORT_OK) && (currentToken == 0)) { + if (mHasDataToBackup && (status == BackupTransport.TRANSPORT_OK) && (currentToken == 0)) { try { IBackupTransport transport = mTransportClient.connectOrThrow(callerLogString); mBackupManagerService.setCurrentToken(transport.getCurrentRestoreSet()); @@ -838,6 +844,8 @@ public class KeyValueBackupTask implements BackupRestoreTask, Runnable { return BackupTransport.TRANSPORT_OK; } + mHasDataToBackup = true; + int status; try (ParcelFileDescriptor backupData = ParcelFileDescriptor.open(backupDataFile, MODE_READ_ONLY)) { 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 ba1d83ee2a53c..44986a0136f3e 100644 --- a/services/robotests/src/com/android/server/backup/keyvalue/KeyValueBackupTaskTest.java +++ b/services/robotests/src/com/android/server/backup/keyvalue/KeyValueBackupTaskTest.java @@ -25,9 +25,12 @@ import static android.app.backup.BackupManager.SUCCESS; import static android.app.backup.ForwardingBackupAgent.forward; import static com.android.server.backup.testing.BackupManagerServiceTestUtils.createBackupWakeLock; -import static com.android.server.backup.testing.BackupManagerServiceTestUtils.createInitializedBackupManagerService; -import static com.android.server.backup.testing.BackupManagerServiceTestUtils.setUpBackupManagerServiceBasics; -import static com.android.server.backup.testing.BackupManagerServiceTestUtils.setUpBinderCallerAndApplicationAsSystem; +import static com.android.server.backup.testing.BackupManagerServiceTestUtils + .createInitializedBackupManagerService; +import static com.android.server.backup.testing.BackupManagerServiceTestUtils + .setUpBackupManagerServiceBasics; +import static com.android.server.backup.testing.BackupManagerServiceTestUtils + .setUpBinderCallerAndApplicationAsSystem; import static com.android.server.backup.testing.PackageData.PM_PACKAGE; import static com.android.server.backup.testing.PackageData.fullBackupPackage; import static com.android.server.backup.testing.PackageData.keyValuePackage; @@ -62,8 +65,8 @@ import static org.mockito.Mockito.verifyZeroInteractions; import static org.mockito.Mockito.when; import static org.robolectric.Shadows.shadowOf; import static org.robolectric.shadow.api.Shadow.extract; -import static org.testng.Assert.fail; import static org.testng.Assert.expectThrows; +import static org.testng.Assert.fail; import static java.nio.file.StandardCopyOption.REPLACE_EXISTING; import static java.util.Collections.emptyList; @@ -332,6 +335,22 @@ public class KeyValueBackupTaskTest { .isEqualTo("packageState".getBytes()); } + /** + * Do not update backup token if the backup queue was empty + */ + @Test + public void testRunTask_whenQueueEmptyOnFirstBackup_doesNotUpdateCurrentToken() + throws Exception { + TransportMock transportMock = setUpInitializedTransport(mTransport); + KeyValueBackupTask task = createKeyValueBackupTask(transportMock, true); + mBackupManagerService.setCurrentToken(0L); + when(transportMock.transport.getCurrentRestoreSet()).thenReturn(1234L); + + runTask(task); + + assertThat(mBackupManagerService.getCurrentToken()).isEqualTo(0L); + } + @Test public void testRunTask_whenOnePackageAndTransportUnavailable() throws Exception { TransportMock transportMock = setUpInitializedTransport(mTransport.unavailable()); @@ -2302,6 +2321,24 @@ public class KeyValueBackupTaskTest { expectThrows(IllegalArgumentException.class, () -> task.handleCancel(false)); } + /** + * Do not update backup token if no data was moved. + */ + @Test + public void testRunTask_whenNoDataToBackupOnFirstBackup_doesNotUpdateCurrentToken() + throws Exception { + TransportMock transportMock = setUpInitializedTransport(mTransport); + mBackupManagerService.setCurrentToken(0L); + when(transportMock.transport.getCurrentRestoreSet()).thenReturn(1234L); + // Set up agent with no data. + setUpAgent(PACKAGE_1); + KeyValueBackupTask task = createKeyValueBackupTask(transportMock, true, PACKAGE_1); + + runTask(task); + + assertThat(mBackupManagerService.getCurrentToken()).isEqualTo(0L); + } + private void runTask(KeyValueBackupTask task) { // Pretend we are not on the main-thread to prevent RemoteCall from complaining mShadowMainLooper.setCurrentThread(false);