Bug 1472835 - Exclude the root from the `parentsWithGaps` check in the bookmarks mirror. r=markh, a=lizzard
authorLina Cambridge <lina@yakshaving.ninja>
Mon, 02 Jul 2018 23:17:12 +0000
changeset 477809 6ce7f5a3eaf3e8935f8d6b9c60b3a3fd8558828b
parent 477808 a1992e3234f46097648c27175d2ac8b540f561df
child 477810 505d8765ce63f12bbeca8e7be96e52033a137687
push id9432
push userryanvm@gmail.com
push dateWed, 04 Jul 2018 00:21:37 +0000
treeherdermozilla-beta@505d8765ce63 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmarkh, lizzard
bugs1472835
milestone62.0
Bug 1472835 - Exclude the root from the `parentsWithGaps` check in the bookmarks mirror. r=markh, a=lizzard Differential Revision: https://phabricator.services.mozilla.com/D1918
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
@@ -927,19 +927,21 @@ class SyncedBookmarksMirror {
              0 AS parentsWithGaps
       FROM structure s
       LEFT JOIN items v ON v.guid = s.guid
       WHERE v.guid IS NULL
       UNION ALL
       SELECT s.parentGuid AS guid, 0 AS missingParent, 0 AS missingChild,
              1 AS parentWithGaps
       FROM structure s
+      WHERE s.guid <> :rootGuid
       GROUP BY s.parentGuid
       HAVING (sum(DISTINCT position + 1) -
-                  (count(*) * (count(*) + 1) / 2)) <> 0`);
+                  (count(*) * (count(*) + 1) / 2)) <> 0`,
+      { rootGuid: PlacesUtils.bookmarks.rootGuid });
 
     for await (let row of yieldingIterator(orphanRows)) {
       let guid = row.getResultByName("guid");
       let missingParent = row.getResultByName("missingParent");
       if (missingParent) {
         infos.missingParents.push(guid);
       }
       let missingChild = row.getResultByName("missingChild");
--- a/toolkit/components/places/tests/sync/test_bookmark_corruption.js
+++ b/toolkit/components/places/tests/sync/test_bookmark_corruption.js
@@ -124,19 +124,21 @@ add_task(async function test_missing_chi
       children: [{
         guid: "bookmarkCCCC",
         type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
         index: 0,
         title: "C",
         url: "http://example.com/c",
       }],
     }, "Menu children should be (C)");
-    let { missingChildren } = await buf.fetchRemoteOrphans();
-    deepEqual(missingChildren.sort(), ["bookmarkBBBB", "bookmarkDDDD",
-      "bookmarkEEEE"], "Should report (B D E) as missing");
+    deepEqual(await buf.fetchRemoteOrphans(), {
+      missingChildren: ["bookmarkBBBB", "bookmarkDDDD", "bookmarkEEEE"],
+      missingParents: [],
+      parentsWithGaps: [],
+    }, "Should report (B D E) as missing");
   }
 
   info("Add (B E) to remote");
   {
     await storeRecords(buf, shuffle([{
       id: "bookmarkBBBB",
       type: "bookmark",
       title: "B",
@@ -175,19 +177,21 @@ add_task(async function test_missing_chi
       }, {
         guid: "bookmarkEEEE",
         type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
         index: 2,
         title: "E",
         url: "http://example.com/e",
       }],
     }, "Menu children should be (B C E)");
-    let { missingChildren } = await buf.fetchRemoteOrphans();
-    deepEqual(missingChildren, ["bookmarkDDDD"],
-      "Should report (D) as missing");
+    deepEqual(await buf.fetchRemoteOrphans(), {
+      missingChildren: ["bookmarkDDDD"],
+      missingParents: [],
+      parentsWithGaps: [],
+    }, "Should report (D) as missing");
   }
 
   info("Add D to remote");
   {
     await storeRecords(buf, [{
       id: "bookmarkDDDD",
       type: "bookmark",
       title: "D",
@@ -227,18 +231,21 @@ add_task(async function test_missing_chi
       }, {
         guid: "bookmarkEEEE",
         type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
         index: 3,
         title: "E",
         url: "http://example.com/e",
       }],
     }, "Menu children should be (B C D E)");
-    let { missingChildren } = await buf.fetchRemoteOrphans();
-    deepEqual(missingChildren, [], "Should not report any missing children");
+    deepEqual(await buf.fetchRemoteOrphans(), {
+      missingChildren: [],
+      missingParents: [],
+      parentsWithGaps: [],
+    }, "Should not report any missing children");
   }
 
   await buf.finalize();
   await PlacesUtils.bookmarks.eraseEverything();
   await PlacesSyncUtils.bookmarks.reset();
 });
 
 add_task(async function test_new_orphan_without_local_parent() {
@@ -1590,8 +1597,53 @@ add_task(async function test_complete_cy
       title: MobileBookmarksTitle,
     }],
   }, "Should not be confused into creating a cycle");
 
   await buf.finalize();
   await PlacesUtils.bookmarks.eraseEverything();
   await PlacesSyncUtils.bookmarks.reset();
 });
+
+add_task(async function test_invalid_guid() {
+  let buf = await openMirror("invalid_guid");
+
+  info("Set up empty mirror");
+  await PlacesTestUtils.markBookmarksAsSynced();
+
+  info("Make remote changes");
+  await storeRecords(buf, [{
+    id: "menu",
+    type: "folder",
+    children: ["bookmarkAAAA", "bad!guid~", "bookmarkBBBB"],
+  }, {
+    id: "bookmarkAAAA",
+    type: "bookmark",
+    title: "A",
+    bmkUri: "http://example.com/a",
+  }, {
+    // Should be ignored.
+    id: "bad!guid~",
+    type: "bookmark",
+    title: "Bad GUID",
+    bmkUri: "http://example.com/bad-guid",
+  }, {
+    id: "bookmarkBBBB",
+    type: "bookmark",
+    title: "B",
+    bmkUri: "http://example.com/b",
+  }]);
+
+  let changesToUpload = await buf.apply();
+  deepEqual(await buf.fetchUnmergedGuids(), [], "Should merge all items");
+
+  deepEqual(changesToUpload, {}, "Should not reupload menu with gaps");
+
+  deepEqual(await buf.fetchRemoteOrphans(), {
+    missingChildren: [],
+    missingParents: [],
+    parentsWithGaps: [PlacesUtils.bookmarks.menuGuid],
+  }, "Should report gaps in menu");
+
+  await buf.finalize();
+  await PlacesUtils.bookmarks.eraseEverything();
+  await PlacesSyncUtils.bookmarks.reset();
+});