Bug 1444262 - ensure that a bookmark that later becomes a query doesn't break the mirror. r?kitcambridge draft
authorMark Hammond <mhammond@skippinet.com.au>
Fri, 09 Mar 2018 11:52:14 +1100
changeset 769731 c93bd583a0eb6071368aa7baee639134f56b02d0
parent 769726 bfb7edfd0436db388bb9e103b8ad817fc50bfdcf
push id103210
push userbmo:markh@mozilla.com
push dateTue, 20 Mar 2018 01:03:25 +0000
reviewerskitcambridge
bugs1444262
milestone61.0a1
Bug 1444262 - ensure that a bookmark that later becomes a query doesn't break the mirror. r?kitcambridge MozReview-Commit-ID: F0bBgkECPCp
toolkit/components/places/SyncedBookmarksMirror.jsm
toolkit/components/places/tests/sync/head_sync.js
toolkit/components/places/tests/sync/test_bookmark_kinds.js
--- a/toolkit/components/places/SyncedBookmarksMirror.jsm
+++ b/toolkit/components/places/SyncedBookmarksMirror.jsm
@@ -389,19 +389,23 @@ class SyncedBookmarksMirror {
       let newRemoteContents = await this.fetchNewRemoteContents();
 
       MirrorLog.debug("Fetching content info for new Places items");
       let newLocalContents = await this.fetchNewLocalContents();
 
       MirrorLog.debug("Building complete merged tree");
       let merger = new BookmarkMerger(localTree, newLocalContents,
                                       remoteTree, newRemoteContents);
-      let mergedRoot = await merger.merge();
-      for (let { value, extra } of merger.telemetryEvents) {
-        this.recordTelemetryEvent("mirror", "merge", value, extra);
+      let mergedRoot;
+      try {
+        mergedRoot = await merger.merge();
+      } finally {
+        for (let { value, extra } of merger.telemetryEvents) {
+          this.recordTelemetryEvent("mirror", "merge", value, extra);
+        }
       }
 
       if (MirrorLog.level <= Log.Level.Trace) {
         MirrorLog.trace([
           "Built new merged tree",
           mergedRoot.toASCIITreeString(),
           ...merger.deletionsToStrings(),
         ].join("\n"));
@@ -505,18 +509,18 @@ class SyncedBookmarksMirror {
       MirrorLog.warn("Ignoring bookmark with invalid ID", record.id);
       this.recordTelemetryEvent("mirror", "ignore", "bookmark",
                                 { why: "id" });
       return;
     }
 
     let url = validateURL(record.bmkUri);
     if (!url) {
-      MirrorLog.trace("Ignoring bookmark ${guid} with invalid URL ${url}",
-                      { guid, url: record.bmkUri });
+      MirrorLog.warn("Ignoring bookmark ${guid} with invalid URL ${url}",
+                     { guid, url: record.bmkUri });
       this.recordTelemetryEvent("mirror", "ignore", "bookmark",
                                 { why: "url" });
       return;
     }
 
     await this.maybeStoreRemoteURL(url);
 
     let serverModified = determineServerModified(record);
@@ -562,18 +566,18 @@ class SyncedBookmarksMirror {
       MirrorLog.warn("Ignoring query with invalid ID", record.id);
       this.recordTelemetryEvent("mirror", "ignore", "query",
                                 { why: "id" });
       return;
     }
 
     let url = validateURL(record.bmkUri);
     if (!url) {
-      MirrorLog.trace("Ignoring query ${guid} with invalid URL ${url}",
-                      { guid, url: record.bmkUri });
+      MirrorLog.warn("Ignoring query ${guid} with invalid URL ${url}",
+                     { guid, url: record.bmkUri });
       this.recordTelemetryEvent("mirror", "ignore", "query",
                                 { why: "url" });
       return;
     }
 
     await this.maybeStoreRemoteURL(url);
 
     let serverModified = determineServerModified(record);
@@ -657,18 +661,18 @@ class SyncedBookmarksMirror {
       MirrorLog.warn("Ignoring livemark with invalid ID", record.id);
       this.recordTelemetryEvent("mirror", "ignore", "livemark",
                                 { why: "id" });
       return;
     }
 
     let feedURL = validateURL(record.feedUri);
     if (!feedURL) {
-      MirrorLog.trace("Ignoring livemark ${guid} with invalid feed URL ${url}",
-                      { guid, url: record.feedUri });
+      MirrorLog.warn("Ignoring livemark ${guid} with invalid feed URL ${url}",
+                     { guid, url: record.feedUri });
       this.recordTelemetryEvent("mirror", "ignore", "livemark",
                                 { why: "feed" });
       return;
     }
 
     let serverModified = determineServerModified(record);
     let dateAdded = determineDateAdded(record);
     let title = validateTitle(record.title);
@@ -2914,16 +2918,55 @@ class BookmarkNode {
       yield node;
       if (node.isFolder()) {
         yield* node.descendants();
       }
     }
   }
 
   /**
+   * Checks if remoteNode has a kind that's compatible with this *local* node.
+   * - Nodes with the same kind are always compatible.
+   * - Local folders are compatible with remote livemarks, but not vice-versa
+   *   (ie, remote folders are *not* compatible with local livemarks)
+   * - Bookmarks and queries are always compatible.
+   *
+   * @return {Boolean}
+   */
+  hasCompatibleKind(remoteNode) {
+    if (this.kind == remoteNode.kind) {
+      return true;
+    }
+    // bookmarks and queries are interchangable as simply changing the URL
+    // can cause it to flip kinds - and webextensions are able to change the
+    // URL of any bookmark.
+    if ((this.kind == SyncedBookmarksMirror.KIND.BOOKMARK &&
+         remoteNode.kind == SyncedBookmarksMirror.KIND.QUERY) ||
+        (this.kind == SyncedBookmarksMirror.KIND.QUERY &&
+         remoteNode.kind == SyncedBookmarksMirror.KIND.BOOKMARK)) {
+      return true;
+    }
+    // A local folder can become a livemark as the remote may have synced
+    // as a folder before the annotation was added. However, we don't allow
+    // a local livemark to "downgrade" to a folder.
+    // We allow merging local folders and remote livemarks because Places
+    // stores livemarks as empty folders with feed and site URL annotations.
+    // The livemarks service first inserts the folder, and *then* sets
+    // annotations. Since this isn't wrapped in a transaction, we might sync
+    // before the annotations are set, and upload a folder record instead
+    // of a livemark record (bug 632287), then replace the folder with a
+    // livemark on the next sync.
+    if (this.kind == SyncedBookmarksMirror.KIND.FOLDER &&
+        remoteNode.kind == SyncedBookmarksMirror.KIND.LIVEMARK) {
+      return true;
+    }
+    return false;
+  }
+
+  /**
    * Generates a human-readable, ASCII art representation of the node and its
    * descendants. This is useful for visualizing the tree structure in trace
    * logs.
    *
    * @return {String}
    */
   toASCIITreeString(prefix = "") {
     if (!this.isFolder()) {
@@ -3372,60 +3415,43 @@ class BookmarkMerger {
     let mergeState = this.resolveTwoWayValueConflict(mergedGuid, localNode,
                                                      remoteNode);
     MirrorLog.trace("Merge state for ${mergedGuid} is ${mergeState}",
                     { mergedGuid, mergeState });
 
     let mergedNode = new MergedBookmarkNode(mergedGuid, localNode, remoteNode,
                                             mergeState);
 
+    if (!localNode.hasCompatibleKind(remoteNode)) {
+      MirrorLog.error("Merging local ${localNode} and remote ${remoteNode} " +
+                      "with different kinds", { localNode, remoteNode });
+      this.telemetryEvents.push({
+        value: "kind-mismatch",
+        extra: { local: "" + localNode.kind, remote: "" + remoteNode.kind },
+      });
+      throw new SyncedBookmarksMirror.ConsistencyError(
+        "Can't merge different item kinds");
+    }
+
     if (localNode.isFolder()) {
       if (remoteNode.isFolder()) {
         // Merging two folders, so we need to walk their children to handle
         // structure changes.
         MirrorLog.trace("Merging folders ${localNode} and ${remoteNode}",
                         { localNode, remoteNode });
         await this.mergeChildListsIntoMergedNode(mergedNode, localNode, remoteNode);
         return mergedNode;
       }
-
-      if (remoteNode.kind == SyncedBookmarksMirror.KIND.LIVEMARK) {
-        // We allow merging local folders and remote livemarks because Places
-        // stores livemarks as empty folders with feed and site URL annotations.
-        // The livemarks service first inserts the folder, and *then* sets
-        // annotations. Since this isn't wrapped in a transaction, we might sync
-        // before the annotations are set, and upload a folder record instead
-        // of a livemark record (bug 632287), then replace the folder with a
-        // livemark on the next sync.
-        MirrorLog.trace("Merging local folder ${localNode} and remote " +
-                        "livemark ${remoteNode}", { localNode, remoteNode });
-        this.telemetryEvents.push({
-          value: "kind",
-          extra: { local: "folder", remote: "folder" },
-        });
-        return mergedNode;
-      }
-
-      MirrorLog.error("Merging local folder ${localNode} and remote " +
-                      "non-folder ${remoteNode}", { localNode, remoteNode });
-      throw new SyncedBookmarksMirror.ConsistencyError(
-        "Can't merge folder and non-folder");
+      // Otherwise it must be a livemark, so fall through.
     }
-
-    if (localNode.kind == remoteNode.kind) {
-      // Merging two non-folders, so no need to walk children.
-      MirrorLog.trace("Merging non-folders ${localNode} and ${remoteNode}",
-                      { localNode, remoteNode });
-      return mergedNode;
-    }
-
-    MirrorLog.error("Merging local ${localNode} and remote ${remoteNode} " +
-                    "with different kinds", { localNode, remoteNode });
-    throw new SyncedBookmarksMirror.ConsistencyError(
-      "Can't merge different item kinds");
+    // Otherwise are compatible kinds of non-folder, so there's no need to
+    // walk children - just return the merged node.
+    MirrorLog.trace("Merging non-folders ${localNode} and ${remoteNode}",
+                    { localNode, remoteNode });
+    return mergedNode;
   }
 
   /**
    * Determines the merge state for a node that exists locally and remotely.
    *
    * @param  {String} mergedGuid
    *         The GUID of the merged node. This is the same as the remote GUID,
    *         and usually the same as the local GUID. The local GUID may be
--- a/toolkit/components/places/tests/sync/head_sync.js
+++ b/toolkit/components/places/tests/sync/head_sync.js
@@ -119,21 +119,25 @@ function shuffle(array) {
 }
 
 async function fetchAllKeywords(info) {
   let entries = [];
   await PlacesUtils.keywords.fetch(info, entry => entries.push(entry));
   return entries;
 }
 
-async function openMirror(name) {
+async function openMirror(name, options = {}) {
   let path = OS.Path.join(OS.Constants.Path.profileDir, `${name}_buf.sqlite`);
   let buf = await SyncedBookmarksMirror.open({
     path,
-    recordTelemetryEvent() {},
+    recordTelemetryEvent(...args) {
+      if (options.recordTelemetryEvent) {
+        options.recordTelemetryEvent.call(this, ...args);
+      }
+    },
   });
   return buf;
 }
 
 function BookmarkObserver({ ignoreDates = true, skipTags = false } = {}) {
   this.notifications = [];
   this.ignoreDates = ignoreDates;
   this.skipTags = skipTags;
--- a/toolkit/components/places/tests/sync/test_bookmark_kinds.js
+++ b/toolkit/components/places/tests/sync/test_bookmark_kinds.js
@@ -308,17 +308,17 @@ add_task(async function test_queries() {
     Assert.strictEqual(changes.queryCCCCCCC.cleartext.folderName, undefined);
   } finally {
     await PlacesUtils.bookmarks.eraseEverything();
     await PlacesSyncUtils.bookmarks.reset();
   }
 });
 
 // Bug 632287.
-add_task(async function test_mismatched_types() {
+add_task(async function test_mismatched_but_compatible_folder_types() {
   let buf = await openMirror("mismatched_types");
 
   info("Set up mirror");
   await PlacesUtils.bookmarks.insertTree({
     guid: PlacesUtils.bookmarks.menuGuid,
     children: [{
       guid: "l1nZZXfB8nC7",
       type: PlacesUtils.bookmarks.TYPE_FOLDER,
@@ -352,8 +352,180 @@ add_task(async function test_mismatched_
     updated: [],
     deleted: [],
   }, "Should not reupload merged livemark");
 
   await buf.finalize();
   await PlacesUtils.bookmarks.eraseEverything();
   await PlacesSyncUtils.bookmarks.reset();
 });
+
+add_task(async function test_mismatched_but_incompatible_folder_types() {
+  let sawMismatchEvent = false;
+  let recordTelemetryEvent = (object, method, value, extra) => {
+    // expecting to see a kind-mismatch event.
+    if (value == "kind-mismatch" &&
+        extra.local && typeof extra.local == "string" &&
+        extra.local == SyncedBookmarksMirror.KIND.LIVEMARK &&
+        extra.remote && typeof extra.remote == "string" &&
+        extra.remote == SyncedBookmarksMirror.KIND.FOLDER) {
+      sawMismatchEvent = true;
+    }
+  };
+  let buf = await openMirror("mismatched_incompatible_types",
+                             {recordTelemetryEvent});
+  try {
+    info("Set up mirror");
+    await PlacesUtils.bookmarks.insertTree({
+      guid: PlacesUtils.bookmarks.menuGuid,
+      children: [{
+        guid: "livemarkAAAA",
+        type: PlacesUtils.bookmarks.TYPE_FOLDER,
+        title: "LiveMark",
+        annos: [{
+          name: PlacesUtils.LMANNO_FEEDURI,
+          value: "http://example.com/feed/a",
+        }],
+      }],
+    });
+    await PlacesTestUtils.markBookmarksAsSynced();
+
+    info("Make remote changes");
+    await buf.store([{
+      "id": "livemarkAAAA",
+      "type": "folder",
+      "title": "not really a Livemark",
+      "description": null,
+      "parentid": "menu"
+    }]);
+
+    info("Apply remote, should fail");
+    await Assert.rejects(buf.apply(), /Can't merge different item kinds/);
+    Assert.ok(sawMismatchEvent, "saw the correct mismatch event");
+  } finally {
+    await buf.finalize();
+    await PlacesUtils.bookmarks.eraseEverything();
+    await PlacesSyncUtils.bookmarks.reset();
+  }
+});
+
+add_task(async function test_different_but_compatible_bookmark_types() {
+  try {
+    let buf = await openMirror("partial_queries");
+
+    await PlacesUtils.bookmarks.insertTree({
+      guid: PlacesUtils.bookmarks.menuGuid,
+      children: [
+        {
+          guid: "bookmarkAAAA",
+          type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
+          title: "not yet a query",
+          url: "about:blank",
+        },
+        {
+          guid: "bookmarkBBBB",
+          type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
+          title: "a query",
+          url: "place:foo",
+
+        }
+      ],
+    });
+
+    let changes = await buf.apply();
+    // We should have an outgoing record for bookmarkA with type=bookmark
+    // and bookmarkB with type=query.
+    Assert.equal(changes.bookmarkAAAA.cleartext.type, "bookmark");
+    Assert.equal(changes.bookmarkBBBB.cleartext.type, "query");
+
+    // Now pretend that same records are already on the server.
+    await buf.store([{
+      id: "menu",
+      type: "folder",
+      children: ["bookmarkAAAA", "bookmarkBBBB"],
+    }, {
+      id: "bookmarkAAAA",
+      parentid: "menu",
+      type: "bookmark",
+      title: "not yet a query",
+      bmkUri: "about:blank",
+    }, {
+      id: "bookmarkBBBB",
+      parentid: "menu",
+      type: "query",
+      title: "a query",
+      bmkUri: "place:foo",
+
+    }], { needsMerge: false });
+    await PlacesTestUtils.markBookmarksAsSynced();
+
+    // change the url of bookmarkA to be a "real" query and of bookmarkB to
+    // no longer be a query.
+    await PlacesUtils.bookmarks.update({
+      guid: "bookmarkAAAA",
+      url: "place:type=6&sort=14&maxResults=10",
+    });
+    await PlacesUtils.bookmarks.update({
+      guid: "bookmarkBBBB",
+      url: "about:robots",
+    });
+
+    changes = await buf.apply();
+    // We should have an outgoing record for bookmarkA with type=query and
+    // for bookmarkB with type=bookmark
+    Assert.equal(changes.bookmarkAAAA.cleartext.type, "query");
+    Assert.equal(changes.bookmarkBBBB.cleartext.type, "bookmark");
+  } finally {
+    await PlacesUtils.bookmarks.eraseEverything();
+    await PlacesSyncUtils.bookmarks.reset();
+  }
+});
+
+add_task(async function test_incompatible_types() {
+  let sawMismatchEvent = false;
+  let recordTelemetryEvent = (object, method, value, extra) => {
+    // expecting to see a kind-mismatch event.
+    if (value == "kind-mismatch" &&
+        extra.local && typeof extra.local == "string" &&
+        extra.local == SyncedBookmarksMirror.KIND.BOOKMARK &&
+        extra.remote && typeof extra.remote == "string" &&
+        extra.remote == SyncedBookmarksMirror.KIND.FOLDER) {
+      sawMismatchEvent = true;
+    }
+  };
+  try {
+    let buf = await openMirror("partial_queries", {recordTelemetryEvent});
+
+    await PlacesUtils.bookmarks.insertTree({
+      guid: PlacesUtils.bookmarks.menuGuid,
+      children: [
+        {
+          guid: "AAAAAAAAAAAA",
+          type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
+          title: "a bookmark",
+          url: "about:blank",
+        },
+      ],
+    });
+
+    await buf.apply();
+
+    // Now pretend that same records are already on the server with incompatible
+    // types.
+    await buf.store([{
+      id: "menu",
+      type: "folder",
+      children: ["AAAAAAAAAAAA"],
+    }, {
+      id: "AAAAAAAAAAAA",
+      parentId: PlacesSyncUtils.bookmarks.guidToRecordId(PlacesUtils.bookmarks.menuGuid),
+      type: "folder",
+      title: "conflicting folder",
+    }], { needsMerge: true });
+    await PlacesTestUtils.markBookmarksAsSynced();
+
+    await Assert.rejects(buf.apply(), /Can't merge different item kinds/);
+    Assert.ok(sawMismatchEvent, "saw expected mismatch event");
+  } finally {
+    await PlacesUtils.bookmarks.eraseEverything();
+    await PlacesSyncUtils.bookmarks.reset();
+  }
+});