Bug 1455164 - Ignore moved user content roots in the bookmarks mirror. r=markh
authorKit Cambridge <kit@yakshaving.ninja>
Tue, 17 Apr 2018 15:41:58 -0700
changeset 468040 d42b55d603032606bc40ae6b24a0aa27e6d30c72
parent 468039 72c16dae1e7f849b431801db533811c41f0220bd
child 468041 2fefab7d7dc192cdaf66d88a90b76e763b762465
push id9165
push userasasaki@mozilla.com
push dateThu, 26 Apr 2018 21:04:54 +0000
treeherdermozilla-beta@064c3804de2e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmarkh
bugs1455164
milestone61.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1455164 - Ignore moved user content roots in the bookmarks mirror. r=markh MozReview-Commit-ID: 7M9gErmjb4J
toolkit/components/places/SyncedBookmarksMirror.jsm
toolkit/components/places/tests/sync/test_bookmark_corruption.js
--- a/toolkit/components/places/SyncedBookmarksMirror.jsm
+++ b/toolkit/components/places/SyncedBookmarksMirror.jsm
@@ -317,17 +317,17 @@ class SyncedBookmarksMirror {
    *        records. Tests can also pass `false` to set up an existing mirror.
    */
   async store(records, { needsMerge = true } = {}) {
     let options = { needsMerge };
     let ignoreCounts = {
       bookmark: { id: 0, url: 0 },
       query: { id: 0, url: 0 },
       folder: { id: 0, root: 0 },
-      child: { id: 0, },
+      child: { id: 0, root: 0 },
       livemark: { id: 0, feed: 0 },
       separator: { id: 0 },
       tombstone: { id: 0, root: 0 },
     };
     let extraTelemetryEvents = [];
     try {
       await this.db.executeBeforeShutdown(
         "SyncedBookmarksMirror: store",
@@ -775,29 +775,37 @@ class SyncedBookmarksMirror {
         let childGuid = validateGuid(childRecordId);
         if (!childGuid) {
           MirrorLog.warn("Ignoring child of folder ${parentGuid} with " +
                          "invalid ID ${childRecordId}", { parentGuid: guid,
                                                           childRecordId });
           ignoreCounts.child.id++;
           continue;
         }
+        if (childGuid == PlacesUtils.bookmarks.rootGuid ||
+            PlacesUtils.bookmarks.userContentRoots.includes(childGuid)) {
+          MirrorLog.warn("Ignoring move for root", childGuid);
+          ignoreCounts.child.root++;
+          continue;
+        }
         await this.db.executeCached(`
           REPLACE INTO structure(guid, parentGuid, position)
           VALUES(:childGuid, :parentGuid, :position)`,
           { childGuid, parentGuid: guid, position });
       }
     }
     // parentChild relationships are usually determined via the children list
     // in the parent. However, this doesn't work for an item that is directly
     // under the root - such items are invalid, but we still want to store
     // them so we can ignore that entire sub-tree - so such items need special
-    // treatment.
+    // treatment. We ignore the four syncable roots, since they're already in
+    // the mirror.
     let parentGuid = validateGuid(record.parentid);
-    if (parentGuid == PlacesUtils.bookmarks.rootGuid) {
+    if (parentGuid == PlacesUtils.bookmarks.rootGuid &&
+        !PlacesUtils.bookmarks.userContentRoots.includes(guid)) {
         await this.db.executeCached(`
           INSERT OR IGNORE INTO structure(guid, parentGuid, position)
           VALUES(:guid, :parentGuid, -1)`,
           { guid, parentGuid });
       }
   }
 
   async storeRemoteLivemark(record, ignoreCounts, { needsMerge }) {
@@ -856,18 +864,19 @@ class SyncedBookmarksMirror {
   async storeRemoteTombstone(record, ignoreCounts, { needsMerge }) {
     let guid = validateGuid(record.id);
     if (!guid) {
       MirrorLog.warn("Ignoring tombstone with invalid ID", record.id);
       ignoreCounts.tombstone.id++;
       return;
     }
 
-    if (PlacesUtils.bookmarks.userContentRoots.includes(guid)) {
-      MirrorLog.warn("Ignoring tombstone for syncable root", guid);
+    if (guid == PlacesUtils.bookmarks.rootGuid ||
+        PlacesUtils.bookmarks.userContentRoots.includes(guid)) {
+      MirrorLog.warn("Ignoring tombstone for root", guid);
       ignoreCounts.tombstone.root++;
       return;
     }
 
     await this.db.executeCached(`
       REPLACE INTO items(guid, serverModified, needsMerge, isDeleted)
       VALUES(:guid, :serverModified, :needsMerge, 1)`,
       { guid, serverModified: determineServerModified(record), needsMerge });
--- a/toolkit/components/places/tests/sync/test_bookmark_corruption.js
+++ b/toolkit/components/places/tests/sync/test_bookmark_corruption.js
@@ -2,16 +2,118 @@
  * http://creativecommons.org/publicdomain/zero/1.0/ */
 
 async function getCountOfBookmarkRows(db) {
   let queryRows = await db.execute("SELECT COUNT(*) FROM moz_bookmarks");
   Assert.equal(queryRows.length, 1);
   return queryRows[0].getResultByIndex(0);
 }
 
+add_task(async function test_corrupt_roots() {
+  let telemetryEvents = [];
+  let buf = await openMirror("corrupt_roots", {
+    recordTelemetryEvent(object, method, value, extra) {
+      if (object == "mirror" && ["open", "apply"].includes(method)) {
+        // Ignore timings, mirror database file, and tree sizes.
+        return;
+      }
+      telemetryEvents.push({ object, method, value, extra });
+    },
+  });
+
+  info("Set up empty mirror");
+  await PlacesTestUtils.markBookmarksAsSynced();
+
+  info("Make remote changes: Menu > Unfiled");
+  await storeRecords(buf, [{
+    id: "menu",
+    type: "folder",
+    children: ["unfiled", "bookmarkAAAA"],
+  }, {
+    id: "unfiled",
+    type: "folder",
+    children: ["bookmarkBBBB"],
+  }, {
+    id: "bookmarkAAAA",
+    type: "bookmark",
+    title: "A",
+    bmkUri: "http://example.com/a",
+  }, {
+    id: "bookmarkBBBB",
+    type: "bookmark",
+    title: "B",
+    bmkUri: "http://example.com/b",
+  }, {
+    id: "toolbar",
+    deleted: true,
+  }]);
+
+  let changesToUpload = await buf.apply();
+  deepEqual(await buf.fetchUnmergedGuids(), [], "Should merge all items");
+  deepEqual(telemetryEvents, [{
+    object: "mirror",
+    method: "ignore",
+    value: "child",
+    extra: { root: "1" },
+  }, {
+    object: "mirror",
+    method: "ignore",
+    value: "tombstone",
+    extra: { root: "1" },
+  }], "Should record telemetry for ignored invalid roots");
+
+  deepEqual(changesToUpload, {}, "Should not reupload invalid roots");
+
+  await assertLocalTree(PlacesUtils.bookmarks.rootGuid, {
+    guid: PlacesUtils.bookmarks.rootGuid,
+    type: PlacesUtils.bookmarks.TYPE_FOLDER,
+    index: 0,
+    title: "",
+    children: [{
+      guid: PlacesUtils.bookmarks.menuGuid,
+      type: PlacesUtils.bookmarks.TYPE_FOLDER,
+      index: 0,
+      title: BookmarksMenuTitle,
+      children: [{
+        guid: "bookmarkAAAA",
+        type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
+        index: 0,
+        title: "A",
+        url: "http://example.com/a",
+      }],
+    }, {
+      guid: PlacesUtils.bookmarks.toolbarGuid,
+      type: PlacesUtils.bookmarks.TYPE_FOLDER,
+      index: 1,
+      title: BookmarksToolbarTitle,
+    }, {
+      guid: PlacesUtils.bookmarks.unfiledGuid,
+      type: PlacesUtils.bookmarks.TYPE_FOLDER,
+      index: 3,
+      title: UnfiledBookmarksTitle,
+      children: [{
+        guid: "bookmarkBBBB",
+        type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
+        index: 0,
+        title: "B",
+        url: "http://example.com/b",
+      }],
+    }, {
+      guid: PlacesUtils.bookmarks.mobileGuid,
+      type: PlacesUtils.bookmarks.TYPE_FOLDER,
+      index: 4,
+      title: MobileBookmarksTitle,
+    }],
+  }, "Should not corrupt local roots");
+
+  await buf.finalize();
+  await PlacesUtils.bookmarks.eraseEverything();
+  await PlacesSyncUtils.bookmarks.reset();
+});
+
 add_task(async function test_missing_children() {
   let buf = await openMirror("missing_childen");
 
   info("Set up empty mirror");
   await PlacesTestUtils.markBookmarksAsSynced();
 
   info("Make remote changes: A > ([B] C [D E])");
   {