Bug 1308841 - Sanity check client-side bookmark roots in sync validator r?markh draft
authorThom Chiovoloni <tchiovoloni@mozilla.com>
Tue, 11 Oct 2016 12:54:01 -0400
changeset 424375 d6125e289d52673b15ec751d35094fe571914041
parent 424355 d26b211c3a2bb56e9254f4c1a97c253054c4cefa
child 533665 0ebbd043d1d769d88a4a0a556ebac4ad7114405a
push id32144
push userbmo:tchiovoloni@mozilla.com
push dateWed, 12 Oct 2016 18:13:31 +0000
reviewersmarkh
bugs1308841
milestone52.0a1
Bug 1308841 - Sanity check client-side bookmark roots in sync validator r?markh MozReview-Commit-ID: 6Y5qfCtVYWf
services/sync/modules/bookmark_validator.js
services/sync/tests/unit/test_bookmark_validator.js
--- 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();