Bug 1433182 - bookmark mirror now handles left-pane-roots on the server. r?kitcambridge draft
authorMark Hammond <mhammond@skippinet.com.au>
Fri, 16 Mar 2018 14:39:17 +1100
changeset 770930 1b213d43bdb59fa51dd786e4843f11d9c0e6a840
parent 770928 b41c7c1ff91f49b1500b087ff23c288dd88f1fde
push id103532
push userbmo:markh@mozilla.com
push dateThu, 22 Mar 2018 01:34:47 +0000
reviewerskitcambridge
bugs1433182
milestone61.0a1
Bug 1433182 - bookmark mirror now handles left-pane-roots on the server. r?kitcambridge MozReview-Commit-ID: CwOqI30jP7O
toolkit/components/places/SyncedBookmarksMirror.jsm
toolkit/components/places/tests/sync/head_sync.js
toolkit/components/places/tests/sync/test_bookmark_corruption.js
--- a/toolkit/components/places/SyncedBookmarksMirror.jsm
+++ b/toolkit/components/places/SyncedBookmarksMirror.jsm
@@ -612,17 +612,17 @@ class SyncedBookmarksMirror {
     if (!guid) {
       MirrorLog.warn("Ignoring folder with invalid ID", record.id);
       this.recordTelemetryEvent("mirror", "ignore", "folder",
                                 { why: "id" });
       return;
     }
     if (guid == PlacesUtils.bookmarks.rootGuid) {
       // The Places root shouldn't be synced at all.
-      MirrorLog.warn("Ignoring Places root record", record.cleartext);
+      MirrorLog.warn("Ignoring Places root record", record);
       this.recordTelemetryEvent("mirror", "ignore", "folder",
                                 { why: "root" });
       return;
     }
 
     let serverModified = determineServerModified(record);
     let dateAdded = determineDateAdded(record);
     let title = validateTitle(record.title);
@@ -652,16 +652,28 @@ class SyncedBookmarksMirror {
           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.
+    let parentGuid = validateGuid(record.parentid);
+    if (parentGuid == PlacesUtils.bookmarks.rootGuid) {
+        await this.db.executeCached(`
+          INSERT OR IGNORE INTO structure(guid, parentGuid, position)
+          VALUES(:guid, :parentGuid, -1)`,
+          { guid, parentGuid });
+      }
   }
 
   async storeRemoteLivemark(record, { needsMerge }) {
     let guid = validateGuid(record.id);
     if (!guid) {
       MirrorLog.warn("Ignoring livemark with invalid ID", record.id);
       this.recordTelemetryEvent("mirror", "ignore", "livemark",
                                 { why: "id" });
@@ -908,27 +920,36 @@ class SyncedBookmarksMirror {
   async fetchRemoteTree(remoteTimeSeconds) {
     let remoteTree = new BookmarkTree(BookmarkNode.root());
     let startTime = Cu.now();
 
     // First, build a flat mapping of parents to children. The `LEFT JOIN`
     // includes items orphaned by an interrupted upload on another device.
     // We keep the orphans in "unfiled" until the other device returns and
     // uploads the missing parent.
+    // Note that we avoid returning orphaned queries here - there's a really
+    // good chance that orphaned queries are actually left-pane queries with
+    // the left-pane root missing.
+    // In some bizarre edge-cases this might mean some legitimate queries are
+    // lost for the user, but this should happen far less often than us finding
+    // illegitimate left-pane queries - and if the actual parents ever do
+    // appear, they will spring back into life.
     let itemRows = await this.db.execute(`
       SELECT v.guid, IFNULL(s.parentGuid, :unfiledGuid) AS parentGuid,
              IFNULL(s.position, -1) AS position, v.serverModified, v.kind,
              v.needsMerge
       FROM items v
       LEFT JOIN structure s ON s.guid = v.guid
       WHERE NOT v.isDeleted AND
-            v.guid <> :rootGuid
+            v.guid <> :rootGuid AND
+            (s.parentGuid IS NOT NULL OR v.kind <> :queryKind)
       ORDER BY parentGuid, position = -1, position, v.guid`,
       { rootGuid: PlacesUtils.bookmarks.rootGuid,
-        unfiledGuid: PlacesUtils.bookmarks.unfiledGuid });
+        unfiledGuid: PlacesUtils.bookmarks.unfiledGuid,
+        queryKind: SyncedBookmarksMirror.KIND.QUERY });
 
     let pseudoTree = new Map();
     for await (let row of yieldingIterator(itemRows)) {
       let parentGuid = row.getResultByName("parentGuid");
       let node = BookmarkNode.fromRemoteRow(row, remoteTimeSeconds);
       if (pseudoTree.has(parentGuid)) {
         let nodes = pseudoTree.get(parentGuid);
         nodes.push(node);
@@ -1980,18 +2001,20 @@ async function initializeMirrorDatabase(
   )`);
 
   await db.execute(`CREATE INDEX mirror.urlHashes ON urls(hash)`);
 
   await createMirrorRoots(db);
 }
 
 /**
- * Sets up the syncable roots. All items in the mirror should descend from these
- * roots.
+ * Sets up the syncable roots. All items in the mirror we apply will descend
+ * from these roots - however, malformed records from the server which create
+ * a different root *will* be created in the mirror - just not applied.
+ *
  *
  * @param {Sqlite.OpenedConnection} db
  *        The mirror database connection.
  */
 async function createMirrorRoots(db) {
   const syncableRoots = [{
     guid: PlacesUtils.bookmarks.rootGuid,
     // The Places root is its own parent, to satisfy the foreign key and
@@ -2912,21 +2935,16 @@ class BookmarkNode {
     let age = Math.max(remoteTimeSeconds - serverModified / 1000, 0) || 0;
 
     let kind = row.getResultByName("kind");
     let needsMerge = !!row.getResultByName("needsMerge");
 
     return new BookmarkNode(guid, age, kind, needsMerge);
   }
 
-  isRoot() {
-    return this.guid == PlacesUtils.bookmarks.rootGuid ||
-           PlacesUtils.bookmarks.userContentRoots.includes(this.guid);
-  }
-
   isFolder() {
     return this.kind == SyncedBookmarksMirror.KIND.FOLDER;
   }
 
   newerThan(otherNode) {
     return this.age < otherNode.age;
   }
 
@@ -3100,22 +3118,27 @@ class BookmarkTree {
     let level = parentInfo ? parentInfo.level + 1 : 0;
     this.infosByNode.set(node, { parentNode, level });
   }
 
   noteDeleted(guid) {
     this.deletedGuids.add(guid);
   }
 
-  * guids() {
-    for (let [guid, node] of this.byGuid) {
-      if (node == this.root) {
-        continue;
+  * syncableGuids() {
+    let nodesToWalk = PlacesUtils.bookmarks.userContentRoots.map(guid => {
+      let node = this.byGuid.get(guid);
+      return node ? node.children : [];
+    });
+    while (nodesToWalk.length) {
+      let childNodes = nodesToWalk.pop();
+      for (let node of childNodes) {
+        yield node.guid;
+        nodesToWalk.push(node.children);
       }
-      yield guid;
     }
     for (let guid of this.deletedGuids) {
       yield guid;
     }
   }
 
   /**
    * Generates an ASCII art representation of the complete tree. Deleted GUIDs
@@ -3309,20 +3332,25 @@ class BookmarkMerger {
     this.matchingDupesByLocalParentNode = new Map();
     this.mergedGuids = new Set();
     this.deleteLocally = new Set();
     this.deleteRemotely = new Set();
     this.telemetryEvents = [];
   }
 
   async merge() {
-    let localRoot = this.localTree.nodeForGuid(PlacesUtils.bookmarks.rootGuid);
-    let remoteRoot = this.remoteTree.nodeForGuid(PlacesUtils.bookmarks.rootGuid);
-    let mergedRoot = await this.mergeNode(PlacesUtils.bookmarks.rootGuid, localRoot,
-                                          remoteRoot);
+    let mergedRoot = new MergedBookmarkNode(PlacesUtils.bookmarks.rootGuid,
+      BookmarkNode.root(), null, BookmarkMergeState.local);
+    for (let guid of PlacesUtils.bookmarks.userContentRoots) {
+      let localSyncableRoot = this.localTree.nodeForGuid(guid);
+      let remoteSyncableRoot = this.remoteTree.nodeForGuid(guid);
+      let mergedSyncableRoot = await this.mergeNode(guid, localSyncableRoot,
+                                                    remoteSyncableRoot);
+      mergedRoot.mergedChildren.push(mergedSyncableRoot);
+    }
 
     // Any remaining deletions on one side should be deleted on the other side.
     // This happens when the remote tree has tombstones for items that don't
     // exist in Places, or Places has tombstones for items that aren't on the
     // server.
     for await (let guid of yieldingIterator(this.localTree.deletedGuids)) {
       if (!this.mentions(guid)) {
         this.deleteRemotely.add(guid);
@@ -3332,17 +3360,17 @@ class BookmarkMerger {
       if (!this.mentions(guid)) {
         this.deleteLocally.add(guid);
       }
     }
     return mergedRoot;
   }
 
   async subsumes(tree) {
-    for await (let guid of Async.yieldingIterator(tree.guids())) {
+    for await (let guid of Async.yieldingIterator(tree.syncableGuids())) {
       if (!this.mentions(guid)) {
         return false;
       }
     }
     return true;
   }
 
   mentions(guid) {
--- a/toolkit/components/places/tests/sync/head_sync.js
+++ b/toolkit/components/places/tests/sync/head_sync.js
@@ -49,17 +49,17 @@ function inspectChangeRecords(changeReco
   for (let [id, record] of Object.entries(changeRecords)) {
     (record.tombstone ? results.deleted : results.updated).push(id);
   }
   results.updated.sort();
   results.deleted.sort();
   return results;
 }
 
-async function assertLocalTree(rootGuid, expected, message) {
+async function fetchLocalTree(rootGuid) {
   function bookmarkNodeToInfo(node) {
     let { guid, index, title, typeCode: type } = node;
     let itemInfo = { guid, index, title, type };
     if (node.annos) {
       let syncableAnnos = node.annos.filter(anno => [
         PlacesSyncUtils.bookmarks.DESCRIPTION_ANNO,
         PlacesSyncUtils.bookmarks.SIDEBAR_ANNO,
         PlacesSyncUtils.bookmarks.SMART_BOOKMARKS_ANNO,
@@ -77,17 +77,21 @@ async function assertLocalTree(rootGuid,
       itemInfo.keyword = node.keyword;
     }
     if (node.children) {
       itemInfo.children = node.children.map(bookmarkNodeToInfo);
     }
     return itemInfo;
   }
   let root = await PlacesUtils.promiseBookmarksTree(rootGuid);
-  let actual = bookmarkNodeToInfo(root);
+  return bookmarkNodeToInfo(root);
+}
+
+async function assertLocalTree(rootGuid, expected, message) {
+  let actual = await fetchLocalTree(rootGuid);
   if (!ObjectUtils.deepEqual(actual, expected)) {
     info(`Expected structure for ${rootGuid}: ${JSON.stringify(expected)}`);
     info(`Actual structure for ${rootGuid}: ${JSON.stringify(actual)}`);
     throw new Assert.constructor.AssertionError({ actual, expected, message });
   }
 }
 
 function makeLivemarkServer() {
--- a/toolkit/components/places/tests/sync/test_bookmark_corruption.js
+++ b/toolkit/components/places/tests/sync/test_bookmark_corruption.js
@@ -1,11 +1,17 @@
 /* Any copyright is dedicated to the Public Domain.
  * 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_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])");
   {
@@ -787,18 +793,107 @@ add_task(async function test_new_orphan_
   await PlacesSyncUtils.bookmarks.reset();
 });
 
 add_task(async function test_tombstone_as_child() {
   // TODO (Bug 1433180): Add a folder that mentions a tombstone in its
   // `children`.
 });
 
+// See what happens when a left-pane root and a left-pane query are on the server
 add_task(async function test_left_pane_root() {
-  // TODO (Bug 1433182): Add a left pane root to the mirror.
+  let buf = await openMirror("lpr");
+
+  let initialTree = await fetchLocalTree(PlacesUtils.bookmarks.rootGuid);
+
+  // This test is expected to not touch bookmarks at all, and if it did
+  // happen to create a new item that's not under our syncable roots, then
+  // just checking the result of fetchLocalTree wouldn't pick that up - so
+  // as an additional safety check, count how many bookmark rows exist.
+  let numRows = await getCountOfBookmarkRows(buf.db);
+
+  // Add a left pane root, a left-pane query and a left-pane folder to the
+  // mirror, all correctly parented.
+  // Because we can determine this is a complete tree that's outside our
+  // syncable trees, we expect none of them to be applied.
+  await buf.store(shuffle([{
+    id: "folderLEFTPR",
+    type: "folder",
+    parentid: "places",
+    title: "",
+    children: ["folderLEFTPQ", "folderLEFTPF"],
+  }, {
+    id: "folderLEFTPQ",
+    type: "query",
+    parentid: "folderLEFTPR",
+    title: "Some query",
+    bmkUri: "place:folder=SOMETHING",
+  }, {
+    id: "folderLEFTPF",
+    type: "folder",
+    parentid: "folderLEFTPR",
+    title: "All Bookmarks",
+    children: ["folderLEFTPC"],
+  }, {
+    id: "folderLEFTPC",
+    type: "query",
+    parentid: "folderLEFTPF",
+    title: "A query under 'All Bookmarks'",
+    bmkUri: "place:folder=SOMETHING_ELSE",
+  }], { needsMerge: true }));
+
+  await buf.apply();
+
+  // should have ignored everything.
+  await assertLocalTree(PlacesUtils.bookmarks.rootGuid, initialTree);
+
+  // and a check we didn't write *any* items to the places database, even
+  // outside of our user roots.
+  Assert.equal(await getCountOfBookmarkRows(buf.db), numRows);
+
+  await buf.finalize();
+  await PlacesUtils.bookmarks.eraseEverything();
+  await PlacesSyncUtils.bookmarks.reset();
+});
+
+// See what happens when a left-pane query (without the left-pane root) is on
+// the server
+add_task(async function test_left_pane_query() {
+  let buf = await openMirror("lpq");
+
+  let initialTree = await fetchLocalTree(PlacesUtils.bookmarks.rootGuid);
+
+  // This test is expected to not touch bookmarks at all, and if it did
+  // happen to create a new item that's not under our syncable roots, then
+  // just checking the result of fetchLocalTree wouldn't pick that up - so
+  // as an additional safety check, count how many bookmark rows exist.
+  let numRows = await getCountOfBookmarkRows(buf.db);
+
+  // Add the left pane root and left-pane folders to the mirror, correctly parented.
+  // We should not apply it because we made a policy decision to not apply
+  // orphaned queries (bug 1433182)
+  await buf.store([{
+    id: "folderLEFTPQ",
+    type: "query",
+    parentid: "folderLEFTPR",
+    title: "Some query",
+    bmkUri: "place:folder=SOMETHING",
+  }], { needsMerge: true });
+
+  await buf.apply();
+
+  // should have ignored everything.
+  await assertLocalTree(PlacesUtils.bookmarks.rootGuid, initialTree);
+
+  // and further check we didn't apply it as mis-rooted.
+  Assert.equal(await getCountOfBookmarkRows(buf.db), numRows);
+
+  await buf.finalize();
+  await PlacesUtils.bookmarks.eraseEverything();
+  await PlacesSyncUtils.bookmarks.reset();
 });
 
 add_task(async function test_partial_cycle() {
   let buf = await openMirror("partial_cycle");
 
   info("Set up mirror: Menu > A > B > C");
   await PlacesUtils.bookmarks.insertTree({
     guid: PlacesUtils.bookmarks.menuGuid,