Bug 1317223 (part 2) - add 'doctor' concept and move bookmark validation logic to it. r=tcsc
☠☠ backed out by 04f6663f94cb ☠ ☠
authorMark Hammond <mhammond@skippinet.com.au>
Thu, 02 Mar 2017 17:14:31 +1100
changeset 345406 705939ae18f00dd3ccba81fc4e77acefdb51561f
parent 345405 091bb2d8a1d93dce909f0ab3ff085ad6df9993e2
child 345407 b7efe4ba02c25a3b04fa0a84bd02d1b4ba511947
push id87597
push usermhammond@skippinet.com.au
push dateThu, 02 Mar 2017 08:05:16 +0000
treeherdermozilla-inbound@6878846abd90 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerstcsc
bugs1317223
milestone54.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1317223 (part 2) - add 'doctor' concept and move bookmark validation logic to it. r=tcsc This patch defines the concept of a "doctor" for collections. The doctor is responsible for running all validators and deciding whether or not to initiate a repair request based on the validation results. MozReview-Commit-ID: 6NLRE6L0OpA
.eslintignore
browser/app/profile/firefox.js
services/sync/modules/bookmark_validator.js
services/sync/modules/doctor.js
services/sync/modules/stages/enginesync.js
services/sync/moz.build
services/sync/services-sync.js
services/sync/tests/unit/head_appinfo.js
services/sync/tests/unit/test_doctor.js
services/sync/tests/unit/xpcshell.ini
tools/lint/eslint/modules.json
--- a/.eslintignore
+++ b/.eslintignore
@@ -284,16 +284,17 @@ security/manager/ssl/security-prefs.js
 
 #NSS
 security/nss/**
 
 # services/ exclusions
 
 # Uses `#filter substitution`
 services/sync/modules/constants.js
+services/sync/services-sync.js
 
 # Third party services
 services/common/kinto-http-client.js
 services/common/kinto-offline-client.js
 services/sync/tps/extensions/mozmill
 
 # toolkit/ exclusions
 
--- a/browser/app/profile/firefox.js
+++ b/browser/app/profile/firefox.js
@@ -1576,22 +1576,16 @@ pref("browser.crashReports.unsubmittedCh
 
 // chancesUntilSuppress is how many times we'll show the unsubmitted
 // crash report notification across different days and shutdown
 // without a user choice before we suppress the notification for
 // some number of days.
 pref("browser.crashReports.unsubmittedCheck.chancesUntilSuppress", 4);
 pref("browser.crashReports.unsubmittedCheck.autoSubmit", false);
 
-#ifdef NIGHTLY_BUILD
-// Enable the (fairly costly) client/server validation on nightly only. The other prefs
-// controlling validation are located in /services/sync/services-sync.js
-pref("services.sync.validation.enabled", true);
-#endif
-
 // Preferences for the form autofill system extension
 pref("browser.formautofill.experimental", false);
 pref("browser.formautofill.enabled", false);
 pref("browser.formautofill.loglevel", "Warn");
 
 // Enable safebrowsing v4 tables (suffixed by "-proto") update.
 #ifdef NIGHTLY_BUILD
 pref("urlclassifier.malwareTable", "goog-malware-shavar,goog-unwanted-shavar,goog-malware-proto,goog-unwanted-proto,test-malware-simple,test-unwanted-simple");
--- a/services/sync/modules/bookmark_validator.js
+++ b/services/sync/modules/bookmark_validator.js
@@ -802,16 +802,18 @@ class BookmarkValidator {
       if (structuralDifferences.length) {
         problemData.structuralDifferences.push({ id, differences: structuralDifferences });
       }
     }
     return inspectionInfo;
   }
 
   _getServerState(engine) {
+// XXXXX - todo - we need to capture last-modified of the server here and
+// ensure the repairer only applys with if-unmodified-since that date.
     let collection = engine.itemSource();
     let collectionKey = engine.service.collectionKeys.keyForCollection(engine.name);
     collection.full = true;
     let items = [];
     collection.recordHandler = function(item) {
       item.decrypt(collectionKey);
       items.push(item.cleartext);
     };
new file mode 100644
--- /dev/null
+++ b/services/sync/modules/doctor.js
@@ -0,0 +1,250 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+// A doctor for our collections. She can be asked to make a consultation, and
+// may just diagnose an issue without attempting to cure it, may diagnose and
+// attempt to cure, or may decide she is overworked and underpaid.
+// Or something - naming is hard :)
+
+"use strict";
+
+this.EXPORTED_SYMBOLS = ["Doctor"];
+
+const Cu = Components.utils;
+
+Cu.import("resource://gre/modules/Services.jsm");
+Cu.import("resource://gre/modules/Log.jsm");
+Cu.import("resource://services-common/async.js");
+Cu.import("resource://services-common/observers.js");
+Cu.import("resource://services-sync/service.js");
+Cu.import("resource://services-sync/resource.js");
+
+Cu.import("resource://services-sync/util.js");
+XPCOMUtils.defineLazyModuleGetter(this, "getRepairRequestor",
+  "resource://services-sync/collection_repair.js");
+XPCOMUtils.defineLazyModuleGetter(this, "getAllRepairRequestors",
+  "resource://services-sync/collection_repair.js");
+
+const log = Log.repository.getLogger("Sync.Doctor");
+
+this.REPAIR_ADVANCE_PERIOD = 86400; // 1 day
+
+this.Doctor = {
+  anyClientsRepairing(service, collection, ignoreFlowID = null) {
+    if (!service || !service.clientsEngine) {
+      log.info("Missing clients engine, assuming we're in test code");
+      return false;
+    }
+    return service.clientsEngine.remoteClients.some(client =>
+      client.commands && client.commands.some(command => {
+        if (command.command != "repairResponse" && command.command != "repairRequest") {
+          return false;
+        }
+        if (!command.args || command.args.length != 1) {
+          return false;
+        }
+        if (command.args[0].collection != collection) {
+          return false;
+        }
+        if (ignoreFlowID != null && command.args[0].flowID == ignoreFlowID) {
+          return false;
+        }
+        return true;
+      })
+    );
+  },
+
+  async consult(recentlySyncedEngines) {
+    if (!Services.telemetry.canRecordBase) {
+      log.info("Skipping consultation: telemetry reporting is disabled");
+      return;
+    }
+
+    let engineInfos = this._getEnginesToValidate(recentlySyncedEngines);
+
+    await this._runValidators(engineInfos);
+
+    // We are called at the end of a sync, which is a good time to periodically
+    // check each repairer to see if it can advance.
+    if (this._now() - this.lastRepairAdvance > REPAIR_ADVANCE_PERIOD) {
+      try {
+        for (let [collection, requestor] of Object.entries(this._getAllRepairRequestors())) {
+          try {
+            let advanced = requestor.continueRepairs();
+            log.info(`${collection} reparier ${advanced ? "advanced" : "did not advance"}.`);
+          } catch (ex) {
+            if (Async.isShutdownException(ex)) {
+              throw ex;
+            }
+            log.error(`${collection} repairer failed`, ex);
+          }
+        }
+      } finally {
+        this.lastRepairAdvance = this._now();
+      }
+    }
+  },
+
+  _getEnginesToValidate(recentlySyncedEngines) {
+    let result = {};
+    for (let e 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");
+      let nowSeconds = this._now();
+
+      if (nowSeconds - lastValidation < validationInterval) {
+        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;
+      if (validationProbability < Math.random()) {
+        log.info(`Skipping validation of ${e.name}: Probability threshold not met`);
+        continue;
+      }
+
+      let maxRecords = Svc.Prefs.get(prefPrefix + "validation.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 };
+    }
+    return result;
+  },
+
+  async _runValidators(engineInfos) {
+    if (Object.keys(engineInfos).length == 0) {
+      log.info("Skipping validation: no engines qualify");
+      return;
+    }
+
+    if (Object.values(engineInfos).filter(i => i.maxRecords != -1).length != 0) {
+      // at least some of the engines have maxRecord restrictions which require
+      // us to ask the server for the counts.
+      let countInfo = this._fetchCollectionCounts();
+      for (let [engineName, recordCount] of Object.entries(countInfo)) {
+        if (engineName in engineInfos) {
+          engineInfos[engineName].recordCount = recordCount;
+        }
+      }
+    }
+
+    for (let [engineName, { engine, maxRecords, recordCount }] of Object.entries(engineInfos)) {
+      // maxRecords of -1 means "any number", so we can skip asking the server.
+      // Used for tests.
+      if (maxRecords >= 0 && recordCount > maxRecords) {
+        log.debug(`Skipping validation for ${engineName} because ` +
+                        `the number of records (${recordCount}) is greater ` +
+                        `than the maximum allowed (${maxRecords}).`);
+        continue;
+      }
+      let validator = engine.getValidator();
+      if (!validator) {
+        continue;
+      }
+
+      // Let's do it!
+      Services.console.logStringMessage(
+        `Sync is about to run a consistency check of ${engine.name}. This may be slow, and ` +
+        `can be controlled using the pref "services.sync.${engine.name}.validation.enabled".\n` +
+        `If you encounter any problems because of this, please file a bug.`);
+
+      // Make a new flowID just incase we end up starting a repair.
+      let flowID = Utils.makeGUID();
+      try {
+        // XXX - must get the flow ID to either the validator, or directly to
+        // telemetry. I guess it's probably OK to always record a flowID even
+        // if we don't end up repairing?
+        log.info(`Running validator for ${engine.name}`);
+        let result = await validator.validate(engine);
+        Observers.notify("weave:engine:validate:finish", result, engine.name);
+        // And see if we should repair.
+        await this._maybeCure(engine, result, flowID);
+      } catch (ex) {
+        if (Async.isShutdownException(ex)) {
+          throw ex;
+        }
+        log.error(`Failed to run validation on ${engine.name}!`, ex);
+        Observers.notify("weave:engine:validate:error", ex, engine.name)
+        // Keep validating -- there's no reason to think that a failure for one
+        // validator would mean the others will fail.
+      }
+    }
+  },
+
+  _maybeCure(engine, validationResults, flowID) {
+    if (!this._shouldRepair(engine)) {
+      log.info(`Skipping repair of ${engine.name} - disabled via preferences`);
+      return;
+    }
+
+    let requestor = this._getRepairRequestor(engine.name);
+    let didStart = false;
+    if (requestor) {
+      didStart = requestor.startRepairs(validationResults, flowID);
+    }
+    log.info(`${didStart ? "did" : "didn't"} start a repair of ${engine.name} with flowID ${flowID}`);
+  },
+
+  _shouldRepair(engine) {
+    return Svc.Prefs.get(`engine.${engine.name}.repair.enabled`, false);
+  },
+
+  // mainly for mocking.
+  _fetchCollectionCounts() {
+    let collectionCountsURL = Service.userBaseURL + "info/collection_counts";
+    try {
+      let infoResp = Service._fetchInfo(collectionCountsURL);
+      if (!infoResp.success) {
+        log.error("Can't fetch collection counts: request to info/collection_counts responded with "
+                        + infoResp.status);
+        return {};
+      }
+      return infoResp.obj; // might throw because obj is a getter which parses json.
+    } catch (ex) {
+      if (Async.isShutdownException(ex)) {
+        throw ex;
+      }
+      // Not running validation is totally fine, so we just write an error log and return.
+      log.error("Caught error when fetching counts", ex);
+      return {};
+    }
+  },
+
+  get lastRepairAdvance() {
+    return Svc.Prefs.get("doctor.lastRepairAdvance", 0);
+  },
+
+  set lastRepairAdvance(value) {
+    Svc.Prefs.set("doctor.lastRepairAdvance", value);
+  },
+
+  // functions used so tests can mock them
+  _now() {
+    // We use the server time, which is SECONDS
+    return AsyncResource.serverTime;
+  },
+
+  _getRepairRequestor(name) {
+    return getRepairRequestor(name);
+  },
+
+  _getAllRepairRequestors() {
+    return getAllRepairRequestors();
+  }
+}
--- a/services/sync/modules/stages/enginesync.js
+++ b/services/sync/modules/stages/enginesync.js
@@ -13,16 +13,18 @@ var {utils: Cu} = Components;
 Cu.import("resource://gre/modules/Log.jsm");
 Cu.import("resource://services-sync/constants.js");
 Cu.import("resource://services-sync/engines.js");
 Cu.import("resource://services-sync/policies.js");
 Cu.import("resource://services-sync/util.js");
 Cu.import("resource://services-common/observers.js");
 Cu.import("resource://services-common/async.js");
 Cu.import("resource://gre/modules/Task.jsm");
+XPCOMUtils.defineLazyModuleGetter(this, "Doctor",
+                                  "resource://services-sync/doctor.js");
 
 /**
  * Perform synchronization of engines.
  *
  * This was originally split out of service.js. The API needs lots of love.
  */
 this.EngineSynchronizer = function EngineSynchronizer(service) {
   this._log = Log.repository.getLogger("Sync.Synchronizer");
@@ -189,17 +191,17 @@ EngineSynchronizer.prototype = {
           this.service.uploadMetaGlobal(meta);
           delete meta.isNew;
           delete meta.changed;
         } catch (error) {
           this._log.error("Unable to upload meta/global. Leaving marked as new.");
         }
       }
 
-      Async.promiseSpinningly(this._tryValidateEngines(enginesToValidate));
+      Async.promiseSpinningly(Doctor.consult(enginesToValidate));
 
       // If there were no sync engine failures
       if (this.service.status.service != SYNC_FAILED_PARTIAL) {
         Svc.Prefs.set("lastSync", new Date().toString());
         this.service.status.sync = SYNC_SUCCEEDED;
       }
     } finally {
       Svc.Prefs.reset("firstSync");
@@ -208,116 +210,16 @@ EngineSynchronizer.prototype = {
       let dateStr = Utils.formatTimestamp(new Date());
       this._log.info("Sync completed at " + dateStr
                      + " after " + syncTime + " secs.");
     }
 
     this.onComplete(null);
   },
 
-  _tryValidateEngines: Task.async(function* (recentlySyncedEngines) {
-    if (!Services.telemetry.canRecordBase || !Svc.Prefs.get("validation.enabled", false)) {
-      this._log.info("Skipping validation: validation or telemetry reporting is disabled");
-      return;
-    }
-
-    let lastValidation = Svc.Prefs.get("validation.lastTime", 0);
-    let validationInterval = Svc.Prefs.get("validation.interval");
-    let nowSeconds = Math.floor(Date.now() / 1000);
-
-    if (nowSeconds - lastValidation < validationInterval) {
-      this._log.info("Skipping validation: too recent since last validation attempt");
-      return;
-    }
-    // Update the time now, even if we may return false still. We don't want to
-    // check the rest of these more frequently than once a day.
-    Svc.Prefs.set("validation.lastTime", nowSeconds);
-
-    // Validation only occurs a certain percentage of the time.
-    let validationProbability = Svc.Prefs.get("validation.percentageChance", 0) / 100.0;
-    if (validationProbability < Math.random()) {
-      this._log.info("Skipping validation: Probability threshold not met");
-      return;
-    }
-    let maxRecords = Svc.Prefs.get("validation.maxRecords");
-    if (!maxRecords) {
-      // Don't bother asking the server for the counts if we know validation
-      // won't happen anyway.
-      return;
-    }
-
-    // maxRecords of -1 means "any number", so we can skip asking the server.
-    // Used for tests.
-    let info;
-    if (maxRecords < 0) {
-      info = {};
-      for (let e of recentlySyncedEngines) {
-        info[e.name] = 1; // needs to be < maxRecords
-      }
-      maxRecords = 2;
-    } else {
-
-      let collectionCountsURL = this.service.userBaseURL + "info/collection_counts";
-      try {
-        let infoResp = this.service._fetchInfo(collectionCountsURL);
-        if (!infoResp.success) {
-          this._log.error("Can't run validation: request to info/collection_counts responded with "
-                          + infoResp.status);
-          return;
-        }
-        info = infoResp.obj; // might throw because obj is a getter which parses json.
-      } catch (e) {
-        // Not running validation is totally fine, so we just write an error log and return.
-        this._log.error("Can't run validation: Caught error when fetching counts", e);
-        return;
-      }
-    }
-
-    if (!info) {
-      return;
-    }
-
-    let engineLookup = new Map(recentlySyncedEngines.map(e => [e.name, e]));
-    let toRun = [];
-    for (let [engineName, recordCount] of Object.entries(info)) {
-      let engine = engineLookup.get(engineName);
-      if (recordCount > maxRecords || !engine) {
-        this._log.debug(`Skipping validation for ${engineName} because it's not an engine or ` +
-                        `the number of records (${recordCount}) is greater than the maximum allowed (${maxRecords}).`);
-        continue;
-      }
-      let validator = engine.getValidator();
-      if (!validator) {
-        continue;
-      }
-      // Put this in an array so that we know how many we're going to do, so we
-      // don't tell users we're going to run some validators when we aren't.
-      toRun.push({ engine, validator });
-    }
-
-    if (!toRun.length) {
-      return;
-    }
-    Services.console.logStringMessage(
-      "Sync is about to run a consistency check. This may be slow, and " +
-      "can be controlled using the pref \"services.sync.validation.enabled\".\n" +
-      "If you encounter any problems because of this, please file a bug.");
-    for (let { validator, engine } of toRun) {
-      try {
-        let result = yield validator.validate(engine);
-        Observers.notify("weave:engine:validate:finish", result, engine.name);
-      } catch (e) {
-        this._log.error(`Failed to run validation on ${engine.name}!`, e);
-        Observers.notify("weave:engine:validate:error", e, engine.name)
-        // Keep validating -- there's no reason to think that a failure for one
-        // validator would mean the others will fail.
-      }
-    }
-  }),
-
   // Returns true if sync should proceed.
   // false / no return value means sync should be aborted.
   _syncEngine: function _syncEngine(engine) {
     try {
       engine.sync();
     } catch (e) {
       if (e.status == 401) {
         // Maybe a 401, cluster update perhaps needed?
--- a/services/sync/moz.build
+++ b/services/sync/moz.build
@@ -18,16 +18,17 @@ EXTRA_COMPONENTS += [
 
 EXTRA_JS_MODULES['services-sync'] += [
     'modules/addonsreconciler.js',
     'modules/addonutils.js',
     'modules/bookmark_validator.js',
     'modules/browserid_identity.js',
     'modules/collection_repair.js',
     'modules/collection_validator.js',
+    'modules/doctor.js',
     'modules/engines.js',
     'modules/keys.js',
     'modules/main.js',
     'modules/policies.js',
     'modules/record.js',
     'modules/resource.js',
     'modules/rest.js',
     'modules/service.js',
@@ -64,11 +65,11 @@ EXTRA_JS_MODULES['services-sync'].stages
 
 TESTING_JS_MODULES.services.sync += [
     'modules-testing/fakeservices.js',
     'modules-testing/fxa_utils.js',
     'modules-testing/rotaryengine.js',
     'modules-testing/utils.js',
 ]
 
-JS_PREFERENCE_FILES += [
+JS_PREFERENCE_PP_FILES += [
     'services-sync.js',
 ]
--- a/services/sync/services-sync.js
+++ b/services/sync/services-sync.js
@@ -60,21 +60,26 @@ pref("services.sync.log.logger.identity"
 pref("services.sync.log.cryptoDebug", false);
 
 pref("services.sync.fxa.termsURL", "https://accounts.firefox.com/legal/terms");
 pref("services.sync.fxa.privacyURL", "https://accounts.firefox.com/legal/privacy");
 
 pref("services.sync.telemetry.submissionInterval", 43200); // 12 hours in seconds
 pref("services.sync.telemetry.maxPayloadCount", 500);
 
-// Note that services.sync.validation.enabled is located in browser/app/profile/firefox.js
+#ifndef RELEASE_OR_BETA
+// Enable the (fairly costly) client/server validation on nightly/aurora only.
+pref("services.sync.engine.bookmarks.validation.enabled", true);
+// Enable repair of bookmarks - requires validation also be enabled.
+pref("services.sync.engine.bookmarks.repair.enabled", true);
+#endif
 
 // We consider validation this frequently. After considering validation, even
 // if we don't end up validating, we won't try again unless this much time has passed.
-pref("services.sync.validation.interval", 86400); // 24 hours in seconds
+pref("services.sync.engine.bookmarks.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);
+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.validation.maxRecords", 1000);
+pref("services.sync.engine.bookmarks.validation.maxRecords", 1000);
--- a/services/sync/tests/unit/head_appinfo.js
+++ b/services/sync/tests/unit/head_appinfo.js
@@ -14,21 +14,21 @@ gSyncProfile = do_get_profile();
 // Init FormHistoryStartup and pretend we opened a profile.
 var fhs = Cc["@mozilla.org/satchel/form-history-startup;1"]
             .getService(Ci.nsIObserver);
 fhs.observe(null, "profile-after-change", null);
 
 // An app is going to have some prefs set which xpcshell tests don't.
 Services.prefs.setCharPref("identity.sync.tokenserver.uri", "http://token-server");
 
-// Set the validation prefs to attempt validation every time to avoid non-determinism.
-Services.prefs.setIntPref("services.sync.validation.interval", 0);
-Services.prefs.setIntPref("services.sync.validation.percentageChance", 100);
-Services.prefs.setIntPref("services.sync.validation.maxRecords", -1);
-Services.prefs.setBoolPref("services.sync.validation.enabled", true);
+// Set the validation prefs to attempt bookmark validation every time to avoid non-determinism.
+Services.prefs.setIntPref("services.sync.engine.bookmarks.validation.interval", 0);
+Services.prefs.setIntPref("services.sync.engine.bookmarks.validation.percentageChance", 100);
+Services.prefs.setIntPref("services.sync.engine.bookmarks.validation.maxRecords", -1);
+Services.prefs.setBoolPref("services.sync.engine.bookmarks.validation.enabled", true);
 
 // Make sure to provide the right OS so crypto loads the right binaries
 function getOS() {
   switch (mozinfo.os) {
     case "win":
       return "WINNT";
     case "mac":
       return "Darwin";
new file mode 100644
--- /dev/null
+++ b/services/sync/tests/unit/test_doctor.js
@@ -0,0 +1,94 @@
+/* Any copyright is dedicated to the Public Domain.
+   http://creativecommons.org/publicdomain/zero/1.0/ */
+
+const { Doctor, REPAIR_ADVANCE_PERIOD } = Cu.import("resource://services-sync/doctor.js", {});
+
+initTestLogging("Trace");
+
+function mockDoctor(mocks) {
+  // Clone the object and put mocks in that.
+  return Object.assign({}, Doctor, mocks);
+}
+
+add_task(async function test_repairs_start() {
+  let repairStarted = false;
+  let problems = {
+    missingChildren: ["a", "b", "c"],
+  }
+  let validator = {
+    validate(engine) {
+      return problems;
+    }
+  }
+  let engine = {
+    name: "test-engine",
+    getValidator() {
+      return validator;
+    }
+  }
+  let requestor = {
+    startRepairs(validationInfo, flowID) {
+      ok(flowID, "got a flow ID");
+      equal(validationInfo, problems);
+      repairStarted = true;
+      return true;
+    }
+  }
+  let doctor = mockDoctor({
+    _getEnginesToValidate(recentlySyncedEngines) {
+      deepEqual(recentlySyncedEngines, [engine]);
+      return {
+        "test-engine": { engine, maxRecords: -1 }
+      };
+    },
+    _getRepairRequestor(engineName) {
+      equal(engineName, engine.name);
+      return requestor;
+    },
+    _shouldRepair(e) {
+      return true;
+    },
+  });
+  let promiseValidationDone = promiseOneObserver("weave:engine:validate:finish");
+  await doctor.consult([engine]);
+  await promiseValidationDone;
+  ok(repairStarted);
+});
+
+add_task(async function test_repairs_advanced_daily() {
+  let repairCalls = 0;
+  let requestor = {
+    continueRepairs() {
+      repairCalls++;
+    }
+  }
+  // start now at just after REPAIR_ADVANCE_PERIOD so we do a a first one.
+  let now = REPAIR_ADVANCE_PERIOD + 1;
+  let doctor = mockDoctor({
+    _getEnginesToValidate() {
+      return {}; // no validations.
+    },
+    _runValidators() {
+      // do nothing.
+    },
+    _getAllRepairRequestors() {
+      return {
+        foo: requestor,
+      }
+    },
+    _now() {
+      return now;
+    },
+  });
+  await doctor.consult();
+  equal(repairCalls, 1);
+  now += 10; // 10 seconds later...
+  await doctor.consult();
+  // should not have done another repair yet - it's too soon.
+  equal(repairCalls, 1);
+  // advance our pretend clock by the advance period (eg, day)
+  now += REPAIR_ADVANCE_PERIOD;
+  await doctor.consult();
+  // should have done another repair
+  equal(repairCalls, 2);
+});
--- a/services/sync/tests/unit/xpcshell.ini
+++ b/services/sync/tests/unit/xpcshell.ini
@@ -141,16 +141,17 @@ tags = addons
 [test_bookmark_decline_undecline.js]
 # Too many intermittent "ASSERTION: thread pool wasn't shutdown: '!mPool'" (bug 804479)
 skip-if = debug
 [test_bookmark_tracker.js]
 requesttimeoutfactor = 4
 [test_bookmark_validator.js]
 [test_clients_engine.js]
 [test_clients_escape.js]
+[test_doctor.js]
 [test_extension_storage_engine.js]
 [test_extension_storage_tracker.js]
 [test_forms_store.js]
 [test_forms_tracker.js]
 # Too many intermittent "ASSERTION: thread pool wasn't shutdown: '!mPool'" (bug 804479)
 skip-if = debug
 [test_history_engine.js]
 [test_history_store.js]
--- a/tools/lint/eslint/modules.json
+++ b/tools/lint/eslint/modules.json
@@ -52,16 +52,17 @@
   "CrashManagerTest.jsm": ["configureLogging", "getManager", "sleep", "TestingCrashManager"],
   "dbg-client.jsm": ["DebuggerTransport", "DebuggerClient", "RootClient", "LongStringClient", "EnvironmentClient", "ObjectClient"],
   "dbg-server.jsm": ["DebuggerServer", "ActorPool", "OriginalLocation"],
   "debug.js": ["NS_ASSERT"],
   "declined.js": ["DeclinedEngines"],
   "dispatcher.js": ["Dispatcher"],
   "distribution.js": ["DistributionCustomizer"],
   "DNSTypes.jsm": ["DNS_QUERY_RESPONSE_CODES", "DNS_AUTHORITATIVE_ANSWER_CODES", "DNS_CLASS_CODES", "DNS_RECORD_TYPES"],
+  "doctor.js": ["Doctor"],
   "dom.js": ["getAttributes"],
   "DOMRequestHelper.jsm": ["DOMRequestIpcHelper"],
   "DownloadCore.jsm": ["Download", "DownloadSource", "DownloadTarget", "DownloadError", "DownloadSaver", "DownloadCopySaver", "DownloadLegacySaver", "DownloadPDFSaver"],
   "DownloadList.jsm": ["DownloadList", "DownloadCombinedList", "DownloadSummary"],
   "E10SAddonsRollout.jsm": ["isAddonPartOfE10SRollout"],
   "elementslib.js": ["ID", "Link", "XPath", "Selector", "Name", "Anon", "AnonXPath", "Lookup", "_byID", "_byName", "_byAttrib", "_byAnonAttrib"],
   "engines.js": ["EngineManager", "Engine", "SyncEngine", "Tracker", "Store", "Changeset"],
   "enginesync.js": ["EngineSynchronizer"],