Bug 1546035 - Remove local and remote livemarks when syncing. r=mak,tcsc

This commit exports livemarks before syncing for the first time, to
avoid losing livemarks that Sync may have resurrected. As of v0.2.4,
Dogear treats livemarks as non-syncable, and deletes them on both
sides.

This also bumps the mirror schema version, to trigger a first sync.

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

services/sync/modules/engines/bookmarks.js

@@ -16,14 +16,15 @@ const {CryptoWrapper} = ChromeUtils.import("resource://services-sync/record.js")
 const {Svc, Utils} = ChromeUtils.import("resource://services-sync/util.js");
 
 XPCOMUtils.defineLazyModuleGetters(this, {
-  SyncedBookmarksMirror: "resource://gre/modules/SyncedBookmarksMirror.jsm",
   BookmarkValidator: "resource://services-sync/bookmark_validator.js",
+  LiveBookmarkMigrator: "resource:///modules/LiveBookmarkMigrator.jsm",
   OS: "resource://gre/modules/osfile.jsm",
   PlacesBackups: "resource://gre/modules/PlacesBackups.jsm",
   PlacesDBUtils: "resource://gre/modules/PlacesDBUtils.jsm",
   PlacesSyncUtils: "resource://gre/modules/PlacesSyncUtils.jsm",
   PlacesUtils: "resource://gre/modules/PlacesUtils.jsm",
   Resource: "resource://services-sync/resource.js",
+  SyncedBookmarksMirror: "resource://gre/modules/SyncedBookmarksMirror.jsm",
 });
 
 XPCOMUtils.defineLazyGetter(this, "PlacesBundle", () => {
@@ -363,6 +364,31 @@ BaseBookmarksEngine.prototype = {
     return newSyncID;
   },
 
+  async _syncStartup() {
+    await super._syncStartup();
+
+    try {
+      // For first syncs, back up the user's bookmarks and livemarks. Livemarks
+      // are unsupported as of bug 1477671, and syncing deletes them locally and
+      // remotely.
+      let lastSync = await this.getLastSync();
+      if (!lastSync) {
+        this._log.debug("Bookmarks backup starting");
+        await PlacesBackups.create(null, true);
+        this._log.debug("Bookmarks backup done");
+
+        this._log.debug("Livemarks backup starting");
+        await LiveBookmarkMigrator.migrate();
+        this._log.debug("Livemarks backup done");
+      }
+    } catch (ex) {
+      // Failure to create a backup is somewhat bad, but probably not bad
+      // enough to prevent syncing of bookmarks - so just log the error and
+      // continue.
+      this._log.warn("Error while backing up bookmarks, but continuing with sync", ex);
+    }
+  },
+
   async _sync() {
     try {
       await super._sync();
@@ -587,23 +613,7 @@ BookmarksEngine.prototype = {
   },
 
   async _syncStartup() {
-    await SyncEngine.prototype._syncStartup.call(this);
-
-    try {
-      // For first-syncs, make a backup for the user to restore
-      let lastSync = await this.getLastSync();
-      if (!lastSync) {
-        this._log.debug("Bookmarks backup starting.");
-        await PlacesBackups.create(null, true);
-        this._log.debug("Bookmarks backup done.");
-      }
-    } catch (ex) {
-      // Failure to create a backup is somewhat bad, but probably not bad
-      // enough to prevent syncing of bookmarks - so just log the error and
-      // continue.
-      this._log.warn("Error while backing up bookmarks, but continuing with sync", ex);
-    }
-
+    await super._syncStartup();
     this._store._childrenToOrder = {};
     this._store.clearPendingDeletions();
   },

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

@@ -1 +1 @@
-{"files":{"CODE_OF_CONDUCT.md":"e85149c44f478f164f7d5f55f6e66c9b5ae236d4a11107d5e2a93fe71dd874b9","Cargo.toml":"9b2fd48cc14073599c7d3a50f74fe6aa9896a862c72651f69e06a1ec37e43c6d","LICENSE":"c71d239df91726fc519c6eb72d318ec65820627232b2f796219e87dcf35d0ab4","README.md":"303ea5ec53d4e86f2c321056e8158e31aa061353a99e52de3d76859d40919efc","src/driver.rs":"10ecce90c6dee4e7b0ecd87f6d4f10c3cb825b544c6413416167248755097ab2","src/error.rs":"c6e661a7b94119dc8770c482681e97e644507e37f0ba32c04cc3a0b43e7b0077","src/guid.rs":"0330e6e893a550e478c8ac678114ebc112add97cb1d5d803d65cda6588ce7ba5","src/lib.rs":"ef42d0d3b234ffb6e459550f36a5f9220a0dd5fd09867affc7f8f9fe0b5430f2","src/merge.rs":"460c6af8ba3b680d072528e9ee27ea62559911b1cce5a511abc3d698bc7b3da3","src/store.rs":"612d90ea0614aa7cc943c4ac0faaee35c155f57b553195ac28518ae7c0b8ebb1","src/tests.rs":"f341d811eb648e8482dd2eb108e110850453e7e0deeccc09610fa234332f3921","src/tree.rs":"b6ba6275716dff398ff81dfcbbc47fa3af59555e452d92b5cb035b73b88f733d"},"package":"6d54506b6b209740d0a7a35ca5976db1ad2ed1aa168acc3561efc6a84fa95afe"}
+{"files":{"CODE_OF_CONDUCT.md":"e85149c44f478f164f7d5f55f6e66c9b5ae236d4a11107d5e2a93fe71dd874b9","Cargo.toml":"ef36c6d2e8475c91f1a28a4ae1de871f8311f1b0044e6ba20b7c21c0af11f0a1","LICENSE":"c71d239df91726fc519c6eb72d318ec65820627232b2f796219e87dcf35d0ab4","README.md":"303ea5ec53d4e86f2c321056e8158e31aa061353a99e52de3d76859d40919efc","src/driver.rs":"541d0d5a3f87ebafb4294bebc8a08b259b174b2c0607fa7edef570b0d7b52b7f","src/error.rs":"b78609cf0f0a87b2e6d01bcaf565f1ce8723f33f22f36e1847639557bcd49a2e","src/guid.rs":"6185985ca3e416c1bb9b1691b83789f718fd532fc011efd4a28c82f1edd23650","src/lib.rs":"0bdb83959fc75d9ec99108e0c4c0ced4b9a80c08e950ad2ac59d095e74b39f0f","src/merge.rs":"176353b45ce1079e20d705ca82a154a375eaf927e5a6075d1469d490ff8662d3","src/store.rs":"612d90ea0614aa7cc943c4ac0faaee35c155f57b553195ac28518ae7c0b8ebb1","src/tests.rs":"8a12b2d571ca4c59d645879b555c321c7a6fb6445956d41fcb37747ac06b54df","src/tree.rs":"194ccd6642d64347cf79dea3237e6d124aa4a75cad654360d65945617e749afc"},"package":"30ac4a8e8f834f02deb2266b1f279aa5494e990c625d8be8f2988a7c708ba1f8"}

third_party/rust/dogear/Cargo.toml

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

third_party/rust/dogear/src/driver.rs

@@ -65,7 +65,7 @@ impl Driver for DefaultDriver {}
 pub fn log<D: Driver>(
     driver: &D,
     level: Level,
-    args: Arguments,
+    args: Arguments<'_>,
     module_path: &'static str,
     file: &'static str,
     line: u32,

third_party/rust/dogear/src/error.rs

@@ -56,7 +56,7 @@ impl From<Utf8Error> for Error {
 }
 
 impl fmt::Display for Error {
-    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         match self.kind() {
             ErrorKind::MismatchedItemKind(local_kind, remote_kind) => write!(
                 f,

third_party/rust/dogear/src/guid.rs

@@ -147,12 +147,9 @@ impl<T: Copy + Into<usize>> IsValidGuid for [T] {
     #[inline]
     fn is_valid_guid(&self) -> bool {
         self.len() == 12
-            && self.iter().all(|&byte| {
-                VALID_GUID_BYTES
-                    .get(byte.into())
-                    .map(|&b| b == 1)
-                    .unwrap_or(false)
-            })
+            && self
+                .iter()
+                .all(|&byte| VALID_GUID_BYTES.get(byte.into()).map_or(false, |&b| b == 1))
     }
 }
 
@@ -252,13 +249,13 @@ impl Hash for Guid {
 
 // The default Debug impl is pretty unhelpful here.
 impl fmt::Debug for Guid {
-    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         write!(f, "Guid({:?})", self.as_str())
     }
 }
 
 impl fmt::Display for Guid {
-    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         f.write_str(self.as_str())
     }
 }

third_party/rust/dogear/src/lib.rs

@@ -12,6 +12,9 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
+#![allow(unknown_lints)]
+#![warn(rust_2018_idioms)]
+
 #[macro_use]
 mod driver;
 mod error;

third_party/rust/dogear/src/merge.rs

@@ -20,7 +20,7 @@ use std::{
 use crate::driver::{DefaultDriver, Driver};
 use crate::error::{ErrorKind, Result};
 use crate::guid::{Guid, IsValidGuid};
-use crate::tree::{Content, Kind, MergeState, MergedNode, MergedRoot, Node, Tree, Validity};
+use crate::tree::{Content, MergeState, MergedNode, MergedRoot, Node, Tree, Validity};
 
 /// Structure change types, used to indicate if a node on one side is moved
 /// or deleted on the other.
@@ -206,20 +206,19 @@ impl<'t, D: Driver> Merger<'t, D> {
 
     /// Returns an iterator for all accepted local and remote deletions.
     #[inline]
-    pub fn deletions(&self) -> impl Iterator<Item = Deletion> {
+    pub fn deletions(&self) -> impl Iterator<Item = Deletion<'_>> {
         self.local_deletions().chain(self.remote_deletions())
     }
 
-    pub(crate) fn local_deletions(&self) -> impl Iterator<Item = Deletion> {
+    pub(crate) fn local_deletions(&self) -> impl Iterator<Item = Deletion<'_>> {
         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(|node| node.level())
-                    .unwrap_or(-1);
+                    .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.
@@ -232,13 +231,12 @@ impl<'t, D: Driver> Merger<'t, D> {
         })
     }
 
-    pub(crate) fn remote_deletions(&self) -> impl Iterator<Item = Deletion> {
+    pub(crate) fn remote_deletions(&self) -> impl Iterator<Item = Deletion<'_>> {
         self.delete_remotely.iter().map(move |guid| {
             let local_level = self
                 .local_tree
                 .node_for_guid(guid)
-                .map(|node| node.level())
-                .unwrap_or(-1);
+                .map_or(-1, |node| node.level());
             Deletion {
                 guid,
                 local_level,
@@ -1006,7 +1004,7 @@ impl<'t, D: Driver> Merger<'t, D> {
         remote_parent_node: Node<'t>,
         remote_node: Node<'t>,
     ) -> Result<StructureChange> {
-        if !remote_node_is_syncable(&remote_node) {
+        if !remote_node.is_syncable() {
             // If the remote node is known to be non-syncable, we unconditionally
             // delete it, even if it's syncable or moved locally.
             return self.delete_remote_node(merged_node, remote_node);
@@ -1110,7 +1108,7 @@ impl<'t, D: Driver> Merger<'t, D> {
 
         if !self.remote_tree.is_deleted(&local_node.guid) {
             if let Some(remote_node) = self.remote_tree.node_for_guid(&local_node.guid) {
-                if !remote_node_is_syncable(&remote_node) {
+                if !remote_node.is_syncable() {
                     // The local node is syncable, but the remote node is not.
                     // This can happen if we applied an orphaned left pane
                     // query in a previous sync, and later saw the left pane
@@ -1541,16 +1539,3 @@ impl<'t, D: Driver> Merger<'t, D> {
         }
     }
 }
-
-/// Indicates if the tree in the remote node is syncable. This filters out
-/// livemarks (bug 1477671) and orphaned Places queries (bug 1433182).
-fn remote_node_is_syncable(remote_node: &Node) -> bool {
-    if !remote_node.is_syncable() {
-        return false;
-    }
-    match remote_node.kind {
-        Kind::Livemark => false,
-        Kind::Query if remote_node.diverged() => false,
-        _ => true,
-    }
-}

third_party/rust/dogear/src/tests.rs

@@ -1750,6 +1750,72 @@ fn left_pane_root() {
     assert_eq!(merger.counts(), &expected_telem);
 }
 
+#[test]
+fn livemarks() {
+    before_each();
+
+    let local_tree = nodes!({
+        ("menu________", Folder, {
+            ("livemarkAAAA", Livemark),
+            ("livemarkBBBB", Folder),
+            ("livemarkCCCC", Livemark)
+        }),
+        ("toolbar_____", Folder, {
+            ("livemarkDDDD", Livemark)
+        })
+    })
+    .into_tree()
+    .unwrap();
+
+    let remote_tree = nodes!({
+        ("menu________", Folder, {
+            ("livemarkAAAA", Livemark),
+            ("livemarkBBBB", Livemark),
+            ("livemarkCCCC", Folder)
+        }),
+        ("unfiled_____", Folder, {
+            ("livemarkEEEE", Livemark)
+        })
+    })
+    .into_tree()
+    .unwrap();
+
+    let mut merger = Merger::new(&local_tree, &remote_tree);
+    let merged_root = merger.merge().unwrap();
+    assert!(merger.subsumes(&local_tree));
+    assert!(merger.subsumes(&remote_tree));
+
+    let expected_tree = nodes!({
+        ("menu________", Folder[needs_merge = true]),
+        ("toolbar_____", Folder[needs_merge = true]),
+        ("unfiled_____", Folder[needs_merge = true])
+    })
+    .into_tree()
+    .unwrap();
+    let expected_deletions = vec![
+        "livemarkAAAA",
+        "livemarkBBBB",
+        "livemarkCCCC",
+        "livemarkDDDD",
+        "livemarkEEEE",
+    ];
+    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()
+    };
+
+    let merged_tree = merged_root.into_tree().unwrap();
+    assert_eq!(merged_tree, expected_tree);
+
+    let mut deletions = merger.deletions().map(|d| d.guid).collect::<Vec<_>>();
+    deletions.sort();
+    assert_eq!(deletions, expected_deletions);
+
+    assert_eq!(merger.counts(), &expected_telem);
+}
+
 #[test]
 fn non_syncable_items() {
     before_each();
@@ -2567,7 +2633,7 @@ fn cycle() {
         .kind()
     {
         ErrorKind::Cycle(guid) => assert_eq!(guid, &Guid::from("folderAAAAAA")),
-        err => assert!(false, "Wrong error kind for cycle: {:?}", err),
+        err => panic!("Wrong error kind for cycle: {:?}", err),
     }
 }