Bug 1409472 - Replace always-wrong instanceof check with an explicit boolean in BatchingUploader. r=Grisha

MozReview-Commit-ID: JmJmMwseH3m

--HG--
extra : rebase_source : 8192222332d46fa1f5f791240b936c2d27e42c81

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;
+  }
+
 }

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

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

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.

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

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

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

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

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

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

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