Bug 1444262 - ensure that a bookmark that later becomes a query doesn't break the mirror. r=kitcambridge
authorMark Hammond <mhammond@skippinet.com.au>
Fri, 09 Mar 2018 11:52:14 +1100
changeset 408928 41c707ae4d6d143f1c63772be63108df34bde197
parent 408927 8715148b05f8799a2de5104078b601dd7706f11f
child 408929 909ff88d91d6b17824da0d7f837a662d8c632afd
push id61369
push usermhammond@skippinet.com.au
push dateTue, 20 Mar 2018 02:47:38 +0000
treeherderautoland@41c707ae4d6d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerskitcambridge
bugs1444262
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 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
@@ -393,19 +393,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"));
@@ -509,18 +513,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);
@@ -566,18 +570,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);
@@ -661,18 +665,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);
@@ -2931,16 +2935,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()) {
@@ -3390,60 +3433,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();
+  }
+});