Bug 1199077 - Sync changes to store the bookmarks sync ID and last sync time in Places. r=tcsc draft
authorKit Cambridge <kit@yakshaving.ninja>
Fri, 02 Mar 2018 15:32:19 -0800
changeset 765150 e31ace2f78c79f1bd304f9992ec44fb8dfe8a9a9
parent 765149 ac0d8657b68ce37e4f4829d1d2bee68d8760ea9d
child 765151 700ac20b3d6487635a158606e18f9b6d346eecdd
push id101984
push userbmo:kit@mozilla.com
push dateFri, 09 Mar 2018 06:22:41 +0000
reviewerstcsc
bugs1199077
milestone60.0a1
Bug 1199077 - Sync changes to store the bookmarks sync ID and last sync time in Places. r=tcsc MozReview-Commit-ID: 6ddzpNtMmzb
services/sync/modules/engines/bookmarks.js
services/sync/tests/unit/test_bookmark_engine.js
--- 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();
+});