Bug 1308841 - Sanity check client-side bookmark roots in sync validator r?markh
MozReview-Commit-ID: 6Y5qfCtVYWf
--- a/services/sync/modules/bookmark_validator.js
+++ b/services/sync/modules/bookmark_validator.js
@@ -36,16 +36,18 @@ this.EXPORTED_SYMBOLS = ["BookmarkValida
* had deleted parents
* - childrenOnNonFolder (array of ids): list of non-folders that still have
* children arrays
* - duplicateChildren (array of ids): list of records who have the same
* child listed multiple times in their children array
* - parentNotFolder (array of ids): list of records that have parents that
* aren't folders
* - rootOnServer (boolean): true if the root came from the server
+ * - badClientRoots (array of ids): Contains any client-side root ids where
+ * the root is missing or isn't a (direct) child of the places root.
*
* - clientMissing: Array of ids on the server missing from the client
* - serverMissing: Array of ids on the client missing from the server
* - serverDeleted: Array of ids on the client that the server had marked as deleted.
* - serverUnexpected: Array of ids that appear on the server but shouldn't
* because the client attempts to never upload them.
* - differences: Array of {id: string, differences: string array} recording
* the non-structural properties that are differente between the client and server
@@ -65,16 +67,17 @@ class BookmarkProblemData {
this.missingChildren = [];
this.deletedChildren = [];
this.multipleParents = [];
this.deletedParents = [];
this.childrenOnNonFolder = [];
this.duplicateChildren = [];
this.parentNotFolder = [];
+ this.badClientRoots = [];
this.clientMissing = [];
this.serverMissing = [];
this.serverDeleted = [];
this.serverUnexpected = [];
this.differences = [];
this.structuralDifferences = [];
}
@@ -117,16 +120,17 @@ class BookmarkProblemData {
{ name: "missingIDs", count: this.missingIDs },
{ name: "rootOnServer", count: this.rootOnServer ? 1 : 0 },
{ name: "duplicates", count: this.duplicates.length },
{ name: "parentChildMismatches", count: this.parentChildMismatches.length },
{ name: "cycles", count: this.cycles.length },
{ name: "clientCycles", count: this.clientCycles.length },
+ { name: "badClientRoots", count: this.badClientRoots.length },
{ name: "orphans", count: this.orphans.length },
{ name: "missingChildren", count: this.missingChildren.length },
{ name: "deletedChildren", count: this.deletedChildren.length },
{ name: "multipleParents", count: this.multipleParents.length },
{ name: "deletedParents", count: this.deletedParents.length },
{ name: "childrenOnNonFolder", count: this.childrenOnNonFolder.length },
{ name: "duplicateChildren", count: this.duplicateChildren.length },
{ name: "parentNotFolder", count: this.parentNotFolder.length },
@@ -553,16 +557,34 @@ class BookmarkValidator {
if (!seenEver.has(record)) {
traverse(record);
}
}
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) {
+ let record = clientRecords.find(record =>
+ record.guid === rootGUID);
+ if (!record || record.parentid !== "places") {
+ problemData.badClientRoots.push(rootGUID);
+ }
+ }
+ }
+
/**
* Compare the list of server records with the client tree.
*
* Returns the same data as described in the inspectServerRecords comment,
* with the following additional fields.
* - clientRecords: an array of client records in a similar format to
* the .records (ie, server records) entry.
* - problemData is the same as for inspectServerRecords, except all properties
@@ -573,17 +595,17 @@ class BookmarkValidator {
let clientRecords = this.createClientRecordsFromTree(clientTree);
let inspectionInfo = this.inspectServerRecords(serverRecords);
inspectionInfo.clientRecords = clientRecords;
// Mainly do this to remove deleted items and normalize child guids.
serverRecords = inspectionInfo.records;
let problemData = inspectionInfo.problemData;
- problemData.clientCycles = this._detectCycles(clientRecords);
+ 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) {
allRecords.set(sr.id, {client: null, server: sr});
--- a/services/sync/tests/unit/test_bookmark_validator.js
+++ b/services/sync/tests/unit/test_bookmark_validator.js
@@ -266,16 +266,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: "sdiff:childGUIDs", count: 1 },
{ name: "serverMissing", count: 1 },
{ name: "structuralDifferences", count: 1 },
]);
});
function run_test() {
run_next_test();