Bug 1512867 - Always update or upload synced bookmarks that only exist on one side. r=tcsc

Differential Revision: https://phabricator.services.mozilla.com/D14034

--HG--
extra : moz-landing-system : lando

toolkit/components/places/SyncedBookmarksMirror.jsm

@@ -2113,7 +2113,8 @@ async function initializeTempMirrorEntities(db) {
   // Inserts items from the mirror that don't exist locally.
   await db.execute(`
     CREATE TEMP TRIGGER insertNewLocalItems
-    INSTEAD OF DELETE ON itemsToMerge WHEN OLD.localId IS NULL
+    INSTEAD OF DELETE ON itemsToMerge WHEN OLD.useRemote AND
+                                           OLD.localId IS NULL
     BEGIN
       /* Record an item added notification for the new item. */
       INSERT INTO itemsAdded(guid, keywordChanged, level)
@@ -3112,12 +3113,21 @@ class MergedBookmarkNode {
   }
 
   /**
-   * Indicates whether to prefer the remote node when applying the merged tree.
-   * Returns `true` if the remote state should be merged into Places; `false`
-   * if the node only exists locally, or isn't changed remotely.
+   * Indicates whether to prefer the remote side when applying the merged tree.
+   *
+   * @return {Boolean}
    */
   useRemote() {
     switch (this.mergeState.value) {
+      case BookmarkMergeState.TYPE.LOCAL:
+        if (!this.localNode) {
+          // Should never happen. See the comment for
+          // `BookmarkMergeState.local`.
+          throw new TypeError(
+            "Can't have local value state without local node");
+        }
+        return false;
+
       case BookmarkMergeState.TYPE.REMOTE:
         if (!this.remoteNode) {
           // Should never happen. See the comment for
@@ -3125,19 +3135,22 @@ class MergedBookmarkNode {
           throw new TypeError(
             "Can't have remote value state without remote node");
         }
-        return this.remoteNode.needsMerge;
-
-      case BookmarkMergeState.TYPE.LOCAL:
-        return false;
+        if (this.localNode) {
+          // If the item exists locally and remotely, check if the remote node
+          // changed.
+          return this.remoteNode.needsMerge;
+        }
+        // Otherwise, the item only exists remotely, so take the remote state
+        // unconditionally.
+        return true;
     }
     throw new TypeError("Unexpected value state");
   }
 
   /**
    * Indicates whether the merged item should be uploaded to the server.
-   * Returns `true` for locally changed nodes and nodes with a new
-   * structure state; `false` for nodes with a remote merge state, or
-   * nodes that aren't changed locally.
+   *
+   * @return {Boolean}
    */
   shouldUpload() {
     switch (this.mergeState.structure) {
@@ -3148,9 +3161,22 @@ class MergedBookmarkNode {
           throw new TypeError(
             "Can't have local structure state without local node");
         }
-        return this.localNode.needsMerge;
+        if (this.remoteNode) {
+          // If the item exists locally and remotely, check if the local node
+          // changed.
+          return this.localNode.needsMerge;
+        }
+        // Otherwise, the item only exists locally, so upload the local state
+        // unconditionally.
+        return true;
 
       case BookmarkMergeState.TYPE.REMOTE:
+        if (!this.remoteNode) {
+          // Should never happen. See the comment for
+          // `BookmarkMergeState.remote`.
+          throw new TypeError(
+            "Can't have remote structure state without remote node");
+        }
         return false;
 
       case BookmarkMergeState.TYPE.NEW:
@@ -3459,12 +3485,8 @@ class BookmarkMerger {
       return BookmarkMergeState.local;
     }
     if (localNode.needsMerge && remoteNode.needsMerge) {
-      // The item changed locally and remotely. We could query storage to
-      // determine if the value state is the same, as iOS does. However, that's
-      // an expensive check that requires joining `moz_bookmarks`,
-      // `moz_items_annos`, and `moz_places` to the mirror. It's unlikely that
-      // the value state is identical, so we skip the value check and use the
-      // timestamp to decide which node is newer.
+      // The item changed locally and remotely. Use the timestamp to decide
+      // which is newer.
       let valueState = localNode.newerThan(remoteNode) ?
                        BookmarkMergeState.local :
                        BookmarkMergeState.remote;

toolkit/components/places/tests/sync/head_sync.js

@@ -101,6 +101,16 @@ async function storeRecords(buf, records, options) {
   await buf.store(records.map(makeRecord), options);
 }
 
+async function storeChangesInMirror(buf, changesToUpload) {
+  let cleartexts = [];
+  for (let recordId in changesToUpload) {
+    changesToUpload[recordId].synced = true;
+    cleartexts.push(changesToUpload[recordId].cleartext);
+  }
+  await storeRecords(buf, cleartexts, { needsMerge: false });
+  await PlacesSyncUtils.bookmarks.pushChanges(changesToUpload);
+}
+
 function inspectChangeRecords(changeRecords) {
   let results = { updated: [], deleted: [] };
   for (let [id, record] of Object.entries(changeRecords)) {

toolkit/components/places/tests/sync/test_bookmark_corruption.js

@@ -1812,3 +1812,213 @@ add_task(async function test_invalid_guid() {
   await PlacesUtils.bookmarks.eraseEverything();
   await PlacesSyncUtils.bookmarks.reset();
 });
+
+add_task(async function test_sync_status_mismatches() {
+  let dateAdded = new Date();
+
+  let mergeTelemetryEvents = [];
+  let buf = await openMirror("sync_status_mismatches", {
+    recordTelemetryEvent(object, method, value, extra) {
+      equal(object, "mirror", "Wrong object for telemetry event");
+      if (method == "merge") {
+        mergeTelemetryEvents.push({ value, extra });
+      }
+    },
+  });
+
+  info("Ensure mirror is up-to-date with Places");
+  let initialChangesToUpload = await buf.apply();
+
+  deepEqual(Object.keys(initialChangesToUpload).sort(),
+    ["menu", "mobile", "toolbar", "unfiled"],
+    "Should upload roots on first merge");
+
+  await storeChangesInMirror(buf, initialChangesToUpload);
+
+  deepEqual(await buf.fetchSyncStatusMismatches(), {
+    missingLocal: [],
+    missingRemote: [],
+    wrongSyncStatus: [],
+  }, "Should not report mismatches after first merge");
+
+  info("Make local changes");
+  await PlacesUtils.bookmarks.insertTree({
+    guid: PlacesUtils.bookmarks.menuGuid,
+    source: PlacesUtils.bookmarks.SOURCES.SYNC,
+    children: [{
+      // A is NORMAL in Places, but doesn't exist in the mirror.
+      guid: "bookmarkAAAA",
+      url: "http://example.com/a",
+      title: "A",
+      dateAdded,
+    }],
+  });
+  await PlacesUtils.bookmarks.insertTree({
+    guid: PlacesUtils.bookmarks.unfiledGuid,
+    children: [{
+      // B is NEW in Places and exists in the mirror.
+      guid: "bookmarkBBBB",
+      url: "http://example.com/b",
+      title: "B",
+      dateAdded,
+    }],
+  });
+
+  info("Make remote changes");
+  await storeRecords(buf, [{
+    id: "unfiled",
+    type: "folder",
+    children: ["bookmarkBBBB"],
+  }, {
+    id: "toolbar",
+    type: "folder",
+    children: ["bookmarkCCCC"],
+  }, {
+    id: "bookmarkBBBB",
+    type: "bookmark",
+    bmkUri: "http://example.com/b",
+    title: "B",
+  }, {
+    // C is flagged as merged in the mirror, but doesn't exist in Places.
+    id: "bookmarkCCCC",
+    type: "bookmark",
+    bmkUri: "http://example.com/c",
+    title: "C",
+  }], { needsMerge: false });
+
+  deepEqual(await buf.fetchSyncStatusMismatches(), {
+    missingLocal: ["bookmarkCCCC"],
+    missingRemote: ["bookmarkAAAA"],
+    wrongSyncStatus: ["bookmarkBBBB"],
+  }, "Should report sync status mismatches");
+
+  info("Apply mirror");
+  let changesToUpload = await buf.apply();
+  deepEqual(await buf.fetchUnmergedGuids(), [], "Should merge all items");
+
+  let datesAdded = await promiseManyDatesAdded([PlacesUtils.bookmarks.menuGuid,
+    PlacesUtils.bookmarks.unfiledGuid]);
+  deepEqual(changesToUpload, {
+    bookmarkAAAA: {
+      tombstone: false,
+      counter: 1,
+      synced: false,
+      cleartext: {
+        id: "bookmarkAAAA",
+        type: "bookmark",
+        parentid: "menu",
+        hasDupe: true,
+        parentName: BookmarksMenuTitle,
+        dateAdded: dateAdded.getTime(),
+        bmkUri: "http://example.com/a",
+        title: "A",
+      },
+    },
+    bookmarkBBBB: {
+      tombstone: false,
+      counter: 1,
+      synced: false,
+      cleartext: {
+        id: "bookmarkBBBB",
+        type: "bookmark",
+        parentid: "unfiled",
+        hasDupe: true,
+        parentName: UnfiledBookmarksTitle,
+        dateAdded: dateAdded.getTime(),
+        bmkUri: "http://example.com/b",
+        title: "B",
+      },
+    },
+    menu: {
+      tombstone: false,
+      counter: 1,
+      synced: false,
+      cleartext: {
+        id: "menu",
+        type: "folder",
+        parentid: "places",
+        hasDupe: true,
+        parentName: "",
+        dateAdded: datesAdded.get(PlacesUtils.bookmarks.menuGuid),
+        title: BookmarksMenuTitle,
+        children: ["bookmarkAAAA"],
+      },
+    },
+    unfiled: {
+      tombstone: false,
+      counter: 1,
+      synced: false,
+      cleartext: {
+        id: "unfiled",
+        type: "folder",
+        parentid: "places",
+        hasDupe: true,
+        parentName: "",
+        dateAdded: datesAdded.get(PlacesUtils.bookmarks.unfiledGuid),
+        title: UnfiledBookmarksTitle,
+        children: ["bookmarkBBBB"],
+      },
+    },
+  }, "Should flag (A B) and their parents for upload");
+
+  await assertLocalTree(PlacesUtils.bookmarks.rootGuid, {
+    guid: PlacesUtils.bookmarks.rootGuid,
+    type: PlacesUtils.bookmarks.TYPE_FOLDER,
+    index: 0,
+    title: "",
+    children: [{
+      guid: PlacesUtils.bookmarks.menuGuid,
+      type: PlacesUtils.bookmarks.TYPE_FOLDER,
+      index: 0,
+      title: BookmarksMenuTitle,
+      children: [{
+        guid: "bookmarkAAAA",
+        type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
+        index: 0,
+        title: "A",
+        url: "http://example.com/a",
+      }],
+    }, {
+      guid: PlacesUtils.bookmarks.toolbarGuid,
+      type: PlacesUtils.bookmarks.TYPE_FOLDER,
+      index: 1,
+      title: BookmarksToolbarTitle,
+      children: [{
+        guid: "bookmarkCCCC",
+        type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
+        index: 0,
+        title: "C",
+        url: "http://example.com/c",
+      }],
+    }, {
+      guid: PlacesUtils.bookmarks.unfiledGuid,
+      type: PlacesUtils.bookmarks.TYPE_FOLDER,
+      index: 3,
+      title: UnfiledBookmarksTitle,
+      children: [{
+        guid: "bookmarkBBBB",
+        type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
+        index: 0,
+        title: "B",
+        url: "http://example.com/b",
+      }],
+    }, {
+      guid: PlacesUtils.bookmarks.mobileGuid,
+      type: PlacesUtils.bookmarks.TYPE_FOLDER,
+      index: 4,
+      title: MobileBookmarksTitle,
+    }],
+  }, "Should parent C correctly");
+
+  await storeChangesInMirror(buf, changesToUpload);
+
+  deepEqual(await buf.fetchSyncStatusMismatches(), {
+    missingLocal: [],
+    missingRemote: [],
+    wrongSyncStatus: [],
+  }, "Applying and storing new changes in mirror should fix inconsistencies");
+
+  await buf.finalize();
+  await PlacesUtils.bookmarks.eraseEverything();
+  await PlacesSyncUtils.bookmarks.reset();
+});

toolkit/components/places/tests/sync/test_bookmark_observer_recorder.js

@@ -208,10 +208,7 @@ add_task(async function test_apply_then_revert() {
     localTimeSeconds,
     remoteTimeSeconds: now,
   });
-  let cleartexts = Object.keys(recordsToUpload).map(recordId =>
-    recordsToUpload[recordId].cleartext);
-  await storeRecords(buf, cleartexts, { needsMerge: false });
-  await PlacesTestUtils.markBookmarksAsSynced();
+  await storeChangesInMirror(buf, recordsToUpload);
   await PlacesUtils.bookmarks.insert({
     guid: "bookmarkEEE1",
     parentGuid: PlacesUtils.bookmarks.menuGuid,