Bug 1444262 - ensure that a bookmark that later becomes a query doesn't break the mirror. r?kitcambridge
MozReview-Commit-ID: F0bBgkECPCp
--- 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();
+ }
+});