Bug 1345754 - Skip sync bookmark repair and validation if we have pending changes r=markh
authorThom Chiovoloni <tchiovoloni@mozilla.com>
Tue, 14 Mar 2017 14:26:20 -0400
changeset 399188 85561efb5a00464fd5c7c7b708323005fc86461a
parent 399187 4ddb3af7cdf809ba0d7d27b62805096d9e870007
child 399189 fc61bbc304d73ccefdb9462fd071bd4320fd805a
push id1490
push usermtabara@mozilla.com
push dateMon, 31 Jul 2017 14:08:16 +0000
treeherdermozilla-release@70e32e6bf15e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmarkh
bugs1345754
milestone55.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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
@@ -60,16 +60,19 @@ add_task(async function test_validation_
 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;
     }
   }
@@ -134,8 +137,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 ` +