Bug 1434800 - Don't treat children moved out of a deleted synced folder as orphans. r?markh draft
authorKit Cambridge <kit@yakshaving.ninja>
Wed, 31 Jan 2018 18:58:27 -0800
changeset 749914 1de7c32cc50c288ccae6ef6e7e78db8b2d836a37
parent 749896 17ade9f88b6ed2232be51bc02c6860562c3d31dc
push id97519
push userbmo:kit@mozilla.com
push dateThu, 01 Feb 2018 04:08:21 +0000
reviewersmarkh
bugs1434800
milestone60.0a1
Bug 1434800 - Don't treat children moved out of a deleted synced folder as orphans. r?markh MozReview-Commit-ID: GAQMYNCvyDj
toolkit/components/places/SyncedBookmarksMirror.jsm
toolkit/components/places/tests/sync/test_bookmark_deletion.js
--- a/toolkit/components/places/SyncedBookmarksMirror.jsm
+++ b/toolkit/components/places/SyncedBookmarksMirror.jsm
@@ -3295,19 +3295,19 @@ class BookmarkMerger {
 
     // Make sure the remote child isn't locally deleted. If it is, we need
     // to move all descendants that aren't also remotely deleted to the
     // merged node. This handles the case where a user deletes a folder
     // on this device, and adds a bookmark to the same folder on another
     // device. We want to keep the folder deleted, but we also don't want
     // to lose the new bookmark, so we move the bookmark to the deleted
     // folder's parent.
-    let locallyDeleted = this.checkForLocalDeletionOfRemoteNode(mergedNode,
-      remoteChildNode);
-    if (locallyDeleted) {
+    let locallyMovedOrDeleted = this.checkForLocalMoveOrDeletionOfRemoteNode(
+      mergedNode, remoteChildNode);
+    if (locallyMovedOrDeleted) {
       return true;
     }
 
     // The remote child isn't locally deleted. Does it exist in the local tree?
     let localChildNode = this.localTree.nodeForGuid(remoteChildNode.guid);
     if (!localChildNode) {
       // Remote child doesn't exist locally, either. Try to find a content
       // match in the containing folder, and dedupe the local item if we can.
@@ -3421,19 +3421,19 @@ class BookmarkMerger {
     MirrorLog.trace("Merging local child ${localChildNode} of " +
                     "${localParentNode} into ${mergedNode}",
                     { localChildNode, localParentNode, mergedNode });
 
     // Now, we know we haven't seen the local child before, and it's not in
     // this folder on the server. Check if the child is remotely deleted.
     // If so, we need to move any new local descendants to the merged node,
     // just as we did for new remote descendants of locally deleted parents.
-    let remotelyDeleted = this.checkForRemoteDeletionOfLocalNode(mergedNode,
-      localChildNode);
-    if (remotelyDeleted) {
+    let remotelyMovedOrDeleted = this.checkForRemoteMoveOrDeletionOfLocalNode(
+      mergedNode, localChildNode);
+    if (remotelyMovedOrDeleted) {
       return true;
     }
 
     // At this point, we know the local child isn't deleted. See if it
     // exists in the remote tree.
     let remoteChildNode = this.remoteTree.nodeForGuid(localChildNode.guid);
     if (!remoteChildNode) {
       // The local child doesn't exist remotely, but we still need to walk
@@ -3572,24 +3572,29 @@ class BookmarkMerger {
         value: "structure",
         extra: { type: "new" },
       });
       mergedNode.mergeState = newMergeState;
     }
   }
 
   /**
-   * Walks a locally deleted remote node's children, reparenting any children
-   * that aren't also deleted remotely to the merged node. Returns `true` if
-   * `remoteNode` is deleted locally; `false` if `remoteNode` is not deleted or
-   * doesn't exist locally.
+   * Checks if a remote node is locally moved or deleted, and reparents any
+   * descendants that aren't also remotely deleted to the merged node. Returns
+   * `true` if `remoteNode` is locally moved or deleted; `false` if `remoteNode`
+   * is not deleted or doesn't exist locally.
    *
-   * This is the inverse of `checkForRemoteDeletionOfLocalNode`.
+   * This is the inverse of `checkForRemoteMoveOrDeletionOfLocalNode`.
    */
-  checkForLocalDeletionOfRemoteNode(mergedNode, remoteNode) {
+  checkForLocalMoveOrDeletionOfRemoteNode(mergedNode, remoteNode) {
+    if (this.mergedGuids.has(remoteNode.guid)) {
+      // Already merged, so must be locally moved.
+      return true;
+    }
+
     if (!this.localTree.isDeleted(remoteNode.guid)) {
       return false;
     }
 
     if (remoteNode.needsMerge) {
       if (!remoteNode.isFolder()) {
         // If a non-folder child is deleted locally and changed remotely, we
         // ignore the local deletion and take the remote child.
@@ -3626,24 +3631,29 @@ class BookmarkMerger {
     this.relocateOrphansTo(mergedNode, mergedOrphanNodes);
     MirrorLog.trace("Relocating remote orphans ${mergedOrphanNodes} to " +
                     "${mergedNode}", { mergedOrphanNodes, mergedNode });
 
     return true;
   }
 
   /**
-   * Walks a remotely deleted local node's children, reparenting any children
-   * that aren't also deleted locally to the merged node. Returns `true` if
-   * `localNode` is deleted remotely; `false` if `localNode` is not deleted or
-   * doesn't exist locally.
+   * Checks if a local node is remotely moved or deleted, and reparents any
+   * descendants that aren't also locally deleted to the merged node. Returns
+   * `true` if `localNode` is moved or deleted remotely; `false` if `localNode`
+   * is not deleted or doesn't exist locally.
    *
-   * This is the inverse of `checkForLocalDeletionOfRemoteNode`.
+   * This is the inverse of `checkForLocalMoveOrDeletionOfRemoteNode`.
    */
-  checkForRemoteDeletionOfLocalNode(mergedNode, localNode) {
+  checkForRemoteMoveOrDeletionOfLocalNode(mergedNode, localNode) {
+    if (this.mergedGuids.has(localNode.guid)) {
+      // Already merged, so must be remotely moved.
+      return true;
+    }
+
     if (!this.remoteTree.isDeleted(localNode.guid)) {
       return false;
     }
 
     if (localNode.needsMerge) {
       if (!localNode.isFolder()) {
         MirrorLog.trace("Local non-folder ${localNode} deleted remotely and " +
                         "changed locally; taking local change", { localNode });
@@ -3683,21 +3693,21 @@ class BookmarkMerger {
    * haven't also been deleted remotely. This can happen if the user adds a
    * bookmark to a folder on another device, and deletes that folder locally.
    * This is the inverse of `processLocalOrphansForNode`.
    */
   processRemoteOrphansForNode(mergedNode, remoteNode) {
     let remoteOrphanNodes = [];
 
     for (let remoteChildNode of remoteNode.children) {
-      let locallyDeleted = this.checkForLocalDeletionOfRemoteNode(mergedNode,
-        remoteChildNode);
-      if (locallyDeleted) {
-        // The remote child doesn't exist locally, or is also deleted locally,
-        // so we can safely delete its parent.
+      let locallyMovedOrDeleted = this.checkForLocalMoveOrDeletionOfRemoteNode(
+        mergedNode, remoteChildNode);
+      if (locallyMovedOrDeleted) {
+        // The remote child doesn't exist locally, or is already moved or
+        // deleted locally, so we can safely ignore it.
         continue;
       }
       remoteOrphanNodes.push(remoteChildNode);
     }
 
     let mergedOrphanNodes = [];
     for (let remoteOrphanNode of remoteOrphanNodes) {
       let localOrphanNode = this.localTree.nodeForGuid(remoteOrphanNode.guid);
@@ -3717,21 +3727,21 @@ class BookmarkMerger {
   processLocalOrphansForNode(mergedNode, localNode) {
     if (!localNode.isFolder()) {
       // The local node isn't a folder, so it won't have orphans.
       return [];
     }
 
     let localOrphanNodes = [];
     for (let localChildNode of localNode.children) {
-      let remotelyDeleted = this.checkForRemoteDeletionOfLocalNode(mergedNode,
-        localChildNode);
-      if (remotelyDeleted) {
-        // The local child doesn't exist or is also deleted on the server, so we
-        // can safely delete its parent without orphaning any local children.
+      let remotelyMovedOrDeleted = this.checkForRemoteMoveOrDeletionOfLocalNode(
+        mergedNode, localChildNode);
+      if (remotelyMovedOrDeleted) {
+        // The local child doesn't exist remotely, or is already moved or
+        // deleted remotely, so we can safely ignore it.
         continue;
       }
       localOrphanNodes.push(localChildNode);
     }
 
     let mergedOrphanNodes = [];
     for (let localOrphanNode of localOrphanNodes) {
       let remoteOrphanNode = this.remoteTree.nodeForGuid(localOrphanNode.guid);
--- a/toolkit/components/places/tests/sync/test_bookmark_deletion.js
+++ b/toolkit/components/places/tests/sync/test_bookmark_deletion.js
@@ -644,8 +644,116 @@ add_task(async function test_nonexistent
       },
     },
   });
 
   await buf.finalize();
   await PlacesUtils.bookmarks.eraseEverything();
   await PlacesSyncUtils.bookmarks.reset();
 });
+
+add_task(async function test_clear_folder_then_delete() {
+  let buf = await openMirror("clear_folder_then_delete");
+
+  info("Set up mirror");
+  await PlacesUtils.bookmarks.insertTree({
+    guid: PlacesUtils.bookmarks.menuGuid,
+    children: [{
+      guid: "folderAAAAAA",
+      type: PlacesUtils.bookmarks.TYPE_FOLDER,
+      title: "A",
+      children: [{
+        guid: "bookmarkBBBB",
+        url: "http://example.com/b",
+        title: "B",
+      }],
+    }, {
+      guid: "folderCCCCCC",
+      type: PlacesUtils.bookmarks.TYPE_FOLDER,
+      title: "C",
+      children: [{
+        guid: "bookmarkDDDD",
+        url: "http://example.com/d",
+        title: "D",
+      }],
+    }],
+  });
+  await buf.store([{
+    id: "menu",
+    type: "folder",
+    title: "Bookmarks Menu",
+    children: ["folderAAAAAA", "folderCCCCCC"],
+  }, {
+    id: "folderAAAAAA",
+    type: "folder",
+    title: "A",
+    children: ["bookmarkBBBB"],
+  }, {
+    id: "bookmarkBBBB",
+    type: "bookmark",
+    title: "A",
+    bmkUri: "http://example.com/b",
+  }, {
+    id: "folderCCCCCC",
+    type: "folder",
+    title: "C",
+    children: ["bookmarkDDDD"],
+  }, {
+    id: "bookmarkDDDD",
+    type: "bookmark",
+    title: "D",
+    bmkUri: "http://example.com/d",
+  }], { needsMerge: false });
+  await PlacesTestUtils.markBookmarksAsSynced();
+
+  info("Make local changes: Menu > D, delete C");
+  await PlacesUtils.bookmarks.update({
+    guid: "bookmarkDDDD",
+    parentGuid: PlacesUtils.bookmarks.menuGuid,
+    index: 0,
+  });
+  await PlacesUtils.bookmarks.remove("folderCCCCCC");
+
+  info("Make remote changes: Menu > B, delete A");
+  await buf.store([{
+    id: "menu",
+    type: "folder",
+    title: "Bookmarks Menu",
+    children: ["bookmarkBBBB"],
+  }, {
+    id: "folderAAAAAA",
+    deleted: true,
+  }]);
+
+  info("Apply remote");
+  let changesToUpload = await buf.apply();
+  deepEqual(await buf.fetchUnmergedGuids(), [], "Should merge all items");
+
+  let idsToUpload = inspectChangeRecords(changesToUpload);
+  deepEqual(idsToUpload, {
+    updated: ["bookmarkDDDD", "menu"],
+    deleted: ["folderCCCCCC"],
+  }, "Should upload locally moved and deleted items");
+
+  await assertLocalTree(PlacesUtils.bookmarks.menuGuid, {
+    guid: PlacesUtils.bookmarks.menuGuid,
+    type: PlacesUtils.bookmarks.TYPE_FOLDER,
+    index: 0,
+    title: "Bookmarks Menu",
+    children: [{
+      guid: "bookmarkBBBB",
+      type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
+      index: 0,
+      title: "B",
+      url: "http://example.com/b",
+    }, {
+      guid: "bookmarkDDDD",
+      type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
+      index: 1,
+      title: "D",
+      url: "http://example.com/d",
+    }],
+  }, "Should not treat moved children of a deleted folder as orphans");
+
+  await buf.finalize();
+  await PlacesUtils.bookmarks.eraseEverything();
+  await PlacesSyncUtils.bookmarks.reset();
+});