Bug 1450152 - Handle changing services.sync.engine.bookmarks.buffer at runtime r=kitcambridge,markh
authorThom Chiovoloni <tchiovoloni@mozilla.com>
Tue, 03 Apr 2018 13:55:27 -0700
changeset 413063 1ec8c31cacc4cf41b7fb4852e37a6af1b12ef4d7
parent 413062 752cb50ca2ea5bce12a023ee92b6fa77fdc58bbb
child 413064 6ea3c1db006049ff57f392c7042db0a91ec9b10b
push id33833
push useraiakab@mozilla.com
push dateFri, 13 Apr 2018 09:41:15 +0000
treeherdermozilla-central@260e4c83c8a9 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerskitcambridge, markh
bugs1450152
milestone61.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 1450152 - Handle changing services.sync.engine.bookmarks.buffer at runtime r=kitcambridge,markh MozReview-Commit-ID: 2ImuLBcJVAl
services/sync/modules/engines.js
services/sync/modules/service.js
services/sync/modules/stages/enginesync.js
services/sync/tests/unit/test_enginemanager.js
--- a/services/sync/modules/engines.js
+++ b/services/sync/modules/engines.js
@@ -462,16 +462,18 @@ Store.prototype = {
   }
 };
 
 function EngineManager(service) {
   this.service = service;
 
   this._engines = {};
 
+  this._altEngineInfo = {};
+
   // This will be populated by Service on startup.
   this._declined = new Set();
   this._log = Log.repository.getLogger("Sync.EngineManager");
   this._log.manageLevelFromPref("services.sync.log.logger.service.engines");
   // define the default level for all engine logs here (although each engine
   // allows its level to be controlled via a specific, non-default pref)
   Log.repository.getLogger(`Sync.Engine`).manageLevelFromPref("services.sync.log.logger.engine");
 }
@@ -503,16 +505,66 @@ EngineManager.prototype = {
     let engines = [];
     for (let [, engine] of Object.entries(this._engines)) {
       engines.push(engine);
     }
     return engines;
   },
 
   /**
+   * If a user has changed a pref that controls which variant of a sync engine
+   * for a given collection we use, unregister the old engine and register the
+   * new one.
+   *
+   * This is called by EngineSynchronizer before every sync.
+   */
+  async switchAlternatives() {
+    for (let [name, info] of Object.entries(this._altEngineInfo)) {
+      let prefValue = info.prefValue;
+      if (prefValue === info.lastValue) {
+        this._log.trace(`No change for engine ${name} (${info.pref} is still ${
+                        prefValue})`);
+        continue;
+      }
+      // Unregister the old engine, register the new one.
+      this._log.info(`Switching ${name} engine ("${info.pref}" went from ${
+                     info.lastValue} => ${prefValue})`);
+      try {
+        await this._removeAndFinalize(name);
+      } catch (e) {
+        this._log.warn(`Failed to remove previous ${name} engine...`, e);
+      }
+      let engineType = prefValue ? info.whenTrue : info.whenFalse;
+      try {
+        // If register throws, we'll try again next sync, but until then there
+        // won't be an engine registered for this collection.
+        await this.register(engineType);
+        info.lastValue = prefValue;
+        // Note: engineType.name is using Function.prototype.name.
+        this._log.info(`Switched the ${name} engine to use ${engineType.name}`);
+      } catch (e) {
+        this._log.warn(`Switching the ${name} engine to use ${
+                       engineType.name} failed (couldn't register)`, e);
+      }
+    }
+  },
+
+  async registerAlternatives(name, pref, whenTrue, whenFalse) {
+    let info = { name, pref, whenTrue, whenFalse };
+
+    XPCOMUtils.defineLazyPreferenceGetter(info, "prefValue", pref, false);
+
+    let chosen = info.prefValue ? info.whenTrue : info.whenFalse;
+    info.lastValue = info.prefValue;
+    this._altEngineInfo[name] = info;
+
+    await this.register(chosen);
+  },
+
+  /**
    * N.B., does not pay attention to the declined list.
    */
   getEnabled() {
     return this.getAll()
                .filter((engine) => engine.enabled)
                .sort((a, b) => a.syncPriority - b.syncPriority);
   },
 
@@ -608,29 +660,38 @@ EngineManager.prototype = {
     }
   },
 
   async unregister(val) {
     let name = val;
     if (val instanceof SyncEngine) {
       name = val.name;
     }
+    await this._removeAndFinalize(name);
+    delete this._altEngineInfo[name];
+  },
+
+  // Common code for disabling an engine by name, that doesn't complain if the
+  // engine doesn't exist. Doesn't touch the engine's alternative info (if any
+  // exists).
+  async _removeAndFinalize(name) {
     if (name in this._engines) {
       let engine = this._engines[name];
       delete this._engines[name];
       await engine.finalize();
     }
   },
 
   async clear() {
     for (let name in this._engines) {
       let engine = this._engines[name];
       delete this._engines[name];
       await engine.finalize();
     }
+    this._altEngineInfo = {};
   },
 };
 
 function SyncEngine(name, service) {
   if (!service) {
     throw new Error("SyncEngine must be associated with a Service instance.");
   }
 
--- a/services/sync/modules/service.js
+++ b/services/sync/modules/service.js
@@ -48,27 +48,22 @@ function getEngineModules() {
     };
   }
   if (Svc.Prefs.get("engine.creditcards.available", false)) {
     result.CreditCards = {
       module: "resource://formautofill/FormAutofillSync.jsm",
       symbol: "CreditCardsEngine",
     };
   }
-  if (Svc.Prefs.get("engine.bookmarks.buffer", false)) {
-    result.Bookmarks = {
-      module: "bookmarks.js",
-      symbol: "BufferedBookmarksEngine",
-    };
-  } else {
-    result.Bookmarks = {
-      module: "bookmarks.js",
-      symbol: "BookmarksEngine",
-    };
-  }
+  result.Bookmarks = {
+    module: "bookmarks.js",
+    controllingPref: "services.sync.engine.bookmarks.buffer",
+    whenFalse: "BookmarksEngine",
+    whenTrue: "BufferedBookmarksEngine",
+  };
   return result;
 }
 
 // A unique identifier for this browser session. Used for logging so
 // we can easily see whether 2 logs are in the same browser session or
 // after the browser restarted.
 XPCOMUtils.defineLazyGetter(this, "browserSessionID", Utils.makeGUID);
 
@@ -382,29 +377,42 @@ Sync11Service.prototype = {
     await clientsEngine.initialize();
     this.clientsEngine = clientsEngine;
 
     for (let name of engines) {
       if (!(name in engineModules)) {
         this._log.info("Do not know about engine: " + name);
         continue;
       }
-      let {module, symbol} = engineModules[name];
-      if (!module.includes(":")) {
-        module = "resource://services-sync/engines/" + module;
+      let modInfo = engineModules[name];
+      if (!modInfo.module.includes(":")) {
+        modInfo.module = "resource://services-sync/engines/" + modInfo.module;
       }
       let ns = {};
       try {
-        ChromeUtils.import(module, ns);
-        if (!(symbol in ns)) {
-          this._log.warn("Could not find exported engine instance: " + symbol);
-          continue;
+        ChromeUtils.import(modInfo.module, ns);
+        if (modInfo.symbol) {
+          let symbol = modInfo.symbol;
+          if (!(symbol in ns)) {
+            this._log.warn("Could not find exported engine instance: " + symbol);
+            continue;
+          }
+          await this.engineManager.register(ns[symbol]);
+        } else {
+          let {whenTrue, whenFalse, controllingPref} = modInfo;
+          if (!(whenTrue in ns) || !(whenFalse in ns)) {
+            this._log.warn("Could not find all exported engine instances",
+                           { whenTrue, whenFalse });
+            continue;
+          }
+          await this.engineManager.registerAlternatives(name.toLowerCase(),
+                                                        controllingPref,
+                                                        ns[whenTrue],
+                                                        ns[whenFalse]);
         }
-
-        await this.engineManager.register(ns[symbol]);
       } catch (ex) {
         this._log.warn("Could not register engine " + name, ex);
       }
     }
 
     this.engineManager.setDeclined(declined);
   },
 
--- a/services/sync/modules/stages/enginesync.js
+++ b/services/sync/modules/stages/enginesync.js
@@ -132,16 +132,18 @@ EngineSynchronizer.prototype = {
     try {
       await this._updateEnabledEngines();
     } catch (ex) {
       this._log.debug("Updating enabled engines failed", ex);
       this.service.errorHandler.checkServerError(ex);
       throw ex;
     }
 
+    await this.service.engineManager.switchAlternatives();
+
     // If the engines to sync has been specified, we sync in the order specified.
     let enginesToSync;
     if (allowEnginesHint && engineNamesToSync) {
       this._log.info("Syncing specified engines", engineNamesToSync);
       enginesToSync = engineManager.get(engineNamesToSync).filter(e => e.enabled);
     } else {
       this._log.info("Syncing all enabled engines.");
       enginesToSync = engineManager.getEnabled();
--- a/services/sync/tests/unit/test_enginemanager.js
+++ b/services/sync/tests/unit/test_enginemanager.js
@@ -106,8 +106,121 @@ add_task(async function test_basics() {
   let actual = await manager.get("actual");
   Assert.ok(actual instanceof ActualEngine);
   Assert.ok(actual instanceof SyncEngine);
 
   await manager.unregister(actual);
   Assert.equal((await manager.get("actual")), undefined);
 });
 
+class AutoEngine {
+  constructor(type) {
+    this.name = "automobile";
+    this.type = type;
+    this.initializeCalled = false;
+    this.finalizeCalled = false;
+    this.isActive = false;
+  }
+
+  async initialize() {
+    Assert.ok(!this.initializeCalled);
+    Assert.equal(AutoEngine.current, undefined);
+    this.initializeCalled = true;
+    this.isActive = true;
+    AutoEngine.current = this;
+  }
+
+  async finalize() {
+    Assert.equal(AutoEngine.current, this);
+    Assert.ok(!this.finalizeCalled);
+    Assert.ok(this.isActive);
+    this.finalizeCalled = true;
+    this.isActive = false;
+    AutoEngine.current = undefined;
+  }
+}
+
+class GasolineEngine extends AutoEngine {
+  constructor() { super("gasoline"); }
+}
+
+class ElectricEngine extends AutoEngine {
+  constructor() { super("electric"); }
+}
+
+add_task(async function test_alternates() {
+  let manager = new EngineManager(Service);
+  let engines = await manager.getAll();
+  Assert.equal(engines.length, 0);
+
+  const prefName = "services.sync.engines.automobile.electric";
+  Services.prefs.clearUserPref(prefName);
+
+  await manager.registerAlternatives("automobile",
+                                     prefName,
+                                     ElectricEngine,
+                                     GasolineEngine);
+
+  let gasEngine = manager.get("automobile");
+  Assert.equal(gasEngine.type, "gasoline");
+
+  Assert.ok(gasEngine.isActive);
+  Assert.ok(gasEngine.initializeCalled);
+  Assert.ok(!gasEngine.finalizeCalled);
+  Assert.equal(AutoEngine.current, gasEngine);
+
+  _("Check that setting the controlling pref to false makes no difference");
+  Services.prefs.setBoolPref(prefName, false);
+  Assert.equal(manager.get("automobile"), gasEngine);
+  Assert.ok(gasEngine.isActive);
+  Assert.ok(gasEngine.initializeCalled);
+  Assert.ok(!gasEngine.finalizeCalled);
+
+  _("Even after the call to switchAlternatives");
+  await manager.switchAlternatives();
+  Assert.equal(manager.get("automobile"), gasEngine);
+  Assert.ok(gasEngine.isActive);
+  Assert.ok(gasEngine.initializeCalled);
+  Assert.ok(!gasEngine.finalizeCalled);
+
+  _("Set the pref to true, we still shouldn't switch yet");
+  Services.prefs.setBoolPref(prefName, true);
+  Assert.equal(manager.get("automobile"), gasEngine);
+  Assert.ok(gasEngine.isActive);
+  Assert.ok(gasEngine.initializeCalled);
+  Assert.ok(!gasEngine.finalizeCalled);
+
+  _("Now we expect to switch from gas to electric");
+  await manager.switchAlternatives();
+  let elecEngine = manager.get("automobile");
+  Assert.equal(elecEngine.type, "electric");
+  Assert.ok(elecEngine.isActive);
+  Assert.ok(elecEngine.initializeCalled);
+  Assert.ok(!elecEngine.finalizeCalled);
+  Assert.equal(AutoEngine.current, elecEngine);
+
+  Assert.ok(!gasEngine.isActive);
+  Assert.ok(gasEngine.finalizeCalled);
+
+  _("Switch back, and ensure we get a new instance that got initialized again");
+  Services.prefs.setBoolPref(prefName, false);
+  await manager.switchAlternatives();
+
+  // First make sure we deactivated the electric engine as we should
+  Assert.ok(!elecEngine.isActive);
+  Assert.ok(elecEngine.initializeCalled);
+  Assert.ok(elecEngine.finalizeCalled);
+
+  let newGasEngine = manager.get("automobile");
+  Assert.notEqual(newGasEngine, gasEngine);
+  Assert.equal(newGasEngine.type, "gasoline");
+
+  Assert.ok(newGasEngine.isActive);
+  Assert.ok(newGasEngine.initializeCalled);
+  Assert.ok(!newGasEngine.finalizeCalled);
+
+  _("Make sure unregister removes the alt info too");
+  await manager.unregister("automobile");
+  Assert.equal(manager.get("automobile"), null);
+  Assert.ok(newGasEngine.finalizeCalled);
+  Assert.deepEqual(Object.keys(manager._altEngineInfo), []);
+});
+