Bug 1323869 - Validate bookmarks on first sync. r?markh draft
authorKit Cambridge <kit@yakshaving.ninja>
Tue, 07 Mar 2017 15:20:07 -0800
changeset 495482 f87772483781ae69050261bfaee4ffc08eb9014e
parent 494872 58753259bfeb3b818eac7870871b0aae1f8de64a
child 548392 f99e00f28ceaca0eb6959bf76349e32c13fdd393
push id48349
push userbmo:kit@mozilla.com
push dateWed, 08 Mar 2017 22:02:13 +0000
reviewersmarkh
bugs1323869
milestone55.0a1
Bug 1323869 - Validate bookmarks on first sync. r?markh This patch adds two preferences to control the percentage and maximum records for validation after the first sync. The default is 100% and 1k records, but we can experiment with the thresholds as needed. I'm not sure if we can reuse the downloaded records, or the tree we built for the GUID map. It's possible for bookmarks to change during the sync, and validation happens long after the engine has finished. OTOH, it's a shame that we duplicate the work, especially for the likely case where there are no changes. MozReview-Commit-ID: DSBQENpdCKm
services/sync/modules/doctor.js
services/sync/modules/stages/enginesync.js
services/sync/services-sync.js
services/sync/tests/unit/test_telemetry.js
--- a/services/sync/modules/doctor.js
+++ b/services/sync/modules/doctor.js
@@ -83,17 +83,17 @@ this.Doctor = {
       } finally {
         this.lastRepairAdvance = this._now();
       }
     }
   },
 
   _getEnginesToValidate(recentlySyncedEngines) {
     let result = {};
-    for (let e of recentlySyncedEngines) {
+    for (let { engine: e, firstSync } of recentlySyncedEngines) {
       let prefPrefix = `engine.${e.name}.`;
       if (!Svc.Prefs.get(prefPrefix + "validation.enabled", false)) {
         log.info(`Skipping check of ${e.name} - disabled via preferences`);
         continue;
       }
       // Check the last validation time for the engine.
       let lastValidation = Svc.Prefs.get(prefPrefix + "validation.lastTime", 0);
       let validationInterval = Svc.Prefs.get(prefPrefix + "validation.interval");
@@ -103,24 +103,31 @@ this.Doctor = {
         log.info(`Skipping validation of ${e.name}: too recent since last validation attempt`);
         continue;
       }
       // Update the time now, even if we decline to actually perform a
       // validation. We don't want to check the rest of these more frequently
       // than once a day.
       Svc.Prefs.set("validation.lastTime", Math.floor(nowSeconds));
 
-      // Validation only occurs a certain percentage of the time.
-      let validationProbability = Svc.Prefs.get(prefPrefix + "validation.percentageChance", 0) / 100.0;
+      // Validation only occurs a certain percentage of the time, and with a
+      // maximum record threshold. The limits are different for the first
+      // sync, since it's already expensive.
+      let thresholdPrefPrefix = prefPrefix + "validation.";
+      if (firstSync) {
+        thresholdPrefPrefix += "firstSync.";
+      }
+
+      let validationProbability = Svc.Prefs.get(thresholdPrefPrefix + "percentageChance", 0) / 100.0;
       if (validationProbability < Math.random()) {
         log.info(`Skipping validation of ${e.name}: Probability threshold not met`);
         continue;
       }
 
-      let maxRecords = Svc.Prefs.get(prefPrefix + "validation.maxRecords");
+      let maxRecords = Svc.Prefs.get(thresholdPrefPrefix + "maxRecords");
       if (!maxRecords) {
         log.info(`Skipping validation of ${e.name}: No maxRecords specified`);
         continue;
       }
       // OK, so this is a candidate - the final decision will be based on the
       // number of records actually found.
       result[e.name] = { engine: e, maxRecords };
     }
--- a/services/sync/modules/stages/enginesync.js
+++ b/services/sync/modules/stages/enginesync.js
@@ -160,22 +160,23 @@ EngineSynchronizer.prototype = {
     } else {
       this._log.info("Syncing all enabled engines.");
       enginesToSync = engineManager.getEnabled();
     }
     try {
       // We don't bother validating engines that failed to sync.
       let enginesToValidate = [];
       for (let engine of enginesToSync) {
+        let firstSync = engine.lastSyncLocal === 0;
         // If there's any problems with syncing the engine, report the failure
         if (!(this._syncEngine(engine)) || this.service.status.enforceBackoff) {
           this._log.info("Aborting sync for failure in " + engine.name);
           break;
         }
-        enginesToValidate.push(engine);
+        enginesToValidate.push({ engine, firstSync });
       }
 
       // If _syncEngine fails for a 401, we might not have a cluster URL here.
       // If that's the case, break out of this immediately, rather than
       // throwing an exception when trying to fetch metaURL.
       if (!this.service.clusterURL) {
         this._log.debug("Aborting sync, no cluster URL: " +
                         "not uploading new meta/global.");
@@ -242,32 +243,34 @@ EngineSynchronizer.prototype = {
   },
 
   _updateEnabledFromMeta(meta, numClients, engineManager = this.service.engineManager) {
     this._log.info("Updating enabled engines: " +
                     numClients + " clients.");
 
     if (meta.isNew || !meta.payload.engines) {
       this._log.debug("meta/global isn't new, or is missing engines. Not updating enabled state.");
+      Svc.Prefs.resetBranch("engineStatusChanged.");
       return;
     }
 
     // If we're the only client, and no engines are marked as enabled,
     // thumb our noses at the server data: it can't be right.
     // Belt-and-suspenders approach to Bug 615926.
     let hasEnabledEngines = false;
     for (let e in meta.payload.engines) {
       if (e != "clients") {
         hasEnabledEngines = true;
         break;
       }
     }
 
     if ((numClients <= 1) && !hasEnabledEngines) {
       this._log.info("One client and no enabled engines: not touching local engine status.");
+      Svc.Prefs.resetBranch("engineStatusChanged.");
       return;
     }
 
     this.service._ignorePrefObserver = true;
 
     let enabled = engineManager.enabledEngineNames;
 
     let toDecline = new Set();
--- a/services/sync/services-sync.js
+++ b/services/sync/services-sync.js
@@ -78,8 +78,11 @@ pref("services.sync.engine.bookmarks.val
 
 // 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.engine.bookmarks.validation.percentageChance", 10);
 
 // We won't validate an engine if it has more than this many records on the server.
 pref("services.sync.engine.bookmarks.validation.maxRecords", 1000);
+
+pref("services.sync.engine.bookmarks.validation.firstSync.percentageChance", 100);
+pref("services.sync.engine.bookmarks.validation.firstSync.maxRecords", 1000);
--- a/services/sync/tests/unit/test_telemetry.js
+++ b/services/sync/tests/unit/test_telemetry.js
@@ -62,19 +62,79 @@ BogusEngine.prototype = Object.create(St
 async function cleanAndGo(engine, server) {
   engine._tracker.clearChangedIDs();
   Svc.Prefs.resetBranch("");
   Svc.Prefs.set("log.logger.engine.rotary", "Trace");
   Service.recordManager.clearCache();
   await promiseStopServer(server);
 }
 
+function getEngineValidationData(ping, name) {
+  let engine = ping.engines.find(e => e.name == name);
+  return engine ? engine.validation : null;
+}
+
 // Avoid addon manager complaining about not being initialized
 Service.engineManager.unregister("addons");
 
+add_task(async function test_validate_on_first_sync() {
+  Svc.Prefs.set("engine.bookmarks.validation.enabled", true);
+  Svc.Prefs.set("engine.bookmarks.validation.firstSync.percentageChance", 100);
+  Svc.Prefs.set("engine.bookmarks.validation.firstSync.maxRecords", -1);
+  Svc.Prefs.set("engine.bookmarks.validation.percentageChance", 0);
+  Svc.Prefs.set("engine.bookmarks.validation.maxRecords", -1);
+
+  let engine = Service.engineManager.get("bookmarks");
+  let server = serverForUsers({ foo: "password" }, {
+    meta: {
+      global: {
+        engines: {
+          bookmarks: {
+            version: engine.version,
+            syncID: engine.syncID,
+          },
+        },
+      },
+    },
+  });
+  await SyncTestingInfrastructure(server);
+
+  try {
+    _("First sync; disabled engine");
+    engine.enabled = false;
+    {
+      let ping = await sync_and_validate_telem();
+      ok(!getEngineValidationData(ping, "bookmarks"),
+        "Should not validate disabled bookmarks engine");
+    }
+    ok(!Svc.Prefs.get("engineStatusChanged.bookmarks", false),
+      "Should reset status for disabled bookmarks engine after first sync");
+
+    _("First engine sync should have validation data; chance is 100%");
+    engine.enabled = true;
+    {
+      let ping = await sync_and_validate_telem();
+      ok(getEngineValidationData(ping, "bookmarks"),
+        "Should validate on first bookmarks sync");
+    }
+    ok(!Svc.Prefs.get("engineStatusChanged.bookmarks", false),
+      "Should reset status for enabled bookmarks engine after first engine sync");
+
+    _("Second engine sync should not have validation data; chance is 0%");
+    {
+      let ping = await sync_and_validate_telem();
+      ok(!getEngineValidationData(ping, "bookmarks"),
+        "Should not validate on second bookmarks sync");
+    }
+  } finally {
+    Service.startOver();
+    await promiseStopServer(server);
+  }
+});
+
 add_task(async function test_basic() {
   let helper = track_collections_helper();
   let upd = helper.with_updated_collection;
 
   let handlers = {
     "/1.1/johndoe/info/collections": helper.handler,
     "/1.1/johndoe/storage/crypto/keys": upd("crypto", new ServerWBO("keys").handler()),
     "/1.1/johndoe/storage/meta/global": upd("meta", new ServerWBO("global").handler())