Bug 1345754 - Skip sync bookmark repair and validation if we have pending changes r?markh draft
authorThom Chiovoloni <tchiovoloni@mozilla.com>
Tue, 14 Mar 2017 14:26:20 -0400
changeset 498434 5931f22cbc4e06a8aa37bafc894050dc2ce5a77e
parent 498376 8afe7f683eaa6e449c2399e994e78932f20d5e0a
child 549149 98cc4e7d2f3b0d5523e6eae7addfc141b9ecb895
push id49181
push userbmo:tchiovoloni@mozilla.com
push dateTue, 14 Mar 2017 18:27:01 +0000
reviewersmarkh
bugs1345754
milestone55.0a1
Bug 1345754 - Skip sync bookmark repair and validation if we have pending changes r?markh MozReview-Commit-ID: ClQRXGZGV9p
services/sync/modules/bookmark_validator.js
services/sync/modules/collection_validator.js
services/sync/modules/doctor.js
services/sync/tests/unit/test_doctor.js
toolkit/components/places/PlacesSyncUtils.jsm
--- a/services/sync/modules/bookmark_validator.js
+++ b/services/sync/modules/bookmark_validator.js
@@ -217,16 +217,20 @@ XPCOMUtils.defineLazyGetter(this, "SYNCE
   PlacesUtils.bookmarks.menuGuid,
   PlacesUtils.bookmarks.toolbarGuid,
   PlacesUtils.bookmarks.unfiledGuid,
   PlacesUtils.bookmarks.mobileGuid,
 ]);
 
 class BookmarkValidator {
 
+  async canValidate() {
+    return !await PlacesSyncUtils.bookmarks.havePendingChanges();
+  }
+
   _followQueries(recordMap) {
     for (let [guid, entry] of recordMap) {
       if (entry.type !== "query" && (!entry.bmkUri || !entry.bmkUri.startsWith(QUERY_PROTOCOL))) {
         continue;
       }
       // Might be worth trying to parse the place: query instead so that this
       // works "automatically" with things like aboutsync.
       let queryNodeParent = PlacesUtils.getFolderContents(entry, false, true);
--- a/services/sync/modules/collection_validator.js
+++ b/services/sync/modules/collection_validator.js
@@ -77,16 +77,25 @@ class CollectionValidator {
     return items;
   }
 
   // Should return a promise that resolves to an array of client items.
   getClientItems() {
     return Promise.reject("Must implement");
   }
 
+  /**
+   * Can we guarantee validation will fail with a reason that isn't actually a
+   * problem? For example, if we know there are pending changes left over from
+   * the last sync, this should resolve to false. By default resolves to true.
+   */
+  async canValidate() {
+    return true;
+  }
+
   // Turn the client item into something that can be compared with the server item,
   // and is also safe to mutate.
   normalizeClientItem(item) {
     return Cu.cloneInto(item, {});
   }
 
   // Turn the server item into something that can be easily compared with the client
   // items.
--- a/services/sync/modules/doctor.js
+++ b/services/sync/modules/doctor.js
@@ -153,16 +153,21 @@ this.Doctor = {
                         `than the maximum allowed (${maxRecords}).`);
         continue;
       }
       let validator = engine.getValidator();
       if (!validator) {
         continue;
       }
 
+      if (!await validator.canValidate()) {
+        log.debug(`Skipping validation for ${engineName} because validator.canValidate() is false`);
+        continue;
+      }
+
       // Let's do it!
       Services.console.logStringMessage(
         `Sync is about to run a consistency check of ${engine.name}. This may be slow, and ` +
         `can be controlled using the pref "services.sync.${engine.name}.validation.enabled".\n` +
         `If you encounter any problems because of this, please file a bug.`);
 
       // Make a new flowID just incase we end up starting a repair.
       let flowID = Utils.makeGUID();
--- a/services/sync/tests/unit/test_doctor.js
+++ b/services/sync/tests/unit/test_doctor.js
@@ -13,16 +13,19 @@ function mockDoctor(mocks) {
 add_task(async function test_repairs_start() {
   let repairStarted = false;
   let problems = {
     missingChildren: ["a", "b", "c"],
   }
   let validator = {
     validate(engine) {
       return problems;
+    },
+    canValidate() {
+      return Promise.resolve(true);
     }
   }
   let engine = {
     name: "test-engine",
     getValidator() {
       return validator;
     }
   }
@@ -87,8 +90,43 @@ add_task(async function test_repairs_adv
   // should not have done another repair yet - it's too soon.
   equal(repairCalls, 1);
   // advance our pretend clock by the advance period (eg, day)
   now += REPAIR_ADVANCE_PERIOD;
   await doctor.consult();
   // should have done another repair
   equal(repairCalls, 2);
 });
+
+add_task(async function test_repairs_skip_if_cant_vaidate() {
+  let validator = {
+    canValidate() {
+      return Promise.resolve(false);
+    },
+    validate() {
+      ok(false, "Shouldn't validate");
+    }
+  }
+  let engine = {
+    name: "test-engine",
+    getValidator() {
+      return validator;
+    }
+  }
+  let requestor = {
+    startRepairs(validationInfo, flowID) {
+      assert.ok(false, "Never should start repairs");
+    }
+  }
+  let doctor = mockDoctor({
+    _getEnginesToValidate(recentlySyncedEngines) {
+      deepEqual(recentlySyncedEngines, [engine]);
+      return {
+        "test-engine": { engine, maxRecords: -1 }
+      };
+    },
+    _getRepairRequestor(engineName) {
+      equal(engineName, engine.name);
+      return requestor;
+    },
+  });
+  await doctor.consult([engine]);
+});
--- a/toolkit/components/places/PlacesSyncUtils.jsm
+++ b/toolkit/components/places/PlacesSyncUtils.jsm
@@ -294,16 +294,26 @@ const BookmarkSyncUtils = PlacesSyncUtil
       return undefined;
     }
     let orderedChildrenGuids = childSyncIds.map(BookmarkSyncUtils.syncIdToGuid);
     return PlacesUtils.bookmarks.reorder(parentGuid, orderedChildrenGuids,
                                          { source: SOURCE_SYNC });
   }),
 
   /**
+   * Resolves to true if there are known sync changes.
+   */
+  havePendingChanges() {
+    // This could be optimized to use a more efficient query -- We don't need
+    // grab all the records if all we care about is whether or not any exist.
+    return PlacesUtils.withConnectionWrapper("BookmarkSyncUtils: havePendingChanges",
+      db => pullSyncChanges(db, true).then(changes => Object.keys(changes).length > 0));
+  },
+
+  /**
    * Returns a changeset containing local bookmark changes since the last sync.
    * Updates the sync status of all "NEW" bookmarks to "NORMAL", so that Sync
    * can recover correctly after an interrupted sync.
    *
    * @return {Promise} resolved once all items have been fetched.
    * @resolves to an object containing records for changed bookmarks, keyed by
    *           the sync ID.
    * @see pullSyncChanges for the implementation, and markChangesAsSyncing for
@@ -1682,21 +1692,23 @@ function addRowToChangeRecords(row, chan
 
 /**
  * Queries the database for synced bookmarks and tombstones, updates the sync
  * status of all "NEW" bookmarks to "NORMAL", and returns a changeset for the
  * Sync bookmarks engine.
  *
  * @param db
  *        The Sqlite.jsm connection handle.
+ * @param preventUpdate {boolean}
+ *        Should we skip updating the records we pull.
  * @return {Promise} resolved once all items have been fetched.
  * @resolves to an object containing records for changed bookmarks, keyed by
  *           the sync ID.
  */
-var pullSyncChanges = Task.async(function* (db) {
+var pullSyncChanges = Task.async(function* (db, preventUpdate = false) {
   let changeRecords = {};
 
   yield db.executeCached(`
     WITH RECURSIVE
     syncedItems(id, guid, modified, syncChangeCounter, syncStatus) AS (
       SELECT b.id, b.guid, b.lastModified, b.syncChangeCounter, b.syncStatus
        FROM moz_bookmarks b
        WHERE b.guid IN ('menu________', 'toolbar_____', 'unfiled_____',
@@ -1711,17 +1723,19 @@ var pullSyncChanges = Task.async(functio
     WHERE syncChangeCounter >= 1
     UNION ALL
     SELECT guid, dateRemoved AS modified, 1 AS syncChangeCounter,
            :deletedSyncStatus, 1 AS tombstone
     FROM moz_bookmarks_deleted`,
     { deletedSyncStatus: PlacesUtils.bookmarks.SYNC_STATUS.NORMAL },
     row => addRowToChangeRecords(row, changeRecords));
 
-  yield markChangesAsSyncing(db, changeRecords);
+  if (!preventUpdate) {
+    yield markChangesAsSyncing(db, changeRecords);
+  }
 
   return changeRecords;
 });
 
 var touchSyncBookmark = Task.async(function* (db, bookmarkItem) {
   if (BookmarkSyncLog.level <= Log.Level.Trace) {
     BookmarkSyncLog.trace(
       `touch: Reviving item "${bookmarkItem.guid}" and marking parent ` +