Bug 1433182 - bookmark mirror now handles left-pane-roots on the server. r?kitcambridge
MozReview-Commit-ID: CwOqI30jP7O
--- 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,