Bug 1309774 - Part 1. Sync bookmark validator should only expect to see children of roots on server. r?markh
MozReview-Commit-ID: EEoDmsnBraA
--- a/services/sync/modules/bookmark_validator.js
+++ b/services/sync/modules/bookmark_validator.js
@@ -4,16 +4,18 @@
"use strict";
const Cu = Components.utils;
Cu.import("resource://gre/modules/PlacesUtils.jsm");
Cu.import("resource://gre/modules/PlacesSyncUtils.jsm");
Cu.import("resource://gre/modules/Task.jsm");
+Cu.import("resource://gre/modules/XPCOMUtils.jsm");
+
this.EXPORTED_SYMBOLS = ["BookmarkValidator", "BookmarkProblemData"];
const LEFT_PANE_ROOT_ANNO = "PlacesOrganizer/OrganizerFolder";
const LEFT_PANE_QUERY_ANNO = "PlacesOrganizer/OrganizerQuery";
// Indicates if a local bookmark tree node should be excluded from syncing.
function isNodeIgnored(treeNode) {
@@ -147,16 +149,24 @@ class BookmarkProblemData {
if (full) {
let structural = this._summarizeDifferences("sdiff", this.structuralDifferences);
result.push.apply(result, structural);
}
return result;
}
}
+// Defined lazily to avoid initializing PlacesUtils.bookmarks too soon.
+XPCOMUtils.defineLazyGetter(this, "SYNCED_ROOTS", () => [
+ PlacesUtils.bookmarks.menuGuid,
+ PlacesUtils.bookmarks.toolbarGuid,
+ PlacesUtils.bookmarks.unfiledGuid,
+ PlacesUtils.bookmarks.mobileGuid,
+]);
+
class BookmarkValidator {
_followQueries(recordMap) {
for (let [guid, entry] of recordMap) {
if (entry.type !== "query" && (!entry.bmkUri || !entry.bmkUri.startsWith("place:"))) {
continue;
}
// Might be worth trying to parse the place: query instead so that this
@@ -203,20 +213,26 @@ class BookmarkValidator {
}
}
createClientRecordsFromTree(clientTree) {
// Iterate over the treeNode, converting it to something more similar to what
// the server stores.
let records = [];
let recordsByGuid = new Map();
- function traverse(treeNode) {
+ let syncedRoots = SYNCED_ROOTS;
+ function traverse(treeNode, synced) {
+ if (!synced) {
+ synced = syncedRoots.includes(treeNode.guid);
+ } else if (isNodeIgnored(treeNode)) {
+ synced = false;
+ }
let guid = PlacesSyncUtils.bookmarks.guidToSyncId(treeNode.guid);
let itemType = 'item';
- treeNode.ignored = isNodeIgnored(treeNode);
+ treeNode.ignored = !synced;
treeNode.id = guid;
switch (treeNode.type) {
case PlacesUtils.TYPE_X_MOZ_PLACE:
let query = null;
if (treeNode.annos && treeNode.uri.startsWith("place:")) {
query = treeNode.annos.find(({name}) =>
name === PlacesSyncUtils.bookmarks.SMART_BOOKMARKS_ANNO);
}
@@ -258,24 +274,24 @@ class BookmarkValidator {
// We want to use the "real" guid here.
recordsByGuid.set(treeNode.guid, treeNode);
if (treeNode.type === 'folder') {
treeNode.childGUIDs = [];
if (!treeNode.children) {
treeNode.children = [];
}
for (let child of treeNode.children) {
- traverse(child);
+ traverse(child, synced);
child.parent = treeNode;
child.parentid = guid;
treeNode.childGUIDs.push(child.guid);
}
}
}
- traverse(clientTree);
+ traverse(clientTree, false);
clientTree.id = 'places';
this._followQueries(recordsByGuid);
return records;
}
/**
* Process the server-side list. Mainly this builds the records into a tree,
* but it also records information about problems, and produces arrays of the
@@ -363,17 +379,17 @@ class BookmarkValidator {
}
}
let root = idToRecord.get('places');
if (!root) {
// Fabricate a root. We want to remember that it's fake so that we can
// avoid complaining about stuff like it missing it's childGUIDs later.
- root = { id: 'places', children: [], type: 'folder', title: '' };
+ root = { id: 'places', children: [], type: 'folder', title: '', fake: true };
resultRecords.push(root);
idToRecord.set('places', root);
} else {
problemData.rootOnServer = true;
}
// Build the tree, find orphans, and record most problems having to do with
// the tree structure.
@@ -569,23 +585,17 @@ class BookmarkValidator {
}
return cycles;
}
// Perform client-side sanity checking that doesn't involve server data
_validateClient(problemData, clientRecords) {
problemData.clientCycles = this._detectCycles(clientRecords);
- const rootsToCheck = [
- PlacesUtils.bookmarks.menuGuid,
- PlacesUtils.bookmarks.toolbarGuid,
- PlacesUtils.bookmarks.unfiledGuid,
- PlacesUtils.bookmarks.mobileGuid,
- ];
- for (let rootGUID of rootsToCheck) {
+ for (let rootGUID of SYNCED_ROOTS) {
let record = clientRecords.find(record =>
record.guid === rootGUID);
if (!record || record.parentid !== "places") {
problemData.badClientRoots.push(rootGUID);
}
}
}
@@ -612,16 +622,19 @@ class BookmarkValidator {
this._validateClient(problemData, clientRecords);
let matches = [];
let allRecords = new Map();
let serverDeletedLookup = new Set(inspectionInfo.deletedRecords.map(r => r.id));
for (let sr of serverRecords) {
+ if (sr.fake) {
+ continue;
+ }
allRecords.set(sr.id, {client: null, server: sr});
}
for (let cr of clientRecords) {
let unified = allRecords.get(cr.id);
if (!unified) {
allRecords.set(cr.id, {client: cr, server: null});
} else {
--- a/services/sync/tests/unit/test_bookmark_validator.js
+++ b/services/sync/tests/unit/test_bookmark_validator.js
@@ -114,49 +114,49 @@ add_test(function test_isr_duplicateChil
run_next_test();
});
// Each compareServerWithClient test mutates these, so we can't just keep them
// global
function getDummyServerAndClient() {
let server = [
{
- id: 'aaaaaaaaaaaa',
+ id: 'menu',
parentid: 'places',
type: 'folder',
parentName: '',
title: 'foo',
children: ['bbbbbbbbbbbb', 'cccccccccccc']
},
{
id: 'bbbbbbbbbbbb',
type: 'bookmark',
- parentid: 'aaaaaaaaaaaa',
+ parentid: 'menu',
parentName: 'foo',
title: 'bar',
bmkUri: 'http://baz.com'
},
{
id: 'cccccccccccc',
- parentid: 'aaaaaaaaaaaa',
+ parentid: 'menu',
parentName: 'foo',
title: '',
type: 'query',
bmkUri: 'place:type=6&sort=14&maxResults=10'
}
];
let client = {
"guid": "root________",
"title": "",
"id": 1,
"type": "text/x-moz-place-container",
"children": [
{
- "guid": "aaaaaaaaaaaa",
+ "guid": "menu________",
"title": "foo",
"id": 1000,
"type": "text/x-moz-place-container",
"children": [
{
"guid": "bbbbbbbbbbbb",
"title": "bar",
"id": 1001,
@@ -198,28 +198,28 @@ add_test(function test_cswc_serverMissin
let {server, client} = getDummyServerAndClient();
// remove c
server.pop();
server[0].children.pop();
let c = new BookmarkValidator().compareServerWithClient(server, client).problemData;
deepEqual(c.serverMissing, ['cccccccccccc']);
equal(c.clientMissing.length, 0);
- deepEqual(c.structuralDifferences, [{id: 'aaaaaaaaaaaa', differences: ['childGUIDs']}]);
+ deepEqual(c.structuralDifferences, [{id: 'menu', differences: ['childGUIDs']}]);
run_next_test();
});
add_test(function test_cswc_clientMissing() {
let {server, client} = getDummyServerAndClient();
client.children[0].children.pop();
let c = new BookmarkValidator().compareServerWithClient(server, client).problemData;
deepEqual(c.clientMissing, ['cccccccccccc']);
equal(c.serverMissing.length, 0);
- deepEqual(c.structuralDifferences, [{id: 'aaaaaaaaaaaa', differences: ['childGUIDs']}]);
+ deepEqual(c.structuralDifferences, [{id: 'menu', differences: ['childGUIDs']}]);
run_next_test();
});
add_test(function test_cswc_differences() {
{
let {server, client} = getDummyServerAndClient();
client.children[0].children[0].title = 'asdf';
let c = new BookmarkValidator().compareServerWithClient(server, client).problemData;
@@ -326,17 +326,17 @@ add_task(function *test_telemetry_integr
let bme = ping.engines.find(e => e.name === "bookmarks");
ok(bme);
ok(bme.validation);
ok(bme.validation.problems)
equal(bme.validation.checked, server.length);
equal(bme.validation.took, duration);
bme.validation.problems.sort((a, b) => String.localeCompare(a.name, b.name));
deepEqual(bme.validation.problems, [
- { name: "badClientRoots", count: 4 },
+ { name: "badClientRoots", count: 3 },
{ name: "sdiff:childGUIDs", count: 1 },
{ name: "serverMissing", count: 1 },
{ name: "structuralDifferences", count: 1 },
]);
});
function run_test() {
run_next_test();
--- a/toolkit/components/places/PlacesSyncUtils.jsm
+++ b/toolkit/components/places/PlacesSyncUtils.jsm
@@ -337,17 +337,16 @@ const BookmarkSyncUtils = PlacesSyncUtil
return PlacesUtils.bookmarks.fetch(guid)
.then(item => {
if (!item) {
return null;
}
return getKindForItem(item)
});
},
-
});
XPCOMUtils.defineLazyGetter(this, "BookmarkSyncLog", () => {
return Log.repository.getLogger("BookmarkSyncUtils");
});
function validateSyncBookmarkObject(input, behavior) {
return PlacesUtils.validateItemProperties(