Backed out changeset b876032a0f26 (bug 1775391) for causing xpcshell failures on test_bookmark_kinds.js

Cargo.lock

@@ -1455,9 +1455,9 @@ dependencies = [
 
 [[package]]
 name = "dogear"
-version = "0.5.0"
+version = "0.4.0"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "3f430ca247b6a905681a3cce3eb4f1a72062f3e8dc178e7660c1acd06c64ecce"
+checksum = "268360cf7696c0c2c83061edb6af090cfea85cbde7d1b8425d6e4ffe9f1c0ec9"
 dependencies = [
  "log",
  "smallbitvec",

services/sync/modules/telemetry.js

@@ -242,10 +242,6 @@ class ErrorSanitizer {
     // these in error messages. Note that JSON.stringified stuff comes through
     // here, so we explicitly ignore double-quotes as well.
     error = error.replace(/[^\s"]+:[^\s"]+/g, "<URL>");
-
-    // Anywhere that's normalized the guid in errors we can easily filter
-    // to make it easier to aggregate these types of errors
-    error = error.replace(/<guid: ([^>]+)>/g, "<GUID>");
     return this.#cleanOSErrorMessage(error);
   }
 }

services/sync/tests/unit/test_telemetry.js

@@ -619,27 +619,6 @@ add_task(async function test_clean_urls() {
   }
 });
 
-// Test sanitizing guid-related errors with the pattern of <guid: {guid}>
-add_task(async function test_sanitize_bookmarks_guid() {
-  let { ErrorSanitizer } = ChromeUtils.import(
-    "resource://services-sync/telemetry.js"
-  );
-
-  for (let [original, expected] of [
-    [
-      "Can't insert Bookmark <guid: sknD84IdnSY2> into Folder <guid: odfninDdi93_3>",
-      "Can't insert Bookmark <GUID> into Folder <GUID>",
-    ],
-    [
-      "Merge Error: Item <guid: H6fmPA16gZs9> can't contain itself",
-      "Merge Error: Item <GUID> can't contain itself",
-    ],
-  ]) {
-    const sanitized = ErrorSanitizer.cleanErrorMessage(original);
-    Assert.equal(sanitized, expected);
-  }
-});
-
 // Test sanitization of some hard-coded error strings.
 add_task(async function test_clean_errors() {
   let { ErrorSanitizer } = ChromeUtils.import(

supply-chain/audits.toml

@@ -34,12 +34,6 @@ who = "Mike Hommey <mh+mozilla@glandium.org>"
 criteria = "safe-to-run"
 delta = "1.1.0 -> 1.1.1"
 
-[[audits.dogear]]
-who = "Sammy Khamis <skhamis@mozilla.com>"
-criteria = "safe-to-deploy"
-delta = "0.4.0 -> 0.5.0"
-notes = "The repository for this crate belongs in the Mozilla org."
-
 [[audits.getrandom]]
 who = "Mike Hommey <mh+mozilla@glandium.org>"
 criteria = "safe-to-deploy"

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

@@ -1 +1 @@
-{"files":{"CODE_OF_CONDUCT.md":"e85149c44f478f164f7d5f55f6e66c9b5ae236d4a11107d5e2a93fe71dd874b9","Cargo.toml":"ccce7edeb25f77186292488dfdb98c9fe7a32ea928c11bcc149dc798f6bcb6b4","LICENSE":"c71d239df91726fc519c6eb72d318ec65820627232b2f796219e87dcf35d0ab4","README.md":"ec5eb7d274f54920b2a76c45aaf84833e8401342575b410f959fac3f8b7b8880","src/driver.rs":"912c55a4fafc956fc69d7f0daab9ec2fa4a4af6fa9ad1164114e2c9fffa61226","src/error.rs":"75b252b2ff3c20666a5500b6c1a33c660a4bd77b6432f590e2fbe45c1534b744","src/guid.rs":"c82af64fba3ad87948a9b599241e48753d17587e8c642f610949163be3d499bf","src/lib.rs":"0606e69b235650bf404ae0b03a1e85c2063bb4b7147fa4d5e8ff2c128a757453","src/merge.rs":"5550c249e069117bd539fc294d8721124a3b2d2a070acddbe157d8a03ed000db","src/store.rs":"42db376d64a8fc53f59ba2825ebb697a9d3dd2340e7bfa98fd9000e8238d09eb","src/tests.rs":"f2a2e8ef081c56942f787a7aeac51af8c29b7bc6a074534d1e055390e36b4d72","src/tree.rs":"92513236b2f38cb74a1035f8032408bd9ec65ad47cbb967cd9c02df64186e4c6"},"package":"3f430ca247b6a905681a3cce3eb4f1a72062f3e8dc178e7660c1acd06c64ecce"}
+{"files":{"CODE_OF_CONDUCT.md":"e85149c44f478f164f7d5f55f6e66c9b5ae236d4a11107d5e2a93fe71dd874b9","Cargo.toml":"74a18824f821751da07620d4f05f3bf08726d77ef8c4fe55a8b2ed0619792e27","LICENSE":"c71d239df91726fc519c6eb72d318ec65820627232b2f796219e87dcf35d0ab4","README.md":"303ea5ec53d4e86f2c321056e8158e31aa061353a99e52de3d76859d40919efc","src/driver.rs":"912c55a4fafc956fc69d7f0daab9ec2fa4a4af6fa9ad1164114e2c9fffa61226","src/error.rs":"d4ef0cba5c7fc54959ed62da166f10435548d705e0a817eed449fb001fe4e21d","src/guid.rs":"c82af64fba3ad87948a9b599241e48753d17587e8c642f610949163be3d499bf","src/lib.rs":"0606e69b235650bf404ae0b03a1e85c2063bb4b7147fa4d5e8ff2c128a757453","src/merge.rs":"376f83de1e2975d8877864ef671e5372574a4b39c66f1b5e02c7ea54bf3e0368","src/store.rs":"42db376d64a8fc53f59ba2825ebb697a9d3dd2340e7bfa98fd9000e8238d09eb","src/tests.rs":"b0ed59b180a434f3c01504ce326cbeb78138ef2bf33ae6fd73cb9ed46b91eaed","src/tree.rs":"0481b18a5542bda8b6ef14f46f910bc0b9d3aa3edd468ef2cac757b6cc8a14a3"},"package":"268360cf7696c0c2c83061edb6af090cfea85cbde7d1b8425d6e4ffe9f1c0ec9"}

third_party/rust/dogear/Cargo.toml

@@ -3,33 +3,27 @@
 # When uploading crates to the registry Cargo will automatically
 # "normalize" Cargo.toml files for maximal compatibility
 # with all versions of Cargo and also rewrite `path` dependencies
-# to registry (e.g., crates.io) dependencies.
+# to registry (e.g., crates.io) dependencies
 #
-# If you are reading this file be aware that the original Cargo.toml
-# will likely look very different (and much more reasonable).
-# See Cargo.toml.orig for the original contents.
+# If you believe there's an error in this file please file an
+# issue against the rust-lang/cargo repository. If you're
+# editing this file be aware that the upstream Cargo.toml
+# will likely look very different (and much more reasonable)
 
 [package]
 edition = "2018"
 name = "dogear"
-version = "0.5.0"
+version = "0.4.0"
 authors = ["Lina Cambridge <lina@mozilla.com>"]
-exclude = [
-    "/.travis/**",
-    ".travis.yml",
-    "/docs/**",
-    "book.toml",
-]
+exclude = ["/.travis/**", ".travis.yml", "/docs/**", "book.toml"]
 description = "A library for merging bookmark trees."
 readme = "README.md"
 license = "Apache-2.0"
 repository = "https://github.com/mozilla/dogear"
-
 [dependencies.log]
 version = "0.4"
 
 [dependencies.smallbitvec]
 version = "2.3.0"
-
 [dev-dependencies.env_logger]
 version = "0.5.6"

third_party/rust/dogear/README.md

@@ -7,12 +7,3 @@ Dogear implements the merge algorithm only; it doesn't handle syncing, storage,
 ## Requirements
 
 * Rust 1.31.0 or higher
-
-
-## Updating this package
-Once a new version of Dogear is ready to release. The new version will need to be published to [crates.io](https://crates.io/crates/dogear). Dogear follows the documentation detailed in the [Cargo book](https://doc.rust-lang.org/cargo/reference/publishing.html#publishing-a-new-version-of-an-existing-crate).
-### Steps to publish a new verison
-1. Bump the version in the `Cargo.toml` file
-2. Run `cargo publish --dry-run`
-    - Validate it does what you want it to do
-3. Run `cargo publish` and follow the steps cargo provides

third_party/rust/dogear/src/error.rs

@@ -15,7 +15,7 @@
 use std::{error, fmt, result, str::Utf8Error, string::FromUtf16Error};
 
 use crate::guid::Guid;
-use crate::Item;
+use crate::tree::Kind;
 
 pub type Result<T> = result::Result<T, Error>;
 
@@ -57,42 +57,25 @@ impl From<Utf8Error> for Error {
 
 impl fmt::Display for Error {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        // We format the guid-specific params with <guid: {}> to make it easier on the
-        // telemetry side to parse out the user-specific guid and normalize the errors
-        // to better aggregate the data
         match self.kind() {
-            ErrorKind::MismatchedItemKind(local_item, remote_item) => write!(
+            ErrorKind::MismatchedItemKind(local_kind, remote_kind) => write!(
                 f,
-                "Can't merge local {} <guid: {}> and remote {} <guid: {}>",
-                local_item.kind, local_item.guid, remote_item.kind, remote_item.guid,
+                "Can't merge local kind {} and remote kind {}",
+                local_kind, remote_kind
             ),
-            ErrorKind::DuplicateItem(guid) => {
-                write!(f, "Item <guid: {}> already exists in tree", guid)
-            }
-            ErrorKind::MissingItem(guid) => {
-                write!(f, "Item <guid: {}> doesn't exist in tree", guid)
-            }
-            ErrorKind::InvalidParent(child, parent) => write!(
+            ErrorKind::DuplicateItem(guid) => write!(f, "Item {} already exists in tree", guid),
+            ErrorKind::MissingItem(guid) => write!(f, "Item {} doesn't exist in tree", guid),
+            ErrorKind::InvalidParent(child_guid, parent_guid) => write!(
                 f,
-                "Can't insert {} <guid: {}> into {} <guid: {}>",
-                child.kind, child.guid, parent.kind, parent.guid,
+                "Can't insert item {} into non-folder {}",
+                child_guid, parent_guid
             ),
-            ErrorKind::InvalidParentForUnknownChild(child_guid, parent) => write!(
+            ErrorKind::MissingParent(child_guid, parent_guid) => write!(
                 f,
-                "Can't insert unknown child <guid: {}> into {} <guid: {}>",
-                child_guid, parent.kind, parent.guid,
+                "Can't insert item {} into nonexistent parent {}",
+                child_guid, parent_guid
             ),
-            ErrorKind::MissingParent(child, parent_guid) => write!(
-                f,
-                "Can't insert {} <guid: {}> into nonexistent parent <guid: {}>",
-                child.kind, child.guid, parent_guid,
-            ),
-            ErrorKind::MissingParentForUnknownChild(child_guid, parent_guid) => write!(
-                f,
-                "Can't insert unknown child <guid: {}> into nonexistent parent <guid: {}>",
-                child_guid, parent_guid,
-            ),
-            ErrorKind::Cycle(guid) => write!(f, "Item <guid: {}> can't contain itself", guid),
+            ErrorKind::Cycle(guid) => write!(f, "Item {} can't contain itself", guid),
             ErrorKind::MergeConflict => write!(f, "Local tree changed during merge"),
             ErrorKind::UnmergedLocalItems => {
                 write!(f, "Merged tree doesn't mention all items from local tree")
@@ -101,13 +84,9 @@ impl fmt::Display for Error {
                 write!(f, "Merged tree doesn't mention all items from remote tree")
             }
             ErrorKind::InvalidGuid(invalid_guid) => {
-                write!(
-                    f,
-                    "Merged tree contains invalid GUID <guid: {}>",
-                    invalid_guid
-                )
+                write!(f, "Merged tree contains invalid GUID {}", invalid_guid)
             }
-            ErrorKind::InvalidByte(b) => write!(f, "Invalid byte <byte: {}> in UTF-16 encoding", b),
+            ErrorKind::InvalidByte(b) => write!(f, "Invalid byte {} in UTF-16 encoding", b),
             ErrorKind::MalformedString(err) => err.fmt(f),
             ErrorKind::Abort => write!(f, "Operation aborted"),
         }
@@ -116,12 +95,10 @@ impl fmt::Display for Error {
 
 #[derive(Debug)]
 pub enum ErrorKind {
-    MismatchedItemKind(Item, Item),
+    MismatchedItemKind(Kind, Kind),
     DuplicateItem(Guid),
-    InvalidParent(Item, Item),
-    InvalidParentForUnknownChild(Guid, Item),
-    MissingParent(Item, Guid),
-    MissingParentForUnknownChild(Guid, Guid),
+    InvalidParent(Guid, Guid),
+    MissingParent(Guid, Guid),
     MissingItem(Guid),
     Cycle(Guid),
     MergeConflict,

third_party/rust/dogear/src/merge.rs

@@ -318,11 +318,7 @@ impl<'t, D: Driver, A: AbortSignal> Merger<'t, D, A> {
                 self.driver,
                 "Merging local {} and remote {} with different kinds", local_node, remote_node
             );
-            return Err(ErrorKind::MismatchedItemKind(
-                local_node.item().clone(),
-                remote_node.item().clone(),
-            )
-            .into());
+            return Err(ErrorKind::MismatchedItemKind(local_node.kind, remote_node.kind).into());
         }
 
         self.merged_guids.insert(local_node.guid.clone());
@@ -1684,7 +1680,10 @@ impl<'t, D: Driver, A: AbortSignal> Merger<'t, D, A> {
                     *node
                 })
             };
-            self.matching_dupes_by_local_parent_guid = matching_dupes_by_local_parent_guid;
+            mem::replace(
+                &mut self.matching_dupes_by_local_parent_guid,
+                matching_dupes_by_local_parent_guid,
+            );
             Ok(new_remote_node)
         } else {
             trace!(
@@ -1740,7 +1739,10 @@ impl<'t, D: Driver, A: AbortSignal> Merger<'t, D, A> {
                     *node
                 })
             };
-            self.matching_dupes_by_local_parent_guid = matching_dupes_by_local_parent_guid;
+            mem::replace(
+                &mut self.matching_dupes_by_local_parent_guid,
+                matching_dupes_by_local_parent_guid,
+            );
             Ok(new_local_node)
         } else {
             trace!(

third_party/rust/dogear/src/tests.rs

@@ -2860,7 +2860,6 @@ fn problems() {
                 DivergedParentGuid::NonFolder("bookmarkGGGG".into()).into(),
             ]),
         )
-        .note(&"bookmarkRRRR".into(), Problem::InvalidItem)
         .note(
             &"bookmarkHHHH".into(),
             Problem::DivergedParents(vec![
@@ -2887,8 +2886,7 @@ fn problems() {
             Problem::DivergedParents(vec![
                 DivergedParentGuid::Deleted("folderQQQQQQ".into()).into()
             ]),
-        )
-        .note(&"bookmarkQQQQ".into(), Problem::InvalidItem);
+        );
 
     let mut summary = problems.summarize().collect::<Vec<_>>();
     summary.sort_by(|a, b| a.guid().cmp(b.guid()));
@@ -2902,8 +2900,6 @@ fn problems() {
              nonexistent parent folderKKKKKK",
             "bookmarkLLLL has diverged parents",
             "bookmarkPPPP has deleted parent folderQQQQQQ",
-            "bookmarkQQQQ is invalid",
-            "bookmarkRRRR is invalid",
             "folderMMMMMM has nonexistent child bookmarkNNNN",
             "folderMMMMMM has nonexistent child bookmarkOOOO",
             "menu________ is a user content root, but is in children of unfiled_____",
@@ -2923,7 +2919,6 @@ fn problems() {
             parent_child_disagreements: 7,
             deleted_children: 0,
             missing_children: 2,
-            invalid_items: 2,
         }
     );
 }

third_party/rust/dogear/src/tree.rs

@@ -331,9 +331,6 @@ impl TryFrom<Builder> for Tree {
         let mut parents = Vec::with_capacity(builder.entries.len());
         let mut reparented_child_indices_by_parent: HashMap<Index, Vec<Index>> = HashMap::new();
         for (entry_index, entry) in builder.entries.iter().enumerate() {
-            if entry.item.validity == Validity::Replace {
-                problems.note(&entry.item.guid, Problem::InvalidItem);
-            }
             let r = ResolveParent::new(&builder, entry, &mut problems);
             let resolved_parent = r.resolve();
             if let ResolvedParent::ByParentGuid(parent_index) = resolved_parent {
@@ -488,7 +485,7 @@ impl<'b> ItemBuilder<'b> {
     /// items with similar contents and different GUIDs.
     #[inline]
     pub fn content<'c>(&'c mut self, content: Content) -> &'c mut ItemBuilder<'b> {
-        self.0.entries[self.1].content = Some(content);
+        mem::replace(&mut self.0.entries[self.1].content, Some(content));
         self
     }
 
@@ -526,35 +523,14 @@ impl<'b> ParentBuilder<'b> {
     pub fn by_children(self, parent_guid: &Guid) -> Result<&'b mut Builder> {
         let parent_index = match self.0.entry_index_by_guid.get(parent_guid) {
             Some(&parent_index) if self.0.entries[parent_index].item.is_folder() => parent_index,
-            Some(&parent_index) => {
-                let parent = &self.0.entries[parent_index].item;
-
-                let child = match &self.1 {
-                    BuilderEntryChild::Exists(index) => &self.0.entries[*index].item,
-                    BuilderEntryChild::Missing(child_guid) => {
-                        return Err(ErrorKind::InvalidParentForUnknownChild(
-                            child_guid.clone(),
-                            parent.clone(),
-                        )
-                        .into())
-                    }
-                };
-
-                return Err(ErrorKind::InvalidParent(child.clone(), parent.clone()).into());
-            }
             _ => {
-                let child = match &self.1 {
-                    BuilderEntryChild::Exists(index) => &self.0.entries[*index].item,
-                    BuilderEntryChild::Missing(child_guid) => {
-                        return Err(ErrorKind::MissingParentForUnknownChild(
-                            child_guid.clone(),
-                            parent_guid.clone(),
-                        )
-                        .into())
-                    }
+                let child_guid = match &self.1 {
+                    BuilderEntryChild::Exists(index) => &self.0.entries[*index].item.guid,
+                    BuilderEntryChild::Missing(guid) => guid,
                 };
-
-                return Err(ErrorKind::MissingParent(child.clone(), parent_guid.clone()).into());
+                return Err(
+                    ErrorKind::InvalidParent(child_guid.clone(), parent_guid.clone()).into(),
+                );
             }
         };
         if let BuilderEntryChild::Exists(child_index) = &self.1 {
@@ -612,35 +588,14 @@ impl<'b> ParentBuilder<'b> {
     pub fn by_structure(self, parent_guid: &Guid) -> Result<&'b mut Builder> {
         let parent_index = match self.0.entry_index_by_guid.get(parent_guid) {
             Some(&parent_index) if self.0.entries[parent_index].item.is_folder() => parent_index,
-            Some(&parent_index) => {
-                let parent = &self.0.entries[parent_index].item;
-
-                let child = match &self.1 {
-                    BuilderEntryChild::Exists(index) => &self.0.entries[*index].item,
-                    BuilderEntryChild::Missing(child_guid) => {
-                        return Err(ErrorKind::InvalidParentForUnknownChild(
-                            child_guid.clone(),
-                            parent.clone(),
-                        )
-                        .into())
-                    }
-                };
-
-                return Err(ErrorKind::InvalidParent(child.clone(), parent.clone()).into());
-            }
             _ => {
-                let child = match &self.1 {
-                    BuilderEntryChild::Exists(index) => &self.0.entries[*index].item,
-                    BuilderEntryChild::Missing(child_guid) => {
-                        return Err(ErrorKind::MissingParentForUnknownChild(
-                            child_guid.clone(),
-                            parent_guid.clone(),
-                        )
-                        .into())
-                    }
+                let child_guid = match &self.1 {
+                    BuilderEntryChild::Exists(index) => &self.0.entries[*index].item.guid,
+                    BuilderEntryChild::Missing(guid) => guid,
                 };
-
-                return Err(ErrorKind::MissingParent(child.clone(), parent_guid.clone()).into());
+                return Err(
+                    ErrorKind::InvalidParent(child_guid.clone(), parent_guid.clone()).into(),
+                );
             }
         };
         if let BuilderEntryChild::Exists(child_index) = &self.1 {
@@ -700,7 +655,7 @@ impl BuilderEntry {
         let old_parent = mem::replace(&mut self.parent, BuilderEntryParent::None);
         let new_parent = match old_parent {
             BuilderEntryParent::Root => {
-                self.parent = BuilderEntryParent::Root;
+                mem::replace(&mut self.parent, BuilderEntryParent::Root);
                 return Err(ErrorKind::DuplicateItem(self.item.guid.clone()).into());
             }
             BuilderEntryParent::None => match new_parents {
@@ -728,7 +683,7 @@ impl BuilderEntry {
                 BuilderEntryParent::Partial(parents)
             }
         };
-        self.parent = new_parent;
+        mem::replace(&mut self.parent, new_parent);
         Ok(())
     }
 }
@@ -1170,17 +1125,10 @@ pub enum Problem {
     DivergedParents(Vec<DivergedParent>),
 
     /// The item is mentioned in a folder's `children`, but doesn't exist.
-    MissingChild {
-        child_guid: Guid,
-    },
+    MissingChild { child_guid: Guid },
 
     /// The item is mentioned in a folder's `children`, but is deleted.
-    DeletedChild {
-        child_guid: Guid,
-    },
-
-    // This item is invalid e.g the URL is malformed
-    InvalidItem,
+    DeletedChild { child_guid: Guid },
 }
 
 impl Problem {
@@ -1219,12 +1167,6 @@ impl Problem {
                 },
             ),
             Problem::DivergedParents(parents) => (parents, ProblemCounts::default()),
-            Problem::InvalidItem => {
-                return ProblemCounts {
-                    invalid_items: 1,
-                    ..ProblemCounts::default()
-                }
-            }
         };
         let deltas = match parents.as_slice() {
             // For items with different parents `by_parent_guid` and
@@ -1417,7 +1359,6 @@ impl<'a> fmt::Display for ProblemSummary<'a> {
             Problem::DeletedChild { child_guid } => {
                 return write!(f, "{} has deleted child {}", self.guid(), child_guid);
             }
-            Problem::InvalidItem => return write!(f, "{} is invalid", self.guid()),
         };
         match parents.as_slice() {
             [a] => write!(f, "{}", a)?,
@@ -1463,8 +1404,6 @@ pub struct ProblemCounts {
     pub deleted_children: usize,
     /// Number of nonexistent items mentioned in all parents' `children`.
     pub missing_children: usize,
-    // Number of items with malformed URLs
-    pub invalid_items: usize,
 }
 
 impl ProblemCounts {
@@ -1482,7 +1421,6 @@ impl ProblemCounts {
                 + other.parent_child_disagreements,
             deleted_children: self.deleted_children + other.deleted_children,
             missing_children: self.missing_children + other.missing_children,
-            invalid_items: self.invalid_items + other.invalid_items,
         }
     }
 }
@@ -1668,7 +1606,7 @@ impl<'t> fmt::Display for Node<'t> {
 }
 
 /// An item in a local or remote bookmark tree.
-#[derive(Clone, Debug, Eq, PartialEq)]
+#[derive(Debug, Eq, PartialEq)]
 pub struct Item {
     pub guid: Guid,
     pub kind: Kind,

toolkit/components/places/bookmark_sync/Cargo.toml

@@ -6,7 +6,7 @@ edition = "2018"
 
 [dependencies]
 atomic_refcell = "0.1"
-dogear = "0.5.0"
+dogear = "0.4.0"
 libc = "0.2"
 log = "0.4"
 cstr = "0.2"