From 1dce1c984c17d963b8f0402c0504256a02fd3324 Mon Sep 17 00:00:00 2001 From: Lina Cambridge Date: Tue, 17 Sep 2019 06:27:12 +0000 Subject: [PATCH] Bug 1579728 - Use completion ops to upload synced bookmark tombstones. r=markh The latest version of Dogear adds completion ops for deleting local items (to apply incoming tombstones), inserting new local tombstones (to stage deletions for non-syncable and invalid items), and uploading tombstones (to avoid an extra scan of `moz_bookmarks_deleted`). These ops are only emitted for GUIDs that exist in both trees. We'll remove any local tombstones for items that don't exist or are already deleted on the server, and flag any remote tombstones for items that don't exist in Places as merged. Differential Revision: https://phabricator.services.mozilla.com/D45310 --- Cargo.lock | 6 +- third_party/rust/dogear/.cargo-checksum.json | 2 +- third_party/rust/dogear/Cargo.toml | 2 +- third_party/rust/dogear/src/driver.rs | 1 + third_party/rust/dogear/src/merge.rs | 335 +++++++++++++----- third_party/rust/dogear/src/store.rs | 7 +- third_party/rust/dogear/src/tests.rs | 261 +++++++------- third_party/rust/dogear/src/tree.rs | 34 +- .../places/SyncedBookmarksMirror.jsm | 48 +-- .../places/bookmark_sync/Cargo.toml | 2 +- .../places/bookmark_sync/src/driver.rs | 3 +- .../places/bookmark_sync/src/store.rs | 248 +++++++------ .../places/mozISyncedBookmarksMirror.idl | 4 +- .../tests/sync/test_bookmark_abort_merging.js | 4 +- .../tests/sync/test_bookmark_corruption.js | 50 ++- .../tests/sync/test_bookmark_deletion.js | 10 +- .../places/tests/sync/test_bookmark_kinds.js | 13 +- .../sync/test_bookmark_structure_changes.js | 7 + 18 files changed, 653 insertions(+), 384 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index f01f04072ec0..5b844364ff35 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -368,7 +368,7 @@ version = "0.1.0" dependencies = [ "atomic_refcell 0.1.0 (registry+https://github.com/rust-lang/crates.io-index)", "cstr 0.1.3 (registry+https://github.com/rust-lang/crates.io-index)", - "dogear 0.3.3 (registry+https://github.com/rust-lang/crates.io-index)", + "dogear 0.4.0 (registry+https://github.com/rust-lang/crates.io-index)", "libc 0.2.60 (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", @@ -910,7 +910,7 @@ dependencies = [ [[package]] name = "dogear" -version = "0.3.3" +version = "0.4.0" source = "registry+https://github.com/rust-lang/crates.io-index" dependencies = [ "log 0.4.6 (registry+https://github.com/rust-lang/crates.io-index)", @@ -3812,7 +3812,7 @@ dependencies = [ "checksum digest 0.8.0 (registry+https://github.com/rust-lang/crates.io-index)" = "05f47366984d3ad862010e22c7ce81a7dbcaebbdfb37241a620f8b6596ee135c" "checksum dirs 1.0.5 (registry+https://github.com/rust-lang/crates.io-index)" = "3fd78930633bd1c6e35c4b42b1df7b0cbc6bc191146e512bb3bedf243fcc3901" "checksum dns-parser 0.8.0 (registry+https://github.com/rust-lang/crates.io-index)" = "c4d33be9473d06f75f58220f71f7a9317aca647dc061dbd3c361b0bef505fbea" -"checksum dogear 0.3.3 (registry+https://github.com/rust-lang/crates.io-index)" = "57cd6ee785daa898686f3e2fb4a2b1ce490fcd6d69665c857d16fb61b48f4aae" +"checksum dogear 0.4.0 (registry+https://github.com/rust-lang/crates.io-index)" = "268360cf7696c0c2c83061edb6af090cfea85cbde7d1b8425d6e4ffe9f1c0ec9" "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.9.0 (registry+https://github.com/rust-lang/crates.io-index)" = "0bd1369e02db5e9b842a9b67bce8a2fcc043beafb2ae8a799dd482d46ea1ff0d" diff --git a/third_party/rust/dogear/.cargo-checksum.json b/third_party/rust/dogear/.cargo-checksum.json index 61399f5009c2..8ba2ceaab58e 100644 --- a/third_party/rust/dogear/.cargo-checksum.json +++ b/third_party/rust/dogear/.cargo-checksum.json @@ -1 +1 @@ -{"files":{"CODE_OF_CONDUCT.md":"e85149c44f478f164f7d5f55f6e66c9b5ae236d4a11107d5e2a93fe71dd874b9","Cargo.toml":"9c145c9ecdad0143f95fb00d5646fb3f46203779a201f2f2389961563ca37397","LICENSE":"c71d239df91726fc519c6eb72d318ec65820627232b2f796219e87dcf35d0ab4","README.md":"303ea5ec53d4e86f2c321056e8158e31aa061353a99e52de3d76859d40919efc","src/driver.rs":"913de08a578b38902f4eb0d78147289897af5d3b1ecdad818c4ea6f83da6c714","src/error.rs":"d4ef0cba5c7fc54959ed62da166f10435548d705e0a817eed449fb001fe4e21d","src/guid.rs":"c82af64fba3ad87948a9b599241e48753d17587e8c642f610949163be3d499bf","src/lib.rs":"0606e69b235650bf404ae0b03a1e85c2063bb4b7147fa4d5e8ff2c128a757453","src/merge.rs":"bfc83aa6f42c196a5c6c75e095d1e44470198a38f3a62a6537cd5854d51208cd","src/store.rs":"629410e44485c0473617911be0e06dc5ce504c7427782be02f05a2f4096b5351","src/tests.rs":"fd2f03fd47e02459b7a3c7f799896be71536054184a2eca7e30cdbe817da0a97","src/tree.rs":"30c7d0c80f180936d1f99d2a3fbe9add36f3fe36a64998e3176d0242f425c488"},"package":"57cd6ee785daa898686f3e2fb4a2b1ce490fcd6d69665c857d16fb61b48f4aae"} \ No newline at end of file +{"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"} \ No newline at end of file diff --git a/third_party/rust/dogear/Cargo.toml b/third_party/rust/dogear/Cargo.toml index c66194fe10f5..dc1c418adcc1 100644 --- a/third_party/rust/dogear/Cargo.toml +++ b/third_party/rust/dogear/Cargo.toml @@ -13,7 +13,7 @@ [package] edition = "2018" name = "dogear" -version = "0.3.3" +version = "0.4.0" authors = ["Lina Cambridge "] exclude = ["/.travis/**", ".travis.yml", "/docs/**", "book.toml"] description = "A library for merging bookmark trees." diff --git a/third_party/rust/dogear/src/driver.rs b/third_party/rust/dogear/src/driver.rs index 791bfced70b2..1c98ebdfb10e 100644 --- a/third_party/rust/dogear/src/driver.rs +++ b/third_party/rust/dogear/src/driver.rs @@ -68,6 +68,7 @@ pub enum TelemetryEvent { pub struct TreeStats { pub time: Duration, pub items: usize, + pub deletions: usize, pub problems: ProblemCounts, } diff --git a/third_party/rust/dogear/src/merge.rs b/third_party/rust/dogear/src/merge.rs index ec86bb82e486..e823d6775101 100644 --- a/third_party/rust/dogear/src/merge.rs +++ b/third_party/rust/dogear/src/merge.rs @@ -50,22 +50,12 @@ pub struct StructureCounts { /// Total number of nodes in the merged tree, excluding the /// root. pub merged_nodes: usize, - /// Total number of deletions to apply, local and remote. - pub merged_deletions: usize, } /// Holds (matching remote dupes for local GUIDs, matching local dupes for /// remote GUIDs). type MatchingDupes<'t> = (HashMap>, HashMap>); -/// Represents an accepted local or remote deletion. -#[derive(Clone, Copy, Debug, Eq, Hash, PartialEq)] -pub struct Deletion<'t> { - pub guid: &'t Guid, - pub local_level: i64, - pub should_upload_tombstone: bool, -} - /// Indicates which side to take in case of a merge conflict. #[derive(Clone, Copy, Debug)] enum ConflictResolution { @@ -172,14 +162,12 @@ impl<'t, D: Driver, A: AbortSignal> Merger<'t, D, A> { self.signal.err_if_aborted()?; if !self.mentions(guid) { self.delete_remotely.insert(guid.clone()); - self.structure_counts.merged_deletions += 1; } } for guid in self.remote_tree.deletions() { self.signal.err_if_aborted()?; if !self.mentions(guid) { self.delete_locally.insert(guid.clone()); - self.structure_counts.merged_deletions += 1; } } @@ -283,7 +271,6 @@ impl<'t, D: Driver, A: AbortSignal> Merger<'t, D, A> { self.merged_guids.insert(new_guid.clone()); // Upload tombstones for changed remote GUIDs. self.delete_remotely.insert(remote_node.guid.clone()); - self.structure_counts.merged_deletions += 1; } new_guid }; @@ -353,7 +340,6 @@ impl<'t, D: Driver, A: AbortSignal> Merger<'t, D, A> { self.merged_guids.insert(new_guid.clone()); // Upload tombstones for changed remote GUIDs. self.delete_remotely.insert(remote_node.guid.clone()); - self.structure_counts.merged_deletions += 1; } new_guid }; @@ -1431,7 +1417,6 @@ impl<'t, D: Driver, A: AbortSignal> Merger<'t, D, A> { } } } - self.structure_counts.merged_deletions += 1; Ok(StructureChange::Deleted) } @@ -1494,7 +1479,6 @@ impl<'t, D: Driver, A: AbortSignal> Merger<'t, D, A> { } } } - self.structure_counts.merged_deletions += 1; Ok(StructureChange::Deleted) } @@ -1792,66 +1776,128 @@ impl<'t> MergedRoot<'t> { } /// Returns a sequence of completion operations, or "completion ops", to - /// apply to the local tree so that it matches the merged tree. - pub fn completion_ops(&self) -> CompletionOps<'_> { + /// apply to the local tree so that it matches the merged tree. The abort + /// signal can be used to interrupt fetching the ops. + pub fn completion_ops_with_signal( + &self, + signal: &impl AbortSignal, + ) -> Result> { let mut ops = CompletionOps::default(); - accumulate(&mut ops, self.node(), 1, false); - for guid in self.delete_locally.iter() { - if self.local_tree.mentions(guid) && self.remote_tree.mentions(guid) { - // Only flag tombstones for items that exist in both trees, - // excluding ones for items that don't exist in the local - // tree. This can be removed once we persist tombstones in - // bug 1343103. - let flag_as_merged = FlagAsMerged(guid); - ops.flag_as_merged.push(flag_as_merged); + accumulate(signal, &mut ops, self.node(), 1, false)?; + + // Clean up tombstones for local and remote items that are revived on + // the other side. + for guid in self + .local_tree + .deletions() + .difference(&self.delete_remotely) + { + // For ignored local deletions, we remove the local tombstone. If + // the item is already deleted remotely, we also flag the remote + // tombstone as merged. + signal.err_if_aborted()?; + ops.delete_local_tombstones.push(DeleteLocalTombstone(guid)); + if self.remote_tree.is_deleted(guid) { + ops.set_remote_merged.push(SetRemoteMerged(guid)); + } + } + for guid in self + .remote_tree + .deletions() + .difference(&self.delete_locally) + .filter(|guid| !self.local_tree.exists(guid)) + { + // Ignored remote deletions are handled a little differently. Unlike + // local tombstones, which are stored separately from items, remote + // tombstones and items are stored in the same table. This means we + // only need to flag the remote tombstone as merged if it's for an + // item that doesn't exist locally. If the local item does exist, + // we can avoid an extra write to flag the tombstone that we'll + // replace with the item, anyway. If the item is already deleted + // locally, we also delete the local tombstone. + signal.err_if_aborted()?; + ops.set_remote_merged.push(SetRemoteMerged(guid)); + if self.local_tree.is_deleted(guid) { + ops.delete_local_tombstones.push(DeleteLocalTombstone(guid)); + } + } + + // Emit completion ops for deleted items. + for guid in self.deletions() { + signal.err_if_aborted()?; + match ( + self.local_tree.node_for_guid(guid), + self.remote_tree.node_for_guid(guid), + ) { + (Some(local_node), Some(remote_node)) => { + // Delete items that are non-syncable or invalid on both + // sides. + ops.delete_local_items.push(DeleteLocalItem(local_node)); + ops.insert_local_tombstones + .push(InsertLocalTombstone(remote_node)); + ops.upload_tombstones.push(UploadTombstone(guid)); + } + (Some(local_node), None) => { + // Apply remote tombstones, or delete invalid local-only + // items. If the item is deleted remotely, flag the remote + // tombstone as merged. If not, we don't need to upload one, + // since the item is only known locally. + ops.delete_local_items.push(DeleteLocalItem(local_node)); + if self.remote_tree.is_deleted(guid) { + ops.set_remote_merged.push(SetRemoteMerged(guid)); + } + } + (None, Some(remote_node)) => { + // Take local tombstones, or delete invalid remote-only + // items. If it's not already deleted locally, insert a + // tombstone for the item. + if !self.local_tree.is_deleted(guid) { + ops.insert_local_tombstones + .push(InsertLocalTombstone(remote_node)); + } + ops.upload_tombstones.push(UploadTombstone(guid)); + } + (None, None) => { + // Clean up local tombstones, and flag remote tombstones as + // merged, for items deleted on both sides. + if self.local_tree.is_deleted(guid) { + ops.delete_local_tombstones.push(DeleteLocalTombstone(guid)); + } + if self.remote_tree.is_deleted(guid) { + ops.set_remote_merged.push(SetRemoteMerged(guid)); + } + } } } - ops + + Ok(ops) + } + + /// Returns a sequence of completion ops, without interruption. + #[inline] + pub fn completion_ops(&self) -> CompletionOps<'_> { + self.completion_ops_with_signal(&DefaultAbortSignal) + .unwrap() } /// Returns an iterator for all accepted local and remote deletions. #[inline] - pub fn deletions(&self) -> impl Iterator> { - self.local_deletions().chain(self.remote_deletions()) + pub fn deletions(&self) -> impl Iterator { + self.delete_locally.union(&self.delete_remotely) } /// Returns an iterator for all items that should be deleted from the /// local tree. - pub fn local_deletions(&self) -> impl Iterator> { - self.delete_locally.iter().filter_map(move |guid| { - if self.delete_remotely.contains(guid) { - None - } else { - let local_level = self - .local_tree - .node_for_guid(guid) - .map_or(-1, |node| node.level()); - // Items that should be deleted locally already have tombstones - // on the server, so we don't need to upload tombstones for - // these deletions. - Some(Deletion { - guid, - local_level, - should_upload_tombstone: false, - }) - } - }) + #[inline] + pub fn local_deletions(&self) -> impl Iterator { + self.delete_locally.difference(&self.delete_remotely) } /// Returns an iterator for all items that should be deleted from the /// remote tree. - pub fn remote_deletions(&self) -> impl Iterator> { - self.delete_remotely.iter().map(move |guid| { - let local_level = self - .local_tree - .node_for_guid(guid) - .map_or(-1, |node| node.level()); - Deletion { - guid, - local_level, - should_upload_tombstone: true, - } - }) + #[inline] + pub fn remote_deletions(&self) -> impl Iterator { + self.delete_remotely.iter() } /// Returns structure change counts for this merged root. @@ -1869,10 +1915,14 @@ pub struct CompletionOps<'t> { pub change_guids: Vec>, pub apply_remote_items: Vec>, pub apply_new_local_structure: Vec>, - pub flag_for_upload: Vec>, - pub skip_upload: Vec>, - pub flag_as_merged: Vec>, - pub upload: Vec>, + pub set_local_unmerged: Vec>, + pub set_local_merged: Vec>, + pub set_remote_merged: Vec>, + pub delete_local_tombstones: Vec>, + pub insert_local_tombstones: Vec>, + pub delete_local_items: Vec>, + pub upload_items: Vec>, + pub upload_tombstones: Vec>, } impl<'t> CompletionOps<'t> { @@ -1882,10 +1932,31 @@ impl<'t> CompletionOps<'t> { self.change_guids.is_empty() && self.apply_remote_items.is_empty() && self.apply_new_local_structure.is_empty() - && self.flag_for_upload.is_empty() - && self.skip_upload.is_empty() - && self.flag_as_merged.is_empty() - && self.upload.is_empty() + && self.set_local_unmerged.is_empty() + && self.set_local_merged.is_empty() + && self.set_remote_merged.is_empty() + && self.delete_local_tombstones.is_empty() + && self.insert_local_tombstones.is_empty() + && self.delete_local_items.is_empty() + && self.upload_items.is_empty() + && self.upload_tombstones.is_empty() + } + + /// Returns a printable summary of all completion ops to apply. + pub fn summarize(&self) -> Vec { + std::iter::empty() + .chain(to_strings(&self.change_guids)) + .chain(to_strings(&self.apply_remote_items)) + .chain(to_strings(&self.apply_new_local_structure)) + .chain(to_strings(&self.set_local_unmerged)) + .chain(to_strings(&self.set_local_merged)) + .chain(to_strings(&self.set_remote_merged)) + .chain(to_strings(&self.delete_local_tombstones)) + .chain(to_strings(&self.insert_local_tombstones)) + .chain(to_strings(&self.delete_local_items)) + .chain(to_strings(&self.upload_items)) + .chain(to_strings(&self.upload_tombstones)) + .collect() } } @@ -1982,46 +2053,64 @@ impl<'t> fmt::Display for ApplyNewLocalStructure<'t> { /// A completion op to flag a local item for upload. #[derive(Clone, Copy, Debug)] -pub struct FlagForUpload<'t> { +pub struct SetLocalUnmerged<'t> { pub merged_node: &'t MergedNode<'t>, } -impl<'t> fmt::Display for FlagForUpload<'t> { +impl<'t> fmt::Display for SetLocalUnmerged<'t> { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - write!(f, "Flag {} for upload", self.merged_node.guid) + write!(f, "Flag local {} as unmerged", self.merged_node.guid) } } /// A completion op to skip uploading a local item after resolving merge /// conflicts. #[derive(Clone, Copy, Debug)] -pub struct SkipUpload<'t> { +pub struct SetLocalMerged<'t> { pub merged_node: &'t MergedNode<'t>, } -impl<'t> fmt::Display for SkipUpload<'t> { +impl<'t> fmt::Display for SetLocalMerged<'t> { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - write!(f, "Don't upload {}", self.merged_node.guid) + write!(f, "Flag local {} as merged", self.merged_node.guid) } } /// A completion op to upload or reupload a merged item. #[derive(Clone, Copy, Debug)] -pub struct Upload<'t> { +pub struct UploadItem<'t> { pub merged_node: &'t MergedNode<'t>, } -impl<'t> fmt::Display for Upload<'t> { +impl<'t> fmt::Display for UploadItem<'t> { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - write!(f, "Upload {}", self.merged_node.guid) + write!(f, "Upload item {}", self.merged_node.guid) + } +} + +/// A completion op to upload a tombstone. +#[derive(Clone, Copy, Debug)] +pub struct UploadTombstone<'t>(&'t Guid); + +impl<'t> UploadTombstone<'t> { + /// Returns the GUID to use for the tombstone. + #[inline] + pub fn guid(self) -> &'t Guid { + self.0 + } +} + +impl<'t> fmt::Display for UploadTombstone<'t> { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "Upload tombstone {}", self.0) } } /// A completion op to flag a remote item as merged. #[derive(Clone, Copy, Debug)] -pub struct FlagAsMerged<'t>(&'t Guid); +pub struct SetRemoteMerged<'t>(&'t Guid); -impl<'t> FlagAsMerged<'t> { +impl<'t> SetRemoteMerged<'t> { /// Returns the remote GUID for the item to flag as merged. #[inline] pub fn guid(self) -> &'t Guid { @@ -2029,21 +2118,77 @@ impl<'t> FlagAsMerged<'t> { } } -impl<'t> fmt::Display for FlagAsMerged<'t> { +impl<'t> fmt::Display for SetRemoteMerged<'t> { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - write!(f, "Flag {} as merged", self.guid()) + write!(f, "Flag remote {} as merged", self.guid()) + } +} + +/// A completion op to store a tombstone for a remote item. +#[derive(Clone, Copy, Debug)] +pub struct InsertLocalTombstone<'t>(Node<'t>); + +impl<'t> InsertLocalTombstone<'t> { + /// Returns the node for the item to delete remotely. + #[inline] + pub fn remote_node(&self) -> Node<'t> { + self.0 + } +} + +impl<'t> fmt::Display for InsertLocalTombstone<'t> { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "Insert local tombstone {}", self.0.guid) + } +} + +/// A completion op to delete a local tombstone. +#[derive(Clone, Copy, Debug)] +pub struct DeleteLocalTombstone<'t>(&'t Guid); + +impl<'t> DeleteLocalTombstone<'t> { + /// Returns the GUID of the tombstone. + #[inline] + pub fn guid(self) -> &'t Guid { + self.0 + } +} + +impl<'t> fmt::Display for DeleteLocalTombstone<'t> { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "Delete local tombstone {}", self.0) + } +} + +/// A completion op to delete an item from the local tree. +#[derive(Clone, Copy, Debug)] +pub struct DeleteLocalItem<'t>(Node<'t>); + +impl<'t> DeleteLocalItem<'t> { + // Returns the node for the item to delete locally. + #[inline] + pub fn local_node(&self) -> Node<'t> { + self.0 + } +} + +impl<'t> fmt::Display for DeleteLocalItem<'t> { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "Delete local item {}", self.0.guid) } } /// Recursively accumulates completion ops, starting at `merged_node` and /// drilling down into all its descendants. -fn accumulate<'t>( +fn accumulate<'t, A: AbortSignal>( + signal: &A, ops: &mut CompletionOps<'t>, merged_node: &'t MergedNode<'t>, level: usize, is_tagging: bool, -) { +) -> Result<()> { for (position, merged_child_node) in merged_node.merged_children.iter().enumerate() { + signal.err_if_aborted()?; let is_tagging = if merged_child_node.guid == TAGS_GUID { true } else { @@ -2094,17 +2239,17 @@ fn accumulate<'t>( match (local_needs_merge, should_upload) { (false, true) => { // Local item isn't flagged for upload, but should be. - let flag_for_upload = FlagForUpload { + let set_local_unmerged = SetLocalUnmerged { merged_node: merged_child_node, }; - ops.flag_for_upload.push(flag_for_upload); + ops.set_local_unmerged.push(set_local_unmerged); } (true, false) => { // Local item flagged for upload when it doesn't need to be. - let skip_upload = SkipUpload { + let set_local_merged = SetLocalMerged { merged_node: merged_child_node, }; - ops.skip_upload.push(skip_upload); + ops.set_local_merged.push(set_local_merged); } _ => {} } @@ -2112,7 +2257,7 @@ fn accumulate<'t>( // (Re)upload items. Ignore the tags root and its descendants: // they're part of the local tree on Desktop (and will be removed // in bug 424160), but aren't synced as part of the structure. - ops.upload.push(Upload { + ops.upload_items.push(UploadItem { merged_node: merged_child_node, }); } @@ -2122,10 +2267,16 @@ fn accumulate<'t>( // reuploaded, flag it as merged in the remote tree. Note that // we _don't_ emit this for locally revived items, or items with // new remote structure. - let flag_as_merged = FlagAsMerged(&remote_child_node.guid); - ops.flag_as_merged.push(flag_as_merged); + let set_remote_merged = SetRemoteMerged(&remote_child_node.guid); + ops.set_remote_merged.push(set_remote_merged); } } - accumulate(ops, merged_child_node, level + 1, is_tagging); + accumulate(signal, ops, merged_child_node, level + 1, is_tagging)?; } + Ok(()) +} + +/// Converts all items in the list to strings. +pub(crate) fn to_strings<'a, T: ToString>(items: &'a [T]) -> impl Iterator + 'a { + items.iter().map(ToString::to_string) } diff --git a/third_party/rust/dogear/src/store.rs b/third_party/rust/dogear/src/store.rs index f39af686a58e..7ada8407fc24 100644 --- a/third_party/rust/dogear/src/store.rs +++ b/third_party/rust/dogear/src/store.rs @@ -18,6 +18,7 @@ use crate::driver::{ AbortSignal, DefaultAbortSignal, DefaultDriver, Driver, TelemetryEvent, TreeStats, }; use crate::error::Error; +use crate::guid::Guid; use crate::merge::{MergedRoot, Merger}; use crate::tree::Tree; @@ -63,6 +64,7 @@ pub trait Store { let (local_tree, time) = with_timing(|| self.fetch_local_tree())?; driver.record_telemetry_event(TelemetryEvent::FetchLocalTree(TreeStats { items: local_tree.size(), + deletions: local_tree.deletions().len(), problems: local_tree.problems().counts(), time, })); @@ -73,6 +75,7 @@ pub trait Store { let (remote_tree, time) = with_timing(|| self.fetch_remote_tree())?; driver.record_telemetry_event(TelemetryEvent::FetchRemoteTree(TreeStats { items: remote_tree.size(), + deletions: local_tree.deletions().len(), problems: remote_tree.problems().counts(), time, })); @@ -89,12 +92,12 @@ pub trait Store { merged_root.node().to_ascii_string(), merged_root .local_deletions() - .map(|d| d.guid.as_str()) + .map(Guid::as_str) .collect::>() .join(", "), merged_root .remote_deletions() - .map(|d| d.guid.as_str()) + .map(Guid::as_str) .collect::>() .join(", ") ); diff --git a/third_party/rust/dogear/src/tests.rs b/third_party/rust/dogear/src/tests.rs index 86189e452c0f..7d3510c4dbcc 100644 --- a/third_party/rust/dogear/src/tests.rs +++ b/third_party/rust/dogear/src/tests.rs @@ -23,7 +23,7 @@ use env_logger; use crate::driver::{DefaultAbortSignal, Driver}; use crate::error::{Error, ErrorKind, Result}; use crate::guid::{Guid, ROOT_GUID, UNFILED_GUID}; -use crate::merge::{Merger, StructureCounts}; +use crate::merge::{to_strings, Merger, StructureCounts}; use crate::tree::{ self, Builder, Content, DivergedParent, DivergedParentGuid, Item, Kind, MergeState, Problem, ProblemCounts, Problems, Tree, Validity, @@ -476,20 +476,17 @@ fn unchanged_newer_changed_older() { ("bookmarkFFFF", RemoteWithNewRemoteStructure) }) }); - let expected_deletions = vec!["folderAAAAAA", "folderCCCCCC"]; + let expected_deletions = &["folderAAAAAA", "folderCCCCCC"]; let expected_telem = StructureCounts { - remote_revives: 0, local_deletes: 1, - local_revives: 0, remote_deletes: 1, - dupes: 0, merged_nodes: 6, - merged_deletions: 2, + ..StructureCounts::default() }; assert_eq!(&expected_tree, merged_root.node()); - let mut deletions = merged_root.deletions().map(|d| d.guid).collect::>(); + let mut deletions = merged_root.deletions().collect::>(); deletions.sort(); assert_eq!(deletions, expected_deletions); @@ -927,20 +924,17 @@ fn complex_orphaning() { }) }) }); - let expected_deletions = vec!["folderBBBBBB", "folderEEEEEE"]; + let expected_deletions = &["folderBBBBBB", "folderEEEEEE"]; let expected_telem = StructureCounts { - remote_revives: 0, local_deletes: 1, - local_revives: 0, remote_deletes: 1, - dupes: 0, merged_nodes: 7, - merged_deletions: 2, + ..StructureCounts::default() }; assert_eq!(&expected_tree, merged_root.node()); - let mut deletions = merged_root.deletions().map(|d| d.guid).collect::>(); + let mut deletions = merged_root.deletions().collect::>(); deletions.sort(); assert_eq!(deletions, expected_deletions); @@ -1021,20 +1015,17 @@ fn locally_modified_remotely_deleted() { }) }) }); - let expected_deletions = vec!["folderBBBBBB", "folderEEEEEE"]; + let expected_deletions = &["folderBBBBBB", "folderEEEEEE"]; let expected_telem = StructureCounts { - remote_revives: 0, local_deletes: 1, - local_revives: 0, remote_deletes: 1, - dupes: 0, merged_nodes: 7, - merged_deletions: 2, + ..StructureCounts::default() }; assert_eq!(&expected_tree, merged_root.node()); - let mut deletions = merged_root.deletions().map(|d| d.guid).collect::>(); + let mut deletions = merged_root.deletions().collect::>(); deletions.sort(); assert_eq!(deletions, expected_deletions); @@ -1095,7 +1086,7 @@ fn locally_deleted_remotely_modified() { ("bookmarkGGGG", RemoteWithNewRemoteStructure) }) }); - let expected_deletions = vec![ + let expected_deletions = &[ "bookmarkCCCC", "bookmarkEEEE", "folderBBBBBB", @@ -1104,16 +1095,13 @@ fn locally_deleted_remotely_modified() { let expected_telem = StructureCounts { remote_revives: 1, local_deletes: 2, - local_revives: 0, - remote_deletes: 0, - dupes: 0, merged_nodes: 4, - merged_deletions: 4, + ..StructureCounts::default() }; assert_eq!(&expected_tree, merged_root.node()); - let mut deletions = merged_root.deletions().map(|d| d.guid).collect::>(); + let mut deletions = merged_root.deletions().collect::>(); deletions.sort(); assert_eq!(deletions, expected_deletions); @@ -1140,18 +1128,26 @@ fn nonexistent_on_one_side() { let mut expected_root = Item::new(ROOT_GUID, Kind::Folder); expected_root.needs_merge = true; let expected_tree = merged_nodes!(ROOT_GUID, Unchanged, {}); - let expected_deletions = vec!["bookmarkAAAA", "bookmarkBBBB"]; + let expected_deletions = &["bookmarkAAAA", "bookmarkBBBB"]; let expected_telem = StructureCounts { - merged_deletions: 2, ..StructureCounts::default() }; assert_eq!(&expected_tree, merged_root.node()); - let mut deletions = merged_root.deletions().map(|d| d.guid).collect::>(); + let mut deletions = merged_root.deletions().collect::>(); deletions.sort(); assert_eq!(deletions, expected_deletions); + let ops = merged_root.completion_ops(); + assert_eq!( + ops.summarize(), + &[ + "Flag remote bookmarkBBBB as merged", + "Delete local tombstone bookmarkAAAA", + ] + ); + assert_eq!(merged_root.counts(), &expected_telem); } @@ -1221,16 +1217,15 @@ fn clear_folder_then_delete() { ("bookmarkCCCC", Remote) }) }); - let expected_deletions = vec!["folderAAAAAA", "folderDDDDDD"]; + let expected_deletions = &["folderAAAAAA", "folderDDDDDD"]; let expected_telem = StructureCounts { merged_nodes: 7, - merged_deletions: 2, ..StructureCounts::default() }; assert_eq!(&expected_tree, merged_root.node()); - let mut deletions = merged_root.deletions().map(|d| d.guid).collect::>(); + let mut deletions = merged_root.deletions().collect::>(); deletions.sort(); assert_eq!(deletions, expected_deletions); @@ -1303,20 +1298,17 @@ fn newer_move_to_deleted() { ("bookmarkBBBB", Remote) }) }); - let expected_deletions = vec!["folderAAAAAA", "folderCCCCCC"]; + let expected_deletions = &["folderAAAAAA", "folderCCCCCC"]; let expected_telem = StructureCounts { - remote_revives: 0, local_deletes: 1, - local_revives: 0, remote_deletes: 1, - dupes: 0, merged_nodes: 6, - merged_deletions: 2, + ..StructureCounts::default() }; assert_eq!(&expected_tree, merged_root.node()); - let mut deletions = merged_root.deletions().map(|d| d.guid).collect::>(); + let mut deletions = merged_root.deletions().collect::>(); deletions.sort(); assert_eq!(deletions, expected_deletions); @@ -1383,13 +1375,8 @@ fn deduping_multiple_candidates() { }) }); let expected_telem = StructureCounts { - remote_revives: 0, - local_deletes: 0, - local_revives: 0, - remote_deletes: 0, - dupes: 0, merged_nodes: 6, - merged_deletions: 0, + ..StructureCounts::default() }; assert_eq!(&expected_tree, merged_root.node()); @@ -1465,13 +1452,9 @@ fn deduping_local_newer() { }) }); let expected_telem = StructureCounts { - remote_revives: 0, - local_deletes: 0, - local_revives: 0, - remote_deletes: 0, dupes: 2, merged_nodes: 5, - merged_deletions: 0, + ..StructureCounts::default() }; assert_eq!(&expected_tree, merged_root.node()); @@ -1621,13 +1604,9 @@ fn deduping_remote_newer() { }) }); let expected_telem = StructureCounts { - remote_revives: 0, - local_deletes: 0, - local_revives: 0, - remote_deletes: 0, dupes: 6, merged_nodes: 11, - merged_deletions: 0, + ..StructureCounts::default() }; assert_eq!(&expected_tree, merged_root.node()); @@ -1760,13 +1739,9 @@ fn complex_deduping() { }) }); let expected_telem = StructureCounts { - remote_revives: 0, - local_deletes: 0, - local_revives: 0, - remote_deletes: 0, dupes: 6, merged_nodes: 9, - merged_deletions: 0, + ..StructureCounts::default() }; assert_eq!(&expected_tree, merged_root.node()); @@ -1799,20 +1774,19 @@ fn left_pane_root() { let merged_root = merger.merge().unwrap(); let expected_tree = merged_nodes!(ROOT_GUID, Local); - let expected_deletions = vec![ + let expected_deletions = &[ "folderLEFTPC", "folderLEFTPF", "folderLEFTPQ", "folderLEFTPR", ]; let expected_telem = StructureCounts { - merged_deletions: 4, ..StructureCounts::default() }; assert_eq!(&expected_tree, merged_root.node()); - let mut deletions = merged_root.deletions().map(|d| d.guid).collect::>(); + let mut deletions = merged_root.deletions().collect::>(); deletions.sort(); assert_eq!(deletions, expected_deletions); @@ -1857,7 +1831,7 @@ fn livemarks() { ("toolbar_____", LocalWithNewLocalStructure), ("unfiled_____", RemoteWithNewRemoteStructure) }); - let expected_deletions = vec![ + let expected_deletions = &[ "livemarkAAAA", "livemarkBBBB", "livemarkCCCC", @@ -1866,14 +1840,12 @@ fn livemarks() { ]; let expected_telem = StructureCounts { merged_nodes: 3, - // A, B, and C are counted twice, since they exist on both sides. - merged_deletions: 8, ..StructureCounts::default() }; assert_eq!(&expected_tree, merged_root.node()); - let mut deletions = merged_root.deletions().map(|d| d.guid).collect::>(); + let mut deletions = merged_root.deletions().collect::>(); deletions.sort(); assert_eq!(deletions, expected_deletions); @@ -1956,7 +1928,7 @@ fn non_syncable_items() { ("bookmarkGGGG", Remote) }) }); - let expected_deletions = vec![ + let expected_deletions = &[ "bookmarkEEEE", // Non-syncable locally. "bookmarkFFFF", // Non-syncable locally. "bookmarkIIII", // Non-syncable remotely. @@ -1971,13 +1943,12 @@ fn non_syncable_items() { ]; let expected_telem = StructureCounts { merged_nodes: 5, - merged_deletions: 16, ..StructureCounts::default() }; assert_eq!(&expected_tree, merged_root.node()); - let mut deletions = merged_root.deletions().map(|d| d.guid).collect::>(); + let mut deletions = merged_root.deletions().collect::>(); deletions.sort(); assert_eq!(deletions, expected_deletions); @@ -2081,13 +2052,9 @@ fn applying_two_empty_folders_matches_only_one() { }) }); let expected_telem = StructureCounts { - remote_revives: 0, - local_deletes: 0, - local_revives: 0, - remote_deletes: 0, dupes: 1, merged_nodes: 4, - merged_deletions: 0, + ..StructureCounts::default() }; assert_eq!(&expected_tree, merged_root.node()); @@ -2150,13 +2117,9 @@ fn deduping_ignores_parent_title() { }) }); let expected_telem = StructureCounts { - remote_revives: 0, - local_deletes: 0, - local_revives: 0, - remote_deletes: 0, dupes: 1, merged_nodes: 2, - merged_deletions: 0, + ..StructureCounts::default() }; assert_eq!(&expected_tree, merged_root.node()); @@ -2253,7 +2216,7 @@ fn invalid_guids() { let count = self.0.get(); self.0.set(count + 1); assert!( - &[")(*&", "shortGUID", "loooooongGUID", "!@#$%^", "",].contains(&old_guid.as_str()), + &[")(*&", "shortGUID", "loooooongGUID", "!@#$%^", ""].contains(&old_guid.as_str()), "Didn't expect to generate new GUID for {}", old_guid ); @@ -2307,16 +2270,15 @@ fn invalid_guids() { ("item00000004", LocalWithNewLocalStructure) }) }); - let expected_deletions = vec!["", "!@#$%^", "loooooongGUID", "shortGUID"]; + let expected_deletions = &["", "!@#$%^", "loooooongGUID", "shortGUID"]; let expected_telem = StructureCounts { merged_nodes: 9, - merged_deletions: 4, ..StructureCounts::default() }; assert_eq!(&expected_tree, merged_root.node()); - let mut deletions = merged_root.deletions().map(|d| d.guid).collect::>(); + let mut deletions = merged_root.deletions().collect::>(); deletions.sort(); assert_eq!(deletions, expected_deletions); @@ -2498,20 +2460,13 @@ fn deleted_user_content_roots() { }) }); let expected_telem = StructureCounts { - remote_revives: 0, - local_deletes: 0, - local_revives: 0, - remote_deletes: 0, - dupes: 0, merged_nodes: 4, - merged_deletions: 1, + ..StructureCounts::default() }; assert_eq!(&expected_tree, merged_root.node()); - // TODO(lina): Remove invalid tombstones from both sides. - let deletions = merged_root.deletions().map(|d| d.guid).collect::>(); - assert_eq!(deletions, vec![Into::::into("toolbar_____")]); + assert_eq!(merged_root.deletions().count(), 1); assert_eq!(merged_root.counts(), &expected_telem); } @@ -2700,44 +2655,68 @@ fn reupload_replace() { ("folderGGGGGG", Local) }) }); - let expected_deletions = vec![ + let expected_deletions = &[ // C is invalid on both sides, so we need to upload a tombstone. - ("bookmarkCCCC", true), + "bookmarkCCCC", // E is invalid locally and deleted remotely, so doesn't need a // tombstone. - ("bookmarkEEEE", false), + "bookmarkEEEE", // H is invalid locally and doesn't exist remotely, so doesn't need a // tombstone. - ("bookmarkHHHH", false), + "bookmarkHHHH", // I is deleted locally and invalid remotely, so needs a tombstone. - ("bookmarkIIII", true), + "bookmarkIIII", // J doesn't exist locally and invalid remotely, so needs a tombstone. - ("bookmarkJJJJ", true), + "bookmarkJJJJ", ]; 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() }; assert_eq!(&expected_tree, merged_root.node()); - let mut deletions = merged_root - .deletions() - .map(|d| (d.guid.as_ref(), d.should_upload_tombstone)) - .collect::>(); - deletions.sort_by(|a, b| a.0.cmp(&b.0)); + let mut deletions = merged_root.deletions().collect::>(); + deletions.sort(); assert_eq!(deletions, expected_deletions); + let ops = merged_root.completion_ops(); + let mut summary = ops.summarize(); + summary.sort(); + assert_eq!( + summary, + &[ + "Apply remote bookmarkFFFF", + "Apply remote bookmarkKKKK", + "Delete local item bookmarkCCCC", + "Delete local item bookmarkEEEE", + "Delete local item bookmarkHHHH", + "Flag local bookmarkAAAA as unmerged", + "Flag local bookmarkKKKK as unmerged", + "Flag local folderBBBBBB as unmerged", + "Flag local folderGGGGGG as unmerged", + "Flag local toolbar_____ as unmerged", + "Flag remote bookmarkEEEE as merged", + "Insert local tombstone bookmarkCCCC", + "Insert local tombstone bookmarkJJJJ", + "Move bookmarkKKKK into unfiled_____ at 0", + "Upload item bookmarkAAAA", + "Upload item bookmarkKKKK", + "Upload item folderBBBBBB", + "Upload item folderGGGGGG", + "Upload item toolbar_____", + "Upload tombstone bookmarkCCCC", + "Upload tombstone bookmarkIIII", + "Upload tombstone bookmarkJJJJ", + ] + ); + assert_eq!(merged_root.counts(), &expected_telem); } #[test] fn completion_ops() { - let local_tree = nodes!({ + let mut local_tree_builder = Builder::try_from(nodes!({ ("menu________", Folder, { ("bookmarkAAAA", Bookmark), ("bookmarkBBBB", Bookmark), @@ -2747,15 +2726,18 @@ fn completion_ops() { ("toolbar_____", Folder, { ("bookmarkEEEE", Bookmark) }), - ("unfiled_____", Folder), + ("unfiled_____", Folder[needs_merge = true], { + ("bookmarkIIII", Bookmark) + }), ("mobile______", Folder, { ("bookmarkFFFF", Bookmark[needs_merge = true, age = 10]) }) - }) - .into_tree() + })) .unwrap(); + local_tree_builder.deletion("bookmarkJJJJ".into()); + let local_tree = local_tree_builder.into_tree().unwrap(); - let remote_tree = nodes!({ + let mut remote_tree_builder = Builder::try_from(nodes!({ ("menu________", Folder[needs_merge = true], { ("bookmarkAAAA", Bookmark), ("bookmarkDDDD", Bookmark), @@ -2767,14 +2749,16 @@ fn completion_ops() { ("bookmarkGGGG", Bookmark[needs_merge = true]) }), ("unfiled_____", Folder[needs_merge = true], { - ("bookmarkHHHH", Bookmark[needs_merge = true]) + ("bookmarkHHHH", Bookmark[needs_merge = true]), + ("bookmarkJJJJ", Bookmark) }), ("mobile______", Folder, { ("bookmarkFFFF", Bookmark[needs_merge = true, age = 5]) }) - }) - .into_tree() + })) .unwrap(); + remote_tree_builder.deletion("bookmarkIIII".into()); + let remote_tree = remote_tree_builder.into_tree().unwrap(); let merger = Merger::new(&local_tree, &remote_tree); let merged_root = merger.merge().unwrap(); @@ -2790,7 +2774,7 @@ fn completion_ops() { ("toolbar_____", UnchangedWithNewLocalStructure, { ("bookmarkGGGG", Remote) }), - ("unfiled_____", UnchangedWithNewLocalStructure, { + ("unfiled_____", LocalWithNewLocalStructure, { ("bookmarkHHHH", Remote) }), ("mobile______", Unchanged, { @@ -2803,10 +2787,7 @@ fn completion_ops() { let ops = merged_root.completion_ops(); assert!(ops.change_guids.is_empty()); assert_eq!( - ops.apply_remote_items - .iter() - .map(|op| op.to_string()) - .collect::>(), + to_strings(&ops.apply_remote_items).collect::>(), &[ "Apply remote bookmarkEEEE", "Apply remote bookmarkGGGG", @@ -2815,10 +2796,7 @@ fn completion_ops() { ] ); assert_eq!( - ops.apply_new_local_structure - .iter() - .map(|op| op.to_string()) - .collect::>(), + to_strings(&ops.apply_new_local_structure).collect::>(), &[ "Move bookmarkDDDD into menu________ at 1", "Move bookmarkBBBB into menu________ at 3", @@ -2827,14 +2805,34 @@ fn completion_ops() { "Move bookmarkHHHH into unfiled_____ at 0", ] ); - assert!(ops.upload.is_empty()); + assert!(ops.set_local_unmerged.is_empty()); + assert_eq!( + to_strings(&ops.set_local_merged).collect::>(), + &["Flag local bookmarkFFFF as merged"] + ); + assert_eq!( + to_strings(&ops.set_remote_merged).collect::>(), + &[ + "Flag remote menu________ as merged", + "Flag remote bookmarkEEEE as merged", + "Flag remote toolbar_____ as merged", + "Flag remote bookmarkGGGG as merged", + "Flag remote bookmarkHHHH as merged", + "Flag remote bookmarkFFFF as merged", + "Flag remote bookmarkIIII as merged", + ] + ); + let mut delete_local_items = to_strings(&ops.delete_local_items).collect::>(); + delete_local_items.sort(); + assert_eq!(delete_local_items, &["Delete local item bookmarkIIII"]); + assert!(ops.insert_local_tombstones.is_empty()); assert_eq!( - ops.skip_upload - .iter() - .map(|op| op.to_string()) - .collect::>(), - &["Don't upload bookmarkFFFF",] + to_strings(&ops.upload_items).collect::>(), + &["Upload item unfiled_____"] ); + let mut upload_tombstones = to_strings(&ops.upload_tombstones).collect::>(); + upload_tombstones.sort(); + assert_eq!(upload_tombstones, &["Upload tombstone bookmarkJJJJ"]); } #[test] @@ -2893,10 +2891,7 @@ fn problems() { let mut summary = problems.summarize().collect::>(); summary.sort_by(|a, b| a.guid().cmp(b.guid())); assert_eq!( - summary - .into_iter() - .map(|s| s.to_string()) - .collect::>(), + to_strings(&summary).collect::>(), &[ "bookmarkAAAA is an orphan", "bookmarkBBBB is in children of folderCCCCCC and has parent folderDDDDDD", diff --git a/third_party/rust/dogear/src/tree.rs b/third_party/rust/dogear/src/tree.rs index cc8513571364..3a9440c74d2d 100644 --- a/third_party/rust/dogear/src/tree.rs +++ b/third_party/rust/dogear/src/tree.rs @@ -76,10 +76,16 @@ impl Tree { Node(self, &self.entries[0]) } - /// Returns an iterator for all tombstoned GUIDs. + /// Returns the set of all tombstoned GUIDs. #[inline] - pub fn deletions(&self) -> impl Iterator { - self.deleted_guids.iter() + pub fn deletions(&self) -> &HashSet { + &self.deleted_guids + } + + /// Indicates if the GUID exists in the tree. + #[inline] + pub fn exists(&self, guid: &Guid) -> bool { + self.entry_index_by_guid.contains_key(guid) } /// Indicates if the GUID is known to be deleted. If `Tree::node_for_guid` @@ -1425,7 +1431,14 @@ impl ProblemCounts { pub struct Node<'t>(&'t Tree, &'t TreeEntry); impl<'t> Node<'t> { + /// Returns the item for this node. + #[inline] + pub fn item(&self) -> &'t Item { + &self.1.item + } + /// Returns content info for deduping this item, if available. + #[inline] pub fn content(&self) -> Option<&'t Content> { self.1.content.as_ref() } @@ -1528,7 +1541,7 @@ impl<'t> Node<'t> { } fn to_ascii_fragment(&self, prefix: &str) -> String { - match self.1.item.kind { + match self.item().kind { Kind::Folder => { let children_prefix = format!("{}| ", prefix); let children = self @@ -1541,13 +1554,13 @@ impl<'t> Node<'t> { "📂" }; if children.is_empty() { - format!("{}{} {}", prefix, kind, self.1.item) + format!("{}{} {}", prefix, kind, self.item()) } else { format!( "{}{} {}\n{}", prefix, kind, - self.1.item, + self.item(), children.join("\n") ) } @@ -1558,7 +1571,7 @@ impl<'t> Node<'t> { } else { "🔖" }; - format!("{}{} {}", prefix, kind, self.1.item) + format!("{}{} {}", prefix, kind, self.item()) } } } @@ -1573,21 +1586,22 @@ impl<'t> Node<'t> { /// these are non-syncable. #[inline] pub fn is_built_in_root(&self) -> bool { - self.1.item.guid.is_built_in_root() + self.item().guid.is_built_in_root() } } impl<'t> Deref for Node<'t> { type Target = Item; + #[inline] fn deref(&self) -> &Item { - &self.1.item + self.item() } } impl<'t> fmt::Display for Node<'t> { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - self.1.item.fmt(f) + self.item().fmt(f) } } diff --git a/toolkit/components/places/SyncedBookmarksMirror.jsm b/toolkit/components/places/SyncedBookmarksMirror.jsm index 0232ccb28435..e4c7be42451a 100644 --- a/toolkit/components/places/SyncedBookmarksMirror.jsm +++ b/toolkit/components/places/SyncedBookmarksMirror.jsm @@ -703,19 +703,39 @@ class SyncedBookmarksMirror { Ci.mozISyncedBookmarksMirrorCallback, ]), // `mozISyncedBookmarksMirrorProgressListener` methods. - onFetchLocalTree: (took, count, problems) => { - this.progress.stepWithItemCount( + onFetchLocalTree: (took, itemCount, deleteCount, problemsBag) => { + let counts = [ + { + name: "items", + count: itemCount, + }, + { + name: "deletions", + count: deleteCount, + }, + ]; + this.progress.stepWithTelemetry( ProgressTracker.STEPS.FETCH_LOCAL_TREE, took, - count + counts ); // We don't record local tree problems in validation telemetry. }, - onFetchRemoteTree: (took, count, problemsBag) => { - this.progress.stepWithItemCount( + onFetchRemoteTree: (took, itemCount, deleteCount, problemsBag) => { + let counts = [ + { + name: "items", + count: itemCount, + }, + { + name: "deletions", + count: deleteCount, + }, + ]; + this.progress.stepWithTelemetry( ProgressTracker.STEPS.FETCH_REMOTE_TREE, took, - count + counts ); // Record validation telemetry for problems in the remote tree. let problems = bagToNamedCounts(problemsBag, [ @@ -726,12 +746,12 @@ class SyncedBookmarksMirror { "parentChildDisagreements", "missingChildren", ]); - this.recordValidationTelemetry(took, count, problems); + let checked = itemCount + deleteCount; + this.recordValidationTelemetry(took, checked, problems); }, onMerge: (took, countsBag) => { let counts = bagToNamedCounts(countsBag, [ "items", - "deletes", "dupes", "remoteRevives", "localDeletes", @@ -1578,7 +1598,6 @@ async function cleanupMirrorDatabase(db) { await db.execute(`DROP TABLE changeGuidOps`); await db.execute(`DROP TABLE itemsToApply`); await db.execute(`DROP TABLE applyNewLocalStructureOps`); - await db.execute(`DROP TABLE itemsToRemove`); await db.execute(`DROP VIEW localTags`); await db.execute(`DROP TABLE itemsAdded`); await db.execute(`DROP TABLE guidsChanged`); @@ -1730,17 +1749,6 @@ async function initializeTempMirrorEntities(db) { WHERE guid = OLD.mergedGuid; END`); - // Stages all items to delete locally and remotely. Items to delete locally - // don't need tombstones: since we took the remote deletion, the tombstone - // already exists on the server. Items to delete remotely, or non-syncable - // items to delete on both sides, need tombstones. - await db.execute(`CREATE TEMP TABLE itemsToRemove( - guid TEXT PRIMARY KEY, - localLevel INTEGER NOT NULL, - shouldUploadTombstone BOOLEAN NOT NULL, - dateRemovedMicroseconds INTEGER NOT NULL - ) WITHOUT ROWID`); - // A view of local bookmark tags. Tags, like keywords, are associated with // URLs, so two bookmarks with the same URL should have the same tags. Unlike // keywords, one tag may be associated with many different URLs. Tags are also diff --git a/toolkit/components/places/bookmark_sync/Cargo.toml b/toolkit/components/places/bookmark_sync/Cargo.toml index 11ccec906a6e..46bef1a81bbd 100644 --- a/toolkit/components/places/bookmark_sync/Cargo.toml +++ b/toolkit/components/places/bookmark_sync/Cargo.toml @@ -6,7 +6,7 @@ edition = "2018" [dependencies] atomic_refcell = "0.1" -dogear = "0.3.3" +dogear = "0.4.0" libc = "0.2" log = "0.4" cstr = "0.1" diff --git a/toolkit/components/places/bookmark_sync/src/driver.rs b/toolkit/components/places/bookmark_sync/src/driver.rs index 8f64fe5b5389..2d0f52c74fac 100644 --- a/toolkit/components/places/bookmark_sync/src/driver.rs +++ b/toolkit/components/places/bookmark_sync/src/driver.rs @@ -200,6 +200,7 @@ impl Task for RecordTelemetryEventTask { callback.OnFetchLocalTree( as_millis(stats.time), stats.items as i64, + stats.deletions as i64, problem_counts_to_bag(&stats.problems).bag().coerce(), ) }, @@ -207,6 +208,7 @@ impl Task for RecordTelemetryEventTask { callback.OnFetchRemoteTree( as_millis(stats.time), stats.items as i64, + stats.deletions as i64, problem_counts_to_bag(&stats.problems).bag().coerce(), ) }, @@ -255,6 +257,5 @@ fn structure_counts_to_bag(counts: &StructureCounts) -> HashPropertyBag { bag.set("remoteDeletes", counts.remote_deletes as i64); bag.set("dupes", counts.dupes as i64); bag.set("items", counts.merged_nodes as i64); - bag.set("deletes", counts.merged_deletions as i64); bag } diff --git a/toolkit/components/places/bookmark_sync/src/store.rs b/toolkit/components/places/bookmark_sync/src/store.rs index 04be1d74c340..d8625eeaa8bb 100644 --- a/toolkit/components/places/bookmark_sync/src/store.rs +++ b/toolkit/components/places/bookmark_sync/src/store.rs @@ -5,8 +5,8 @@ use std::{collections::HashMap, convert::TryFrom, fmt, time::SystemTime}; use dogear::{ - debug, AbortSignal, CompletionOps, Content, Deletion, Guid, Item, Kind, MergedRoot, Tree, - Upload, Validity, + debug, AbortSignal, CompletionOps, Content, DeleteLocalItem, Guid, Item, Kind, MergedRoot, + Tree, UploadItem, UploadTombstone, Validity, }; use nsstring::nsString; use storage::{Conn, Step}; @@ -370,6 +370,12 @@ impl<'s> dogear::Store for Store<'s> { let is_deleted: i64 = step.get_by_name("isDeleted")?; if is_deleted == 1 { + let needs_merge: i32 = step.get_by_name("needsMerge")?; + if needs_merge == 0 { + // Ignore already-merged tombstones. These aren't persisted + // locally, so merging them is a no-op. + continue; + } let raw_guid: nsString = step.get_by_name("guid")?; let guid = Guid::from_utf16(&*raw_guid)?; builder.deletion(guid); @@ -408,13 +414,9 @@ impl<'s> dogear::Store for Store<'s> { } fn apply<'t>(&mut self, root: MergedRoot<'t>) -> Result { - self.controller.err_if_aborted()?; - let ops = root.completion_ops(); - - self.controller.err_if_aborted()?; - let deletions = root.deletions().collect::>(); + let ops = root.completion_ops_with_signal(self.controller)?; - if ops.is_empty() && deletions.is_empty() && self.weak_uploads.is_empty() { + if ops.is_empty() && self.weak_uploads.is_empty() { // If we don't have any items to apply, upload, or delete, // no need to open a transaction at all. return Ok(ApplyStatus::Skipped); @@ -429,14 +431,15 @@ impl<'s> dogear::Store for Store<'s> { } debug!(self.driver, "Updating local items in Places"); - update_local_items_in_places(&tx, &self.driver, &self.controller, &ops, deletions)?; + update_local_items_in_places(&tx, &self.driver, &self.controller, &ops)?; debug!(self.driver, "Staging items to upload"); stage_items_to_upload( &tx, &self.driver, &self.controller, - &ops.upload, + &ops.upload_items, + &ops.upload_tombstones, &self.weak_uploads, )?; @@ -464,7 +467,6 @@ fn update_local_items_in_places<'t>( driver: &Driver, controller: &AbortController, ops: &CompletionOps<'t>, - deletions: Vec, ) -> Result<()> { debug!( driver, @@ -660,45 +662,50 @@ fn update_local_items_in_places<'t>( statement.execute()?; } - debug!(driver, "Staging deletions"); - for chunk in deletions.chunks(SQLITE_MAX_VARIABLE_NUMBER) { - // Inserting into `itemsToRemove` fires the `noteItemRemoved` trigger, - // which records info for deleted item observer notifications. It's - // important we do this before updating the structure, so that the - // trigger captures the old parent and position. + debug!(driver, "Removing tombstones for revived items"); + for chunk in ops + .delete_local_tombstones + .chunks(SQLITE_MAX_VARIABLE_NUMBER) + { let mut statement = db.prepare(format!( - "INSERT INTO itemsToRemove(guid, localLevel, shouldUploadTombstone, - dateRemovedMicroseconds) - VALUES {}", - repeat_display(chunk.len(), ",", |index, f| { - let d = &chunk[index]; - write!( - f, - "(?, {}, {}, {})", - d.local_level, d.should_upload_tombstone as i8, now, - ) - }) + "DELETE FROM moz_bookmarks_deleted + WHERE guid IN ({})", + repeat_sql_vars(chunk.len()), ))?; - for (index, d) in chunk.iter().enumerate() { + for (index, op) in chunk.iter().enumerate() { controller.err_if_aborted()?; - statement.bind_by_index(index as u32, nsString::from(d.guid.as_str()))?; + statement.bind_by_index(index as u32, nsString::from(&*op.guid().as_str()))?; } statement.execute()?; } - // Remove tombstones for revived items. We intentionally use three `IN` - // subqueries instead of one with `UNION ALL`s because SQLite's query - // planner can't unroll the latter into a multi-index OR. - debug!(driver, "Removing tombstones for revived items"); - db.exec( - "DELETE FROM moz_bookmarks_deleted - WHERE guid IN (SELECT mergedGuid FROM itemsToApply) OR - guid IN (SELECT remoteGuid FROM itemsToApply) OR - guid IN (SELECT localGuid FROM changeGuidOps)", - )?; + debug!( + driver, + "Inserting new tombstones for non-syncable and invalid items" + ); + for chunk in ops + .insert_local_tombstones + .chunks(SQLITE_MAX_VARIABLE_NUMBER) + { + let mut statement = db.prepare(format!( + "INSERT INTO moz_bookmarks_deleted(guid, dateRemoved) + VALUES {}", + repeat_display(chunk.len(), ",", |_, f| write!(f, "(?, {})", now)), + ))?; + for (index, op) in chunk.iter().enumerate() { + controller.err_if_aborted()?; + statement.bind_by_index( + index as u32, + nsString::from(&*op.remote_node().guid.as_str()), + )?; + } + statement.execute()?; + } debug!(driver, "Removing local items"); - remove_local_items(db, driver, controller)?; + for chunk in ops.delete_local_items.chunks(SQLITE_MAX_VARIABLE_NUMBER) { + remove_local_items(&db, driver, controller, chunk)?; + } // Fires the `changeGuids` trigger. debug!(driver, "Changing GUIDs"); @@ -718,9 +725,11 @@ fn update_local_items_in_places<'t>( controller.err_if_aborted()?; db.exec("DELETE FROM applyNewLocalStructureOps")?; - // Reset the change counter for items that we don't need to upload. - debug!(driver, "Applying skip upload ops"); - for chunk in ops.skip_upload.chunks(SQLITE_MAX_VARIABLE_NUMBER) { + debug!( + driver, + "Resetting change counters for items that shouldn't be uploaded" + ); + for chunk in ops.set_local_merged.chunks(SQLITE_MAX_VARIABLE_NUMBER) { let mut statement = db.prepare(format!( "UPDATE moz_bookmarks SET syncChangeCounter = 0 @@ -734,9 +743,11 @@ fn update_local_items_in_places<'t>( statement.execute()?; } - // Bump the counter for items that we should upload. - debug!(driver, "Applying flag for upload ops"); - for chunk in ops.flag_for_upload.chunks(SQLITE_MAX_VARIABLE_NUMBER) { + debug!( + driver, + "Bumping change counters for items that should be uploaded" + ); + for chunk in ops.set_local_unmerged.chunks(SQLITE_MAX_VARIABLE_NUMBER) { let mut statement = db.prepare(format!( "UPDATE moz_bookmarks SET syncChangeCounter = 1 @@ -750,9 +761,8 @@ fn update_local_items_in_places<'t>( statement.execute()?; } - // Flag applied remote items as merged in the mirror. - debug!(driver, "Applying flag as merged ops"); - for chunk in ops.flag_as_merged.chunks(SQLITE_MAX_VARIABLE_NUMBER) { + debug!(driver, "Flagging applied remote items as merged"); + for chunk in ops.set_remote_merged.chunks(SQLITE_MAX_VARIABLE_NUMBER) { let mut statement = db.prepare(format!( "UPDATE items SET needsMerge = 0 @@ -906,69 +916,87 @@ fn apply_remote_items(db: &Conn, driver: &Driver, controller: &AbortController) Ok(()) } -/// Removes items that are deleted on one or both sides from Places, and inserts -/// new tombstones for non-syncable items to delete remotely. -fn remove_local_items(db: &Conn, driver: &Driver, controller: &AbortController) -> Result<()> { +/// Removes deleted local items from Places. +fn remove_local_items( + db: &Conn, + driver: &Driver, + controller: &AbortController, + ops: &[DeleteLocalItem], +) -> Result<()> { debug!(driver, "Recording observer notifications for removed items"); - controller.err_if_aborted()?; - db.exec( - "INSERT INTO itemsRemoved(itemId, parentId, position, type, placeId, + let mut observer_statement = db.prepare(format!( + "WITH + ops(guid, level) AS (VALUES {}) + INSERT INTO itemsRemoved(itemId, parentId, position, type, placeId, guid, parentGuid, level) SELECT b.id, b.parent, b.position, b.type, b.fk, - b.guid, p.guid, d.localLevel - FROM itemsToRemove d - JOIN moz_bookmarks b ON b.guid = d.guid + b.guid, p.guid, n.level + FROM ops n + JOIN moz_bookmarks b ON b.guid = n.guid JOIN moz_bookmarks p ON p.id = b.parent", - )?; + repeat_display(ops.len(), ",", |index, f| { + let op = &ops[index]; + write!(f, "(?, {})", op.local_node().level()) + }), + ))?; + for (index, op) in ops.iter().enumerate() { + controller.err_if_aborted()?; + observer_statement.bind_by_index( + index as u32, + nsString::from(&*op.local_node().guid.as_str()), + )?; + } + observer_statement.execute()?; - debug!(driver, "Flag URL frecencies for recalculation"); - controller.err_if_aborted()?; - db.exec( + debug!(driver, "Recalculating frecencies for removed bookmark URLs"); + let mut frecency_statement = db.prepare(format!( "UPDATE moz_places SET frecency = -frecency WHERE id IN (SELECT b.fk FROM moz_bookmarks b - JOIN itemsToRemove d ON d.guid = b.guid) AND + WHERE b.guid IN ({})) AND frecency > 0", - )?; + repeat_sql_vars(ops.len()) + ))?; + for (index, op) in ops.iter().enumerate() { + controller.err_if_aborted()?; + frecency_statement.bind_by_index( + index as u32, + nsString::from(&*op.local_node().guid.as_str()), + )?; + } + frecency_statement.execute()?; // This can be removed in bug 1460577. debug!(driver, "Removing annos for deleted items"); - controller.err_if_aborted()?; - db.exec( + let mut annos_statement = db.prepare(format!( "DELETE FROM moz_items_annos WHERE item_id = (SELECT b.id FROM moz_bookmarks b - JOIN itemsToRemove d ON d.guid = b.guid)", - )?; - - debug!( - driver, - "Removing tombstones for items already deleted remotely" - ); - controller.err_if_aborted()?; - db.exec( - "DELETE FROM moz_bookmarks_deleted - WHERE guid IN (SELECT guid FROM itemsToRemove - WHERE NOT shouldUploadTombstone)", - )?; - - // Upload tombstones for non-syncable and invalid items. We can remove the - // `shouldUploadTombstone` check and persist tombstones unconditionally in - // bug 1343103. - debug!(driver, "Inserting tombstones for all deleted items"); - controller.err_if_aborted()?; - db.exec( - "INSERT OR IGNORE INTO moz_bookmarks_deleted(guid, dateRemoved) - SELECT guid, dateRemovedMicroseconds - FROM itemsToRemove - WHERE shouldUploadTombstone", - )?; + WHERE b.guid IN ({}))", + repeat_sql_vars(ops.len()), + ))?; + for (index, op) in ops.iter().enumerate() { + controller.err_if_aborted()?; + annos_statement.bind_by_index( + index as u32, + nsString::from(&*op.local_node().guid.as_str()), + )?; + } + annos_statement.execute()?; debug!(driver, "Removing deleted items from Places"); - controller.err_if_aborted()?; - db.exec( + let mut delete_statement = db.prepare(format!( "DELETE FROM moz_bookmarks - WHERE guid IN (SELECT guid FROM itemsToRemove)", - )?; + WHERE guid IN ({})", + repeat_sql_vars(ops.len()), + ))?; + for (index, op) in ops.iter().enumerate() { + controller.err_if_aborted()?; + delete_statement.bind_by_index( + index as u32, + nsString::from(&*op.local_node().guid.as_str()), + )?; + } + delete_statement.execute()?; Ok(()) } @@ -995,7 +1023,8 @@ fn stage_items_to_upload( db: &Conn, driver: &Driver, controller: &AbortController, - upload: &[Upload], + upload_items: &[UploadItem], + upload_tombstones: &[UploadTombstone], weak_upload: &[nsString], ) -> Result<()> { debug!(driver, "Cleaning up staged items left from last sync"); @@ -1038,7 +1067,7 @@ fn stage_items_to_upload( ))?; debug!(driver, "Staging remaining locally changed items for upload"); - for chunk in upload.chunks(SQLITE_MAX_VARIABLE_NUMBER) { + for chunk in upload_items.chunks(SQLITE_MAX_VARIABLE_NUMBER) { let mut statement = db.prepare(format!( "INSERT OR IGNORE INTO itemsToUpload(id, guid, syncChangeCounter, parentGuid, parentTitle, @@ -1082,22 +1111,25 @@ fn stage_items_to_upload( // Finally, stage tombstones for deleted items. Ignore conflicts if we have // tombstones for undeleted items; Places Maintenance should clean these up. - controller.err_if_aborted()?; debug!(driver, "Staging tombstones to upload"); - db.exec( - " - INSERT OR IGNORE INTO itemsToUpload(guid, syncChangeCounter, isDeleted) - SELECT guid, 1, 1 FROM moz_bookmarks_deleted", - )?; + for chunk in upload_tombstones.chunks(SQLITE_MAX_VARIABLE_NUMBER) { + let mut statement = db.prepare(format!( + "INSERT OR IGNORE INTO itemsToUpload(guid, syncChangeCounter, isDeleted) + VALUES {}", + repeat_display(chunk.len(), ",", |_, f| write!(f, "(?, 1, 1)")) + ))?; + for (index, op) in chunk.iter().enumerate() { + controller.err_if_aborted()?; + statement.bind_by_index(index as u32, nsString::from(op.guid().as_str()))?; + } + statement.execute()?; + } Ok(()) } fn cleanup(db: &Conn) -> Result<()> { - db.exec( - "DELETE FROM itemsToApply; - DELETE FROM itemsToRemove;", - )?; + db.exec("DELETE FROM itemsToApply")?; Ok(()) } diff --git a/toolkit/components/places/mozISyncedBookmarksMirror.idl b/toolkit/components/places/mozISyncedBookmarksMirror.idl index f468b4bfeb6f..9a2dce900993 100644 --- a/toolkit/components/places/mozISyncedBookmarksMirror.idl +++ b/toolkit/components/places/mozISyncedBookmarksMirror.idl @@ -15,11 +15,11 @@ interface mozISyncedBookmarksMirrorProgressListener : nsISupports { // taken to build the tree, number of items in the tree, and a map of // structure problem counts for validation telemetry. void onFetchLocalTree(in long long took, in long long itemCount, - in nsIPropertyBag problems); + in long long deletedCount, in nsIPropertyBag problems); // Called after the merger builds the remote tree from the mirror database. void onFetchRemoteTree(in long long took, in long long itemCount, - in nsIPropertyBag problems); + in long long deletedCount, in nsIPropertyBag problems); // Called after the merger builds the merged tree, including structure change // counts for event telemetry. diff --git a/toolkit/components/places/tests/sync/test_bookmark_abort_merging.js b/toolkit/components/places/tests/sync/test_bookmark_abort_merging.js index acad627001dd..247a895e8508 100644 --- a/toolkit/components/places/tests/sync/test_bookmark_abort_merging.js +++ b/toolkit/components/places/tests/sync/test_bookmark_abort_merging.js @@ -64,7 +64,7 @@ add_task(async function test_blocker_state() { ); deepEqual( s.counts, - [{ name: "items", count: 6 }], + [{ name: "items", count: 6 }, { name: "deletions", count: 0 }], "Should report number of items in local tree" ); break; @@ -77,7 +77,7 @@ add_task(async function test_blocker_state() { ); deepEqual( s.counts, - [{ name: "items", count: 6 }], + [{ name: "items", count: 6 }, { name: "deletions", count: 0 }], "Should report number of items in remote tree" ); break; diff --git a/toolkit/components/places/tests/sync/test_bookmark_corruption.js b/toolkit/components/places/tests/sync/test_bookmark_corruption.js index 92c0f1651dfb..609006b1e59b 100644 --- a/toolkit/components/places/tests/sync/test_bookmark_corruption.js +++ b/toolkit/components/places/tests/sync/test_bookmark_corruption.js @@ -470,6 +470,13 @@ add_task(async function test_reupload_replace() { }, }); + let tombstones = await PlacesTestUtils.fetchSyncTombstones(); + deepEqual( + tombstones.map(({ guid }) => guid), + ["bookmarkEEEE", "queryDDDDDDD"], + "Should store local tombstones for (E D)" + ); + await storeChangesInMirror(buf, changesToUpload); deepEqual(await buf.fetchUnmergedGuids(), [], "Should merge all items"); @@ -786,6 +793,9 @@ add_task(async function test_corrupt_remote_roots() { "Should not corrupt local roots" ); + let tombstones = await PlacesTestUtils.fetchSyncTombstones(); + deepEqual(tombstones, [], "Should not store local tombstones"); + await storeChangesInMirror(buf, changesToUpload); deepEqual(await buf.fetchUnmergedGuids(), [], "Should merge all items"); @@ -1634,6 +1644,13 @@ add_task(async function test_move_into_orphaned() { "Should treat local tree as canonical if server is missing new parent" ); + let tombstones = await PlacesTestUtils.fetchSyncTombstones(); + deepEqual( + tombstones.map(({ guid }) => guid), + ["bookmarkDDDD"], + "Should store local tombstone for D" + ); + await storeChangesInMirror(buf, changesToUpload); deepEqual(await buf.fetchUnmergedGuids(), [], "Should merge all items"); @@ -2010,6 +2027,8 @@ add_task(async function test_tombstone_as_child() { }, "Should have ignored tombstone record" ); + let tombstones = await PlacesTestUtils.fetchSyncTombstones(); + deepEqual(tombstones, [], "Should not store local tombstones"); await buf.finalize(); await PlacesUtils.bookmarks.eraseEverything(); await PlacesSyncUtils.bookmarks.reset(); @@ -2190,15 +2209,20 @@ add_task(async function test_non_syncable_items() { deepEqual( await buf.fetchUnmergedGuids(), [ + "bookmarkFFFF", "bookmarkIIII", "bookmarkJJJJ", + "folderAAAAAA", + "folderDDDDDD", + "folderLEFTPC", "folderLEFTPF", + "folderLEFTPQ", "folderLEFTPR", PlacesUtils.bookmarks.menuGuid, "rootHHHHHHHH", PlacesUtils.bookmarks.unfiledGuid, ], - "Should leave non-syncable items unmerged" + "Should leave non-syncable items and roots with new remote structure unmerged" ); let datesAdded = await promiseManyDatesAdded([ @@ -2417,6 +2441,23 @@ add_task(async function test_non_syncable_items() { "Should exclude non-syncable items from new local structure" ); + let tombstones = await PlacesTestUtils.fetchSyncTombstones(); + deepEqual( + tombstones.map(({ guid }) => guid), + [ + "bookmarkFFFF", + "bookmarkIIII", + "folderAAAAAA", + "folderDDDDDD", + "folderLEFTPC", + "folderLEFTPF", + "folderLEFTPQ", + "folderLEFTPR", + "rootHHHHHHHH", + ], + "Should store local tombstones for non-syncable items" + ); + await storeChangesInMirror(buf, changesToUpload); deepEqual(await buf.fetchUnmergedGuids(), [], "Should merge all items"); @@ -2834,6 +2875,13 @@ add_task(async function test_invalid_guid() { ], }); + let tombstones = await PlacesTestUtils.fetchSyncTombstones(); + deepEqual( + tombstones.map(({ guid }) => guid), + ["bad!guid~"], + "Should store local tombstone for C's invalid GUID" + ); + await storeChangesInMirror(buf, changesToUpload); deepEqual(await buf.fetchUnmergedGuids(), [], "Should merge all items"); diff --git a/toolkit/components/places/tests/sync/test_bookmark_deletion.js b/toolkit/components/places/tests/sync/test_bookmark_deletion.js index 03ae808a7cce..ed21c0d3b64f 100644 --- a/toolkit/components/places/tests/sync/test_bookmark_deletion.js +++ b/toolkit/components/places/tests/sync/test_bookmark_deletion.js @@ -166,7 +166,6 @@ add_task(async function test_complex_orphaning() { mergeTelemetryCounts, [ { name: "items", count: 10 }, - { name: "deletes", count: 2 }, { name: "localDeletes", count: 1 }, { name: "remoteDeletes", count: 1 }, ], @@ -437,7 +436,6 @@ add_task(async function test_locally_modified_remotely_deleted() { mergeTelemetryCounts, [ { name: "items", count: 8 }, - { name: "deletes", count: 4 }, { name: "localRevives", count: 1 }, { name: "remoteDeletes", count: 2 }, ], @@ -488,6 +486,9 @@ add_task(async function test_locally_modified_remotely_deleted() { "Should restore A and relocate (F G) to menu" ); + let tombstones = await PlacesTestUtils.fetchSyncTombstones(); + deepEqual(tombstones, [], "Should not store local tombstones"); + await storeChangesInMirror(buf, changesToUpload); deepEqual(await buf.fetchUnmergedGuids(), [], "Should merge all items"); @@ -646,7 +647,6 @@ add_task(async function test_locally_deleted_remotely_modified() { mergeTelemetryCounts, [ { name: "items", count: 8 }, - { name: "deletes", count: 4 }, { name: "remoteRevives", count: 1 }, { name: "localDeletes", count: 2 }, ], @@ -931,8 +931,8 @@ add_task(async function test_nonexistent_on_one_side() { let changesToUpload = await buf.apply(); deepEqual( await buf.fetchUnmergedGuids(), - ["bookmarkBBBB", PlacesUtils.bookmarks.menuGuid], - "Should leave B tombstone and menu with new remote structure unmerged" + [PlacesUtils.bookmarks.menuGuid], + "Should leave menu with new remote structure unmerged" ); let menuInfo = await PlacesUtils.bookmarks.fetch( diff --git a/toolkit/components/places/tests/sync/test_bookmark_kinds.js b/toolkit/components/places/tests/sync/test_bookmark_kinds.js index 872b0359d7fa..5a643ff6b781 100644 --- a/toolkit/components/places/tests/sync/test_bookmark_kinds.js +++ b/toolkit/components/places/tests/sync/test_bookmark_kinds.js @@ -225,10 +225,12 @@ add_task(async function test_mismatched_folder_types() { info("Apply remote"); let changesToUpload = await buf.apply(); + // We leave livemarks unmerged because we're about to upload tombstones for + // them, anyway. deepEqual( await buf.fetchUnmergedGuids(), - [PlacesUtils.bookmarks.toolbarGuid], - "Should leave toolbar with new remote structure unmerged" + ["l1nZZXfB8nC7", "livemarkBBBB", PlacesUtils.bookmarks.toolbarGuid], + "Should leave livemarks and toolbar with new remote structure unmerged" ); let idsToUpload = inspectChangeRecords(changesToUpload); @@ -252,6 +254,13 @@ add_task(async function test_mismatched_folder_types() { "Should delete livemarks locally" ); + let tombstones = await PlacesTestUtils.fetchSyncTombstones(); + deepEqual( + tombstones.map(({ guid }) => guid), + ["l1nZZXfB8nC7", "livemarkBBBB"], + "Should store local tombstones for livemarks" + ); + await storeChangesInMirror(buf, changesToUpload); deepEqual(await buf.fetchUnmergedGuids(), [], "Should merge all items"); diff --git a/toolkit/components/places/tests/sync/test_bookmark_structure_changes.js b/toolkit/components/places/tests/sync/test_bookmark_structure_changes.js index c857b7fadead..99b7cf74a585 100644 --- a/toolkit/components/places/tests/sync/test_bookmark_structure_changes.js +++ b/toolkit/components/places/tests/sync/test_bookmark_structure_changes.js @@ -2873,6 +2873,13 @@ add_task(async function test_unchanged_newer_changed_older() { "Should merge children of changed side first, even if they're older" ); + let tombstones = await PlacesTestUtils.fetchSyncTombstones(); + deepEqual( + tombstones.map(({ guid }) => guid), + ["folderCCCCCC"], + "Should store local tombstone for C" + ); + await storeChangesInMirror(buf, changesToUpload); deepEqual(await buf.fetchUnmergedGuids(), [], "Should merge all items");