Bug 1775391 - Normalize bookmarks guid errors to strip them out in te…

…lemetry r=markh,supply-chain-reviewers

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

services/sync/modules/telemetry.js

@@ -242,6 +242,10 @@ 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,6 +619,27 @@ 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,6 +34,12 @@ who = "Mike Hommey <[email protected]>"
 criteria = "safe-to-run"
 delta = "1.1.0 -> 1.1.1"
 
+[[audits.dogear]]
+who = "Sammy Khamis <[email protected]>"
+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 <[email protected]>"
 criteria = "safe-to-deploy"

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

@@ -1 +1 @@
-{"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"}
+{"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"}

third_party/rust/dogear/Cargo.toml

@@ -3,27 +3,33 @@
 # 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 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)
+# 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.
 
 [package]
 edition = "2018"
 name = "dogear"
-version = "0.4.0"
+version = "0.5.0"
 authors = ["Lina Cambridge <[email protected]>"]
-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,3 +7,12 @@ 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::tree::Kind;
+use crate::Item;
 
 pub type Result<T> = result::Result<T, Error>;
 
@@ -57,25 +57,42 @@ 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_kind, remote_kind) => write!(
+            ErrorKind::MismatchedItemKind(local_item, remote_item) => write!(
                 f,
-                "Can't merge local kind {} and remote kind {}",
-                local_kind, remote_kind
+                "Can't merge local {} <guid: {}> and remote {} <guid: {}>",
+                local_item.kind, local_item.guid, remote_item.kind, remote_item.guid,
             ),
-            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!(
+            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!(
+                f,
+                "Can't insert {} <guid: {}> into {} <guid: {}>",
+                child.kind, child.guid, parent.kind, parent.guid,
+            ),
+            ErrorKind::InvalidParentForUnknownChild(child_guid, parent) => write!(
+                f,
+                "Can't insert unknown child <guid: {}> into {} <guid: {}>",
+                child_guid, parent.kind, parent.guid,
+            ),
+            ErrorKind::MissingParent(child, parent_guid) => write!(
                 f,
-                "Can't insert item {} into non-folder {}",
-                child_guid, parent_guid
+                "Can't insert {} <guid: {}> into nonexistent parent <guid: {}>",
+                child.kind, child.guid, parent_guid,
             ),
-            ErrorKind::MissingParent(child_guid, parent_guid) => write!(
+            ErrorKind::MissingParentForUnknownChild(child_guid, parent_guid) => write!(
                 f,
-                "Can't insert item {} into nonexistent parent {}",
-                child_guid, parent_guid
+                "Can't insert unknown child <guid: {}> into nonexistent parent <guid: {}>",
+                child_guid, parent_guid,
             ),
-            ErrorKind::Cycle(guid) => write!(f, "Item {} can't contain itself", guid),
+            ErrorKind::Cycle(guid) => write!(f, "Item <guid: {}> 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")
@@ -84,9 +101,13 @@ 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 {}", invalid_guid)
+                write!(
+                    f,
+                    "Merged tree contains invalid GUID <guid: {}>",
+                    invalid_guid
+                )
             }
-            ErrorKind::InvalidByte(b) => write!(f, "Invalid byte {} in UTF-16 encoding", b),
+            ErrorKind::InvalidByte(b) => write!(f, "Invalid byte <byte: {}> in UTF-16 encoding", b),
             ErrorKind::MalformedString(err) => err.fmt(f),
             ErrorKind::Abort => write!(f, "Operation aborted"),
         }
@@ -95,10 +116,12 @@ impl fmt::Display for Error {
 
 #[derive(Debug)]
 pub enum ErrorKind {
-    MismatchedItemKind(Kind, Kind),
+    MismatchedItemKind(Item, Item),
     DuplicateItem(Guid),
-    InvalidParent(Guid, Guid),
-    MissingParent(Guid, Guid),
+    InvalidParent(Item, Item),
+    InvalidParentForUnknownChild(Guid, Item),
+    MissingParent(Item, Guid),
+    MissingParentForUnknownChild(Guid, Guid),
     MissingItem(Guid),
     Cycle(Guid),
     MergeConflict,

third_party/rust/dogear/src/merge.rs

@@ -318,7 +318,11 @@ 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.kind, remote_node.kind).into());
+            return Err(ErrorKind::MismatchedItemKind(
+                local_node.item().clone(),
+                remote_node.item().clone(),
+            )
+            .into());
         }
 
         self.merged_guids.insert(local_node.guid.clone());
@@ -1680,10 +1684,7 @@ impl<'t, D: Driver, A: AbortSignal> Merger<'t, D, A> {
                     *node
                 })
             };
-            mem::replace(
-                &mut self.matching_dupes_by_local_parent_guid,
-                matching_dupes_by_local_parent_guid,
-            );
+            self.matching_dupes_by_local_parent_guid = matching_dupes_by_local_parent_guid;
             Ok(new_remote_node)
         } else {
             trace!(
@@ -1739,10 +1740,7 @@ impl<'t, D: Driver, A: AbortSignal> Merger<'t, D, A> {
                     *node
                 })
             };
-            mem::replace(
-                &mut self.matching_dupes_by_local_parent_guid,
-                matching_dupes_by_local_parent_guid,
-            );
+            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,6 +2860,7 @@ fn problems() {
                 DivergedParentGuid::NonFolder("bookmarkGGGG".into()).into(),
             ]),
         )
+        .note(&"bookmarkRRRR".into(), Problem::InvalidItem)
         .note(
             &"bookmarkHHHH".into(),
             Problem::DivergedParents(vec![
@@ -2886,7 +2887,8 @@ 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()));
@@ -2900,6 +2902,8 @@ 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_____",
@@ -2919,6 +2923,7 @@ fn problems() {
             parent_child_disagreements: 7,
             deleted_children: 0,
             missing_children: 2,
+            invalid_items: 2,
         }
     );
 }