Bug 1506287 - Check changed flags when merging child lists for local and remote nodes. r=tcsc
authorLina Cambridge <lina@yakshaving.ninja>
Tue, 20 Nov 2018 02:21:08 +0000
changeset 503604 575c5e68b8f19caf629b59c6bce4e6eb7c8159d4
parent 503603 10bf548f81fa7bdbd94fa565c3d53bbb99c89a1c
child 503605 ed44376f7c7a3fc46cde381224f76c413c23a668
push id10290
push userffxbld-merge
push dateMon, 03 Dec 2018 16:23:23 +0000
treeherdermozilla-beta@700bed2445e6 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerstcsc
bugs1506287
milestone65.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1506287 - Check changed flags when merging child lists for local and remote nodes. r=tcsc Differential Revision: https://phabricator.services.mozilla.com/D12079
toolkit/components/places/SyncedBookmarksMirror.jsm
toolkit/components/places/tests/sync/test_bookmark_structure_changes.js
--- a/toolkit/components/places/SyncedBookmarksMirror.jsm
+++ b/toolkit/components/places/SyncedBookmarksMirror.jsm
@@ -3908,38 +3908,59 @@ class BookmarkMerger {
    *        The local folder node. May be `null` if the folder only exists
    *        remotely.
    * @param {BookmarkNode?} remoteNode
    *        The remote folder node. May be `null` if the folder only exists
    *        locally.
    */
   async mergeChildListsIntoMergedNode(mergedNode, localNode, remoteNode) {
     if (localNode && remoteNode) {
-      if (localNode.newerThan(remoteNode)) {
-        // The folder exists locally and remotely, and the local node is newer.
-        // Walk and merge local children first, followed by remaining unmerged
-        // remote children.
-        await this.mergeLocalChildrenIntoMergedNode(mergedNode, localNode);
-        await this.mergeRemoteChildrenIntoMergedNode(mergedNode, remoteNode);
-      } else {
-        // The folder exists locally and remotely, and the remote node is newer.
-        // Merge remote children first, then remaining local children.
+      if (localNode.needsMerge && remoteNode.needsMerge) {
+        // The folder exists locally and remotely, and changed on both sides.
+        // Compare timestamps to determine which children to merge first,
+        // followed by remaining unmerged children on the other side.
+        if (localNode.newerThan(remoteNode)) {
+          await this.mergeLocalChildrenIntoMergedNode(mergedNode, localNode);
+          await this.mergeRemoteChildrenIntoMergedNode(mergedNode, remoteNode);
+        } else {
+          await this.mergeRemoteChildrenIntoMergedNode(mergedNode, remoteNode);
+          await this.mergeLocalChildrenIntoMergedNode(mergedNode, localNode);
+        }
+        return;
+      }
+
+      if (remoteNode.needsMerge) {
+        // The folder exists locally and remotely, and only changed remotely.
+        // Merge remote children first, followed by remaining local children.
         await this.mergeRemoteChildrenIntoMergedNode(mergedNode, remoteNode);
         await this.mergeLocalChildrenIntoMergedNode(mergedNode, localNode);
+        return;
       }
-    } else if (localNode) {
+
+      // The folder exists locally and remotely, and only changed locally, or
+      // is unchanged on both sides. Merge local first, then remote.
+      await this.mergeLocalChildrenIntoMergedNode(mergedNode, localNode);
+      await this.mergeRemoteChildrenIntoMergedNode(mergedNode, remoteNode);
+      return;
+    }
+
+    if (localNode) {
       // The folder only exists locally, so no remote children to merge.
       await this.mergeLocalChildrenIntoMergedNode(mergedNode, localNode);
-    } else if (remoteNode) {
-      // The folder only exists remotely, so local children to merge.
+      return;
+    }
+
+    if (remoteNode) {
+      // The folder only exists remotely, so no local children to merge.
       await this.mergeRemoteChildrenIntoMergedNode(mergedNode, remoteNode);
-    } else {
-      // Should never happen.
-      throw new TypeError("Can't merge children for two nonexistent nodes");
+      return;
     }
+
+    // Should never happen.
+    throw new TypeError("Can't merge children for two nonexistent nodes");
   }
 
   /**
    * Recursively merges the children of a remote folder node.
    *
    * @param  {MergedBookmarkNode} mergedNode
    *         The merged folder node. This method mutates the merged node to
    *         append remote children.
@@ -4177,16 +4198,21 @@ class BookmarkMerger {
    * @param {MergedBookmarkNode} mergedNode
    *        The closest surviving ancestor, to hold relocated remote orphans.
    * @param {BookmarkNode} remoteNode
    *        The locally deleted remote node.
    */
   async relocateRemoteOrphansToMergedNode(mergedNode, remoteNode) {
     let remoteOrphanNodes = [];
     for await (let remoteChildNode of yieldingIterator(remoteNode.children)) {
+      if (this.mergedGuids.has(remoteChildNode.guid)) {
+        MirrorLog.trace("Remote child ${remoteChildNode} can't be an orphan; " +
+                        "already merged", { remoteChildNode });
+        continue;
+      }
       let structureChange = await this.checkForLocalStructureChangeOfRemoteNode(
         mergedNode, remoteNode, remoteChildNode);
       if (structureChange == BookmarkMerger.STRUCTURE.MOVED ||
           structureChange == BookmarkMerger.STRUCTURE.DELETED) {
         // The remote child is already moved or deleted locally, so we should
         // ignore it instead of treating it as a remote orphan.
         continue;
       }
@@ -4221,16 +4247,21 @@ class BookmarkMerger {
    * @param {MergedBookmarkNode} mergedNode
    *        The closest surviving ancestor, to hold relocated local orphans.
    * @param {BookmarkNode} localNode
    *        The remotely deleted local node.
    */
   async relocateLocalOrphansToMergedNode(mergedNode, localNode) {
     let localOrphanNodes = [];
     for await (let localChildNode of yieldingIterator(localNode.children)) {
+      if (this.mergedGuids.has(localChildNode.guid)) {
+        MirrorLog.trace("Local child ${localChildNode} can't be an orphan; " +
+                        "already merged", { localChildNode });
+        continue;
+      }
       let structureChange = await this.checkForRemoteStructureChangeOfLocalNode(
         mergedNode, localNode, localChildNode);
       if (structureChange == BookmarkMerger.STRUCTURE.MOVED ||
           structureChange == BookmarkMerger.STRUCTURE.DELETED) {
         // The local child is already moved or deleted remotely, so we should
         // ignore it instead of treating it as a local orphan.
         continue;
       }
--- a/toolkit/components/places/tests/sync/test_bookmark_structure_changes.js
+++ b/toolkit/components/places/tests/sync/test_bookmark_structure_changes.js
@@ -1902,8 +1902,284 @@ add_task(async function test_newer_local
       }],
     }],
   }, "Should use newer local parents and order");
 
   await buf.finalize();
   await PlacesUtils.bookmarks.eraseEverything();
   await PlacesSyncUtils.bookmarks.reset();
 });
+
+add_task(async function test_unchanged_newer_changed_older() {
+  let buf = await openMirror("unchanged_newer_changed_older");
+  let modified = new Date(Date.now() - 5000);
+
+  info("Set up mirror");
+  await PlacesUtils.bookmarks.update({
+    guid: PlacesUtils.bookmarks.menuGuid,
+    dateAdded: new Date(modified.getTime() - 5000),
+  });
+  await PlacesUtils.bookmarks.update({
+    guid: PlacesUtils.bookmarks.toolbarGuid,
+    dateAdded: new Date(modified.getTime() - 5000),
+  });
+  await PlacesUtils.bookmarks.insertTree({
+    guid: PlacesUtils.bookmarks.menuGuid,
+    children: [{
+      guid: "folderAAAAAA",
+      type: PlacesUtils.bookmarks.TYPE_FOLDER,
+      title: "A",
+      dateAdded: new Date(modified.getTime() - 5000),
+      lastModified: modified,
+    }, {
+      guid: "bookmarkBBBB",
+      url: "http://example.com/b",
+      title: "B",
+      dateAdded: new Date(modified.getTime() - 5000),
+      lastModified: modified,
+    }],
+  });
+  await PlacesUtils.bookmarks.insertTree({
+    guid: PlacesUtils.bookmarks.toolbarGuid,
+    children: [{
+      guid: "folderCCCCCC",
+      type: PlacesUtils.bookmarks.TYPE_FOLDER,
+      title: "C",
+      dateAdded: new Date(modified.getTime() - 5000),
+      lastModified: modified,
+    }, {
+      guid: "bookmarkDDDD",
+      url: "http://example.com/d",
+      title: "D",
+      dateAdded: new Date(modified.getTime() - 5000),
+      lastModified: modified,
+    }],
+  });
+  await storeRecords(buf, shuffle([{
+    id: "menu",
+    type: "folder",
+    children: ["folderAAAAAA", "bookmarkBBBB"],
+    dateAdded: modified.getTime() - 5000,
+    modified: modified.getTime() / 1000,
+  }, {
+    id: "folderAAAAAA",
+    type: "folder",
+    title: "A",
+    dateAdded: modified.getTime() - 5000,
+    modified: modified.getTime() / 1000,
+  }, {
+    id: "bookmarkBBBB",
+    type: "bookmark",
+    title: "B",
+    bmkUri: "http://example.com/b",
+    dateAdded: modified.getTime() - 5000,
+    modified: modified.getTime() / 1000,
+  }, {
+    id: "toolbar",
+    type: "folder",
+    children: ["folderCCCCCC", "bookmarkDDDD"],
+    dateAdded: modified.getTime() - 5000,
+    modified: modified.getTime() / 1000,
+  }, {
+    id: "folderCCCCCC",
+    type: "folder",
+    title: "C",
+    dateAdded: modified.getTime() - 5000,
+    modified: modified.getTime() / 1000,
+  }, {
+    id: "bookmarkDDDD",
+    type: "bookmark",
+    title: "D",
+    bmkUri: "http://example.com/d",
+    dateAdded: modified.getTime() - 5000,
+    modified: modified.getTime() / 1000,
+  }]), { needsMerge: false });
+  await PlacesTestUtils.markBookmarksAsSynced();
+
+  // Even though the local menu is newer (local = 5s, remote = 9s; adding E
+  // updated the modified times of A and the menu), it's not *changed* locally,
+  // so we should merge remote children first.
+  info("Add A > E locally with newer time; delete A remotely with older time");
+  await PlacesUtils.bookmarks.insert({
+    guid: "bookmarkEEEE",
+    parentGuid: "folderAAAAAA",
+    url: "http://example.com/e",
+    title: "E",
+    index: 0,
+    dateAdded: new Date(modified.getTime() + 5000),
+    lastModified: new Date(modified.getTime() + 5000),
+  });
+  await storeRecords(buf, [{
+    id: "menu",
+    type: "folder",
+    children: ["bookmarkBBBB"],
+    dateAdded: modified.getTime() - 5000,
+    modified: modified.getTime() / 1000 + 1,
+  }, {
+    id: "folderAAAAAA",
+    deleted: true,
+  }]);
+
+  // Even though the remote toolbar is newer (local = 15s, remote = 10s), it's
+  // not changed remotely, so we should merge local children first.
+  info("Add C > F remotely with newer time; delete C locally with older time");
+  await storeRecords(buf, shuffle([{
+    id: "folderCCCCCC",
+    type: "folder",
+    title: "C",
+    children: ["bookmarkFFFF"],
+    dateAdded: modified.getTime() - 5000,
+    modified: modified.getTime() / 1000 + 5,
+  }, {
+    id: "bookmarkFFFF",
+    type: "bookmark",
+    title: "F",
+    bmkUri: "http://example.com/f",
+    dateAdded: modified.getTime() - 5000,
+    modified: modified.getTime() / 1000 + 5,
+  }]));
+  await PlacesUtils.bookmarks.remove("folderCCCCCC");
+  await PlacesUtils.bookmarks.update({
+    guid: PlacesUtils.bookmarks.toolbarGuid,
+    lastModified: new Date(modified.getTime() - 5000),
+    // Use `SOURCES.SYNC` to avoid bumping the change counter and flagging the
+    // local toolbar as modified.
+    source: PlacesUtils.bookmarks.SOURCES.SYNC,
+  });
+
+  info("Apply remote");
+  let changesToUpload = await buf.apply({
+    localTimeSeconds: modified.getTime() / 1000 + 10,
+    remoteTimeSeconds: modified.getTime() / 1000 + 10,
+  });
+  deepEqual(await buf.fetchUnmergedGuids(), [], "Should merge all items");
+
+  deepEqual(changesToUpload, {
+    menu: {
+      tombstone: false,
+      counter: 1,
+      synced: false,
+      cleartext: {
+        id: "menu",
+        type: "folder",
+        parentid: "places",
+        hasDupe: true,
+        parentName: "",
+        dateAdded: modified.getTime() - 5000,
+        children: ["bookmarkBBBB", "bookmarkEEEE"],
+        title: BookmarksMenuTitle,
+      },
+    },
+    toolbar: {
+      tombstone: false,
+      counter: 1,
+      synced: false,
+      cleartext: {
+        id: "toolbar",
+        type: "folder",
+        parentid: "places",
+        hasDupe: true,
+        parentName: "",
+        dateAdded: modified.getTime() - 5000,
+        children: ["bookmarkDDDD", "bookmarkFFFF"],
+        title: BookmarksToolbarTitle,
+      },
+    },
+    // Upload E and F with new `parentid`.
+    bookmarkEEEE: {
+      tombstone: false,
+      counter: 1,
+      synced: false,
+      cleartext: {
+        id: "bookmarkEEEE",
+        type: "bookmark",
+        parentid: "menu",
+        hasDupe: true,
+        parentName: BookmarksMenuTitle,
+        dateAdded: modified.getTime() + 5000,
+        bmkUri: "http://example.com/e",
+        title: "E",
+      },
+    },
+    bookmarkFFFF: {
+      tombstone: false,
+      counter: 1,
+      synced: false,
+      cleartext: {
+        id: "bookmarkFFFF",
+        type: "bookmark",
+        parentid: "toolbar",
+        hasDupe: true,
+        parentName: BookmarksToolbarTitle,
+        dateAdded: modified.getTime() - 5000,
+        bmkUri: "http://example.com/f",
+        title: "F",
+      },
+    },
+    folderCCCCCC: {
+      tombstone: true,
+      counter: 1,
+      synced: false,
+      cleartext: {
+        id: "folderCCCCCC",
+        deleted: true,
+      },
+    },
+  }, "Should reupload menu, toolbar, E, F with new structure; tombstone for C");
+
+  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: "bookmarkBBBB",
+        type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
+        index: 0,
+        title: "B",
+        url: "http://example.com/b",
+      }, {
+        guid: "bookmarkEEEE",
+        type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
+        index: 1,
+        title: "E",
+        url: "http://example.com/e",
+      }],
+    }, {
+      guid: PlacesUtils.bookmarks.toolbarGuid,
+      type: PlacesUtils.bookmarks.TYPE_FOLDER,
+      index: 1,
+      title: BookmarksToolbarTitle,
+      children: [{
+        guid: "bookmarkDDDD",
+        type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
+        index: 0,
+        title: "D",
+        url: "http://example.com/d",
+      }, {
+        guid: "bookmarkFFFF",
+        type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
+        index: 1,
+        title: "F",
+        url: "http://example.com/f",
+      }],
+    }, {
+      guid: PlacesUtils.bookmarks.unfiledGuid,
+      type: PlacesUtils.bookmarks.TYPE_FOLDER,
+      index: 3,
+      title: UnfiledBookmarksTitle,
+    }, {
+      guid: PlacesUtils.bookmarks.mobileGuid,
+      type: PlacesUtils.bookmarks.TYPE_FOLDER,
+      index: 4,
+      title: MobileBookmarksTitle,
+    }],
+  }, "Should merge children of changed side first, even if they're older");
+
+  await buf.finalize();
+  await PlacesUtils.bookmarks.eraseEverything();
+  await PlacesSyncUtils.bookmarks.reset();
+});