From 8c34d786e46adbcf5496549cb3fe1898761632d4 Mon Sep 17 00:00:00 2001 From: wolfbeast Date: Thu, 15 Aug 2019 09:52:38 +0200 Subject: Issue #1208: Fix jsonLoad in Sync's `util.js` to handle errors. - `OS.Path.join` can throw, so we always need to try/catch it. - Also do a sanity check to make sure `callback` is defined before use --- services/sync/modules/util.js | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) (limited to 'services') diff --git a/services/sync/modules/util.js b/services/sync/modules/util.js index 7fd5a7971..73f4d8a80 100644 --- a/services/sync/modules/util.js +++ b/services/sync/modules/util.js @@ -321,10 +321,17 @@ this.Utils = { * could not be loaded, the first argument will be undefined. */ jsonLoad: Task.async(function*(filePath, that, callback) { - let path = OS.Path.join(OS.Constants.Path.profileDir, "weave", filePath + ".json"); + let path; + try { + path = OS.Path.normalize(OS.Path.join(OS.Constants.Path.profileDir, "weave", filePath + ".json")); + } catch (e) { + if (that._log) { + that._log.debug("Path join error: " + e); + } + } if (that._log) { - that._log.trace("Loading json from disk: " + filePath); + that._log.trace("Loading json from disk: " + path); } let json; @@ -341,8 +348,9 @@ this.Utils = { } } } - - callback.call(that, json); + if (callback) { + callback.call(that, json); + } }), /** -- cgit v1.2.3 From 2925fdc3ee9acf17c4aa5494afb7cc18c8dc10bd Mon Sep 17 00:00:00 2001 From: wolfbeast Date: Thu, 15 Aug 2019 09:58:13 +0200 Subject: Issue #1208: Remove `services.sync.enabled` pref. Sync will not do anything unless specifically set up to do so and at least one engine is enabled, so there's no need for this "master switch" to force it disabled based on engines being disabled (which was its previous function to shortcut syncing in that situation). --- services/sync/modules/service.js | 21 ++------------------- services/sync/services-sync.js | 4 ---- services/sync/tests/unit/test_service_login.js | 2 +- 3 files changed, 3 insertions(+), 24 deletions(-) (limited to 'services') diff --git a/services/sync/modules/service.js b/services/sync/modules/service.js index 15884aca0..804eb20cd 100644 --- a/services/sync/modules/service.js +++ b/services/sync/modules/service.js @@ -296,21 +296,6 @@ Sync11Service.prototype = { return false; }, - // The global "enabled" state comes from prefs, and will be set to false - // whenever the UI that exposes what to sync finds all Sync engines disabled. - get enabled() { - return Svc.Prefs.get("enabled"); - }, - set enabled(val) { - // There's no real reason to impose this other than to catch someone doing - // something we don't expect with bad consequences - all setting of this - // pref are in the UI code and external to this module. - if (val) { - throw new Error("Only disabling via this setter is supported"); - } - Svc.Prefs.set("enabled", val); - }, - /** * Prepare to initialize the rest of Weave after waiting a little bit */ @@ -340,6 +325,8 @@ Sync11Service.prototype = { this._clusterManager = this.identity.createClusterManager(this); this.recordManager = new RecordManager(this); + this.enabled = true; + this._registerEngines(); let ua = Cc["@mozilla.org/network/protocol;1?name=http"]. @@ -1236,10 +1223,6 @@ Sync11Service.prototype = { }, sync: function sync() { - if (!this.enabled) { - this._log.debug("Not syncing as Sync is disabled."); - return; - } let dateStr = new Date().toLocaleFormat(LOG_DATE_FORMAT); this._log.debug("User-Agent: " + SyncStorageRequest.prototype.userAgent); this._log.info("Starting sync at " + dateStr); diff --git a/services/sync/services-sync.js b/services/sync/services-sync.js index 640fb4abc..dfce84767 100644 --- a/services/sync/services-sync.js +++ b/services/sync/services-sync.js @@ -24,10 +24,6 @@ pref("services.sync.scheduler.sync11.singleDeviceInterval", 86400); // 1 day pref("services.sync.errorhandler.networkFailureReportTimeout", 1209600); // 2 weeks -// A "master" pref for Sync being enabled. Will be set to false if the sync -// customization UI finds all our builtin engines disabled (and addons are -// free to force this to true if they have their own engine) -pref("services.sync.enabled", true); // Our engines. pref("services.sync.engine.addons", false); pref("services.sync.engine.bookmarks", true); diff --git a/services/sync/tests/unit/test_service_login.js b/services/sync/tests/unit/test_service_login.js index 52ee5e63a..2ecb0a377 100644 --- a/services/sync/tests/unit/test_service_login.js +++ b/services/sync/tests/unit/test_service_login.js @@ -183,7 +183,7 @@ add_test(function test_login_on_sync() { // This test exercises these two branches. _("We're ready to sync if locked."); - Svc.Prefs.set("enabled", true); + Service.enabled = true; Services.io.offline = false; Service.scheduler.checkSyncStatus(); do_check_true(scheduleCalled); -- cgit v1.2.3 From 1e8e5563d8a7809ecf5d421b04fc8e1afa0469c4 Mon Sep 17 00:00:00 2001 From: wolfbeast Date: Thu, 15 Aug 2019 10:40:06 +0200 Subject: Issue #1208: Update Sync policies.js getters/setters to prevent race. --- services/sync/modules/policies.js | 70 ++++++++++++++++++++++++++++++--------- 1 file changed, 55 insertions(+), 15 deletions(-) (limited to 'services') diff --git a/services/sync/modules/policies.js b/services/sync/modules/policies.js index 2d85b1428..48acbe2e6 100644 --- a/services/sync/modules/policies.js +++ b/services/sync/modules/policies.js @@ -60,20 +60,60 @@ SyncScheduler.prototype = { }, // nextSync is in milliseconds, but prefs can't hold that much - get nextSync() Svc.Prefs.get("nextSync", 0) * 1000, - set nextSync(value) Svc.Prefs.set("nextSync", Math.floor(value / 1000)), + get nextSync() { + if (Svc.Prefs) { + return Svc.Prefs.get("nextSync", 0) * 1000 + } + }, + set nextSync(value) { + if (Svc.Prefs) { + Svc.Prefs.set("nextSync", Math.floor(value / 1000)) + } + }, - get syncInterval() Svc.Prefs.get("syncInterval", this.singleDeviceInterval), - set syncInterval(value) Svc.Prefs.set("syncInterval", value), + get syncInterval() { + if (Svc.Prefs) { + return Svc.Prefs.get("syncInterval", this.singleDeviceInterval) + } + }, + set syncInterval(value) { + if (Svc.Prefs) { + Svc.Prefs.set("syncInterval", value) + } + }, - get syncThreshold() Svc.Prefs.get("syncThreshold", SINGLE_USER_THRESHOLD), - set syncThreshold(value) Svc.Prefs.set("syncThreshold", value), + get syncThreshold() { + if (Svc.Prefs) { + return Svc.Prefs.get("syncThreshold", SINGLE_USER_THRESHOLD) + } + }, + set syncThreshold(value) { + if (Svc.Prefs) { + Svc.Prefs.set("syncThreshold", value) + } + }, - get globalScore() Svc.Prefs.get("globalScore", 0), - set globalScore(value) Svc.Prefs.set("globalScore", value), + get globalScore() { + if (Svc.Prefs) { + return Svc.Prefs.get("globalScore", 0) + } + }, + set globalScore(value) { + if (Svc.Prefs) { + Svc.Prefs.set("globalScore", value) + } + }, - get numClients() Svc.Prefs.get("numClients", 0), - set numClients(value) Svc.Prefs.set("numClients", value), + get numClients() { + if (Svc.Prefs) { + return Svc.Prefs.get("numClients", 0) + } + }, + set numClients(value) { + if (Svc.Prefs) { + Svc.Prefs.set("numClients", value) + } + }, init: function init() { this._log.level = Log.Level[Svc.Prefs.get("log.logger.service.main")]; @@ -523,7 +563,7 @@ SyncScheduler.prototype = { }, get isBlocked() { - let until = Svc.Prefs.get("scheduler.blocked-until"); + let until = Svc.Prefs ? Svc.Prefs.get("scheduler.blocked-until") : undefined; if (until === undefined) { return false; } @@ -770,19 +810,19 @@ ErrorHandler.prototype = { }, get currentAlertMode() { - return Svc.Prefs.get("errorhandler.alert.mode"); + return Svc.Prefs ? Svc.Prefs.get("errorhandler.alert.mode") : undefined; }, set currentAlertMode(str) { - return Svc.Prefs.set("errorhandler.alert.mode", str); + return Svc.Prefs ? Svc.Prefs.set("errorhandler.alert.mode", str) : undefined; }, get earliestNextAlert() { - return Svc.Prefs.get("errorhandler.alert.earliestNext", 0) * 1000; + return Svc.Prefs ? Svc.Prefs.get("errorhandler.alert.earliestNext", 0) * 1000 : undefined; }, set earliestNextAlert(msec) { - return Svc.Prefs.set("errorhandler.alert.earliestNext", msec / 1000); + return Svc.Prefs ? Svc.Prefs.set("errorhandler.alert.earliestNext", msec / 1000) : undefined; }, clearServerAlerts: function () { -- cgit v1.2.3