--- a/toolkit/components/places/SyncedBookmarksMirror.jsm
+++ b/toolkit/components/places/SyncedBookmarksMirror.jsm
@@ -4035,33 +4035,41 @@ class BookmarkMerger {
* A structure change type: `UNCHANGED` if the remote node is not
* deleted or doesn't exist locally, `MOVED` if the node is moved
* locally, or `DELETED` if the node is deleted locally.
*/
async checkForLocalStructureChangeOfRemoteNode(mergedNode, remoteParentNode,
remoteNode) {
if (!remoteNode.isSyncable) {
// If the remote node is known to be non-syncable, we unconditionally
- // delete it on both sides, even if it's syncable locally.
- this.deleteLocally.add(remoteNode.guid);
- await this.relocateRemoteOrphansToNode(mergedNode, remoteNode);
+ // delete it from the server, even if it's syncable locally.
+ this.deleteRemotely.add(remoteNode.guid);
+ if (remoteNode.isFolder()) {
+ // If the remote node is a folder, we also need to walk its descendants
+ // and reparent any syncable descendants, and descendants that only
+ // exist remotely, to the merged node.
+ await this.relocateRemoteOrphansToMergedNode(mergedNode, remoteNode);
+ }
return BookmarkMerger.STRUCTURE.DELETED;
}
if (!this.localTree.isDeleted(remoteNode.guid)) {
let localNode = this.localTree.nodeForGuid(remoteNode.guid);
if (!localNode) {
return BookmarkMerger.STRUCTURE.UNCHANGED;
}
if (!localNode.isSyncable) {
// The remote node is syncable, but the local node is non-syncable.
- // This is unlikely, but, for consistency with remote structure changes,
- // we unconditionally delete the node on both sides.
- this.deleteLocally.add(remoteNode.guid);
- await this.relocateRemoteOrphansToNode(mergedNode, remoteNode);
+ // This is unlikely now that Places no longer supports custom roots,
+ // but, for consistency, we unconditionally delete the node from the
+ // server.
+ this.deleteRemotely.add(remoteNode.guid);
+ if (remoteNode.isFolder()) {
+ await this.relocateRemoteOrphansToMergedNode(mergedNode, remoteNode);
+ }
return BookmarkMerger.STRUCTURE.DELETED;
}
let localParentNode = this.localTree.parentNodeFor(localNode);
if (!localParentNode) {
// Should never happen. If a node in the local tree doesn't have a
// parent, we built the tree incorrectly.
throw new TypeError(
"Can't check for structure changes without local parent");
@@ -4087,21 +4095,26 @@ class BookmarkMerger {
// revive the child folder, but it's easier to relocate orphaned
// grandchildren than to partially revive the child folder.
MirrorLog.trace("Remote folder ${remoteNode} deleted locally " +
"and changed remotely; taking local deletion",
{ remoteNode });
this.structureCounts.localDeletes++;
} else {
MirrorLog.trace("Remote node ${remoteNode} deleted locally and not " +
- "changed remotely; taking local deletion",
- { remoteNode });
+ "changed remotely; taking local deletion",
+ { remoteNode });
}
- await this.relocateRemoteOrphansToNode(mergedNode, remoteNode);
+ // Take the local deletion and relocate any new remote descendants to the
+ // merged node.
+ this.deleteRemotely.add(remoteNode.guid);
+ if (remoteNode.isFolder()) {
+ await this.relocateRemoteOrphansToMergedNode(mergedNode, remoteNode);
+ }
return BookmarkMerger.STRUCTURE.DELETED;
}
/**
* Checks if a local node is remotely moved or deleted, and reparents any
* descendants that aren't also locally deleted to the merged node.
*
* This is the inverse of `checkForLocalStructureChangeOfRemoteNode`.
@@ -4116,35 +4129,40 @@ class BookmarkMerger {
* A structure change type: `UNCHANGED` if the local node is not
* deleted or doesn't exist remotely, `MOVED` if the node is moved
* remotely, or `DELETED` if the node is deleted remotely.
*/
async checkForRemoteStructureChangeOfLocalNode(mergedNode, localParentNode,
localNode) {
if (!localNode.isSyncable) {
// If the local node is known to be non-syncable, we unconditionally
- // delete it on both sides, even if it's syncable remotely.
- this.deleteRemotely.add(localNode.guid);
- await this.relocateLocalOrphansToNode(mergedNode, localNode);
+ // delete it from Places, even if it's syncable remotely. This is
+ // unlikely now that Places no longer supports custom roots.
+ this.deleteLocally.add(localNode.guid);
+ if (localNode.isFolder()) {
+ await this.relocateLocalOrphansToMergedNode(mergedNode, localNode);
+ }
return BookmarkMerger.STRUCTURE.DELETED;
}
if (!this.remoteTree.isDeleted(localNode.guid)) {
let remoteNode = this.remoteTree.nodeForGuid(localNode.guid);
if (!remoteNode) {
return BookmarkMerger.STRUCTURE.UNCHANGED;
}
if (!remoteNode.isSyncable) {
// The local node is syncable, but the remote node is non-syncable.
// This can happen if we applied an orphaned left pane query in a
// previous sync, and later saw the left pane root on the server.
// Since we now have the complete subtree, we can remove the item from
- // Places, and upload tombstones to the server.
- this.deleteRemotely.add(localNode.guid);
- await this.relocateLocalOrphansToNode(mergedNode, localNode);
+ // Places.
+ this.deleteLocally.add(localNode.guid);
+ if (remoteNode.isFolder()) {
+ await this.relocateLocalOrphansToMergedNode(mergedNode, localNode);
+ }
return BookmarkMerger.STRUCTURE.DELETED;
}
let remoteParentNode = this.remoteTree.parentNodeFor(remoteNode);
if (!remoteParentNode) {
// Should never happen. If a node in the remote tree doesn't have a
// parent, we built the tree incorrectly.
throw new TypeError(
"Can't check for structure changes without remote parent");
@@ -4165,84 +4183,41 @@ class BookmarkMerger {
MirrorLog.trace("Local folder ${localNode} deleted remotely and " +
"changed locally; taking remote deletion", { localNode });
this.structureCounts.remoteDeletes++;
} else {
MirrorLog.trace("Local node ${localNode} deleted remotely and not " +
"changed locally; taking remote deletion", { localNode });
}
- await this.relocateLocalOrphansToNode(mergedNode, localNode);
+ // Take the remote deletion and relocate any new local descendants to the
+ // merged node.
+ this.deleteLocally.add(localNode.guid);
+ if (localNode.isFolder()) {
+ await this.relocateLocalOrphansToMergedNode(mergedNode, localNode);
+ }
return BookmarkMerger.STRUCTURE.DELETED;
}
/**
* Takes a local deletion for a remote node by marking the node as deleted,
* and relocating all remote descendants that aren't also locally deleted to
- * the closest surviving ancestor.
+ * the closest surviving ancestor. We do this to avoid data loss if the
+ * user adds a bookmark to a folder on another device, and deletes that
+ * folder locally.
*
- * This is the inverse of `relocateLocalOrphansToNode`.
+ * This is the inverse of `relocateLocalOrphansToMergedNode`.
*
* @param {MergedBookmarkNode} mergedNode
* The closest surviving ancestor, to hold relocated remote orphans.
* @param {BookmarkNode} remoteNode
* The locally deleted remote node.
*/
- async relocateRemoteOrphansToNode(mergedNode, remoteNode) {
- this.deleteRemotely.add(remoteNode.guid);
-
- let mergedOrphanNodes = await this.processRemoteOrphansForNode(mergedNode,
- remoteNode);
-
- MirrorLog.trace("Relocating remote orphans ${mergedOrphanNodes} to " +
- "${mergedNode}", { mergedOrphanNodes, mergedNode });
- for await (let mergedOrphanNode of yieldingIterator(mergedOrphanNodes)) {
- // Flag the moved orphans for reupload.
- let mergeState = BookmarkMergeState.new(mergedOrphanNode.mergeState);
- mergedOrphanNode.mergeState = mergeState;
- mergedNode.mergedChildren.push(mergedOrphanNode);
- }
- }
-
- /**
- * Takes a remote deletion for a local node by marking the node as deleted,
- * and relocating all local descendants that aren't also remotely deleted to
- * the closest surviving ancestor.
- *
- * This is the inverse of `relocateRemoteOrphansToNode`.
- *
- * @param {MergedBookmarkNode} mergedNode
- * The closest surviving ancestor, to hold relocated local orphans.
- * @param {BookmarkNode} localNode
- * The remotely deleted local node.
- */
- async relocateLocalOrphansToNode(mergedNode, localNode) {
- this.deleteLocally.add(localNode.guid);
-
- let mergedOrphanNodes = await this.processLocalOrphansForNode(mergedNode,
- localNode);
-
- MirrorLog.trace("Relocating local orphans ${mergedOrphanNodes} to " +
- "${mergedNode}", { mergedOrphanNodes, mergedNode });
- for await (let mergedOrphanNode of yieldingIterator(mergedOrphanNodes)) {
- let mergeState = BookmarkMergeState.new(mergedOrphanNode.mergeState);
- mergedOrphanNode.mergeState = mergeState;
- mergedNode.mergedChildren.push(mergedOrphanNode);
- }
- }
-
- /**
- * Recursively merges all remote children of a locally deleted folder that
- * 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`.
- */
- async processRemoteOrphansForNode(mergedNode, remoteNode) {
+ async relocateRemoteOrphansToMergedNode(mergedNode, remoteNode) {
let remoteOrphanNodes = [];
-
for await (let remoteChildNode of yieldingIterator(remoteNode.children)) {
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;
@@ -4253,30 +4228,39 @@ class BookmarkMerger {
let mergedOrphanNodes = [];
for await (let remoteOrphanNode of yieldingIterator(remoteOrphanNodes)) {
let localOrphanNode = this.localTree.nodeForGuid(remoteOrphanNode.guid);
let mergedOrphanNode = await this.mergeNode(remoteOrphanNode.guid,
localOrphanNode, remoteOrphanNode);
mergedOrphanNodes.push(mergedOrphanNode);
}
- return mergedOrphanNodes;
+ MirrorLog.trace("Relocating remote orphans ${mergedOrphanNodes} to " +
+ "${mergedNode}", { mergedOrphanNodes, mergedNode });
+ for await (let mergedOrphanNode of yieldingIterator(mergedOrphanNodes)) {
+ // Flag the moved orphans for reupload.
+ let mergeState = BookmarkMergeState.new(mergedOrphanNode.mergeState);
+ mergedOrphanNode.mergeState = mergeState;
+ mergedNode.mergedChildren.push(mergedOrphanNode);
+ }
}
/**
- * Recursively merges all local children of a remotely deleted folder that
- * haven't also been deleted locally. This is the inverse of
- * `processRemoteOrphansForNode`.
+ * Takes a remote deletion for a local node by marking the node as deleted,
+ * and relocating all local descendants that aren't also remotely deleted to
+ * the closest surviving ancestor.
+ *
+ * This is the inverse of `relocateRemoteOrphansToMergedNode`.
+ *
+ * @param {MergedBookmarkNode} mergedNode
+ * The closest surviving ancestor, to hold relocated local orphans.
+ * @param {BookmarkNode} localNode
+ * The remotely deleted local node.
*/
- async processLocalOrphansForNode(mergedNode, localNode) {
- if (!localNode.isFolder()) {
- // The local node isn't a folder, so it won't have orphans.
- return [];
- }
-
+ async relocateLocalOrphansToMergedNode(mergedNode, localNode) {
let localOrphanNodes = [];
for await (let localChildNode of yieldingIterator(localNode.children)) {
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.
@@ -4288,17 +4272,24 @@ class BookmarkMerger {
let mergedOrphanNodes = [];
for await (let localOrphanNode of yieldingIterator(localOrphanNodes)) {
let remoteOrphanNode = this.remoteTree.nodeForGuid(localOrphanNode.guid);
let mergedNode = await this.mergeNode(localOrphanNode.guid,
localOrphanNode, remoteOrphanNode);
mergedOrphanNodes.push(mergedNode);
}
- return mergedOrphanNodes;
+ MirrorLog.trace("Relocating local orphans ${mergedOrphanNodes} to " +
+ "${mergedNode}", { mergedOrphanNodes, mergedNode });
+
+ for await (let mergedOrphanNode of yieldingIterator(mergedOrphanNodes)) {
+ let mergeState = BookmarkMergeState.new(mergedOrphanNode.mergeState);
+ mergedOrphanNode.mergeState = mergeState;
+ mergedNode.mergedChildren.push(mergedOrphanNode);
+ }
}
/**
* Finds all children of a local folder with similar content as children of
* the corresponding remote folder. This is used to dedupe local items that
* haven't been uploaded yet, to remote items that don't exist locally.
*
* Recall that we match items by GUID as we walk down the tree. If a GUID on