From 308a20e9b235e9fbd756b3b75ff56310783efebb Mon Sep 17 00:00:00 2001 From: Thom Chiovoloni Date: Tue, 17 Oct 2017 15:33:34 -0400 Subject: [PATCH] Bug 1409472 - Replace always-wrong instanceof check with an explicit boolean in BatchingUploader. r=Grisha MozReview-Commit-ID: JmJmMwseH3m --HG-- extra : rebase_source : 8192222332d46fa1f5f791240b936c2d27e42c81 --- .../ConfigurableServer15Repository.java | 10 +++++++- .../sync/repositories/Server15Repository.java | 4 ++++ .../Server15RepositorySession.java | 2 +- .../uploaders/BatchingUploader.java | 24 ++++++++++--------- .../sync/stage/BookmarksServerSyncStage.java | 3 ++- .../stage/FormHistoryServerSyncStage.java | 1 + .../sync/stage/HistoryServerSyncStage.java | 1 + .../stage/RecentHistoryServerSyncStage.java | 4 +++- .../stage/ValidateBookmarksSyncStage.java | 3 ++- .../uploaders/BatchingUploaderTest.java | 9 +------ .../uploaders/PayloadUploadDelegateTest.java | 3 ++- 11 files changed, 39 insertions(+), 25 deletions(-) diff --git a/mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/ConfigurableServer15Repository.java b/mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/ConfigurableServer15Repository.java index 12f7623e6e7e..ab65cfe7d626 100644 --- a/mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/ConfigurableServer15Repository.java +++ b/mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/ConfigurableServer15Repository.java @@ -28,6 +28,7 @@ public class ConfigurableServer15Repository extends Server15Repository { private final ServerSyncStage.MultipleBatches multipleBatches; private final ServerSyncStage.HighWaterMark highWaterMark; private final boolean forceFullFetch; + private final boolean abortOnStoreFailure; public ConfigurableServer15Repository( String collection, long syncDeadline, @@ -40,7 +41,8 @@ public class ConfigurableServer15Repository extends Server15Repository { ServerSyncStage.MultipleBatches multipleBatches, ServerSyncStage.HighWaterMark highWaterMark, RepositoryStateProvider stateProvider, - boolean forceFullFetch) throws URISyntaxException { + boolean forceFullFetch, + boolean abortOnStoreFailure) throws URISyntaxException { super( collection, syncDeadline, @@ -55,6 +57,7 @@ public class ConfigurableServer15Repository extends Server15Repository { this.multipleBatches = multipleBatches; this.highWaterMark = highWaterMark; this.forceFullFetch = forceFullFetch; + this.abortOnStoreFailure = abortOnStoreFailure; // Sanity check: let's ensure we're configured correctly. At this point in time, it doesn't make // sense to use H.W.M. with a non-persistent state provider. This might change if we start retrying @@ -89,4 +92,9 @@ public class ConfigurableServer15Repository extends Server15Repository { return forceFullFetch; } + @Override + public boolean getAbortOnStoreFailure() { + return abortOnStoreFailure; + } + } diff --git a/mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/Server15Repository.java b/mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/Server15Repository.java index 20aee71ea8aa..b0f0aaabc3c8 100644 --- a/mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/Server15Repository.java +++ b/mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/Server15Repository.java @@ -115,6 +115,10 @@ public class Server15Repository extends Repository { return false; } + public boolean getAbortOnStoreFailure() { + return false; + } + /** * A point in time by which this repository's session must complete fetch and store operations. * Particularly pertinent for batching downloads performed by the session (should we fetch diff --git a/mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/Server15RepositorySession.java b/mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/Server15RepositorySession.java index 0b886ed85e4c..56a55cbee022 100644 --- a/mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/Server15RepositorySession.java +++ b/mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/Server15RepositorySession.java @@ -43,7 +43,7 @@ public class Server15RepositorySession extends RepositorySession { this.uploader = new BatchingUploader( this, storeWorkQueue, storeDelegate, Uri.parse(serverRepository.collectionURI.toString()), serverRepository.getCollectionLastModified(), serverRepository.getInfoConfiguration(), - serverRepository.authHeaderProvider); + serverRepository.authHeaderProvider, serverRepository.getAbortOnStoreFailure()); } private void fetchSince(long timestamp, RepositorySessionFetchRecordsDelegate delegate) { diff --git a/mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/uploaders/BatchingUploader.java b/mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/uploaders/BatchingUploader.java index 685d9d1303e4..5d963b6fc30d 100644 --- a/mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/uploaders/BatchingUploader.java +++ b/mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/uploaders/BatchingUploader.java @@ -105,15 +105,23 @@ public class BatchingUploader { // Set if this channel should ignore further calls to process. private volatile boolean aborted = false; + // Whether or not we should set aborted if there are any issues with the record. + // This is used to prevent corruption with bookmark records, as uploading + // only a subset of the bookmarks is very likely to cause corruption, (e.g. + // uploading a parent without its children or vice versa). + @VisibleForTesting + protected final boolean shouldFailBatchOnFailure; + public BatchingUploader( final RepositorySession repositorySession, final ExecutorService workQueue, final RepositorySessionStoreDelegate sessionStoreDelegate, final Uri baseCollectionUri, final Long localCollectionLastModified, final InfoConfiguration infoConfiguration, - final AuthHeaderProvider authHeaderProvider) { + final AuthHeaderProvider authHeaderProvider, final boolean shouldAbortOnFailure) { this.repositorySession = repositorySession; this.sessionStoreDelegate = sessionStoreDelegate; this.collectionUri = baseCollectionUri; this.authHeaderProvider = authHeaderProvider; + this.shouldFailBatchOnFailure = shouldAbortOnFailure; this.uploaderMeta = new UploaderMeta( payloadLock, infoConfiguration.maxTotalBytes, infoConfiguration.maxTotalRecords); @@ -233,12 +241,6 @@ public class BatchingUploader { }); } - // We fail the batch for bookmark records because uploading only a subset of bookmark records is - // very likely to cause corruption (e.g. uploading a parent without its children or vice versa). - @VisibleForTesting - /* package-local */ boolean shouldFailBatchOnFailure(Record record) { - return record instanceof BookmarkRecord; - } /* package-local */ void setLastStoreTimestamp(AtomicLong lastModifiedTimestamp) { repositorySession.setLastStoreTimestamp(lastModifiedTimestamp.get()); @@ -251,14 +253,14 @@ public class BatchingUploader { // Common handling for marking a record failure and calling our delegate's onRecordStoreFailed. private void failRecordStore(final Exception e, final Record record, boolean sizeOverflow) { // There are three cases we're handling here. See bug 1362206 for some rationale here. - // 1. If `record` is not a bookmark and it failed sanity checks for reasons other than + // 1. shouldFailBatchOnFailure is false, and it failed sanity checks for reasons other than // "it's too large" (say, `record`'s json is 0 bytes), // - Then mark record's store as 'failed' and continue uploading - // 2. If `record` is not a bookmark and it failed sanity checks because it's too large, + // 2. shouldFailBatchOnFailure is false, and it failed sanity checks because it's too large, // - Continue uploading, and don't fail synchronization because of this one. - // 3. If `record` *is* a bookmark, and it failed for any reason + // 3. shouldFailBatchOnFailure is true, and it failed for any reason // - Stop uploading. - if (shouldFailBatchOnFailure(record)) { + if (shouldFailBatchOnFailure) { // case 3 Logger.debug(LOG_TAG, "Batch failed with exception: " + e.toString()); // Start ignoring records, and send off to our delegate that we failed. diff --git a/mobile/android/services/src/main/java/org/mozilla/gecko/sync/stage/BookmarksServerSyncStage.java b/mobile/android/services/src/main/java/org/mozilla/gecko/sync/stage/BookmarksServerSyncStage.java index f4a7cb50274d..b341fb6c31fc 100644 --- a/mobile/android/services/src/main/java/org/mozilla/gecko/sync/stage/BookmarksServerSyncStage.java +++ b/mobile/android/services/src/main/java/org/mozilla/gecko/sync/stage/BookmarksServerSyncStage.java @@ -82,7 +82,8 @@ public class BookmarksServerSyncStage extends VersionedServerSyncStage { getAllowedMultipleBatches(), getAllowedToUseHighWaterMark(), getRepositoryStateProvider(), - false + false, + true ); } diff --git a/mobile/android/services/src/main/java/org/mozilla/gecko/sync/stage/FormHistoryServerSyncStage.java b/mobile/android/services/src/main/java/org/mozilla/gecko/sync/stage/FormHistoryServerSyncStage.java index 2b5cd2efa248..274160df97ca 100644 --- a/mobile/android/services/src/main/java/org/mozilla/gecko/sync/stage/FormHistoryServerSyncStage.java +++ b/mobile/android/services/src/main/java/org/mozilla/gecko/sync/stage/FormHistoryServerSyncStage.java @@ -76,6 +76,7 @@ public class FormHistoryServerSyncStage extends ServerSyncStage { getAllowedMultipleBatches(), getAllowedToUseHighWaterMark(), getRepositoryStateProvider(), + false, false ); } diff --git a/mobile/android/services/src/main/java/org/mozilla/gecko/sync/stage/HistoryServerSyncStage.java b/mobile/android/services/src/main/java/org/mozilla/gecko/sync/stage/HistoryServerSyncStage.java index b2d981a7e7c5..94ce1c8cee13 100644 --- a/mobile/android/services/src/main/java/org/mozilla/gecko/sync/stage/HistoryServerSyncStage.java +++ b/mobile/android/services/src/main/java/org/mozilla/gecko/sync/stage/HistoryServerSyncStage.java @@ -94,6 +94,7 @@ public class HistoryServerSyncStage extends ServerSyncStage { getAllowedMultipleBatches(), getAllowedToUseHighWaterMark(), getRepositoryStateProvider(), + false, false ); } diff --git a/mobile/android/services/src/main/java/org/mozilla/gecko/sync/stage/RecentHistoryServerSyncStage.java b/mobile/android/services/src/main/java/org/mozilla/gecko/sync/stage/RecentHistoryServerSyncStage.java index fae8d5fd7526..f9303562efca 100644 --- a/mobile/android/services/src/main/java/org/mozilla/gecko/sync/stage/RecentHistoryServerSyncStage.java +++ b/mobile/android/services/src/main/java/org/mozilla/gecko/sync/stage/RecentHistoryServerSyncStage.java @@ -95,7 +95,9 @@ public class RecentHistoryServerSyncStage extends HistoryServerSyncStage { getAllowedMultipleBatches(), getAllowedToUseHighWaterMark(), getRepositoryStateProvider(), - false); + false, + false + ); } /** diff --git a/mobile/android/services/src/main/java/org/mozilla/gecko/sync/stage/ValidateBookmarksSyncStage.java b/mobile/android/services/src/main/java/org/mozilla/gecko/sync/stage/ValidateBookmarksSyncStage.java index 86a104579da4..5cd4aefbf88d 100644 --- a/mobile/android/services/src/main/java/org/mozilla/gecko/sync/stage/ValidateBookmarksSyncStage.java +++ b/mobile/android/services/src/main/java/org/mozilla/gecko/sync/stage/ValidateBookmarksSyncStage.java @@ -116,7 +116,8 @@ public class ValidateBookmarksSyncStage extends ServerSyncStage { getAllowedMultipleBatches(), getAllowedToUseHighWaterMark(), getRepositoryStateProvider(), - true + true, + false // We never do any storing, so this is irrelevant ); } diff --git a/mobile/android/tests/background/junit4/src/org/mozilla/gecko/sync/repositories/uploaders/BatchingUploaderTest.java b/mobile/android/tests/background/junit4/src/org/mozilla/gecko/sync/repositories/uploaders/BatchingUploaderTest.java index 8423d029d432..1e68451e8926 100644 --- a/mobile/android/tests/background/junit4/src/org/mozilla/gecko/sync/repositories/uploaders/BatchingUploaderTest.java +++ b/mobile/android/tests/background/junit4/src/org/mozilla/gecko/sync/repositories/uploaders/BatchingUploaderTest.java @@ -611,25 +611,18 @@ public class BatchingUploaderTest { } class MockUploader extends BatchingUploader { - boolean abortOnRecordFail; MockUploader(final RepositorySession repositorySession, final ExecutorService workQueue, final RepositorySessionStoreDelegate sessionStoreDelegate, final Uri baseCollectionUri, final Long localCollectionLastModified, final InfoConfiguration infoConfiguration, final AuthHeaderProvider authHeaderProvider, final boolean abortOnRecordFail) { super(repositorySession, workQueue, sessionStoreDelegate, baseCollectionUri, - localCollectionLastModified, infoConfiguration, authHeaderProvider); - this.abortOnRecordFail = abortOnRecordFail; + localCollectionLastModified, infoConfiguration, authHeaderProvider, abortOnRecordFail); } @Override PayloadDispatcher createPayloadDispatcher(ExecutorService workQueue, Long localCollectionLastModified) { return new MockPayloadDispatcher(workQueue, this, localCollectionLastModified); } - - @Override - boolean shouldFailBatchOnFailure(Record r) { - return abortOnRecordFail; - } } private Server15Repository makeConstrainedRepository(boolean firstSync) { diff --git a/mobile/android/tests/background/junit4/src/org/mozilla/gecko/sync/repositories/uploaders/PayloadUploadDelegateTest.java b/mobile/android/tests/background/junit4/src/org/mozilla/gecko/sync/repositories/uploaders/PayloadUploadDelegateTest.java index 9f90b60d76e1..ea730b1314f8 100644 --- a/mobile/android/tests/background/junit4/src/org/mozilla/gecko/sync/repositories/uploaders/PayloadUploadDelegateTest.java +++ b/mobile/android/tests/background/junit4/src/org/mozilla/gecko/sync/repositories/uploaders/PayloadUploadDelegateTest.java @@ -142,7 +142,8 @@ public class PayloadUploadDelegateTest { Uri.parse("https://example.com"), 0L, mock(InfoConfiguration.class), - mock(AuthHeaderProvider.class) + mock(AuthHeaderProvider.class), + false ) );