Bug 1450152 - Handle changing services.sync.engine.bookmarks.buffer at runtime r?kitcambridge,markh
MozReview-Commit-ID: 2ImuLBcJVAl
--- 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), []);
+});
+