Bug 1512867 - Always update or upload synced bookmarks that only exist on one side. r=tcsc, a=RyanVM
authorLina Cambridge <lina@yakshaving.ninja>
Wed, 12 Dec 2018 01:00:13 +0000
changeset 508976 067a6752b18069ab91d3d46521a9bbeff962390e
parent 508975 ca8fd2a3447af7049cd4666d8a533ffeb64e365a
child 508977 b5a033723f0e3f52688564e5b817b19fd1e8de91
push id1905
push userffxbld-merge
push dateMon, 21 Jan 2019 12:33:13 +0000
treeherdermozilla-release@c2fca1944d8c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerstcsc, RyanVM
bugs1512867
milestone65.0
Bug 1512867 - Always update or upload synced bookmarks that only exist on one side. r=tcsc, a=RyanVM Differential Revision: https://phabricator.services.mozilla.com/D14034
toolkit/components/places/SyncedBookmarksMirror.jsm
toolkit/components/places/tests/sync/head_sync.js
toolkit/components/places/tests/sync/test_bookmark_corruption.js
toolkit/components/places/tests/sync/test_bookmark_observer_recorder.js
--- a/toolkit/components/places/SyncedBookmarksMirror.jsm
+++ b/toolkit/components/places/SyncedBookmarksMirror.jsm
@@ -2108,17 +2108,18 @@ async function initializeTempMirrorEntit
         needsMerge = 0
       WHERE needsMerge AND
             id = OLD.remoteId;
     END`);
 
   // 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)
       VALUES(OLD.newGuid, OLD.newKeyword NOT NULL OR
                           EXISTS(SELECT 1 FROM moz_keywords
                                  WHERE place_id = OLD.newPlaceId OR
                                        keyword = OLD.newKeyword),
              OLD.newLevel);
@@ -3107,55 +3108,80 @@ class MergedBookmarkNode {
     this.guid = guid;
     this.localNode = localNode;
     this.remoteNode = remoteNode;
     this.mergeState = mergeState;
     this.mergedChildren = [];
   }
 
   /**
-   * 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
           // `BookmarkMergeState.remote`.
           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) {
       case BookmarkMergeState.TYPE.LOCAL:
         if (!this.localNode) {
           // Should never happen. See the comment for
           // `BookmarkMergeState.local`.
           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:
         return true;
     }
     throw new TypeError("Unexpected structure state");
   }
 
@@ -3454,22 +3480,18 @@ class BookmarkMerger {
    *         The two-way merge state.
    */
   resolveTwoWayValueConflict(mergedGuid, localNode, remoteNode) {
     if (PlacesUtils.bookmarks.userContentRoots.includes(mergedGuid)) {
       // Don't update root titles or other properties.
       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;
       return valueState;
     }
     if (remoteNode.needsMerge) {
       // The item changed remotely since the last sync, but not locally. Take
       // the remote state.
--- a/toolkit/components/places/tests/sync/head_sync.js
+++ b/toolkit/components/places/tests/sync/head_sync.js
@@ -96,16 +96,26 @@ function makeRecord(cleartext) {
     },
   });
 }
 
 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)) {
     (record.tombstone ? results.deleted : results.updated).push(id);
   }
   results.updated.sort();
   results.deleted.sort();
   return results;
--- a/toolkit/components/places/tests/sync/test_bookmark_corruption.js
+++ b/toolkit/components/places/tests/sync/test_bookmark_corruption.js
@@ -1807,8 +1807,218 @@ add_task(async function test_invalid_gui
     missingParents: [],
     parentsWithGaps: [PlacesUtils.bookmarks.menuGuid],
   }, "Should report gaps in menu");
 
   await buf.finalize();
   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();
+});
--- a/toolkit/components/places/tests/sync/test_bookmark_observer_recorder.js
+++ b/toolkit/components/places/tests/sync/test_bookmark_observer_recorder.js
@@ -203,20 +203,17 @@ add_task(async function test_apply_then_
   let localTimeSeconds = now - 180;
 
   info("Set up initial local tree and mirror");
   await setupLocalTree(localTimeSeconds);
   let recordsToUpload = await buf.apply({
     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,
     title: "E",
     url: "http://example.com/e",
     dateAdded: new Date(localTimeSeconds * 1000),
     lastModified: new Date(localTimeSeconds * 1000),
   });