Bug 1392078 - More detailed strong assertions to help narrow down root cause r=rnewman draft
authorGrigory Kruglov <gkruglov@mozilla.com>
Mon, 21 Aug 2017 17:42:44 -0400
changeset 650153 75c197c8af0d0cebeb8ff7506cab61a0b283cea1
parent 650074 a9d372645a32b8d23d44244f351639af9d73b96a
child 727313 c8bf79b7fd230da1b30d65f65f441068fcffdd48
push id75286
push userbmo:gkruglov@mozilla.com
push dateMon, 21 Aug 2017 21:43:22 +0000
reviewersrnewman
bugs1392078
milestone57.0a1
Bug 1392078 - More detailed strong assertions to help narrow down root cause r=rnewman MozReview-Commit-ID: JSN9Q837u6R
mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java
mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/VersioningDelegateHelper.java
--- a/mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java
+++ b/mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java
@@ -2762,17 +2762,32 @@ public class BrowserProvider extends Sha
         int changed = 0;
         try {
             for (String guid : syncVersionsForGuids.keySet()) {
                 final int syncVersion = syncVersionsForGuids.get(guid);
 
                 compiledStatement.clearBindings();
                 compiledStatement.bindLong(1, syncVersion); // NB: 1-based index.
                 compiledStatement.bindString(2, guid);
-                changed += compiledStatement.executeUpdateDelete();
+
+                // We expect this to be 1.
+                final int didUpdate = compiledStatement.executeUpdateDelete();
+
+                // These strong assertions are here to help figure out root cause of Bug 1392078.
+                // This will throw if there are duplicate GUIDs present.
+                if (didUpdate > 1) {
+                    throw new IllegalStateException("Modified more than a single GUID during syncVersion update");
+                }
+
+                // This will throw if the requested GUID is missing from the database.
+                if (didUpdate == 0) {
+                    throw new IllegalStateException("Expected to modify syncVersion for a guid, but did not");
+                }
+
+                changed += didUpdate;
             }
 
             markWriteSuccessful(db);
 
         } finally {
             endWrite(db);
         }
 
--- a/mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/VersioningDelegateHelper.java
+++ b/mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/VersioningDelegateHelper.java
@@ -85,34 +85,49 @@ public class VersioningDelegateHelper {
     public RepositorySessionStoreDelegate getStoreDelegate(RepositorySessionStoreDelegate delegate) {
         return new VersionedStoreDelegate(delegate);
     }
 
     public void persistSyncVersions() {
         // At this point we can be sure that all records in our guidsMap have been successfully
         // uploaded. Move syncVersions forward for each GUID en masse.
         final Bundle data = new Bundle();
-        data.putSerializable(BrowserContract.METHOD_PARAM_DATA, this.localVersionsOfGuids);
+        final int versionMapSizeBeforeUpdate;
+
+        synchronized (this.localVersionsOfGuids) {
+            data.putSerializable(BrowserContract.METHOD_PARAM_DATA, this.localVersionsOfGuids);
+            versionMapSizeBeforeUpdate = this.localVersionsOfGuids.size();
+        }
 
         final Bundle result = context.getContentResolver().call(
                 repositoryContentUri,
                 BrowserContract.METHOD_UPDATE_SYNC_VERSIONS,
                 repositoryContentUri.toString(),
                 data
         );
 
         if (result == null) {
             throw new IllegalStateException("Expected to receive a result after decrementing change counters");
         }
 
-        int changed = (int) result.getSerializable(BrowserContract.METHOD_RESULT);
-        if (changed != this.localVersionsOfGuids.size()) {
-            // TODO perhaps not worth throwing, but this shouldn't happen either...
-            throw new IllegalStateException("Decremented incorrect number of change counters");
+        final int changed = (int) result.getSerializable(BrowserContract.METHOD_RESULT);
+        final int versionMapSizeAfterUpdate = this.localVersionsOfGuids.size();
 
+        // None of these should happen, but something's clearly amiss. These strong assertions are
+        // here to help figure out root cause for Bug 1392078.
+        if (versionMapSizeBeforeUpdate != versionMapSizeAfterUpdate) {
+            throw new IllegalStateException("Version/guid map changed during syncVersion update");
+        }
+
+        if (changed < versionMapSizeBeforeUpdate) {
+            throw new IllegalStateException("Updated fewer sync versions than expected");
+        }
+
+        if (changed > versionMapSizeBeforeUpdate) {
+            throw new IllegalStateException("Updated more sync versions than expected");
         }
     }
 
     private class VersionedFetchDelegate implements RepositorySessionFetchRecordsDelegate {
         private final RepositorySessionFetchRecordsDelegate inner;
 
         VersionedFetchDelegate(RepositorySessionFetchRecordsDelegate delegate) {
             this.inner = delegate;