Bug 1199077 - Sync changes to store the bookmarks sync ID and last sync time in Places. r=tcsc
MozReview-Commit-ID: 6ddzpNtMmzb
--- a/services/sync/modules/engines/bookmarks.js
+++ b/services/sync/modules/engines/bookmarks.js
@@ -316,16 +316,83 @@ BaseBookmarksEngine.prototype = {
_recordObj: PlacesItem,
_trackerObj: BookmarksTracker,
version: 2,
_defaultSort: "index",
syncPriority: 4,
allowSkippedRecord: false,
+ _migratedSyncMetadata: false,
+ async _migrateSyncMetadata({ migrateLastSync = true } = {}) {
+ if (this._migratedSyncMetadata) {
+ return;
+ }
+ let shouldWipeRemote = await PlacesSyncUtils.bookmarks.shouldWipeRemote();
+ if (!shouldWipeRemote) {
+ // Migrate the bookmarks sync ID and last sync time from prefs, to avoid
+ // triggering a full sync on upgrade. This can be removed in bug 1443021.
+ let existingSyncID = await super.getSyncID();
+ if (existingSyncID) {
+ this._log.debug("Migrating existing sync ID ${existingSyncID} from " +
+ "prefs", { existingSyncID });
+ await PlacesSyncUtils.bookmarks.ensureCurrentSyncId(existingSyncID);
+ }
+ if (migrateLastSync) {
+ let existingLastSync = await super.getLastSync();
+ if (existingLastSync) {
+ this._log.debug("Migrating existing last sync time " +
+ "${existingLastSync} from prefs",
+ { existingLastSync });
+ await PlacesSyncUtils.bookmarks.setLastSync(existingLastSync);
+ }
+ }
+ }
+ this._migratedSyncMetadata = true;
+ },
+
+ async ensureCurrentSyncID(newSyncID) {
+ let shouldWipeRemote = await PlacesSyncUtils.bookmarks.shouldWipeRemote();
+ if (!shouldWipeRemote) {
+ this._log.debug("Checking if server sync ID ${newSyncID} matches " +
+ "existing", { newSyncID });
+ await PlacesSyncUtils.bookmarks.ensureCurrentSyncId(newSyncID);
+ // Update the sync ID in prefs to allow downgrading to older Firefox
+ // releases that don't store Sync metadata in Places. This can be removed
+ // in bug 1443021.
+ super.setSyncIDPref(newSyncID);
+ return newSyncID;
+ }
+ // We didn't take the new sync ID because we need to wipe the server
+ // and other clients after a restore. Send the command, wipe the
+ // server, and reset our sync ID to reupload everything.
+ this._log.debug("Ignoring server sync ID ${newSyncID} after restore; " +
+ "wiping server and resetting sync ID", { newSyncID });
+ await this.service.clientsEngine.sendCommand("wipeEngine", [this.name],
+ null, { reason: "bookmark-restore" });
+ let assignedSyncID = await this.resetSyncID();
+ return assignedSyncID;
+ },
+
+ async resetSyncID() {
+ await this._deleteServerCollection();
+ return this.resetLocalSyncID();
+ },
+
+ async resetLocalSyncID() {
+ let newSyncID = await PlacesSyncUtils.bookmarks.resetSyncId();
+ this._log.debug("Assigned new sync ID ${newSyncID}", { newSyncID });
+ super.setSyncIDPref(newSyncID); // Remove in bug 1443021.
+ return newSyncID;
+ },
+
+ setSyncIDPref(syncID) {
+ throw new Error("Use ensureCurrentSyncID or resetLocalSyncID");
+ },
+
async _syncFinish() {
await SyncEngine.prototype._syncFinish.call(this);
await PlacesSyncUtils.bookmarks.ensureMobileQuery();
},
async _createRecord(id) {
if (this._modified.isTombstone(id)) {
// If we already know a changed item is a tombstone, just create the
@@ -380,16 +447,30 @@ BaseBookmarksEngine.prototype = {
function BookmarksEngine(service) {
BaseBookmarksEngine.apply(this, arguments);
}
BookmarksEngine.prototype = {
__proto__: BaseBookmarksEngine.prototype,
_storeObj: BookmarksStore,
+ async getSyncID() {
+ return PlacesSyncUtils.bookmarks.getSyncId();
+ },
+
+ async getLastSync() {
+ let lastSync = await PlacesSyncUtils.bookmarks.getLastSync();
+ return lastSync;
+ },
+
+ async setLastSync(lastSync) {
+ await PlacesSyncUtils.bookmarks.setLastSync(lastSync);
+ await super.setLastSync(lastSync); // Remove in bug 1443021.
+ },
+
emptyChangeset() {
return new BookmarksChangeset();
},
async _buildGUIDMap() {
let guidMap = {};
let tree = await PlacesUtils.promiseBookmarksTree("");
@@ -512,16 +593,17 @@ BookmarksEngine.prototype = {
}
}
this._log.trace("No dupe found for key " + key + "/" + altKey + ".");
return undefined;
},
async _syncStartup() {
+ await this._migrateSyncMetadata();
await SyncEngine.prototype._syncStartup.call(this);
try {
// For first-syncs, make a backup for the user to restore
let lastSync = await this.getLastSync();
if (!lastSync) {
this._log.debug("Bookmarks backup starting.");
await PlacesBackups.create(null, true);
@@ -691,35 +773,46 @@ BufferedBookmarksEngine.prototype = {
// errors that happen when the buffered engine is enabled vs when the
// non-buffered engine is enabled.
overrideTelemetryName: "bookmarks-buffered",
// Needed to ensure we don't miss items when resuming a sync that failed or
// aborted early.
_defaultSort: "oldest",
+ async _syncStartup() {
+ await this._migrateSyncMetadata({
+ migrateLastSync: false,
+ });
+ await super._syncStartup();
+ },
+
+ async getSyncID() {
+ return PlacesSyncUtils.bookmarks.getSyncId();
+ },
+
+ async resetLocalSyncID() {
+ let buf = await this._store.ensureOpenMirror();
+ await buf.reset();
+ let newSyncID = await super.resetLocalSyncID();
+ return newSyncID;
+ },
+
async getLastSync() {
let mirror = await this._store.ensureOpenMirror();
return mirror.getCollectionHighWaterMark();
},
async setLastSync(lastSync) {
let mirror = await this._store.ensureOpenMirror();
await mirror.setCollectionLastModified(lastSync);
- // Update the pref so that reverting to the original bookmarks engine
- // doesn't download records we've already applied.
- await super.setLastSync(lastSync);
- },
-
- get lastSync() {
- throw new TypeError("Use getLastSync");
- },
-
- set lastSync(value) {
- throw new TypeError("Use setLastSync");
+ // Update the last sync time in Places so that reverting to the original
+ // bookmarks engine doesn't download records we've already applied.
+ await PlacesSyncUtils.bookmarks.setLastSync(lastSync);
+ await super.setLastSync(lastSync); // Remove in bug 1443021.
},
emptyChangeset() {
return new BufferedBookmarksChangeset();
},
async _processIncoming(newitems) {
await super._processIncoming(newitems);
@@ -1170,26 +1263,26 @@ BookmarksTracker.prototype = {
set ignoreAll(value) {},
// We never want to persist changed IDs, as the changes are already stored
// in Places.
persistChangedIDs: false,
onStart() {
PlacesUtils.bookmarks.addObserver(this, true);
- Svc.Obs.add("bookmarks-restore-begin", this.asyncObserver);
- Svc.Obs.add("bookmarks-restore-success", this.asyncObserver);
- Svc.Obs.add("bookmarks-restore-failed", this.asyncObserver);
+ Svc.Obs.add("bookmarks-restore-begin", this);
+ Svc.Obs.add("bookmarks-restore-success", this);
+ Svc.Obs.add("bookmarks-restore-failed", this);
},
onStop() {
PlacesUtils.bookmarks.removeObserver(this);
- Svc.Obs.remove("bookmarks-restore-begin", this.asyncObserver);
- Svc.Obs.remove("bookmarks-restore-success", this.asyncObserver);
- Svc.Obs.remove("bookmarks-restore-failed", this.asyncObserver);
+ Svc.Obs.remove("bookmarks-restore-begin", this);
+ Svc.Obs.remove("bookmarks-restore-success", this);
+ Svc.Obs.remove("bookmarks-restore-failed", this);
},
// Ensure we aren't accidentally using the base persistence.
addChangedID(id, when) {
throw new Error("Don't add IDs to the bookmarks tracker");
},
removeChangedID(id) {
@@ -1203,30 +1296,30 @@ BookmarksTracker.prototype = {
async getChangedIDs() {
return PlacesSyncUtils.bookmarks.pullChanges();
},
set changedIDs(obj) {
throw new Error("Don't set initial changed bookmark IDs");
},
- async observe(subject, topic, data) {
+ observe(subject, topic, data) {
switch (topic) {
case "bookmarks-restore-begin":
this._log.debug("Ignoring changes from importing bookmarks.");
break;
case "bookmarks-restore-success":
this._log.debug("Tracking all items on successful import.");
if (data == "json") {
this._log.debug("Restore succeeded: wiping server and other clients.");
- await this.engine.service.resetClient([this.name]);
- await this.engine.service.wipeServer([this.name]);
- await this.engine.service.clientsEngine.sendCommand("wipeEngine", [this.name],
- null, { reason: "bookmark-restore" });
+ // Trigger an immediate sync. `ensureCurrentSyncID` will notice we
+ // restored, wipe the server and other clients, reset the sync ID, and
+ // upload the restored tree.
+ this.score += SCORE_INCREMENT_XLARGE;
} else {
// "html", "html-initial", or "json-append"
this._log.debug("Import succeeded.");
}
break;
case "bookmarks-restore-failed":
this._log.debug("Tracking all items on failed import.");
break;
--- a/services/sync/tests/unit/test_bookmark_engine.js
+++ b/services/sync/tests/unit/test_bookmark_engine.js
@@ -303,26 +303,21 @@ async function test_restoreOrImport(engi
let wbos = collection.keys(function(id) {
return !["menu", "toolbar", "mobile", "unfiled", folder1.guid].includes(id);
});
Assert.equal(wbos.length, 1);
Assert.equal(wbos[0], bmk2.guid);
_(`Now ${verb} from a backup.`);
await bookmarkUtils.importFromFile(backupFilePath, { replace });
- await engine._tracker.asyncObserver.promiseObserversComplete();
+ // If `replace` is `true`, we'll wipe the server on the next sync.
let bookmarksCollection = server.user("foo").collection("bookmarks");
- if (replace) {
- _("Verify that we wiped the server.");
- Assert.ok(!bookmarksCollection);
- } else {
- _("Verify that we didn't wipe the server.");
- Assert.ok(!!bookmarksCollection);
- }
+ _("Verify that we didn't wipe the server.");
+ Assert.ok(!!bookmarksCollection);
_("Ensure we have the bookmarks we expect locally.");
let recordIds = await fetchAllRecordIds();
_("GUIDs: " + JSON.stringify([...recordIds]));
let bookmarkRecordIds = new Map();
let count = 0;
for (let recordId of recordIds) {
@@ -1018,8 +1013,88 @@ add_task(async function test_resume_buff
// Check that all the children made it onto the correct record.
let toolbarRecord = await engine._store.createRecord("toolbar");
Assert.deepEqual(toolbarRecord.children.sort(), children.sort());
} finally {
await cleanup(engine, server);
}
});
+
+add_task(async function test_legacy_migrate_sync_metadata() {
+ let legacyEngine = new BookmarksEngine(Service);
+ await legacyEngine.initialize();
+ await legacyEngine.resetClient();
+
+ let syncID = Utils.makeGUID();
+ let lastSync = Date.now() / 1000;
+
+ Svc.Prefs.set(`${legacyEngine.name}.syncID`, syncID);
+ Svc.Prefs.set(`${legacyEngine.name}.lastSync`, lastSync.toString());
+
+ strictEqual(await legacyEngine.getSyncID(), "",
+ "Legacy engine should start with empty sync ID");
+ strictEqual(await legacyEngine.getLastSync(), 0,
+ "Legacy engine should start with empty last sync");
+
+ info("Migrate Sync metadata prefs");
+ await legacyEngine._migrateSyncMetadata();
+
+ equal(await legacyEngine.getSyncID(), syncID,
+ "Initializing legacy engine should migrate sync ID");
+ equal(await legacyEngine.getLastSync(), lastSync,
+ "Initializing legacy engine should migrate last sync time");
+
+ let newSyncID = Utils.makeGUID();
+ await legacyEngine.ensureCurrentSyncID(newSyncID);
+
+ equal(await legacyEngine.getSyncID(), newSyncID,
+ "Changing legacy engine sync ID should update Places");
+ strictEqual(await legacyEngine.getLastSync(), 0,
+ "Changing legacy engine sync ID should clear last sync in Places");
+
+ equal(Svc.Prefs.get(`${legacyEngine.name}.syncID`), newSyncID,
+ "Changing legacy engine sync ID should update prefs");
+ strictEqual(Svc.Prefs.get(`${legacyEngine.name}.lastSync`), "0",
+ "Changing legacy engine sync ID should clear last sync pref");
+
+ await legacyEngine.wipeClient();
+});
+
+add_task(async function test_buffered_migate_sync_metadata() {
+ let bufferedEngine = new BufferedBookmarksEngine(Service);
+ await bufferedEngine.initialize();
+ await bufferedEngine.resetClient();
+
+ let syncID = Utils.makeGUID();
+ let lastSync = Date.now() / 1000;
+
+ Svc.Prefs.set(`${bufferedEngine.name}.syncID`, syncID);
+ Svc.Prefs.set(`${bufferedEngine.name}.lastSync`, lastSync.toString());
+
+ strictEqual(await bufferedEngine.getSyncID(), "",
+ "Buffered engine should start with empty sync ID");
+ strictEqual(await bufferedEngine.getLastSync(), 0,
+ "Buffered engine should start with empty last sync");
+
+ info("Migrate Sync metadata prefs");
+ await bufferedEngine._migrateSyncMetadata({
+ migrateLastSync: false,
+ });
+
+ equal(await bufferedEngine.getSyncID(), syncID,
+ "Initializing buffered engine should migrate sync ID");
+ strictEqual(await bufferedEngine.getLastSync(), 0,
+ "Initializing buffered engine should not migrate last sync time");
+
+ let newSyncID = Utils.makeGUID();
+ await bufferedEngine.ensureCurrentSyncID(newSyncID);
+
+ equal(await bufferedEngine.getSyncID(), newSyncID,
+ "Changing buffered engine sync ID should update Places");
+
+ equal(Svc.Prefs.get(`${bufferedEngine.name}.syncID`), newSyncID,
+ "Changing buffered engine sync ID should update prefs");
+ strictEqual(Svc.Prefs.get(`${bufferedEngine.name}.lastSync`), "0",
+ "Changing buffered engine sync ID should clear last sync pref");
+
+ await bufferedEngine.wipeClient();
+});