Bug 1546514 - Remove bookmarks mirror checks for orphans and Sync status mismatches. r=markh
authorLina Cambridge <lina@yakshaving.ninja>
Thu, 16 May 2019 07:31:56 +0000
changeset 536236 b3788c8a7cac8b757c02278f7cbea028819964ca
parent 536235 b84060d7fa8d43fe978058f976308194195d6a2d
child 536237 d51a13b4f0ef895da2f5ba19f3918904c42ae96f
push id2082
push userffxbld-merge
push dateMon, 01 Jul 2019 08:34:18 +0000
treeherdermozilla-release@2fb19d0466d2 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmarkh
bugs1546514
milestone68.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 1546514 - Remove bookmarks mirror checks for orphans and Sync status mismatches. r=markh Differential Revision: https://phabricator.services.mozilla.com/D30121
toolkit/components/places/SyncedBookmarksMirror.jsm
toolkit/components/places/tests/sync/test_bookmark_corruption.js
toolkit/components/places/tests/sync/test_bookmark_deduping.js
toolkit/components/places/tests/sync/test_bookmark_validation.js
toolkit/components/places/tests/sync/xpcshell.ini
--- a/toolkit/components/places/SyncedBookmarksMirror.jsm
+++ b/toolkit/components/places/SyncedBookmarksMirror.jsm
@@ -524,55 +524,16 @@ class SyncedBookmarksMirror {
       throw ex;
     }
 
     return changeRecords;
   }
 
   async tryApply(flowID, localTimeSeconds, remoteTimeSeconds, observersToNotify,
                  weakUpload) {
-    let { missingParents, missingChildren, parentsWithGaps } =
-      await this.fetchRemoteOrphans();
-    if (missingParents.length) {
-      MirrorLog.warn("Temporarily reparenting remote items with missing " +
-                     "parents to unfiled", missingParents);
-    }
-    if (missingChildren.length) {
-      MirrorLog.warn("Remote tree missing items", missingChildren);
-    }
-    if (parentsWithGaps.length) {
-      MirrorLog.warn("Remote tree has parents with gaps in positions",
-                     parentsWithGaps);
-    }
-
-    let { missingLocal, missingRemote, wrongSyncStatus } =
-      await this.fetchSyncStatusMismatches();
-    if (missingLocal.length) {
-      MirrorLog.warn("Remote tree has merged items that don't exist locally",
-                     missingLocal);
-    }
-    if (missingRemote.length) {
-      MirrorLog.warn("Local tree has synced items that don't exist remotely",
-                     missingRemote);
-    }
-    if (wrongSyncStatus.length) {
-      MirrorLog.warn("Local tree has wrong sync statuses for items that " +
-                     "exist remotely", wrongSyncStatus);
-    }
-
-    this.recordTelemetryEvent("mirror", "apply", "problems", {
-      flowID,
-      missingParents: missingParents.length,
-      missingChildren: missingChildren.length,
-      parentsWithGaps: parentsWithGaps.length,
-      missingLocal: missingLocal.length,
-      missingRemote: missingRemote.length,
-      wrongSyncStatus: wrongSyncStatus.length,
-    });
-
     let result = await withTiming(
       "Merging bookmarks in Rust",
       () => {
         return new Promise((resolve, reject) => {
           let callback = {
             handleResult: resolve,
             handleError(code, message) {
               if (code == Cr.NS_ERROR_STORAGE_BUSY) {
@@ -883,143 +844,16 @@ class SyncedBookmarksMirror {
       INSERT OR IGNORE INTO urls(guid, url, hash, revHost)
       VALUES(IFNULL((SELECT guid FROM urls
                      WHERE hash = hash(:url) AND
                                   url = :url),
                     GENERATE_GUID()), :url, hash(:url), :revHost)`,
       { url: url.href, revHost: PlacesUtils.getReversedHost(url) });
   }
 
-  async fetchRemoteOrphans() {
-    let infos = {
-      missingParents: [],
-      missingChildren: [],
-      parentsWithGaps: [],
-    };
-
-    let orphanRows = await this.db.execute(`
-      SELECT v.guid AS guid, 1 AS missingParent, 0 AS missingChild,
-             0 AS parentWithGaps
-      FROM items v
-      LEFT JOIN structure s ON s.guid = v.guid
-      WHERE NOT v.isDeleted AND
-            s.guid IS NULL
-      UNION ALL
-      SELECT s.guid AS guid, 0 AS missingParent, 1 AS missingChild,
-             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
-      ORDER BY guid`,
-      { rootGuid: PlacesUtils.bookmarks.rootGuid });
-
-    await Async.yieldingForEach(orphanRows, row => {
-      let guid = row.getResultByName("guid");
-      let missingParent = row.getResultByName("missingParent");
-      if (missingParent) {
-        infos.missingParents.push(guid);
-      }
-      let missingChild = row.getResultByName("missingChild");
-      if (missingChild) {
-        infos.missingChildren.push(guid);
-      }
-      let parentWithGaps = row.getResultByName("parentWithGaps");
-      if (parentWithGaps) {
-        infos.parentsWithGaps.push(guid);
-      }
-    }, yieldState);
-
-    return infos;
-  }
-
-  /**
-   * Checks the sync statuses of all items for consistency. All merged items in
-   * the remote tree should exist as either items or tombstones in the local
-   * tree, and all NORMAL items and tombstones in the local tree should exist
-   * in the remote tree, if the mirror has any merged items.
-   *
-   * @return {Object.<String, String[]>}
-   *         An object containing GUIDs for each problem type:
-   *           - `missingLocal`: Merged items in the remote tree that aren't
-   *             mentioned in the local tree.
-   *           - `missingRemote`: NORMAL items in the local tree that aren't
-   *             mentioned in the remote tree.
-   */
-  async fetchSyncStatusMismatches() {
-    let infos = {
-      missingLocal: [],
-      missingRemote: [],
-      wrongSyncStatus: [],
-    };
-
-    let problemRows = await this.db.execute(`
-      SELECT v.guid, 1 AS missingLocal, 0 AS missingRemote, 0 AS wrongSyncStatus
-      FROM items v
-      LEFT JOIN moz_bookmarks b ON b.guid = v.guid
-      LEFT JOIN moz_bookmarks_deleted d ON d.guid = v.guid
-      WHERE NOT v.needsMerge AND
-            NOT v.isDeleted AND
-            b.guid IS NULL AND
-            d.guid IS NULL
-      UNION ALL
-      SELECT b.guid, 0 AS missingLocal, 1 AS missingRemote, 0 AS wrongSyncStatus
-      FROM moz_bookmarks b
-      LEFT JOIN items v ON v.guid = b.guid
-      WHERE EXISTS(SELECT 1 FROM items
-                   WHERE NOT needsMerge AND
-                         guid <> :rootGuid) AND
-            b.syncStatus = :syncStatus AND
-            v.guid IS NULL
-      UNION ALL
-      SELECT d.guid, 0 AS missingLocal, 1 AS missingRemote, 0 AS wrongSyncStatus
-      FROM moz_bookmarks_deleted d
-      LEFT JOIN items v ON v.guid = d.guid
-      WHERE EXISTS(SELECT 1 FROM items
-                   WHERE NOT needsMerge AND
-                         guid <> :rootGuid) AND
-            v.guid IS NULL
-      UNION ALL
-      SELECT b.guid, 0 AS missingLocal, 0 AS missingRemote, 1 AS wrongSyncStatus
-      FROM moz_bookmarks b
-      JOIN items v ON v.guid = b.guid
-      WHERE EXISTS(SELECT 1 FROM items
-                   WHERE NOT needsMerge AND
-                         guid <> :rootGuid) AND
-            b.guid <> :rootGuid AND
-            b.syncStatus <> :syncStatus`,
-      { syncStatus: PlacesUtils.bookmarks.SYNC_STATUS.NORMAL,
-        rootGuid: PlacesUtils.bookmarks.rootGuid });
-
-    await Async.yieldingForEach(problemRows, row => {
-      let guid = row.getResultByName("guid");
-      let missingLocal = row.getResultByName("missingLocal");
-      if (missingLocal) {
-        infos.missingLocal.push(guid);
-      }
-      let missingRemote = row.getResultByName("missingRemote");
-      if (missingRemote) {
-        infos.missingRemote.push(guid);
-      }
-      let wrongSyncStatus = row.getResultByName("wrongSyncStatus");
-      if (wrongSyncStatus) {
-        infos.wrongSyncStatus.push(guid);
-      }
-    }, yieldState);
-
-    return infos;
-  }
-
   /*
    * Checks if Places or mirror have any unsynced/unmerged changes.
    *
    * @return {Boolean}
    *         `true` if something has changed.
    */
   async hasChanges() {
     // In the first subquery, we check incoming items with needsMerge = true
--- a/toolkit/components/places/tests/sync/test_bookmark_corruption.js
+++ b/toolkit/components/places/tests/sync/test_bookmark_corruption.js
@@ -684,21 +684,16 @@ 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)");
-    deepEqual(await buf.fetchRemoteOrphans(), {
-      missingChildren: ["bookmarkBBBB", "bookmarkDDDD", "bookmarkEEEE"],
-      missingParents: [],
-      parentsWithGaps: [],
-    }, "Should report (B D E) as missing");
     await storeChangesInMirror(buf, changesToUpload);
   }
 
   info("Add (B E) to remote");
   {
     await storeRecords(buf, shuffle([{
       id: "bookmarkBBBB",
       parentid: "menu",
@@ -740,21 +735,16 @@ 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 (C B E)");
-    deepEqual(await buf.fetchRemoteOrphans(), {
-      missingChildren: [],
-      missingParents: ["bookmarkBBBB", "bookmarkEEEE"],
-      parentsWithGaps: [],
-    }, "Should report missing parents for (B E)");
     await storeChangesInMirror(buf, changesToUpload);
   }
 
   info("Add D to remote");
   {
     await storeRecords(buf, [{
       id: "bookmarkDDDD",
       parentid: "menu",
@@ -796,21 +786,16 @@ add_task(async function test_missing_chi
       }, {
         guid: "bookmarkDDDD",
         type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
         index: 3,
         title: "D",
         url: "http://example.com/d",
       }],
     }, "Menu children should be (C B E D)");
-    deepEqual(await buf.fetchRemoteOrphans(), {
-      missingChildren: [],
-      missingParents: ["bookmarkDDDD"],
-      parentsWithGaps: [],
-    }, "Should report missing parent for D");
     await storeChangesInMirror(buf, changesToUpload);
   }
 
   await buf.finalize();
   await PlacesUtils.bookmarks.eraseEverything();
   await PlacesSyncUtils.bookmarks.reset();
 });
 
@@ -2214,22 +2199,16 @@ add_task(async function test_invalid_gui
       hasDupe: true,
       parentName: "",
       dateAdded: datesAdded.get(PlacesUtils.bookmarks.menuGuid),
       title: BookmarksMenuTitle,
       children: ["bookmarkAAAA", newGuid, "bookmarkBBBB"],
     },
   }, "Should reupload menu with new child GUID for C");
 
-  deepEqual(await buf.fetchRemoteOrphans(), {
-    missingChildren: [],
-    missingParents: [],
-    parentsWithGaps: [],
-  }, "Should not report problems");
-
   await assertLocalTree(PlacesUtils.bookmarks.menuGuid, {
     guid: PlacesUtils.bookmarks.menuGuid,
     type: PlacesUtils.bookmarks.TYPE_FOLDER,
     index: 0,
     title: BookmarksMenuTitle,
     children: [{
       guid: "bookmarkAAAA",
       type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
@@ -2273,22 +2252,16 @@ add_task(async function test_sync_status
   let initialChangesToUpload = await buf.apply();
 
   deepEqual(Object.keys(initialChangesToUpload).sort(),
     ["menu", "mobile", "toolbar", "unfiled"],
     "Should upload roots on first merge");
 
   await storeChangesInMirror(buf, initialChangesToUpload);
 
-  deepEqual(await buf.fetchSyncStatusMismatches(), {
-    missingLocal: [],
-    missingRemote: [],
-    wrongSyncStatus: [],
-  }, "Should not report mismatches after first merge");
-
   info("Make local changes");
   await PlacesUtils.bookmarks.insertTree({
     guid: PlacesUtils.bookmarks.menuGuid,
     source: PlacesUtils.bookmarks.SOURCES.SYNC,
     children: [{
       // A is NORMAL in Places, but doesn't exist in the mirror.
       guid: "bookmarkAAAA",
       url: "http://example.com/a",
@@ -2328,22 +2301,16 @@ add_task(async function test_sync_status
     // C is flagged as merged in the mirror, but doesn't exist in Places.
     id: "bookmarkCCCC",
     parentid: "toolbar",
     type: "bookmark",
     bmkUri: "http://example.com/c",
     title: "C",
   }], { needsMerge: false });
 
-  deepEqual(await buf.fetchSyncStatusMismatches(), {
-    missingLocal: ["bookmarkCCCC"],
-    missingRemote: ["bookmarkAAAA"],
-    wrongSyncStatus: ["bookmarkBBBB"],
-  }, "Should report sync status mismatches");
-
   info("Apply mirror");
   let changesToUpload = await buf.apply();
   deepEqual(await buf.fetchUnmergedGuids(), [], "Should merge all items");
 
   let datesAdded = await promiseManyDatesAdded([PlacesUtils.bookmarks.menuGuid,
     PlacesUtils.bookmarks.unfiledGuid]);
   deepEqual(changesToUpload, {
     bookmarkAAAA: {
@@ -2454,18 +2421,12 @@ add_task(async function test_sync_status
       type: PlacesUtils.bookmarks.TYPE_FOLDER,
       index: 4,
       title: MobileBookmarksTitle,
     }],
   }, "Should parent C correctly");
 
   await storeChangesInMirror(buf, changesToUpload);
 
-  deepEqual(await buf.fetchSyncStatusMismatches(), {
-    missingLocal: [],
-    missingRemote: [],
-    wrongSyncStatus: [],
-  }, "Applying and storing new changes in mirror should fix inconsistencies");
-
   await buf.finalize();
   await PlacesUtils.bookmarks.eraseEverything();
   await PlacesSyncUtils.bookmarks.reset();
 });
--- a/toolkit/components/places/tests/sync/test_bookmark_deduping.js
+++ b/toolkit/components/places/tests/sync/test_bookmark_deduping.js
@@ -23,24 +23,16 @@ add_task(async function test_duping_loca
     id: "bookmarkAAA5",
     parentid: "menu",
     type: "bookmark",
     bmkUri: "http://example.com/a",
     title: "A",
   }], { needsMerge: false });
   await PlacesTestUtils.markBookmarksAsSynced();
 
-  // The mirror is out of sync because `bookmarkAAA5` is marked as merged,
-  // even though it's not in Places, but we should still recover.
-  deepEqual(await buf.fetchSyncStatusMismatches(), {
-    missingLocal: ["bookmarkAAA5"],
-    missingRemote: [],
-    wrongSyncStatus: [],
-  }, "Mirror should be out of sync with Places before deduping");
-
   info("Add newer local dupes");
   await PlacesUtils.bookmarks.insertTree({
     guid: PlacesUtils.bookmarks.menuGuid,
     children: [{
       guid: "bookmarkAAA1",
       title: "A",
       url: "http://example.com/a",
       dateAdded: localModified,
deleted file mode 100644
--- a/toolkit/components/places/tests/sync/test_bookmark_validation.js
+++ /dev/null
@@ -1,126 +0,0 @@
-/* Any copyright is dedicated to the Public Domain.
- * http://creativecommons.org/publicdomain/zero/1.0/ */
-
-add_task(async function test_inconsistencies() {
-  let buf = await openMirror("inconsistencies");
-
-  info("Set up local tree");
-  await PlacesUtils.bookmarks.insertTree({
-    guid: PlacesUtils.bookmarks.menuGuid,
-    source: PlacesUtils.bookmarks.SOURCE_SYNC,
-    children: [{
-      // NORMAL bookmark that exists in the mirror; not an inconsistency.
-      guid: "bookmarkAAAA",
-      title: "A",
-      url: "http://example.com/a",
-    }, {
-      // NORMAL bookmark that doesn't exist in the mirror at all.
-      guid: "bookmarkBBBB",
-      title: "B",
-      url: "http://example.com/b",
-    }, {
-      // NORMAL bookmark with a tombstone that doesn't exist in the mirror.
-      guid: "bookmarkCCCC",
-      title: "C",
-      url: "http://example.com/c",
-    }, {
-      // NORMAL bookmark with a tombstone that exists in the mirror; not an
-      // inconsistency.
-      guid: "bookmarkDDDD",
-      title: "D",
-      url: "http://example.com/d",
-    }],
-  });
-  await PlacesUtils.bookmarks.insertTree({
-    guid: PlacesUtils.bookmarks.toolbarGuid,
-    children: [{
-      // NEW bookmark that doesn't exist in the mirror; not an inconsistency.
-      guid: "bookmarkEEEE",
-      title: "E",
-      url: "http://example.com/e",
-    }, {
-      // NEW bookmark that exists in the mirror.
-      guid: "bookmarkFFFF",
-      title: "F",
-      url: "http://example.com/f",
-    }],
-  });
-  await PlacesUtils.bookmarks.remove("bookmarkCCCC");
-  await PlacesUtils.bookmarks.remove("bookmarkDDDD");
-
-  deepEqual(await buf.fetchSyncStatusMismatches(), {
-    missingLocal: [],
-    missingRemote: [],
-    wrongSyncStatus: [],
-  }, "Should not report inconsistencies with empty mirror");
-
-  info("Set up mirror");
-  await storeRecords(buf, [{
-    id: "menu",
-    parentid: "places",
-    type: "folder",
-    children: ["bookmarkAAAA", "bookmarkDDDD", "bookmarkGGGG"],
-  }, {
-    id: "toolbar",
-    parentid: "places",
-    type: "folder",
-    children: ["bookmarkFFFF"],
-  }, {
-    id: "bookmarkAAAA",
-    parentid: "menu",
-    type: "bookmark",
-    title: "A",
-    bmkUri: "http://example.com/a",
-  }, {
-    id: "bookmarkDDDD",
-    parentid: "menu",
-    type: "bookmark",
-    title: "D",
-    bmkUri: "http://example.com/d",
-  }, {
-    id: "bookmarkFFFF",
-    parentid: "toolbar",
-    type: "bookmark",
-    title: "F",
-    bmkUri: "http://example.com/f",
-  }, {
-    // Merged bookmark that doesn't exist locally.
-    id: "bookmarkGGGG",
-    parentid: "menu",
-    type: "bookmark",
-    title: "G",
-    bmkUri: "http://example.com/g",
-  }], { needsMerge: false });
-
-  await storeRecords(buf, [{
-    id: "menu",
-    parentid: "places",
-    type: "folder",
-    children: ["bookmarkAAAA", "bookmarkDDDD", "bookmarkGGGG", "bookmarkHHHH"],
-  }, {
-    // New bookmark that doesn't exist locally; not an inconsistency.
-    id: "bookmarkHHHH",
-    parentid: "unfiled",
-    type: "bookmark",
-    title: "H",
-    bmkUri: "http://example.com/h",
-  }, {
-    id: "bookmarkIIII",
-    deleted: true,
-  }]);
-
-  let { missingLocal, missingRemote, wrongSyncStatus } =
-    await buf.fetchSyncStatusMismatches();
-  deepEqual(missingLocal, ["bookmarkGGGG"],
-    "Should report merged remote items that don't exist locally");
-  deepEqual(missingRemote.sort(), ["bookmarkBBBB", "bookmarkCCCC"],
-    "Should report NORMAL local items that don't exist remotely");
-  deepEqual(wrongSyncStatus.sort(), [PlacesUtils.bookmarks.menuGuid,
-    PlacesUtils.bookmarks.toolbarGuid, PlacesUtils.bookmarks.unfiledGuid,
-    PlacesUtils.bookmarks.mobileGuid, "bookmarkFFFF"].sort(),
-    "Should report remote items with wrong local sync status");
-
-  await buf.finalize();
-  await PlacesUtils.bookmarks.eraseEverything();
-  await PlacesSyncUtils.bookmarks.reset();
-});
--- a/toolkit/components/places/tests/sync/xpcshell.ini
+++ b/toolkit/components/places/tests/sync/xpcshell.ini
@@ -14,11 +14,10 @@ support-files =
 [test_bookmark_explicit_weakupload.js]
 [test_bookmark_haschanges.js]
 [test_bookmark_kinds.js]
 [test_bookmark_merge_conflicts.js]
 [test_bookmark_mirror_meta.js]
 [test_bookmark_mirror_migration.js]
 [test_bookmark_observer_recorder.js]
 [test_bookmark_structure_changes.js]
-[test_bookmark_validation.js]
 [test_bookmark_value_changes.js]
 [test_sync_utils.js]