From d8477f9e65dbf388eb6f07fbe17683a0a11ae5c6 Mon Sep 17 00:00:00 2001 From: Mark Banner Date: Wed, 19 Apr 2017 12:06:16 +0100 Subject: [PATCH] Bug 1348097 - Fix intermittent failures caused by Experiments.jsm by ensuring the preference monitoring is in sync. r=gfritzsche Change the ExperimentsService to get the current value of the preferences (since it only uses them once or twice), so that they match the values in Experiments, and avoid differences causing promises to be rejected in the updateManifest call. Also fix Experiments to correctly re-enable itself when toolkit.telemetry.enabled is changed from false to true (also fixes bug 1232648). Finally, add a catch for a promise when calling updateManifest so that we don't get an uncaught promise exception. MozReview-Commit-ID: GD6gfcRSgbx --HG-- extra : rebase_source : 44226ad68bc0bc8b9b763016ea54ca022bfcbcf9 --- browser/experiments/Experiments.jsm | 2 +- browser/experiments/ExperimentsService.js | 34 +++++++++---------- .../test/xpcshell/test_telemetry_disabled.js | 9 ++++- .../tests/unit/test_TelemetryController.js | 4 +++ 4 files changed, 30 insertions(+), 19 deletions(-) diff --git a/browser/experiments/Experiments.jsm b/browser/experiments/Experiments.jsm index 56f43fbd4433..84e6359a5024 100644 --- a/browser/experiments/Experiments.jsm +++ b/browser/experiments/Experiments.jsm @@ -594,7 +594,7 @@ Experiments.Experiments.prototype = { }), _telemetryStatusChanged() { - this._toggleExperimentsEnabled(gExperimentsEnabled); + this._toggleExperimentsEnabled(gPrefs.get(PREF_ENABLED, false)); }, /** diff --git a/browser/experiments/ExperimentsService.js b/browser/experiments/ExperimentsService.js index da230879e956..d33a70aaf860 100644 --- a/browser/experiments/ExperimentsService.js +++ b/browser/experiments/ExperimentsService.js @@ -16,30 +16,19 @@ XPCOMUtils.defineLazyModuleGetter(this, "OS", "resource://gre/modules/osfile.jsm"); XPCOMUtils.defineLazyModuleGetter(this, "CommonUtils", "resource://services-common/utils.js"); +XPCOMUtils.defineLazyModuleGetter(this, "TelemetryUtils", + "resource://gre/modules/TelemetryUtils.jsm"); + const PREF_EXPERIMENTS_ENABLED = "experiments.enabled"; const PREF_ACTIVE_EXPERIMENT = "experiments.activeExperiment"; // whether we have an active experiment -const PREF_TELEMETRY_ENABLED = "toolkit.telemetry.enabled"; -const PREF_TELEMETRY_UNIFIED = "toolkit.telemetry.unified"; const DELAY_INIT_MS = 30 * 1000; -// Whether the FHR/Telemetry unification features are enabled. -// Changing this pref requires a restart. -const IS_UNIFIED_TELEMETRY = Preferences.get(PREF_TELEMETRY_UNIFIED, false); - XPCOMUtils.defineLazyGetter( this, "gPrefs", () => { return new Preferences(); }); -XPCOMUtils.defineLazyGetter( - this, "gExperimentsEnabled", () => { - // We can enable experiments if either unified Telemetry or FHR is on, and the user - // has opted into Telemetry. - return gPrefs.get(PREF_EXPERIMENTS_ENABLED, false) && - IS_UNIFIED_TELEMETRY && gPrefs.get(PREF_TELEMETRY_ENABLED, false); - }); - XPCOMUtils.defineLazyGetter( this, "gActiveExperiment", () => { return gPrefs.get(PREF_ACTIVE_EXPERIMENT); @@ -54,8 +43,15 @@ ExperimentsService.prototype = { classID: Components.ID("{f7800463-3b97-47f9-9341-b7617e6d8d49}"), QueryInterface: XPCOMUtils.generateQI([Ci.nsITimerCallback, Ci.nsIObserver]), + get _experimentsEnabled() { + // We can enable experiments if either unified Telemetry or FHR is on, and the user + // has opted into Telemetry. + return gPrefs.get(PREF_EXPERIMENTS_ENABLED, false) && + TelemetryUtils.isTelemetryEnabled; + }, + notify(timer) { - if (!gExperimentsEnabled) { + if (!this._experimentsEnabled) { return; } if (OS.Constants.Path.profileDir === undefined) { @@ -63,7 +59,11 @@ ExperimentsService.prototype = { } let instance = Experiments.instance(); if (instance.isReady) { - instance.updateManifest(); + instance.updateManifest().catch(error => { + // Don't throw, as this breaks tests. In any case the best we can do here + // is to log the failure. + Cu.reportError(error); + }); } }, @@ -77,7 +77,7 @@ ExperimentsService.prototype = { observe(subject, topic, data) { switch (topic) { case "profile-after-change": - if (gExperimentsEnabled) { + if (this._experimentsEnabled) { Services.obs.addObserver(this, "quit-application"); Services.obs.addObserver(this, "sessionstore-state-finalized"); Services.obs.addObserver(this, "EM-loaded"); diff --git a/browser/experiments/test/xpcshell/test_telemetry_disabled.js b/browser/experiments/test/xpcshell/test_telemetry_disabled.js index 74f85ccfc7e8..7ef5f87cead9 100644 --- a/browser/experiments/test/xpcshell/test_telemetry_disabled.js +++ b/browser/experiments/test/xpcshell/test_telemetry_disabled.js @@ -13,9 +13,16 @@ add_test(function test_experiments_activation() { Services.prefs.setBoolPref(PREF_TELEMETRY_ENABLED, false); let experiments = Experiments.instance(); + Assert.ok(!experiments.enabled, "Experiments must be disabled if Telemetry is disabled."); - // TODO: Test that Experiments are turned back on when bug 1232648 lands. + // Patch updateManifest to not do anything when the pref is switched back to true, + // otherwise it attempts to connect to the server. + experiments.updateManifest = () => Promise.resolve(); + + Services.prefs.setBoolPref(PREF_TELEMETRY_ENABLED, true); + + Assert.ok(experiments.enabled, "Experiments must be re-enabled if Telemetry is re-enabled"); run_next_test(); }); diff --git a/toolkit/components/telemetry/tests/unit/test_TelemetryController.js b/toolkit/components/telemetry/tests/unit/test_TelemetryController.js index 968d0ec6d2fa..cbda180a166d 100644 --- a/toolkit/components/telemetry/tests/unit/test_TelemetryController.js +++ b/toolkit/components/telemetry/tests/unit/test_TelemetryController.js @@ -28,6 +28,7 @@ const PLATFORM_VERSION = "1.9.2"; const APP_VERSION = "1"; const APP_NAME = "XPCShell"; +const PREF_EXPERIMENTS_ENABLED = "experiments.enabled"; const PREF_BRANCH = "toolkit.telemetry."; const PREF_ENABLED = PREF_BRANCH + "enabled"; const PREF_ARCHIVE_ENABLED = PREF_BRANCH + "archive.enabled"; @@ -98,6 +99,9 @@ add_task(function* test_setup() { // Make sure we don't generate unexpected pings due to pref changes. yield setEmptyPrefWatchlist(); + // Ensure browser experiments are also disabled, to avoid network activity + // when toggling PREF_ENABLED. + Services.prefs.setBoolPref(PREF_EXPERIMENTS_ENABLED, false); Services.prefs.setBoolPref(PREF_ENABLED, true); Services.prefs.setBoolPref(PREF_FHR_UPLOAD_ENABLED, true);