Bug 1323867 - Bump bookmark validation threshold to 1k. r=tcsc draft
authorKit Cambridge <kit@yakshaving.ninja>
Thu, 12 Jan 2017 14:02:45 -0700
changeset 480058 3d26fb0b0501ac285a423eca9d7293265ef5e901
parent 479958 e677ba018b22558fef1d07b74d416fd3a28a5dc3
child 544865 e25cf301bb3273390072f527da532b45fa18db64
push id44447
push userbmo:kit@mozilla.com
push dateTue, 07 Feb 2017 20:02:37 +0000
reviewerstcsc
bugs1323867
milestone54.0a1
Bug 1323867 - Bump bookmark validation threshold to 1k. r=tcsc This patch also refactors `compareServerWithClient` and `inspectServerRecords` to periodically yield back to the main thread, since validating a large number of bookmarks in a tight loop causes jank. Yielding every 50 records for 50ms is somewhat arbitrary, but keeps things snappy for my profile (2947 records). MozReview-Commit-ID: 99DJtNvNeHI
services/sync/modules/bookmark_validator.js
services/sync/services-sync.js
services/sync/tests/unit/sync_ping_schema.json
services/sync/tests/unit/test_bookmark_validator.js
services/sync/tps/extensions/tps/resource/tps.jsm
--- a/services/sync/modules/bookmark_validator.js
+++ b/services/sync/modules/bookmark_validator.js
@@ -3,20 +3,21 @@
  * You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 "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/Services.jsm");
 Cu.import("resource://gre/modules/Task.jsm");
+Cu.import("resource://gre/modules/Timer.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) {
   return treeNode.annos && treeNode.annos.some(anno => anno.name == LEFT_PANE_ROOT_ANNO ||
@@ -209,23 +210,28 @@ class BookmarkValidator {
       let concreteItem = recordMap.get(concreteId);
       if (!concreteItem) {
         continue;
       }
       entry.concrete = concreteItem;
     }
   }
 
-  createClientRecordsFromTree(clientTree) {
+  async createClientRecordsFromTree(clientTree) {
     // Iterate over the treeNode, converting it to something more similar to what
     // the server stores.
     let records = [];
     let recordsByGuid = new Map();
     let syncedRoots = SYNCED_ROOTS;
-    function traverse(treeNode, synced) {
+    let yieldCounter = 0;
+    let self = this;
+    async function traverse(treeNode, synced) {
+      if (++yieldCounter % 50 === 0) {
+        await new Promise(resolve => setTimeout(resolve, 50));
+      }
       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 = !synced;
@@ -275,24 +281,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, synced);
+          await traverse(child, synced);
           child.parent = treeNode;
           child.parentid = guid;
           treeNode.childGUIDs.push(child.guid);
         }
       }
     }
-    traverse(clientTree, false);
+    await 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
@@ -313,27 +319,28 @@ class BookmarkValidator {
    * - root: Root of the server-side bookmark tree. Has the same properties as
    *   above.
    * - deletedRecords: As above, but only contains items that the server sent
    *   where it also sent indication that the item should be deleted.
    * - problemData: a BookmarkProblemData object, with the caveat that
    *   the fields describing client/server relationship will not have been filled
    *   out yet.
    */
-  inspectServerRecords(serverRecords) {
+  async inspectServerRecords(serverRecords) {
     let deletedItemIds = new Set();
     let idToRecord = new Map();
     let deletedRecords = [];
 
     let folders = [];
 
     let problemData = new BookmarkProblemData();
 
     let resultRecords = [];
 
+    let yieldCounter = 0;
     for (let record of serverRecords) {
       if (!record.id) {
         ++problemData.missingIDs;
         continue;
       }
       if (record.deleted) {
         deletedItemIds.add(record.id);
       } else if (idToRecord.has(record.id)) {
@@ -362,16 +369,19 @@ class BookmarkValidator {
         // The children array stores special guids as their local guid values,
         // e.g. 'menu________' instead of 'menu', but all other parts of the
         // serverside bookmark info stores it as the special value ('menu').
         record.childGUIDs = record.children;
         record.children = record.children.map(childID => {
           return PlacesSyncUtils.bookmarks.guidToSyncId(childID);
         });
       }
+      if (++yieldCounter % 50 === 0) {
+        await new Promise(resolve => setTimeout(resolve, 50));
+      }
     }
 
     for (let deletedId of deletedItemIds) {
       let record = idToRecord.get(deletedId);
       if (record && !record.isDeleted) {
         deletedRecords.push(record);
         record.isDeleted = true;
       }
@@ -602,20 +612,20 @@ class BookmarkValidator {
    *
    * 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
    *   will be filled out.
    */
-  compareServerWithClient(serverRecords, clientTree) {
+  async compareServerWithClient(serverRecords, clientTree) {
 
-    let clientRecords = this.createClientRecordsFromTree(clientTree);
-    let inspectionInfo = this.inspectServerRecords(serverRecords);
+    let clientRecords = await this.createClientRecordsFromTree(clientTree);
+    let inspectionInfo = await this.inspectServerRecords(serverRecords);
     inspectionInfo.clientRecords = clientRecords;
 
     // Mainly do this to remove deleted items and normalize child guids.
     serverRecords = inspectionInfo.records;
     let problemData = inspectionInfo.problemData;
 
     this._validateClient(problemData, clientRecords);
 
@@ -747,33 +757,30 @@ class BookmarkValidator {
     };
     let resp = collection.getBatched();
     if (!resp.success) {
       throw resp;
     }
     return items;
   }
 
-  validate(engine) {
-    let self = this;
-    return Task.spawn(function*() {
-      let start = Date.now();
-      let clientTree = yield PlacesUtils.promiseBookmarksTree("", {
-        includeItemIds: true
-      });
-      let serverState = self._getServerState(engine);
-      let serverRecordCount = serverState.length;
-      let result = self.compareServerWithClient(serverState, clientTree);
-      let end = Date.now();
-      let duration = end - start;
-      return {
-        duration,
-        version: self.version,
-        problems: result.problemData,
-        recordCount: serverRecordCount
-      };
+  async validate(engine) {
+    let start = Date.now();
+    let clientTree = await PlacesUtils.promiseBookmarksTree("", {
+      includeItemIds: true
     });
+    let serverState = this._getServerState(engine);
+    let serverRecordCount = serverState.length;
+    let result = await this.compareServerWithClient(serverState, clientTree);
+    let end = Date.now();
+    let duration = end - start;
+    return {
+      duration,
+      version: this.version,
+      problems: result.problemData,
+      recordCount: serverRecordCount
+    };
   }
 
 }
 
 BookmarkValidator.prototype.version = BOOKMARK_VALIDATOR_VERSION;
 
--- a/services/sync/services-sync.js
+++ b/services/sync/services-sync.js
@@ -72,9 +72,9 @@ pref("services.sync.telemetry.maxPayload
 pref("services.sync.validation.interval", 86400); // 24 hours in seconds
 
 // We only run validation `services.sync.validation.percentageChance` percent of
 // the time, even if it's been the right amount of time since the last validation,
 // and you meet the maxRecord checks.
 pref("services.sync.validation.percentageChance", 10);
 
 // We won't validate an engine if it has more than this many records on the server.
-pref("services.sync.validation.maxRecords", 100);
+pref("services.sync.validation.maxRecords", 1000);
--- a/services/sync/tests/unit/sync_ping_schema.json
+++ b/services/sync/tests/unit/sync_ping_schema.json
@@ -71,17 +71,17 @@
         "version": { "type": "string" }
       }
     },
     "engine": {
       "required": ["name"],
       "additionalProperties": false,
       "properties": {
         "failureReason": { "$ref": "#/definitions/error" },
-        "name": { "enum": ["addons", "bookmarks", "clients", "forms", "history", "passwords", "prefs", "tabs"] },
+        "name": { "enum": ["addons", "bookmarks", "clients", "forms", "history", "passwords", "prefs", "tabs", "extension-storage"] },
         "took": { "type": "integer", "minimum": 1 },
         "status": { "type": "string" },
         "incoming": {
           "type": "object",
           "additionalProperties": false,
           "anyOf": [
             {"required": ["applied"]},
             {"required": ["failed"]},
--- a/services/sync/tests/unit/test_bookmark_validator.js
+++ b/services/sync/tests/unit/test_bookmark_validator.js
@@ -1,125 +1,121 @@
 /* Any copyright is dedicated to the Public Domain.
    http://creativecommons.org/publicdomain/zero/1.0/ */
 
 Components.utils.import("resource://services-sync/bookmark_validator.js");
 Components.utils.import("resource://services-sync/util.js");
 
-function inspectServerRecords(data) {
-  return new BookmarkValidator().inspectServerRecords(data);
+async function inspectServerRecords(data) {
+  let validator = new BookmarkValidator();
+  return validator.inspectServerRecords(data);
 }
 
-add_test(function test_isr_rootOnServer() {
-  let c = inspectServerRecords([{
+async function compareServerWithClient(server, client) {
+  let validator = new BookmarkValidator();
+  return validator.compareServerWithClient(server, client);
+}
+
+add_task(async function test_isr_rootOnServer() {
+  let c = await inspectServerRecords([{
     id: "places",
     type: "folder",
     children: [],
   }]);
   ok(c.problemData.rootOnServer);
-  run_next_test();
 });
 
-add_test(function test_isr_empty() {
-  let c = inspectServerRecords([]);
+add_task(async function test_isr_empty() {
+  let c = await inspectServerRecords([]);
   ok(!c.problemData.rootOnServer);
   notEqual(c.root, null);
-  run_next_test();
 });
 
-add_test(function test_isr_cycles() {
-  let c = inspectServerRecords([
+add_task(async function test_isr_cycles() {
+  let c = (await inspectServerRecords([
     {id: "C", type: "folder", children: ["A", "B"], parentid: "places"},
     {id: "A", type: "folder", children: ["B"], parentid: "B"},
     {id: "B", type: "folder", children: ["A"], parentid: "A"},
-  ]).problemData;
+  ])).problemData;
 
   equal(c.cycles.length, 1);
   ok(c.cycles[0].indexOf("A") >= 0);
   ok(c.cycles[0].indexOf("B") >= 0);
-  run_next_test();
 });
 
-add_test(function test_isr_orphansMultiParents() {
-  let c = inspectServerRecords([
+add_task(async function test_isr_orphansMultiParents() {
+  let c = (await inspectServerRecords([
     { id: "A", type: "bookmark", parentid: "D" },
     { id: "B", type: "folder", parentid: "places", children: ["A"]},
     { id: "C", type: "folder", parentid: "places", children: ["A"]},
 
-  ]).problemData;
+  ])).problemData;
   deepEqual(c.orphans, [{ id: "A", parent: "D" }]);
   equal(c.multipleParents.length, 1)
   ok(c.multipleParents[0].parents.indexOf("B") >= 0);
   ok(c.multipleParents[0].parents.indexOf("C") >= 0);
-  run_next_test();
 });
 
-add_test(function test_isr_orphansMultiParents2() {
-  let c = inspectServerRecords([
+add_task(async function test_isr_orphansMultiParents2() {
+  let c = (await inspectServerRecords([
     { id: "A", type: "bookmark", parentid: "D" },
     { id: "B", type: "folder", parentid: "places", children: ["A"]},
-  ]).problemData;
+  ])).problemData;
   equal(c.orphans.length, 1);
   equal(c.orphans[0].id, "A");
   equal(c.multipleParents.length, 0);
-  run_next_test();
 });
 
-add_test(function test_isr_deletedParents() {
-  let c = inspectServerRecords([
+add_task(async function test_isr_deletedParents() {
+  let c = (await inspectServerRecords([
     { id: "A", type: "bookmark", parentid: "B" },
     { id: "B", type: "folder", parentid: "places", children: ["A"]},
     { id: "B", type: "item", deleted: true},
-  ]).problemData;
-  deepEqual(c.deletedParents, ["A"])
-  run_next_test();
+  ])).problemData;
+  deepEqual(c.deletedParents, ["A"]);
 });
 
-add_test(function test_isr_badChildren() {
-  let c = inspectServerRecords([
+add_task(async function test_isr_badChildren() {
+  let c = (await inspectServerRecords([
     { id: "A", type: "bookmark", parentid: "places", children: ["B", "C"] },
     { id: "C", type: "bookmark", parentid: "A" }
-  ]).problemData;
+  ])).problemData;
   deepEqual(c.childrenOnNonFolder, ["A"])
   deepEqual(c.missingChildren, [{parent: "A", child: "B"}]);
   deepEqual(c.parentNotFolder, ["C"]);
-  run_next_test();
 });
 
 
-add_test(function test_isr_parentChildMismatches() {
-  let c = inspectServerRecords([
+add_task(async function test_isr_parentChildMismatches() {
+  let c = (await inspectServerRecords([
     { id: "A", type: "folder", parentid: "places", children: [] },
     { id: "B", type: "bookmark", parentid: "A" }
-  ]).problemData;
+  ])).problemData;
   deepEqual(c.parentChildMismatches, [{parent: "A", child: "B"}]);
-  run_next_test();
 });
 
-add_test(function test_isr_duplicatesAndMissingIDs() {
-  let c = inspectServerRecords([
+add_task(async function test_isr_duplicatesAndMissingIDs() {
+  let c = (await inspectServerRecords([
     {id: "A", type: "folder", parentid: "places", children: []},
     {id: "A", type: "folder", parentid: "places", children: []},
     {type: "folder", parentid: "places", children: []}
-  ]).problemData;
+  ])).problemData;
   equal(c.missingIDs, 1);
   deepEqual(c.duplicates, ["A"]);
-  run_next_test();
 });
 
-add_test(function test_isr_duplicateChildren() {
-  let c = inspectServerRecords([
+add_task(async function test_isr_duplicateChildren()  {
+  let c = (await inspectServerRecords([
     {id: "A", type: "folder", parentid: "places", children: ["B", "B"]},
     {id: "B", type: "bookmark", parentid: "A"},
-  ]).problemData;
+  ])).problemData;
   deepEqual(c.duplicateChildren, ["A"]);
-  run_next_test();
 });
 
-// Each compareServerWithClient test mutates these, so we can't just keep them
+// Each compareServerWithClient test mutates these, so we can"t just keep them
 // global
 function getDummyServerAndClient() {
   let server = [
     {
       id: "menu",
       parentid: "places",
       type: "folder",
       parentName: "",
@@ -179,72 +175,68 @@ function getDummyServerAndClient() {
         ]
       }
     ]
   };
   return {server, client};
 }
 
 
-add_test(function test_cswc_valid() {
+add_task(async function test_cswc_valid() {
   let {server, client} = getDummyServerAndClient();
 
-  let c = new BookmarkValidator().compareServerWithClient(server, client).problemData;
+  let c = (await compareServerWithClient(server, client)).problemData;
   equal(c.clientMissing.length, 0);
   equal(c.serverMissing.length, 0);
   equal(c.differences.length, 0);
-  run_next_test();
 });
 
-add_test(function test_cswc_serverMissing() {
+add_task(async function test_cswc_serverMissing() {
   let {server, client} = getDummyServerAndClient();
   // remove c
   server.pop();
   server[0].children.pop();
 
-  let c = new BookmarkValidator().compareServerWithClient(server, client).problemData;
+  let c = (await compareServerWithClient(server, client)).problemData;
   deepEqual(c.serverMissing, ["cccccccccccc"]);
   equal(c.clientMissing.length, 0);
   deepEqual(c.structuralDifferences, [{id: "menu", differences: ["childGUIDs"]}]);
-  run_next_test();
 });
 
-add_test(function test_cswc_clientMissing() {
+add_task(async function test_cswc_clientMissing() {
   let {server, client} = getDummyServerAndClient();
   client.children[0].children.pop();
 
-  let c = new BookmarkValidator().compareServerWithClient(server, client).problemData;
+  let c = (await compareServerWithClient(server, client)).problemData;
   deepEqual(c.clientMissing, ["cccccccccccc"]);
   equal(c.serverMissing.length, 0);
   deepEqual(c.structuralDifferences, [{id: "menu", differences: ["childGUIDs"]}]);
-  run_next_test();
 });
 
-add_test(function test_cswc_differences() {
+add_task(async function test_cswc_differences() {
   {
     let {server, client} = getDummyServerAndClient();
     client.children[0].children[0].title = "asdf";
-    let c = new BookmarkValidator().compareServerWithClient(server, client).problemData;
+    let c = (await compareServerWithClient(server, client)).problemData;
     equal(c.clientMissing.length, 0);
     equal(c.serverMissing.length, 0);
     deepEqual(c.differences, [{id: "bbbbbbbbbbbb", differences: ["title"]}]);
   }
 
   {
     let {server, client} = getDummyServerAndClient();
     server[2].type = "bookmark";
-    let c = new BookmarkValidator().compareServerWithClient(server, client).problemData;
+    let c = (await compareServerWithClient(server, client)).problemData;
     equal(c.clientMissing.length, 0);
     equal(c.serverMissing.length, 0);
     deepEqual(c.differences, [{id: "cccccccccccc", differences: ["type"]}]);
   }
-  run_next_test();
 });
 
-add_test(function test_cswc_serverUnexpected() {
+add_task(async function test_cswc_serverUnexpected() {
   let {server, client} = getDummyServerAndClient();
   client.children.push({
     "guid": "dddddddddddd",
     "title": "",
     "id": 2000,
     "annos": [{
       "name": "places/excludeFromBackup",
       "flags": 0,
@@ -286,41 +278,41 @@ add_test(function test_cswc_serverUnexpe
     id: "eeeeeeeeeeee",
     parentid: "dddddddddddd",
     parentName: "",
     title: "History",
     type: "query",
     bmkUri: "place:type=3&sort=4"
   });
 
-  let c = new BookmarkValidator().compareServerWithClient(server, client).problemData;
+  let c = (await compareServerWithClient(server, client)).problemData;
   equal(c.clientMissing.length, 0);
   equal(c.serverMissing.length, 0);
   equal(c.serverUnexpected.length, 2);
   deepEqual(c.serverUnexpected, ["dddddddddddd", "eeeeeeeeeeee"]);
-  run_next_test();
 });
 
-function validationPing(server, client, duration) {
-  return wait_for_ping(function() {
-    // fake this entirely
-    Svc.Obs.notify("weave:service:sync:start");
-    Svc.Obs.notify("weave:engine:sync:start", null, "bookmarks");
-    Svc.Obs.notify("weave:engine:sync:finish", null, "bookmarks");
-    let validator = new BookmarkValidator();
-    let data = {
-      // We fake duration and version just so that we can verify they're passed through.
-      duration,
-      version: validator.version,
-      recordCount: server.length,
-      problems: validator.compareServerWithClient(server, client).problemData,
-    };
-    Svc.Obs.notify("weave:engine:validate:finish", data, "bookmarks");
-    Svc.Obs.notify("weave:service:sync:finish");
-  }, true); // Allow "failing" pings, since having validation info indicates failure.
+async function validationPing(server, client, duration) {
+  let pingPromise = wait_for_ping(() => {}, true); // Allow "failing" pings, since having validation info indicates failure.
+  // fake this entirely
+  Svc.Obs.notify("weave:service:sync:start");
+  Svc.Obs.notify("weave:engine:sync:start", null, "bookmarks");
+  Svc.Obs.notify("weave:engine:sync:finish", null, "bookmarks");
+  let validator = new BookmarkValidator();
+  let {problemData} = await validator.compareServerWithClient(server, client);
+  let data = {
+    // We fake duration and version just so that we can verify they"re passed through.
+    duration,
+    version: validator.version,
+    recordCount: server.length,
+    problems: problemData,
+  };
+  Svc.Obs.notify("weave:engine:validate:finish", data, "bookmarks");
+  Svc.Obs.notify("weave:service:sync:finish");
+  return pingPromise;
 }
 
 add_task(async function test_telemetry_integration() {
   let {server, client} = getDummyServerAndClient();
   // remove "c"
   server.pop();
   server[0].children.pop();
   const duration = 50;
@@ -336,12 +328,8 @@ add_task(async function test_telemetry_i
   equal(bme.validation.version, new BookmarkValidator().version);
   deepEqual(bme.validation.problems, [
     { name: "badClientRoots", count: 3 },
     { name: "sdiff:childGUIDs", count: 1 },
     { name: "serverMissing", count: 1 },
     { name: "structuralDifferences", count: 1 },
   ]);
 });
-
-function run_test() {
-  run_next_test();
-}
--- a/services/sync/tps/extensions/tps/resource/tps.jsm
+++ b/services/sync/tps/extensions/tps/resource/tps.jsm
@@ -130,19 +130,16 @@ var TPS = {
 
   _init: function TPS__init() {
     this.delayAutoSync();
 
     OBSERVER_TOPICS.forEach(function(aTopic) {
       Services.obs.addObserver(this, aTopic, true);
     }, this);
 
-    // Configure some logging prefs for Sync itself.
-    Weave.Svc.Prefs.set("log.appender.dump", "Debug");
-
     /* global Authentication */
     Cu.import("resource://tps/auth/fxaccounts.jsm", module);
   },
 
   DumpError(msg, exc = null) {
     this._errors++;
     let errInfo;
     if (exc) {
@@ -619,17 +616,17 @@ var TPS = {
       let clientTree = Async.promiseSpinningly(PlacesUtils.promiseBookmarksTree("", {
         includeItemIds: true
       }));
       let serverRecords = getServerBookmarkState();
       // We can't wait until catch to stringify this, since at that point it will have cycles.
       serverRecordDumpStr = JSON.stringify(serverRecords);
 
       let validator = new BookmarkValidator();
-      let {problemData} = validator.compareServerWithClient(serverRecords, clientTree);
+      let {problemData} = Async.promiseSpinningly(validator.compareServerWithClient(serverRecords, clientTree));
 
       for (let {name, count} of problemData.getSummary()) {
         // Exclude mobile showing up on the server hackily so that we don't
         // report it every time, see bug 1273234 and 1274394 for more information.
         if (name === "serverUnexpected" && problemData.serverUnexpected.indexOf("mobile") >= 0) {
           --count;
         }
         if (count) {