Bug 1345754 - Skip sync bookmark repair and validation if we have pending changes r?markh
MozReview-Commit-ID: ClQRXGZGV9p
--- 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 ` +