Bug 1540894 - Vendor Dogear v0.2.3. r=tcsc

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

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

Cargo.lock

@@ -357,7 +357,7 @@ name = "bookmark_sync"
 version = "0.1.0"
 dependencies = [
  "atomic_refcell 0.1.0 (registry+https://github.com/rust-lang/crates.io-index)",
- "dogear 0.2.2 (registry+https://github.com/rust-lang/crates.io-index)",
+ "dogear 0.2.3 (registry+https://github.com/rust-lang/crates.io-index)",
  "libc 0.2.51 (registry+https://github.com/rust-lang/crates.io-index)",
  "log 0.4.6 (registry+https://github.com/rust-lang/crates.io-index)",
  "moz_task 0.1.0",
@@ -914,7 +914,7 @@ dependencies = [
 
 [[package]]
 name = "dogear"
-version = "0.2.2"
+version = "0.2.3"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 dependencies = [
  "log 0.4.6 (registry+https://github.com/rust-lang/crates.io-index)",
@@ -3597,7 +3597,7 @@ dependencies = [
 "checksum digest 0.8.0 (registry+https://github.com/rust-lang/crates.io-index)" = "05f47366984d3ad862010e22c7ce81a7dbcaebbdfb37241a620f8b6596ee135c"
 "checksum dirs 1.0.4 (registry+https://github.com/rust-lang/crates.io-index)" = "88972de891f6118092b643d85a0b28e0678e0f948d7f879aa32f2d5aafe97d2a"
 "checksum docopt 0.8.3 (registry+https://github.com/rust-lang/crates.io-index)" = "d8acd393692c503b168471874953a2531df0e9ab77d0b6bbc582395743300a4a"
-"checksum dogear 0.2.2 (registry+https://github.com/rust-lang/crates.io-index)" = "bcecbcd636b901efb0b61eea73972bda173c02c98a07fc66dd76e8ee1421ffbf"
+"checksum dogear 0.2.3 (registry+https://github.com/rust-lang/crates.io-index)" = "6d54506b6b209740d0a7a35ca5976db1ad2ed1aa168acc3561efc6a84fa95afe"
 "checksum dtoa 0.4.2 (registry+https://github.com/rust-lang/crates.io-index)" = "09c3753c3db574d215cba4ea76018483895d7bff25a31b49ba45db21c48e50ab"
 "checksum dtoa-short 0.3.1 (registry+https://github.com/rust-lang/crates.io-index)" = "068d4026697c1a18f0b0bb8cfcad1b0c151b90d8edb9bf4c235ad68128920d1d"
 "checksum dwrote 0.8.0 (registry+https://github.com/rust-lang/crates.io-index)" = "c31c624339dab99c223a4b26c2e803b7c248adaca91549ce654c76f39a03f5c8"

third_party/rust/dogear/.cargo-checksum.json

@@ -1 +1 @@
-{"files":{"Cargo.toml":"f427f0dba2855a2e32ccaf3258e6517a90c2b69f5b7a9c34d8669d4a83fb84e7","LICENSE":"c71d239df91726fc519c6eb72d318ec65820627232b2f796219e87dcf35d0ab4","README.md":"303ea5ec53d4e86f2c321056e8158e31aa061353a99e52de3d76859d40919efc","src/driver.rs":"10ecce90c6dee4e7b0ecd87f6d4f10c3cb825b544c6413416167248755097ab2","src/error.rs":"c6e661a7b94119dc8770c482681e97e644507e37f0ba32c04cc3a0b43e7b0077","src/guid.rs":"0330e6e893a550e478c8ac678114ebc112add97cb1d5d803d65cda6588ce7ba5","src/lib.rs":"ef42d0d3b234ffb6e459550f36a5f9220a0dd5fd09867affc7f8f9fe0b5430f2","src/merge.rs":"2d94c9507725de7477d7dc4ca372c721e770d049f57ff4c14a2006e346231a40","src/store.rs":"612d90ea0614aa7cc943c4ac0faaee35c155f57b553195ac28518ae7c0b8ebb1","src/tests.rs":"e5a3a1b9b4cefda9b871348a739f2d66e05a940ad14fb72515cea373f9f3be8b","src/tree.rs":"17d5640e42dcbd979f4e5cc8c52c4b2b634f80596a3504baa40b2e6b55f213b8"},"package":"bcecbcd636b901efb0b61eea73972bda173c02c98a07fc66dd76e8ee1421ffbf"}
+{"files":{"CODE_OF_CONDUCT.md":"e85149c44f478f164f7d5f55f6e66c9b5ae236d4a11107d5e2a93fe71dd874b9","Cargo.toml":"9b2fd48cc14073599c7d3a50f74fe6aa9896a862c72651f69e06a1ec37e43c6d","LICENSE":"c71d239df91726fc519c6eb72d318ec65820627232b2f796219e87dcf35d0ab4","README.md":"303ea5ec53d4e86f2c321056e8158e31aa061353a99e52de3d76859d40919efc","src/driver.rs":"10ecce90c6dee4e7b0ecd87f6d4f10c3cb825b544c6413416167248755097ab2","src/error.rs":"c6e661a7b94119dc8770c482681e97e644507e37f0ba32c04cc3a0b43e7b0077","src/guid.rs":"0330e6e893a550e478c8ac678114ebc112add97cb1d5d803d65cda6588ce7ba5","src/lib.rs":"ef42d0d3b234ffb6e459550f36a5f9220a0dd5fd09867affc7f8f9fe0b5430f2","src/merge.rs":"460c6af8ba3b680d072528e9ee27ea62559911b1cce5a511abc3d698bc7b3da3","src/store.rs":"612d90ea0614aa7cc943c4ac0faaee35c155f57b553195ac28518ae7c0b8ebb1","src/tests.rs":"f341d811eb648e8482dd2eb108e110850453e7e0deeccc09610fa234332f3921","src/tree.rs":"b6ba6275716dff398ff81dfcbbc47fa3af59555e452d92b5cb035b73b88f733d"},"package":"6d54506b6b209740d0a7a35ca5976db1ad2ed1aa168acc3561efc6a84fa95afe"}

third_party/rust/dogear/CODE_OF_CONDUCT.md

@@ -0,0 +1,25 @@
+# Community Participation Guidelines
+
+This repository is governed by Mozilla's code of conduct and etiquette guidelines.
+For more details, please read the
+[Mozilla Community Participation Guidelines](https://www.mozilla.org/about/governance/policies/participation/).
+
+## How to Report
+For more information on how to report violations of the Community Participation Guidelines, please read our '[How to Report](https://www.mozilla.org/about/governance/policies/participation/reporting/)' page.
+
+## Project Specific Etiquette
+
+### Our Responsibilities
+
+Project maintainers are responsible for clarifying the standards of acceptable
+behavior and are expected to take appropriate and fair corrective action in
+response to any instances of unacceptable behavior.
+
+Project maintainers have the right and responsibility to remove, edit, or
+reject comments, commits, code, wiki edits, issues, and other contributions
+that are not aligned to this Code of Conduct, or to ban temporarily or
+permanently any contributor for other behaviors that they deem inappropriate,
+threatening, offensive, or harmful.
+
+Project maintainers who do not follow or enforce Mozilla's Participation Guidelines in good
+faith may face temporary or permanent repercussions.

third_party/rust/dogear/Cargo.toml

@@ -13,7 +13,7 @@
 [package]
 edition = "2018"
 name = "dogear"
-version = "0.2.2"
+version = "0.2.3"
 authors = ["Lina Cambridge <lina@mozilla.com>"]
 exclude = ["/.travis/**", ".travis.yml"]
 description = "A library for merging bookmark trees."

third_party/rust/dogear/src/merge.rs

@@ -35,7 +35,7 @@ enum StructureChange {
 }
 
 /// Records structure change counters for telemetry.
-#[derive(Clone, Copy, Default, Debug, Eq, PartialEq)]
+#[derive(Clone, Copy, Default, Debug, Eq, Hash, PartialEq)]
 pub struct StructureCounts {
     /// Remote non-folder change wins over local deletion.
     pub remote_revives: usize,
@@ -59,7 +59,7 @@ pub struct StructureCounts {
 type MatchingDupes<'t> = (HashMap<Guid, Node<'t>>, HashMap<Guid, Node<'t>>);
 
 /// Represents an accepted local or remote deletion.
-#[derive(Clone, Debug, Eq, PartialEq)]
+#[derive(Clone, Copy, Debug, Eq, Hash, PartialEq)]
 pub struct Deletion<'t> {
     pub guid: &'t Guid,
     pub local_level: i64,
@@ -886,7 +886,33 @@ impl<'t, D: Driver> Merger<'t, D> {
         match (local_node.needs_merge, remote_node.needs_merge) {
             (true, true) => {
                 // The item changed locally and remotely.
-                let newer_side = if local_node.age < remote_node.age {
+                let item = if local_node.is_user_content_root() {
+                    // For roots, we always prefer the local side for item
+                    // changes, like the title (bug 1432614).
+                    ConflictResolution::Local
+                } else {
+                    // For other items, we check the validity to decide
+                    // which side to take.
+                    match remote_node.validity {
+                        Validity::Valid | Validity::Reupload => {
+                            // If the remote item is valid, or valid but needs
+                            // reupload, compare timestamps to decide which side is
+                            // newer.
+                            if local_node.age < remote_node.age {
+                                ConflictResolution::Local
+                            } else {
+                                ConflictResolution::Remote
+                            }
+                        }
+                        // If the remote item must be replaced, take the local
+                        // side. This _loses remote changes_, but we can't
+                        // apply those changes, anyway.
+                        Validity::Replace => ConflictResolution::Local,
+                    }
+                };
+                // For children, it's easier: we always use the newer side, even
+                // if we're taking local changes for the item.
+                let children = if local_node.age < remote_node.age {
                     // The local change is newer, so merge local children first,
                     // followed by remaining unmerged remote children.
                     ConflictResolution::Local
@@ -895,16 +921,7 @@ impl<'t, D: Driver> Merger<'t, D> {
                     // children first, then remaining local children.
                     ConflictResolution::Remote
                 };
-                if local_node.is_user_content_root() {
+                (item, children)
-                    // For roots, we always prefer the local side for item
-                    // changes, like the title (bug 1432614), but prefer the
-                    // newer side for children.
-                    (ConflictResolution::Local, newer_side)
-                } else {
-                    // For all other items, we prefer the newer side for the
-                    // item and children.
-                    (newer_side, newer_side)
-                }
             }
 
             (true, false) => {
@@ -916,15 +933,20 @@ impl<'t, D: Driver> Merger<'t, D> {
 
             (false, true) => {
                 // The item changed remotely, but not locally.
-                if local_node.is_user_content_root() {
+                let item = if local_node.is_user_content_root() {
-                    // For roots, we ignore remote item changes, but prefer
+                    // For roots, we ignore remote item changes.
-                    // the remote side for children.
+                    ConflictResolution::Unchanged
-                    (ConflictResolution::Unchanged, ConflictResolution::Remote)
                 } else {
-                    // For other items, we prefer the remote side for the item
+                    match remote_node.validity {
-                    // and children.
+                        Validity::Valid | Validity::Reupload => ConflictResolution::Remote,
-                    (ConflictResolution::Remote, ConflictResolution::Remote)
+                        // And, for invalid remote items, we must reupload the
-                }
+                        // local side. This _loses remote changes_, but we can't
+                        // apply those changes, anyway.
+                        Validity::Replace => ConflictResolution::Local,
+                    }
+                };
+                // For children, we always use the remote side.
+                (item, ConflictResolution::Remote)
             }
 
             (false, false) => {
@@ -984,57 +1006,25 @@ impl<'t, D: Driver> Merger<'t, D> {
         remote_parent_node: Node<'t>,
         remote_node: Node<'t>,
     ) -> Result<StructureChange> {
-        if remote_node.is_user_content_root() {
-            if let Some(local_node) = self.local_tree.node_for_guid(&remote_node.guid) {
-                let local_parent_node = local_node
-                    .parent()
-                    .expect("Can't check for structure changes without local parent");
-                if remote_parent_node.guid != local_parent_node.guid {
-                    return Ok(StructureChange::Moved);
-                }
-                return Ok(StructureChange::Unchanged);
-            }
-            return Ok(StructureChange::Unchanged);
-        }
-
         if !remote_node_is_syncable(&remote_node) {
             // If the remote node is known to be non-syncable, we unconditionally
-            // delete it from the server, even if it's syncable locally.
+            // delete it, even if it's syncable or moved locally.
-            self.delete_remotely.insert(remote_node.guid.clone());
+            return self.delete_remote_node(merged_node, remote_node);
-            if remote_node.is_folder() {
-                // 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.
-                self.relocate_remote_orphans_to_merged_node(merged_node, remote_node)?;
-            }
-            self.structure_counts.merged_deletions += 1;
-            return Ok(StructureChange::Deleted);
         }
 
         if !self.local_tree.is_deleted(&remote_node.guid) {
             if let Some(local_node) = self.local_tree.node_for_guid(&remote_node.guid) {
                 if !local_node.is_syncable() {
-                    // The remote node is syncable, but the local node is non-syncable.
+                    // The remote node is syncable, but the local node is
-                    // For consistency with Desktop, we unconditionally delete the
+                    // non-syncable. Unconditionally delete it.
-                    // node from the server.
+                    return self.delete_remote_node(merged_node, remote_node);
-                    self.delete_remotely.insert(remote_node.guid.clone());
-                    if remote_node.is_folder() {
-                        self.relocate_remote_orphans_to_merged_node(merged_node, remote_node)?;
-                    }
-                    self.structure_counts.merged_deletions += 1;
-                    return Ok(StructureChange::Deleted);
                 }
                 if local_node.validity == Validity::Replace
                     && remote_node.validity == Validity::Replace
                 {
-                    // The nodes are invalid on both sides, so we can't apply or reupload
+                    // The nodes are invalid on both sides, so we can't apply
-                    // a valid copy. Delete the item from the server.
+                    // or reupload a valid copy. Delete it.
-                    self.delete_remotely.insert(remote_node.guid.clone());
+                    return self.delete_remote_node(merged_node, remote_node);
-                    if remote_node.is_folder() {
-                        self.relocate_remote_orphans_to_merged_node(merged_node, remote_node)?;
-                    }
-                    self.structure_counts.merged_deletions += 1;
-                    return Ok(StructureChange::Deleted);
                 }
                 let local_parent_node = local_node
                     .parent()
@@ -1043,20 +1033,24 @@ impl<'t, D: Driver> Merger<'t, D> {
                     return Ok(StructureChange::Moved);
                 }
                 return Ok(StructureChange::Unchanged);
-            } else {
-                return Ok(StructureChange::Unchanged);
             }
+            if remote_node.validity == Validity::Replace {
+                // The remote node is invalid and doesn't exist locally, so we
+                // can't reupload a valid copy. We must delete it.
+                return self.delete_remote_node(merged_node, remote_node);
+            }
+            return Ok(StructureChange::Unchanged);
         }
 
         if remote_node.validity == Validity::Replace {
-            // If the remote node is invalid, and deleted locally, unconditionally
+            // The remote node is invalid and deleted locally, so we can't
-            // delete the item from the server.
+            // reupload a valid copy. Delete it.
-            self.delete_remotely.insert(remote_node.guid.clone());
+            return self.delete_remote_node(merged_node, remote_node);
-            if remote_node.is_folder() {
+        }
-                self.relocate_remote_orphans_to_merged_node(merged_node, remote_node)?;
+
-            }
+        if remote_node.is_user_content_root() {
-            self.structure_counts.merged_deletions += 1;
+            // If the remote node is a content root, don't delete it locally.
-            return Ok(StructureChange::Deleted);
+            return Ok(StructureChange::Unchanged);
         }
 
         if remote_node.needs_merge {
@@ -1094,12 +1088,7 @@ impl<'t, D: Driver> Merger<'t, D> {
 
         // Take the local deletion and relocate any new remote descendants to the
         // merged node.
-        self.delete_remotely.insert(remote_node.guid.clone());
+        self.delete_remote_node(merged_node, remote_node)
-        if remote_node.is_folder() {
-            self.relocate_remote_orphans_to_merged_node(merged_node, remote_node)?;
-        }
-        self.structure_counts.merged_deletions += 1;
-        Ok(StructureChange::Deleted)
     }
 
     /// Checks if a local node is remotely moved or deleted, and reparents any
@@ -1113,55 +1102,28 @@ impl<'t, D: Driver> Merger<'t, D> {
         local_parent_node: Node<'t>,
         local_node: Node<'t>,
     ) -> Result<StructureChange> {
-        if local_node.is_user_content_root() {
-            if let Some(remote_node) = self.remote_tree.node_for_guid(&local_node.guid) {
-                let remote_parent_node = remote_node
-                    .parent()
-                    .expect("Can't check for structure changes without remote parent");
-                if remote_parent_node.guid != local_parent_node.guid {
-                    return Ok(StructureChange::Moved);
-                }
-                return Ok(StructureChange::Unchanged);
-            }
-            return Ok(StructureChange::Unchanged);
-        }
-
         if !local_node.is_syncable() {
             // If the local node is known to be non-syncable, we unconditionally
-            // delete it from the local tree, even if it's syncable remotely.
+            // delete it, even if it's syncable or moved remotely.
-            self.delete_locally.insert(local_node.guid.clone());
+            return self.delete_local_node(merged_node, local_node);
-            if local_node.is_folder() {
-                self.relocate_local_orphans_to_merged_node(merged_node, local_node)?;
-            }
-            self.structure_counts.merged_deletions += 1;
-            return Ok(StructureChange::Deleted);
         }
 
         if !self.remote_tree.is_deleted(&local_node.guid) {
             if let Some(remote_node) = self.remote_tree.node_for_guid(&local_node.guid) {
                 if !remote_node_is_syncable(&remote_node) {
-                    // The local node is syncable, but the remote node is non-syncable.
+                    // The local node is syncable, but the remote node is not.
-                    // This can happen if we applied an orphaned left pane query in a
+                    // This can happen if we applied an orphaned left pane
-                    // previous sync, and later saw the left pane root on the server.
+                    // query in a previous sync, and later saw the left pane
-                    // Since we now have the complete subtree, we can remove the item.
+                    // root on the server. Since we now have the complete
-                    self.delete_locally.insert(local_node.guid.clone());
+                    // subtree, we can remove it.
-                    if remote_node.is_folder() {
+                    return self.delete_local_node(merged_node, local_node);
-                        self.relocate_local_orphans_to_merged_node(merged_node, local_node)?;
-                    }
-                    self.structure_counts.merged_deletions += 1;
-                    return Ok(StructureChange::Deleted);
                 }
                 if remote_node.validity == Validity::Replace
                     && local_node.validity == Validity::Replace
                 {
-                    // The nodes are invalid on both sides, so we can't apply or reupload
+                    // The nodes are invalid on both sides, so we can't replace
-                    // a valid copy. Delete the item from Places.
+                    // the local copy with a remote one. Delete it.
-                    self.delete_locally.insert(local_node.guid.clone());
+                    return self.delete_local_node(merged_node, local_node);
-                    if local_node.is_folder() {
-                        self.relocate_local_orphans_to_merged_node(merged_node, local_node)?;
-                    }
-                    self.structure_counts.merged_deletions += 1;
-                    return Ok(StructureChange::Deleted);
                 }
                 // Otherwise, either both nodes are valid; or the remote node
                 // is invalid but the local node is valid, so we can reupload a
@@ -1174,20 +1136,27 @@ impl<'t, D: Driver> Merger<'t, D> {
                 }
                 return Ok(StructureChange::Unchanged);
             }
+            if local_node.validity == Validity::Replace {
+                // The local node is invalid and doesn't exist remotely, so
+                // we can't replace the local copy. Delete it.
+                return self.delete_local_node(merged_node, local_node);
+            }
             return Ok(StructureChange::Unchanged);
         }
 
         if local_node.validity == Validity::Replace {
-            // If the local node is invalid, and deleted remotely, unconditionally
+            // The local node is invalid and deleted remotely, so we can't
-            // delete the item from Places.
+            // replace the local copy. Delete it.
-            self.delete_locally.insert(local_node.guid.clone());
+            return self.delete_local_node(merged_node, local_node);
-            if local_node.is_folder() {
-                self.relocate_local_orphans_to_merged_node(merged_node, local_node)?;
-            }
-            self.structure_counts.merged_deletions += 1;
-            return Ok(StructureChange::Deleted);
         }
 
+        if local_node.is_user_content_root() {
+            // If the local node is a content root, don't delete it remotely.
+            return Ok(StructureChange::Unchanged);
+        }
+
+        // See `check_for_local_structure_change_of_remote_node` for an
+        // explanation of how we decide to take or ignore a deletion.
         if local_node.needs_merge {
             if !local_node.is_folder() {
                 trace!(
@@ -1214,26 +1183,21 @@ impl<'t, D: Driver> Merger<'t, D> {
 
         // Take the remote deletion and relocate any new local descendants to the
         // merged node.
-        self.delete_locally.insert(local_node.guid.clone());
+        self.delete_local_node(merged_node, local_node)
-        if local_node.is_folder() {
-            self.relocate_local_orphans_to_merged_node(merged_node, local_node)?;
-        }
-        self.structure_counts.merged_deletions += 1;
-        Ok(StructureChange::Deleted)
     }
 
-    /// Takes a local deletion for a remote node by marking the node as deleted,
+    /// Marks a remote node as deleted, and relocates all remote descendants
-    /// and relocating all remote descendants that aren't also locally deleted
+    /// that aren't also locally deleted to the merged node. This avoids data
-    /// to the closest surviving ancestor. We do this to avoid data loss if
+    /// loss if the user adds a bookmark to a folder on another device, and
-    /// the user adds a bookmark to a folder on another device, and deletes
+    /// deletes that folder locally.
-    /// that folder locally.
     ///
-    /// This is the inverse of `relocate_local_orphans_to_merged_node`.
+    /// This is the inverse of `delete_local_node`.
-    fn relocate_remote_orphans_to_merged_node(
+    fn delete_remote_node(
         &mut self,
         merged_node: &mut MergedNode<'t>,
         remote_node: Node<'t>,
-    ) -> Result<()> {
+    ) -> Result<StructureChange> {
+        self.delete_remotely.insert(remote_node.guid.clone());
         for remote_child_node in remote_node.children() {
             if self.merged_guids.contains(&remote_child_node.guid) {
                 trace!(
@@ -1277,19 +1241,20 @@ impl<'t, D: Driver> Merger<'t, D> {
                 }
             }
         }
-        Ok(())
+        self.structure_counts.merged_deletions += 1;
+        Ok(StructureChange::Deleted)
     }
 
-    /// Takes a remote deletion for a local node by marking the node as deleted,
+    /// Marks a local node as deleted, and relocates all local descendants
-    /// and relocating all local descendants that aren't also remotely deleted
+    /// that aren't also remotely deleted to the merged node.
-    /// to the closest surviving ancestor.
     ///
-    /// This is the inverse of `relocate_remote_orphans_to_merged_node`.
+    /// This is the inverse of `delete_remote_node`.
-    fn relocate_local_orphans_to_merged_node(
+    fn delete_local_node(
         &mut self,
         merged_node: &mut MergedNode<'t>,
         local_node: Node<'t>,
-    ) -> Result<()> {
+    ) -> Result<StructureChange> {
+        self.delete_locally.insert(local_node.guid.clone());
         for local_child_node in local_node.children() {
             if self.merged_guids.contains(&local_child_node.guid) {
                 trace!(
@@ -1333,7 +1298,8 @@ impl<'t, D: Driver> Merger<'t, D> {
                 }
             }
         }
-        Ok(())
+        self.structure_counts.merged_deletions += 1;
+        Ok(StructureChange::Deleted)
     }
 
     /// Finds all children of a local folder with similar content as children of

third_party/rust/dogear/src/tests.rs

@@ -20,7 +20,10 @@ use crate::driver::Driver;
 use crate::error::{ErrorKind, Result};
 use crate::guid::{Guid, ROOT_GUID, UNFILED_GUID};
 use crate::merge::{Merger, StructureCounts};
-use crate::tree::{Builder, Content, IntoTree, Item, Kind, Tree, Validity};
+use crate::tree::{
+    Builder, Content, DivergedParent, DivergedParentGuid, IntoTree, Item, Kind, Problem, Problems,
+    Tree, Validity,
+};
 
 #[derive(Debug)]
 struct Node {
@@ -2567,3 +2570,176 @@ fn cycle() {
         err => assert!(false, "Wrong error kind for cycle: {:?}", err),
     }
 }
+
+#[test]
+fn reupload_replace() {
+    before_each();
+
+    let mut local_tree = nodes!({
+        ("menu________", Folder, {
+            ("bookmarkAAAA", Bookmark)
+        }),
+        ("toolbar_____", Folder, {
+            ("folderBBBBBB", Folder, {
+                ("bookmarkCCCC", Bookmark[validity = Validity::Replace])
+            }),
+            ("folderDDDDDD", Folder, {
+                ("bookmarkEEEE", Bookmark[validity = Validity::Replace])
+            })
+        }),
+        ("unfiled_____", Folder),
+        ("mobile______", Folder, {
+            ("bookmarkFFFF", Bookmark[validity = Validity::Replace]),
+            ("folderGGGGGG", Folder),
+            ("bookmarkHHHH", Bookmark[validity = Validity::Replace])
+        })
+    })
+    .into_tree()
+    .unwrap();
+    local_tree.note_deleted("bookmarkIIII".into());
+
+    let mut remote_tree = nodes!({
+        ("menu________", Folder, {
+            ("bookmarkAAAA", Bookmark[validity = Validity::Replace])
+        }),
+        ("toolbar_____", Folder, {
+            ("bookmarkJJJJ", Bookmark[validity = Validity::Replace]),
+            ("folderBBBBBB", Folder, {
+                ("bookmarkCCCC", Bookmark[validity = Validity::Replace])
+            }),
+            ("folderDDDDDD", Folder)
+        }),
+        ("unfiled_____", Folder, {
+            ("bookmarkKKKK", Bookmark[validity = Validity::Reupload])
+        }),
+        ("mobile______", Folder, {
+            ("bookmarkFFFF", Bookmark),
+            ("folderGGGGGG", Folder, {
+                ("bookmarkIIII", Bookmark[validity = Validity::Replace])
+            })
+        })
+    })
+    .into_tree()
+    .unwrap();
+    remote_tree.note_deleted("bookmarkEEEE".into());
+
+    let mut merger = Merger::new(&local_tree, &remote_tree);
+    let merged_root = merger.merge().unwrap();
+    assert!(merger.subsumes(&local_tree));
+    assert!(merger.subsumes(&remote_tree));
+
+    let expected_tree = nodes!({
+        ("menu________", Folder, {
+            // A is invalid remotely and valid locally, so replace.
+            ("bookmarkAAAA", Bookmark[needs_merge = true])
+        }),
+        // Toolbar has new children.
+        ("toolbar_____", Folder[needs_merge = true], {
+            // B has new children.
+            ("folderBBBBBB", Folder[needs_merge = true]),
+            ("folderDDDDDD", Folder)
+        }),
+        ("unfiled_____", Folder, {
+            // K was flagged for reupload.
+           ("bookmarkKKKK", Bookmark[needs_merge = true])
+        }),
+        ("mobile______", Folder, {
+            // F is invalid locally, so replace with remote. This isn't
+            // possible in Firefox Desktop or Rust Places, where the local
+            // tree is always valid, but we handle it for symmetry.
+            ("bookmarkFFFF", Bookmark),
+            ("folderGGGGGG", Folder[needs_merge = true])
+        })
+    })
+    .into_tree()
+    .unwrap();
+    let expected_deletions = vec![
+        // C is invalid on both sides, so we need to upload a tombstone.
+        ("bookmarkCCCC", true),
+        // E is invalid locally and deleted remotely, so doesn't need a
+        // tombstone.
+        ("bookmarkEEEE", false),
+        // H is invalid locally and doesn't exist remotely, so doesn't need a
+        // tombstone.
+        ("bookmarkHHHH", false),
+        // I is deleted locally and invalid remotely, so needs a tombstone.
+        ("bookmarkIIII", true),
+        // J doesn't exist locally and invalid remotely, so needs a tombstone.
+        ("bookmarkJJJJ", true),
+    ];
+    let expected_telem = StructureCounts {
+        merged_nodes: 10,
+        // C is double-counted: it's deleted on both sides, so
+        // `merged_deletions` is 6, even though we only have 5 expected
+        // deletions.
+        merged_deletions: 6,
+        ..StructureCounts::default()
+    };
+
+    let merged_tree = merged_root.into_tree().unwrap();
+    assert_eq!(merged_tree, expected_tree);
+
+    let mut deletions = merger
+        .deletions()
+        .map(|d| (d.guid.as_ref(), d.should_upload_tombstone))
+        .collect::<Vec<(&str, bool)>>();
+    deletions.sort_by(|a, b| a.0.cmp(&b.0));
+    assert_eq!(deletions, expected_deletions);
+
+    assert_eq!(merger.counts(), &expected_telem);
+}
+
+#[test]
+fn problems() {
+    let mut problems = Problems::default();
+
+    problems
+        .note(&"bookmarkAAAA".into(), Problem::Orphan)
+        .note(
+            &"menu________".into(),
+            Problem::MisparentedRoot(vec![DivergedParent::ByChildren("unfiled_____".into())]),
+        )
+        .note(&"toolbar_____".into(), Problem::MisparentedRoot(Vec::new()))
+        .note(
+            &"bookmarkBBBB".into(),
+            Problem::DivergedParents(vec![
+                DivergedParent::ByChildren("folderCCCCCC".into()),
+                DivergedParentGuid::Folder("folderDDDDDD".into()).into(),
+            ]),
+        )
+        .note(
+            &"bookmarkEEEE".into(),
+            Problem::DivergedParents(vec![
+                DivergedParent::ByChildren("folderFFFFFF".into()),
+                DivergedParentGuid::NonFolder("bookmarkGGGG".into()).into(),
+            ]),
+        )
+        .note(
+            &"bookmarkHHHH".into(),
+            Problem::DivergedParents(vec![
+                DivergedParent::ByChildren("folderIIIIII".into()),
+                DivergedParent::ByChildren("folderJJJJJJ".into()),
+                DivergedParentGuid::Missing("folderKKKKKK".into()).into(),
+            ]),
+        )
+        .note(&"bookmarkLLLL".into(), Problem::DivergedParents(Vec::new()));
+
+    let mut summary = problems.summarize().collect::<Vec<_>>();
+    summary.sort_by(|a, b| a.guid().cmp(b.guid()));
+    assert_eq!(
+        summary
+            .into_iter()
+            .map(|s| s.to_string())
+            .collect::<Vec<String>>(),
+        &[
+            "bookmarkAAAA is an orphan",
+            "bookmarkBBBB is in children of folderCCCCCC and has parent folderDDDDDD",
+            "bookmarkEEEE is in children of folderFFFFFF and has non-folder parent bookmarkGGGG",
+            "bookmarkHHHH is in children of folderIIIIII, is in children of folderJJJJJJ, and has \
+             nonexistent parent folderKKKKKK",
+            "bookmarkLLLL has diverged parents",
+            "menu________ is a user content root, but is in children of unfiled_____",
+            "toolbar_____ is a user content root",
+        ]
+    );
+}

third_party/rust/dogear/src/tree.rs

@@ -13,6 +13,7 @@
 // limitations under the License.
 
 use std::{
+    borrow::Cow,
     cmp::Ordering,
     collections::{HashMap, HashSet},
     fmt, mem,
@@ -46,6 +47,7 @@ pub struct Tree {
     entry_index_by_guid: HashMap<Guid, Index>,
     entries: Vec<TreeEntry>,
     deleted_guids: HashSet<Guid>,
+    problems: Problems,
 }
 
 impl Tree {
@@ -108,6 +110,12 @@ impl Tree {
             .get(guid)
             .map(|&index| Node(self, &self.entries[index]))
     }
+
+    /// Returns the structure divergences found when building the tree.
+    #[inline]
+    pub fn problems(&self) -> &Problems {
+        &self.problems
+    }
 }
 
 impl IntoTree for Tree {
@@ -120,20 +128,26 @@ impl IntoTree for Tree {
 impl fmt::Display for Tree {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
         let root = self.root();
-        let deleted_guids = self
+        f.write_str(&root.to_ascii_string())?;
-            .deleted_guids
+        if !self.deleted_guids.is_empty() {
-            .iter()
+            f.write_str("\nDeleted: [")?;
-            .map(|guid| guid.as_ref())
+            for (i, guid) in self.deleted_guids.iter().enumerate() {
-            .collect::<Vec<&str>>();
+                if i != 0 {
-        match deleted_guids.len() {
+                    f.write_str(", ")?;
-            0 => write!(f, "{}", root.to_ascii_string()),
+                }
-            _ => write!(
+                f.write_str(guid.as_ref())?;
-                f,
+            }
-                "{}\nDeleted: [{}]",
-                root.to_ascii_string(),
-                deleted_guids.join(",")
-            ),
         }
+        if !self.problems.is_empty() {
+            f.write_str("\nProblems:\n")?;
+            for (i, summary) in self.problems.summarize().enumerate() {
+                if i != 0 {
+                    f.write_str("\n")?;
+                }
+                write!(f, "❗️ {}", summary)?;
+            }
+        }
+        Ok(())
     }
 }
 
@@ -273,179 +287,6 @@ impl Builder {
         };
         ParentBuilder(self, entry_child)
     }
-
-    /// Returns the index of the default parent entry for reparented orphans.
-    /// This is either the default folder (rule 4), or the root, if the
-    /// default folder isn't set, doesn't exist, or isn't a folder (rule 5).
-    fn reparent_orphans_to_default_index(&self) -> Index {
-        self.reparent_orphans_to
-            .as_ref()
-            .and_then(|guid| self.entry_index_by_guid.get(guid))
-            .cloned()
-            .filter(|&parent_index| {
-                let parent_entry = &self.entries[parent_index];
-                parent_entry.item.is_folder()
-            })
-            .unwrap_or(0)
-    }
-
-    /// Resolves parents for all entries. Returns a vector of resolved parents
-    /// by the entry index, and a lookup table for reparented orphans.
-    fn resolve(&self) -> (Vec<ResolvedParent>, HashMap<Index, Vec<Index>>) {
-        let mut parents = Vec::with_capacity(self.entries.len());
-        let mut reparented_orphans_by_parent: HashMap<Index, Vec<Index>> = HashMap::new();
-        for (entry_index, entry) in self.entries.iter().enumerate() {
-            let mut resolved_parent = match &entry.parent {
-                BuilderEntryParent::Root => ResolvedParent::Root,
-                BuilderEntryParent::None => {
-                    // The item doesn't have a `parentid` _or_ `children`.
-                    // Reparent to the default folder.
-                    let parent_index = self.reparent_orphans_to_default_index();
-                    ResolvedParent::ByParentGuid(parent_index)
-                }
-                BuilderEntryParent::Complete(index) => {
-                    // The item has a complete structure. This is the fast path
-                    // for local trees.
-                    ResolvedParent::Unchanged(*index)
-                }
-                BuilderEntryParent::Partial(parents) => match parents.as_slice() {
-                    [BuilderParentBy::UnknownItem(by_item), BuilderParentBy::Children(by_children)]
-                    | [BuilderParentBy::Children(by_children), BuilderParentBy::UnknownItem(by_item)] =>
-                    {
-                        self.entry_index_by_guid
-                            .get(by_item)
-                            .filter(|by_item| by_item == &by_children)
-                            .map(|&by_item| {
-                                // The partial structure is actually complete.
-                                // This is the "fast slow path" for remote
-                                // trees, because we add their structure in
-                                // two passes.
-                                ResolvedParent::Unchanged(by_item)
-                            })
-                            .unwrap_or_else(|| ResolvedParent::ByChildren(*by_children))
-                    }
-
-                    parents => {
-                        // For items with zero, one, or more than two parents, we pick
-                        // the newest (minimum age), preferring parents from `children`
-                        // over `parentid` (rules 2-3).
-                        parents
-                            .iter()
-                            .min_by(|parent, other_parent| {
-                                let (parent_index, other_parent_index) =
-                                    match (parent, other_parent) {
-                                        (
-                                            BuilderParentBy::Children(parent_index),
-                                            BuilderParentBy::Children(other_parent_index),
-                                        ) => (*parent_index, *other_parent_index),
-                                        (
-                                            BuilderParentBy::Children(_),
-                                            BuilderParentBy::KnownItem(_),
-                                        ) => {
-                                            return Ordering::Less;
-                                        }
-                                        (
-                                            BuilderParentBy::Children(_),
-                                            BuilderParentBy::UnknownItem(_),
-                                        ) => {
-                                            return Ordering::Less;
-                                        }
-
-                                        (
-                                            BuilderParentBy::KnownItem(parent_index),
-                                            BuilderParentBy::KnownItem(other_parent_index),
-                                        ) => (*parent_index, *other_parent_index),
-                                        (
-                                            BuilderParentBy::KnownItem(_),
-                                            BuilderParentBy::Children(_),
-                                        ) => {
-                                            return Ordering::Greater;
-                                        }
-                                        (
-                                            BuilderParentBy::KnownItem(_),
-                                            BuilderParentBy::UnknownItem(_),
-                                        ) => {
-                                            return Ordering::Less;
-                                        }
-
-                                        (
-                                            BuilderParentBy::UnknownItem(parent_guid),
-                                            BuilderParentBy::UnknownItem(other_parent_guid),
-                                        ) => {
-                                            match (
-                                                self.entry_index_by_guid.get(parent_guid),
-                                                self.entry_index_by_guid.get(other_parent_guid),
-                                            ) {
-                                                (Some(parent_index), Some(other_parent_index)) => {
-                                                    (*parent_index, *other_parent_index)
-                                                }
-                                                (Some(_), None) => return Ordering::Less,
-                                                (None, Some(_)) => return Ordering::Greater,
-                                                (None, None) => return Ordering::Equal,
-                                            }
-                                        }
-                                        (
-                                            BuilderParentBy::UnknownItem(_),
-                                            BuilderParentBy::Children(_),
-                                        ) => {
-                                            return Ordering::Greater;
-                                        }
-                                        (
-                                            BuilderParentBy::UnknownItem(_),
-                                            BuilderParentBy::KnownItem(_),
-                                        ) => {
-                                            return Ordering::Greater;
-                                        }
-                                    };
-                                let parent_entry = &self.entries[parent_index];
-                                let other_parent_entry = &self.entries[other_parent_index];
-                                parent_entry.item.age.cmp(&other_parent_entry.item.age)
-                            })
-                            .and_then(|parent_from| match parent_from {
-                                BuilderParentBy::Children(index) => {
-                                    Some(ResolvedParent::ByChildren(*index))
-                                }
-                                BuilderParentBy::KnownItem(index) => {
-                                    Some(ResolvedParent::ByParentGuid(*index))
-                                }
-                                BuilderParentBy::UnknownItem(guid) => self
-                                    .entry_index_by_guid
-                                    .get(guid)
-                                    .filter(|&&index| self.entries[index].item.is_folder())
-                                    .map(|&index| ResolvedParent::ByParentGuid(index)),
-                            })
-                            .unwrap_or_else(|| {
-                                // Fall back to the default folder (rule 4) or root
-                                // (rule 5) if we didn't find a parent.
-                                let parent_index = self.reparent_orphans_to_default_index();
-                                ResolvedParent::ByParentGuid(parent_index)
-                            })
-                    }
-                },
-            };
-            if entry.item.guid.is_user_content_root() {
-                // ...But user content roots should always be in the Places
-                // root (rule 1).
-                resolved_parent = match resolved_parent {
-                    ResolvedParent::Unchanged(parent_index) if parent_index == 0 => {
-                        ResolvedParent::Unchanged(parent_index)
-                    }
-                    _ => ResolvedParent::ByParentGuid(0),
-                };
-            }
-            if let ResolvedParent::ByParentGuid(parent_index) = &resolved_parent {
-                // Reparented orphans are special: since we don't know their positions,
-                // we want to move them to the end of their chosen parents, after any
-                // `children` (rules 3-4).
-                let reparented_orphans = reparented_orphans_by_parent
-                    .entry(*parent_index)
-                    .or_default();
-                reparented_orphans.push(entry_index);
-            }
-            parents.push(resolved_parent);
-        }
-        (parents, reparented_orphans_by_parent)
-    }
 }
 
 impl IntoTree for Builder {
@@ -453,94 +294,133 @@ impl IntoTree for Builder {
     /// resolving inconsistencies like orphans, multiple parents, and
     /// parent-child disagreements.
     fn into_tree(self) -> Result<Tree> {
-        // First, resolve parents for all entries. We build two data structures:
+        let mut problems = Problems::default();
-        // a vector of resolved parents, and a lookup table for reparented
+
-        // orphaned children.
+        // First, resolve parents for all entries, and build a lookup table for
-        let (parents, mut reparented_orphans_by_parent) = self.resolve();
+        // items without a position.
+        let mut parents = Vec::with_capacity(self.entries.len());
+        let mut reparented_child_indices_by_parent: HashMap<Index, Vec<Index>> = HashMap::new();
+        for (entry_index, entry) in self.entries.iter().enumerate() {
+            let r = ResolveParent::new(&self, entry, &mut problems);
+            let resolved_parent = r.resolve();
+            if let ResolvedParent::ByParentGuid(parent_index) = &resolved_parent {
+                // Reparented items are special: since they aren't mentioned in
+                // that parent's `children`, we don't know their positions. Note
+                // them for when we resolve children. We also clone the GUID,
+                // since we use it for sorting, but can't access it by
+                // reference once we call `self.entries.into_iter()` below.
+                let reparented_child_indices = reparented_child_indices_by_parent
+                    .entry(*parent_index)
+                    .or_default();
+                reparented_child_indices.push(entry_index);
+            }
+            parents.push(resolved_parent);
+        }
+
+        // If any parents form cycles, abort. We haven't seen cyclic trees in
+        // the wild, and breaking cycles would add complexity.
         if let Some(index) = detect_cycles(&parents) {
             return Err(ErrorKind::Cycle(self.entries[index].item.guid.clone()).into());
         }
-        for reparented_orphans in reparented_orphans_by_parent.values_mut() {
-            // Use a deterministic order for reparented orphans.
-            reparented_orphans.sort_unstable_by(|&index, &other_index| {
-                self.entries[index]
-                    .item
-                    .guid
-                    .cmp(&self.entries[other_index].item.guid)
-            });
-        }
 
-        // Transform our builder entries into tree entries, with resolved
+        // Then, resolve children, and build a slab of entries for the tree.
-        // parents and children.
+        let mut entries = Vec::with_capacity(self.entries.len());
-        let entries = self
+        for (entry_index, entry) in self.entries.into_iter().enumerate() {
-            .entries
+            // Each entry is consistent, until proven otherwise!
-            .into_iter()
+            let mut divergence = Divergence::Consistent;
-            .enumerate()
-            .map(|(entry_index, entry)| {
-                let mut divergence = Divergence::Consistent;
 
-                let parent_index = match &parents[entry_index] {
+            let parent_index = match &parents[entry_index] {
-                    ResolvedParent::Root => None,
+                ResolvedParent::Root => {
-                    ResolvedParent::Unchanged(index) => Some(*index),
+                    // The Places root doesn't have a parent, and should always
-                    ResolvedParent::ByChildren(index) | ResolvedParent::ByParentGuid(index) => {
+                    // be the first entry.
-                        divergence = Divergence::Diverged;
+                    assert_eq!(entry_index, 0);
-                        Some(*index)
+                    None
-                    }
+                }
-                };
+                ResolvedParent::ByStructure(index) => {
+                    // The entry has a valid parent by structure, yay!
+                    Some(*index)
+                }
+                ResolvedParent::ByChildren(index) | ResolvedParent::ByParentGuid(index) => {
+                    // The entry has multiple parents, and we resolved one,
+                    // so it's diverged.
+                    divergence = Divergence::Diverged;
+                    Some(*index)
+                }
+            };
 
-                let mut child_indices = entry
+            // Check if the entry's children exist and agree that this entry is
-                    .children
+            // their parent.
-                    .iter()
+            let mut child_indices = Vec::with_capacity(entry.children.len());
-                    .filter_map(|child_index| {
+            for child in entry.children {
-                        // Filter out missing children and children that moved to a
+                match child {
-                        // different parent.
+                    BuilderEntryChild::Exists(child_index) => match &parents[child_index] {
-                        match child_index {
+                        ResolvedParent::Root => {
-                            BuilderEntryChild::Exists(child_index) => {
+                            // The Places root can't be a child of another entry.
-                                match &parents[*child_index] {
+                            unreachable!("A child can't be a top-level root");
-                                    ResolvedParent::Root | ResolvedParent::Unchanged(_) => {
+                        }
-                                        Some(*child_index)
+                        ResolvedParent::ByStructure(parent_index) => {
-                                    }
+                            // If the child has a valid parent by structure, it
-
+                            // must be the entry. If it's not, there's a bug
-                                    ResolvedParent::ByChildren(parent_index)
+                            // in `ResolveParent` or `BuilderEntry`.
-                                    | ResolvedParent::ByParentGuid(parent_index) => {
+                            assert_eq!(*parent_index, entry_index);
-                                        divergence = Divergence::Diverged;
+                            child_indices.push(child_index);
-                                        if *parent_index == entry_index {
+                        }
-                                            Some(*child_index)
+                        ResolvedParent::ByChildren(parent_index) => {
-                                        } else {
+                            // If the child has multiple parents, we may have
-                                            None
+                            // resolved a different one, so check if we decided
-                                        }
+                            // to keep the child in this entry.
-                                    }
+                            divergence = Divergence::Diverged;
-                                }
+                            if *parent_index == entry_index {
-                            }
+                                child_indices.push(child_index);
-                            BuilderEntryChild::Missing(_) => {
-                                divergence = Divergence::Diverged;
-                                None
                             }
                         }
-                    })
+                        ResolvedParent::ByParentGuid(parent_index) => {
-                    .collect::<Vec<_>>();
+                            // We should only ever prefer parents
-                if let Some(mut reparented_orphans) =
+                            // `by_parent_guid` over parents `by_children` for
-                    reparented_orphans_by_parent.get_mut(&entry_index)
+                            // misparented user content roots. Otherwise,
-                {
+                            // there's a bug in `ResolveParent`.
-                    // Add reparented orphans to the end.
+                            assert_eq!(*parent_index, 0);
-                    divergence = Divergence::Diverged;
+                            divergence = Divergence::Diverged;
-                    child_indices.append(&mut reparented_orphans);
+                        }
+                    },
+                    BuilderEntryChild::Missing(child_guid) => {
+                        // If the entry's `children` mentions a GUID for which
+                        // we don't have an entry, note it as a problem, and
+                        // ignore the child.
+                        divergence = Divergence::Diverged;
+                        problems.note(
+                            &entry.item.guid,
+                            Problem::MissingChild {
+                                child_guid: child_guid.clone(),
+                            },
+                        );
+                    }
                 }
+            }
 
-                TreeEntry {
+            // Reparented items don't appear in our `children`, so we move them
-                    item: entry.item,
+            // to the end, after existing children (rules 3-4).
-                    parent_index,
+            if let Some(reparented_child_indices) =
-                    child_indices,
+                reparented_child_indices_by_parent.get(&entry_index)
-                    divergence,
+            {
-                }
+                divergence = Divergence::Diverged;
-            })
+                child_indices.extend_from_slice(reparented_child_indices);
-            .collect::<Vec<_>>();
+            }
+
+            entries.push(TreeEntry {
+                item: entry.item,
+                parent_index,
+                child_indices,
+                divergence,
+            });
+        }
 
         // Now we have a consistent tree.
         Ok(Tree {
             entry_index_by_guid: self.entry_index_by_guid,
             entries,
             deleted_guids: HashSet::new(),
+            problems,
         })
     }
 }
@@ -727,6 +607,7 @@ impl BuilderEntry {
     }
 }
 
+/// Holds an existing child index, or missing child GUID, for a builder entry.
 #[derive(Debug)]
 enum BuilderEntryChild {
     Exists(Index),
@@ -734,7 +615,7 @@ enum BuilderEntryChild {
 }
 
 /// Holds one or more parents for a builder entry.
-#[derive(Debug)]
+#[derive(Clone, Debug)]
 enum BuilderEntryParent {
     /// The entry is an orphan.
     None,
@@ -769,11 +650,321 @@ enum BuilderParentBy {
     KnownItem(Index),
 }
 
+/// Resolves the parent for a builder entry.
+struct ResolveParent<'a> {
+    builder: &'a Builder,
+    entry: &'a BuilderEntry,
+    problems: &'a mut Problems,
+}
+
+impl<'a> ResolveParent<'a> {
+    fn new(
+        builder: &'a Builder,
+        entry: &'a BuilderEntry,
+        problems: &'a mut Problems,
+    ) -> ResolveParent<'a> {
+        ResolveParent {
+            builder,
+            entry,
+            problems,
+        }
+    }
+
+    fn resolve(self) -> ResolvedParent {
+        if self.entry.item.guid.is_user_content_root() {
+            self.user_content_root()
+        } else {
+            self.item()
+        }
+    }
+
+    /// Returns the parent for this builder entry. This unifies parents
+    /// `by_structure`, which are known to be consistent, and parents
+    /// `by_children` and `by_parent_guid`, which are consistent if they match.
+    fn parent(&self) -> Cow<'a, BuilderEntryParent> {
+        let parents = match &self.entry.parent {
+            // Roots and orphans pass through as-is.
+            BuilderEntryParent::Root => return Cow::Owned(BuilderEntryParent::Root),
+            BuilderEntryParent::None => return Cow::Owned(BuilderEntryParent::None),
+            BuilderEntryParent::Complete(index) => {
+                // The entry is known to have a valid parent by structure. This
+                // is the fast path, used for local trees in Desktop.
+                return Cow::Owned(BuilderEntryParent::Complete(*index));
+            }
+            BuilderEntryParent::Partial(parents) => parents,
+        };
+        // The entry has zero, one, or many parents, recorded separately. Check
+        // if it has exactly two: one `by_parent_guid`, and one `by_children`.
+        let (index_by_guid, index_by_children) = match parents.as_slice() {
+            [BuilderParentBy::UnknownItem(guid), BuilderParentBy::Children(index_by_children)]
+            | [BuilderParentBy::Children(index_by_children), BuilderParentBy::UnknownItem(guid)] => {
+                match self.builder.entry_index_by_guid.get(guid) {
+                    Some(&index_by_guid) => (index_by_guid, *index_by_children),
+                    None => return Cow::Borrowed(&self.entry.parent),
+                }
+            }
+            [BuilderParentBy::KnownItem(index_by_guid), BuilderParentBy::Children(index_by_children)]
+            | [BuilderParentBy::Children(index_by_children), BuilderParentBy::KnownItem(index_by_guid)] => {
+                (*index_by_guid, *index_by_children)
+            }
+            // In all other cases (missing `parentid`, missing from `children`,
+            // multiple parents), return all possible parents. We'll pick one
+            // when we resolve the parent.
+            _ => return Cow::Borrowed(&self.entry.parent),
+        };
+        // If the entry has matching parents `by_children` and `by_parent_guid`,
+        // it has a valid parent by structure. This is the "fast slow path",
+        // used for remote trees in Desktop, because their structure is built in
+        // two passes. In all other cases, we have a parent-child disagreement,
+        // so return all possible parents.
+        if index_by_guid == index_by_children {
+            Cow::Owned(BuilderEntryParent::Complete(index_by_children))
+        } else {
+            Cow::Borrowed(&self.entry.parent)
+        }
+    }
+
+    /// Resolves the parent for a user content root: menu, mobile, toolbar, and
+    /// unfiled. These are simpler to resolve than non-roots because they must
+    /// be children of the Places root (rule 1), which is always the first
+    /// entry.
+    fn user_content_root(self) -> ResolvedParent {
+        match self.parent().as_ref() {
+            BuilderEntryParent::None => {
+                // Orphaned content root. This should only happen if the content
+                // root doesn't have a parent `by_parent_guid`.
+                self.problems.note(&self.entry.item.guid, Problem::Orphan);
+                ResolvedParent::ByParentGuid(0)
+            }
+            BuilderEntryParent::Root => {
+                unreachable!("A user content root can't be a top-level root")
+            }
+            BuilderEntryParent::Complete(index) => {
+                if *index == 0 {
+                    ResolvedParent::ByStructure(*index)
+                } else {
+                    // Move misparented content roots to the Places root.
+                    let parent_guid = self.builder.entries[*index].item.guid.clone();
+                    self.problems.note(
+                        &self.entry.item.guid,
+                        Problem::MisparentedRoot(vec![
+                            DivergedParent::ByChildren(parent_guid.clone()),
+                            DivergedParentGuid::Folder(parent_guid).into(),
+                        ]),
+                    );
+                    ResolvedParent::ByParentGuid(0)
+                }
+            }
+            BuilderEntryParent::Partial(parents_by) => {
+                // Ditto for content roots with multiple parents or parent-child
+                // disagreements.
+                self.problems.note(
+                    &self.entry.item.guid,
+                    Problem::MisparentedRoot(
+                        parents_by
+                            .iter()
+                            .map(|parent_by| {
+                                PossibleParent::new(self.builder, parent_by).summarize()
+                            })
+                            .collect(),
+                    ),
+                );
+                ResolvedParent::ByParentGuid(0)
+            }
+        }
+    }
+
+    /// Resolves the parent for a top-level Places root or other item, using
+    /// rules 2-5.
+    fn item(self) -> ResolvedParent {
+        match self.parent().as_ref() {
+            BuilderEntryParent::Root => ResolvedParent::Root,
+            BuilderEntryParent::None => {
+                // The item doesn't have a `parentid`, and isn't mentioned in
+                // any `children`. Reparent to the default folder (rule 4) or
+                // Places root (rule 5).
+                let parent_index = self.reparent_orphans_to_default_index();
+                self.problems.note(&self.entry.item.guid, Problem::Orphan);
+                ResolvedParent::ByParentGuid(parent_index)
+            }
+            BuilderEntryParent::Complete(index) => {
+                // The item's `parentid` and parent's `children` match, so keep
+                // it in its current parent.
+                ResolvedParent::ByStructure(*index)
+            }
+            BuilderEntryParent::Partial(parents) => {
+                // For items with one or more than two parents, pick the
+                // youngest (minimum age).
+                let possible_parents = parents
+                    .iter()
+                    .map(|parent_by| PossibleParent::new(self.builder, parent_by))
+                    .collect::<Vec<_>>();
+                self.problems.note(
+                    &self.entry.item.guid,
+                    Problem::DivergedParents(
+                        possible_parents.iter().map(|p| p.summarize()).collect(),
+                    ),
+                );
+                possible_parents
+                    .into_iter()
+                    .min()
+                    .and_then(|p| match p.parent_by {
+                        BuilderParentBy::Children(index) => {
+                            Some(ResolvedParent::ByChildren(*index))
+                        }
+                        BuilderParentBy::KnownItem(index) => {
+                            Some(ResolvedParent::ByParentGuid(*index))
+                        }
+                        BuilderParentBy::UnknownItem(guid) => self
+                            .builder
+                            .entry_index_by_guid
+                            .get(guid)
+                            .filter(|&&index| self.builder.entries[index].item.is_folder())
+                            .map(|&index| ResolvedParent::ByParentGuid(index)),
+                    })
+                    .unwrap_or_else(|| {
+                        // Fall back to the default folder (rule 4) or root
+                        // (rule 5) if we didn't find a parent.
+                        let parent_index = self.reparent_orphans_to_default_index();
+                        ResolvedParent::ByParentGuid(parent_index)
+                    })
+            }
+        }
+    }
+
+    /// Returns the index of the default parent entry for reparented orphans.
+    /// This is either the default folder (rule 4), or the root, if the
+    /// default folder isn't set, doesn't exist, or isn't a folder (rule 5).
+    fn reparent_orphans_to_default_index(&self) -> Index {
+        self.builder
+            .reparent_orphans_to
+            .as_ref()
+            .and_then(|guid| self.builder.entry_index_by_guid.get(guid))
+            .cloned()
+            .filter(|&parent_index| {
+                let parent_entry = &self.builder.entries[parent_index];
+                parent_entry.item.is_folder()
+            })
+            .unwrap_or(0)
+    }
+}
+
+// A possible parent for an item with conflicting parents. We use this wrapper's
+// `Ord` implementation to decide which parent is youngest.
+#[derive(Clone, Copy, Debug)]
+struct PossibleParent<'a> {
+    builder: &'a Builder,
+    parent_by: &'a BuilderParentBy,
+}
+
+impl<'a> PossibleParent<'a> {
+    fn new(builder: &'a Builder, parent_by: &'a BuilderParentBy) -> PossibleParent<'a> {
+        PossibleParent { builder, parent_by }
+    }
+
+    /// Returns the problem with this conflicting parent.
+    fn summarize(&self) -> DivergedParent {
+        let entry = match self.parent_by {
+            BuilderParentBy::Children(index) => {
+                return DivergedParent::ByChildren(self.builder.entries[*index].item.guid.clone());
+            }
+            BuilderParentBy::KnownItem(index) => &self.builder.entries[*index],
+            BuilderParentBy::UnknownItem(guid) => {
+                match self.builder.entry_index_by_guid.get(guid) {
+                    Some(index) => &self.builder.entries[*index],
+                    None => return DivergedParentGuid::Missing(guid.clone()).into(),
+                }
+            }
+        };
+        if entry.item.is_folder() {
+            DivergedParentGuid::Folder(entry.item.guid.clone()).into()
+        } else {
+            DivergedParentGuid::NonFolder(entry.item.guid.clone()).into()
+        }
+    }
+}
+
+impl<'a> Ord for PossibleParent<'a> {
+    /// Compares two possible parents to determine which is younger
+    /// (`Ordering::Less`). Prefers parents from `children` over `parentid`
+    /// (rule 2), and `parentid`s that reference folders over non-folders
+    /// (rule 4).
+    fn cmp(&self, other: &PossibleParent) -> Ordering {
+        let (index, other_index) = match (&self.parent_by, &other.parent_by) {
+            (BuilderParentBy::Children(index), BuilderParentBy::Children(other_index)) => {
+                // Both `self` and `other` mention the item in their `children`.
+                (*index, *other_index)
+            }
+            (BuilderParentBy::Children(_), BuilderParentBy::KnownItem(_)) => {
+                // `self` mentions the item in its `children`, and the item's
+                // `parentid` is `other`, so prefer `self`.
+                return Ordering::Less;
+            }
+            (BuilderParentBy::Children(_), BuilderParentBy::UnknownItem(_)) => {
+                // As above, except we don't know if `other` exists. We don't
+                // need to look it up, though, because we can unconditionally
+                // prefer `self`.
+                return Ordering::Less;
+            }
+            (BuilderParentBy::KnownItem(_), BuilderParentBy::Children(_)) => {
+                // The item's `parentid` is `self`, and `other` mentions the
+                // item in its `children`, so prefer `other`.
+                return Ordering::Greater;
+            }
+            (BuilderParentBy::UnknownItem(_), BuilderParentBy::Children(_)) => {
+                // As above. We don't know if `self` exists, but we
+                // unconditionally prefer `other`.
+                return Ordering::Greater;
+            }
+            // Cases where `self` and `other` are `parentid`s, existing or not,
+            // are academic, since it doesn't make sense for an item to have
+            // multiple `parentid`s.
+            _ => return Ordering::Equal,
+        };
+        // If both `self` and `other` are folders, compare timestamps. If one is
+        // a folder, but the other isn't, we prefer the folder. If neither is a
+        // folder, it doesn't matter.
+        let entry = &self.builder.entries[index];
+        let other_entry = &self.builder.entries[other_index];
+        match (entry.item.is_folder(), other_entry.item.is_folder()) {
+            (true, true) => entry.item.age.cmp(&other_entry.item.age),
+            (false, true) => Ordering::Greater,
+            (true, false) => Ordering::Less,
+            (false, false) => Ordering::Equal,
+        }
+    }
+}
+
+impl<'a> PartialOrd for PossibleParent<'a> {
+    fn partial_cmp(&self, other: &PossibleParent) -> Option<Ordering> {
+        Some(self.cmp(other))
+    }
+}
+
+impl<'a> PartialEq for PossibleParent<'a> {
+    fn eq(&self, other: &PossibleParent) -> bool {
+        self.cmp(other) == Ordering::Equal
+    }
+}
+
+impl<'a> Eq for PossibleParent<'a> {}
+
+/// Describes a resolved parent for an item.
 #[derive(Debug)]
 enum ResolvedParent {
+    /// The item is a top-level root, and has no parent.
     Root,
-    Unchanged(Index),
+
+    /// The item has a valid, consistent structure.
+    ByStructure(Index),
+
+    /// The item has multiple parents; this is the one we picked.
     ByChildren(Index),
+
+    /// The item has a parent-child disagreement: the folder referenced by the
+    /// item's `parentid` doesn't mention the item in its `children`, the
+    /// `parentid` doesn't exist at all, or the item is a misparented content
+    /// root.
     ByParentGuid(Index),
 }
 
@@ -781,7 +972,7 @@ impl ResolvedParent {
     fn index(&self) -> Option<Index> {
         match self {
             ResolvedParent::Root => None,
-            ResolvedParent::Unchanged(index)
+            ResolvedParent::ByStructure(index)
             | ResolvedParent::ByChildren(index)
             | ResolvedParent::ByParentGuid(index) => Some(*index),
         }
@@ -816,17 +1007,168 @@ fn detect_cycles(parents: &[ResolvedParent]) -> Option<Index> {
     None
 }
 
+/// Indicates if a tree entry's structure diverged.
 #[derive(Debug)]
 enum Divergence {
-    /// The node's structure is already correct, and doesn't need to be
+    /// The structure is already correct, and doesn't need to be reuploaded.
-    /// reuploaded.
     Consistent,
 
-    /// The node exists in multiple parents, or is a reparented orphan.
+    /// The node has structure problems, and should be flagged for reupload
-    /// The merger should reupload the node.
+    /// when merging.
     Diverged,
 }
 
+/// Describes a structure divergence for an item in a bookmark tree. These are
+/// used for logging and validation telemetry.
+#[derive(Clone, Debug, Eq, Hash, PartialEq)]
+pub enum Problem {
+    /// The item doesn't have a `parentid`, and isn't mentioned in any folders.
+    Orphan,
+
+    /// The item is a user content root (menu, mobile, toolbar, or unfiled),
+    /// but `parent_guid` isn't the Places root.
+    MisparentedRoot(Vec<DivergedParent>),
+
+    /// The item has diverging parents. If the vector contains more than one
+    /// `DivergedParent::ByChildren`, the item has multiple parents. If the
+    /// vector contains a `DivergedParent::ByParentGuid`, with or without a
+    /// `DivergedParent::ByChildren`, the item has a parent-child disagreement.
+    DivergedParents(Vec<DivergedParent>),
+
+    /// The item is mentioned in a folder's `children`, but doesn't exist or is
+    /// deleted.
+    MissingChild { child_guid: Guid },
+}
+
+/// Describes where an invalid parent comes from.
+#[derive(Clone, Debug, Eq, Hash, PartialEq)]
+pub enum DivergedParent {
+    /// The item appears in this folder's `children`.
+    ByChildren(Guid),
+    /// The `parentid` references this folder.
+    ByParentGuid(DivergedParentGuid),
+}
+
+impl From<DivergedParentGuid> for DivergedParent {
+    fn from(d: DivergedParentGuid) -> DivergedParent {
+        DivergedParent::ByParentGuid(d)
+    }
+}
+
+impl fmt::Display for DivergedParent {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        match self {
+            DivergedParent::ByChildren(parent_guid) => {
+                write!(f, "is in children of {}", parent_guid)
+            }
+            DivergedParent::ByParentGuid(p) => match p {
+                DivergedParentGuid::Folder(parent_guid) => write!(f, "has parent {}", parent_guid),
+                DivergedParentGuid::NonFolder(parent_guid) => {
+                    write!(f, "has non-folder parent {}", parent_guid)
+                }
+                DivergedParentGuid::Missing(parent_guid) => {
+                    write!(f, "has nonexistent parent {}", parent_guid)
+                }
+            },
+        }
+    }
+}
+
+/// Describes an invalid `parentid`.
+#[derive(Clone, Debug, Eq, Hash, PartialEq)]
+pub enum DivergedParentGuid {
+    /// Exists and is a folder.
+    Folder(Guid),
+    /// Exists, but isn't a folder.
+    NonFolder(Guid),
+    /// Doesn't exist at all.
+    Missing(Guid),
+}
+
+/// Records problems for all items in a tree.
+#[derive(Debug, Default)]
+pub struct Problems(HashMap<Guid, Vec<Problem>>);
+
+impl Problems {
+    /// Notes a problem for an item.
+    pub fn note(&mut self, guid: &Guid, problem: Problem) -> &mut Problems {
+        self.0.entry(guid.clone()).or_default().push(problem);
+        self
+    }
+
+    /// Returns `true` if there are no problems.
+    pub fn is_empty(&self) -> bool {
+        self.0.is_empty()
+    }
+
+    /// Returns an iterator for all problems.
+    pub fn summarize(&self) -> impl Iterator<Item = ProblemSummary> {
+        self.0.iter().flat_map(|(guid, problems)| {
+            problems
+                .iter()
+                .map(move |problem| ProblemSummary(guid, problem))
+        })
+    }
+}
+
+/// A printable summary of a problem for an item.
+#[derive(Clone, Copy, Debug)]
+pub struct ProblemSummary<'a>(&'a Guid, &'a Problem);
+
+impl<'a> ProblemSummary<'a> {
+    #[inline]
+    pub fn guid(&self) -> &Guid {
+        &self.0
+    }
+
+    #[inline]
+    pub fn problem(&self) -> &Problem {
+        &self.1
+    }
+}
+
+impl<'a> fmt::Display for ProblemSummary<'a> {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        let parents = match self.problem() {
+            Problem::Orphan => return write!(f, "{} is an orphan", self.guid()),
+            Problem::MisparentedRoot(parents) => {
+                write!(f, "{} is a user content root", self.guid())?;
+                if parents.is_empty() {
+                    return Ok(());
+                }
+                f.write_str(", but ")?;
+                parents
+            }
+            Problem::DivergedParents(parents) => {
+                if parents.is_empty() {
+                    return write!(f, "{} has diverged parents", self.guid());
+                }
+                write!(f, "{} ", self.guid())?;
+                parents
+            }
+            Problem::MissingChild { child_guid } => {
+                return write!(f, "{} has nonexistent child {}", self.guid(), child_guid);
+            }
+        };
+        match parents.as_slice() {
+            [a] => write!(f, "{}", a)?,
+            [a, b] => write!(f, "{} and {}", a, b)?,
+            _ => {
+                for (i, parent) in parents.iter().enumerate() {
+                    if i != 0 {
+                        f.write_str(", ")?;
+                    }
+                    if i == parents.len() - 1 {
+                        f.write_str("and ")?;
+                    }
+                    write!(f, "{}", parent)?;
+                }
+            }
+        }
+        Ok(())
+    }
+}
+
 /// A node in a bookmark tree that knows its parent and children, and
 /// dereferences to its item.
 #[derive(Clone, Copy, Debug)]
@@ -920,7 +1262,13 @@ impl<'t> Node<'t> {
                 if children.is_empty() {
                     format!("{}{} {}", prefix, kind, self.1.item)
                 } else {
-                    format!("{}📂 {}\n{}", prefix, self.1.item, children.join("\n"))
+                    format!(
+                        "{}{} {}\n{}",
+                        prefix,
+                        kind,
+                        self.1.item,
+                        children.join("\n")
+                    )
                 }
             }
             _ => {

toolkit/components/places/bookmark_sync/Cargo.toml

@@ -6,7 +6,7 @@ edition = "2018"
 
 [dependencies]
 atomic_refcell = "0.1"
-dogear = "0.2.2"
+dogear = "0.2.3"
 libc = "0.2"
 log = "0.4"
 moz_task = { path = "../../../../xpcom/rust/moz_task" }