diff --git a/recipe-client-addon/bootstrap.js b/recipe-client-addon/bootstrap.js index 981332c3..f29eb0d5 100644 --- a/recipe-client-addon/bootstrap.js +++ b/recipe-client-addon/bootstrap.js @@ -45,7 +45,7 @@ const log = Log.repository.getLogger(BOOTSTRAP_LOGGER_NAME); log.addAppender(new Log.ConsoleAppender(new Log.BasicFormatter())); log.level = Services.prefs.getIntPref(PREF_LOGGING_LEVEL, Log.Level.Warn); -const studyPrefsChanged = {}; +let studyPrefsChanged = {}; this.Bootstrap = { initShieldPrefs(defaultPrefs) { @@ -68,6 +68,7 @@ this.Bootstrap = { }, initExperimentPrefs() { + studyPrefsChanged = {}; const defaultBranch = Services.prefs.getDefaultBranch(""); const experimentBranch = Services.prefs.getBranch(STARTUP_EXPERIMENT_PREFS_BRANCH); @@ -80,31 +81,40 @@ this.Bootstrap = { continue; } + // record the value of the default branch before setting it + switch (realPrefType) { + case Services.prefs.PREF_STRING: + studyPrefsChanged[prefName] = defaultBranch.getCharPref(prefName); + break; + + case Services.prefs.PREF_INT: + studyPrefsChanged[prefName] = defaultBranch.getIntPref(prefName); + break; + + case Services.prefs.PREF_BOOL: + studyPrefsChanged[prefName] = defaultBranch.getBoolPref(prefName); + break; + + case Services.prefs.PREF_INVALID: + studyPrefsChanged[prefName] = null; + break; + + default: + // This should never happen + log.error(`Error getting startup pref ${prefName}; unknown value type ${experimentPrefType}.`); + } + + // now set the new default value switch (experimentPrefType) { case Services.prefs.PREF_STRING: - try { // eslint-disable-line mozilla/use-default-preference-values - studyPrefsChanged[prefName] = defaultBranch.getCharPref(prefName); - } catch (e) { - Cu.reportError(e); - } defaultBranch.setCharPref(prefName, experimentBranch.getCharPref(prefName)); break; case Services.prefs.PREF_INT: - try { // eslint-disable-line mozilla/use-default-preference-values - studyPrefsChanged[prefName] = defaultBranch.getIntPref(prefName); - } catch (e) { - Cu.reportError(e); - } defaultBranch.setIntPref(prefName, experimentBranch.getIntPref(prefName)); break; case Services.prefs.PREF_BOOL: - try { // eslint-disable-line mozilla/use-default-preference-values - studyPrefsChanged[prefName] = defaultBranch.getBoolPref(prefName); - } catch (e) { - Cu.reportError(e); - } defaultBranch.setBoolPref(prefName, experimentBranch.getBoolPref(prefName)); break; diff --git a/recipe-client-addon/lib/PreferenceExperiments.jsm b/recipe-client-addon/lib/PreferenceExperiments.jsm index bed67ce4..5f34fd91 100644 --- a/recipe-client-addon/lib/PreferenceExperiments.jsm +++ b/recipe-client-addon/lib/PreferenceExperiments.jsm @@ -102,16 +102,22 @@ const log = LogManager.getLogger("preference-experiments"); let experimentObservers = new Map(); CleanupManager.addCleanupHandler(() => PreferenceExperiments.stopAllObservers()); -function getPref(prefBranch, prefName, prefType, defaultVal) { +function getPref(prefBranch, prefName, prefType) { + if (prefBranch.getPrefType(prefName) === 0) { + // pref doesn't exist + return null; + } + switch (prefType) { - case "boolean": - return prefBranch.getBoolPref(prefName, defaultVal); + case "boolean": { + return prefBranch.getBoolPref(prefName); + } case "string": - return prefBranch.getStringPref(prefName, defaultVal); + return prefBranch.getStringPref(prefName); case "integer": - return prefBranch.getIntPref(prefName, defaultVal); + return prefBranch.getIntPref(prefName); default: throw new TypeError(`Unexpected preference type (${prefType}) for ${prefName}.`); @@ -171,7 +177,7 @@ this.PreferenceExperiments = { for (const experiment of await this.getAllActive()) { // Check that the current value of the preference is still what we set it to - if (getPref(UserPreferences, experiment.preferenceName, experiment.preferenceType, undefined) !== experiment.preferenceValue) { + if (getPref(UserPreferences, experiment.preferenceName, experiment.preferenceType) !== experiment.preferenceValue) { // if not, stop the experiment, and skip the remaining steps log.info(`Stopping experiment "${experiment.name}" because its value changed`); await this.stop(experiment.name, false); @@ -299,7 +305,7 @@ this.PreferenceExperiments = { preferenceName, preferenceValue, preferenceType, - previousPreferenceValue: getPref(preferences, preferenceName, preferenceType, undefined), + previousPreferenceValue: getPref(preferences, preferenceName, preferenceType), preferenceBranchType, }; @@ -347,7 +353,7 @@ this.PreferenceExperiments = { const observerInfo = { preferenceName, observer() { - const newValue = getPref(UserPreferences, preferenceName, preferenceType, undefined); + const newValue = getPref(UserPreferences, preferenceName, preferenceType); if (newValue !== preferenceValue) { PreferenceExperiments.stop(experimentName, false) .catch(Cu.reportError); @@ -448,13 +454,19 @@ this.PreferenceExperiments = { if (resetValue) { const {preferenceName, preferenceType, previousPreferenceValue, preferenceBranchType} = experiment; const preferences = PreferenceBranchType[preferenceBranchType]; - if (previousPreferenceValue !== undefined) { + + if (previousPreferenceValue !== null) { setPref(preferences, preferenceName, preferenceType, previousPreferenceValue); - } else { - // This does nothing if we're on the default branch, which is fine. The - // preference will be reset on next restart, and most preferences should - // have had a default value set before the experiment anyway. + } else if (preferenceBranchType === "user") { + // Remove the "user set" value (which Shield set), but leave the default intact. preferences.clearUserPref(preferenceName); + } else { + // Remove both the user and default branch preference. This + // is ok because we only do this when studies expire, not + // when users actively leave a study by changing the + // preference, so there should not be a user branch value at + // this point. + Services.prefs.getDefaultBranch("").deleteBranch(preferenceName); } } diff --git a/recipe-client-addon/test/browser/browser_PreferenceExperiments.js b/recipe-client-addon/test/browser/browser_PreferenceExperiments.js index d3c2f092..7b4613f3 100644 --- a/recipe-client-addon/test/browser/browser_PreferenceExperiments.js +++ b/recipe-client-addon/test/browser/browser_PreferenceExperiments.js @@ -427,7 +427,7 @@ add_task(withMockExperiments(withMockPreferences(async function(experiments, moc preferenceName: "fake.preference", preferenceValue: "experimentvalue", preferenceType: "string", - previousPreferenceValue: undefined, + previousPreferenceValue: null, preferenceBranchType: "user", }); @@ -694,7 +694,7 @@ decorate_task( }, ); -// test that default branch prefs restore to the right value +// test that default branch prefs restore to the right value if the default pref changes decorate_task( withMockExperiments, withMockPreferences, @@ -741,3 +741,50 @@ decorate_task( ); }, ); + +// test that default branch prefs restore to the right value if the preference is removed +decorate_task( + withMockExperiments, + withMockPreferences, + withStub(PreferenceExperiments, "startObserver"), + withStub(PreferenceExperiments, "stopObserver"), + async function testDefaultBranchStop(mockExperiments, mockPreferences, stopObserverStub) { + const prefName = "fake.preference"; + mockPreferences.set(prefName, "old version's value", "default"); + + // start an experiment + await PreferenceExperiments.start({ + name: "test", + branch: "branch", + preferenceName: prefName, + preferenceValue: "experiment value", + preferenceBranchType: "default", + preferenceType: "string", + }); + + is( + Services.prefs.getCharPref(prefName), + "experiment value", + "Starting an experiment should change the pref", + ); + + // Now pretend that firefox has updated and restarted to a version + // where fake.preference has been removed in the default pref set. + // Bootstrap has run and changed the pref to the experimental + // value, and produced the call to recordOriginalValues below. + PreferenceExperiments.recordOriginalValues({ [prefName]: null }); + is( + Services.prefs.getCharPref(prefName), + "experiment value", + "Recording original values shouldn't affect the preference." + ); + + // Now stop the experiment. It should remove the preference + await PreferenceExperiments.stop("test"); + is( + Services.prefs.getCharPref(prefName, "DEFAULT"), + "DEFAULT", + "Preference should be absent", + ); + }, +); diff --git a/recipe-client-addon/test/browser/browser_bootstrap.js b/recipe-client-addon/test/browser/browser_bootstrap.js index c0718b62..6bd2755e 100644 --- a/recipe-client-addon/test/browser/browser_bootstrap.js +++ b/recipe-client-addon/test/browser/browser_bootstrap.js @@ -1,6 +1,7 @@ "use strict"; Cu.import("resource://shield-recipe-client/lib/ShieldRecipeClient.jsm", this); +Cu.import("resource://shield-recipe-client/lib/PreferenceExperiments.jsm", this); // We can't import bootstrap.js directly since it isn't in the jar manifest, but // we can use Addon.getResourceURI to get a path to the file and import using @@ -23,6 +24,12 @@ function withBootstrap(testFunction) { const initPref1 = "test.initShieldPrefs1"; const initPref2 = "test.initShieldPrefs2"; const initPref3 = "test.initShieldPrefs3"; + +const experimentPref1 = "test.initExperimentPrefs1"; +const experimentPref2 = "test.initExperimentPrefs2"; +const experimentPref3 = "test.initExperimentPrefs3"; +const experimentPref4 = "test.initExperimentPrefs4"; + decorate_task( withPrefEnv({ clear: [[initPref1], [initPref2], [initPref3]], @@ -80,9 +87,6 @@ decorate_task( }, ); -const experimentPref1 = "test.initExperimentPrefs1"; -const experimentPref2 = "test.initExperimentPrefs2"; -const experimentPref3 = "test.initExperimentPrefs3"; decorate_task( withPrefEnv({ set: [ @@ -204,3 +208,47 @@ decorate_task( } }, ); + +// During startup, preferences that are changed for experiments should +// be record by calling PreferenceExperiments.recordOriginalValues. +decorate_task( + withPrefEnv({ + set: [ + [`extensions.shield-recipe-client.startupExperimentPrefs.${experimentPref1}`, true], + [`extensions.shield-recipe-client.startupExperimentPrefs.${experimentPref2}`, 2], + [`extensions.shield-recipe-client.startupExperimentPrefs.${experimentPref3}`, "string"], + [`extensions.shield-recipe-client.startupExperimentPrefs.${experimentPref4}`, "another string"], + ], + clear: [ + [experimentPref1], + [experimentPref2], + [experimentPref3], + [experimentPref4], + ["extensions.shield-recipe-client.startupExperimentPrefs.existingPref"], + ], + }), + withBootstrap, + withStub(PreferenceExperiments, "recordOriginalValues"), + async function testInitExperimentPrefs(Bootstrap, recordOriginalValuesStub) { + const defaultBranch = Services.prefs.getDefaultBranch(""); + + defaultBranch.setBoolPref(experimentPref1, false); + defaultBranch.setIntPref(experimentPref2, 1); + defaultBranch.setCharPref(experimentPref3, "original string"); + // experimentPref4 is left unset + + Bootstrap.initExperimentPrefs(); + await Bootstrap.finishStartup(); + + Assert.deepEqual( + recordOriginalValuesStub.getCall(0).args, + [{ + [experimentPref1]: false, + [experimentPref2]: 1, + [experimentPref3]: "original string", + [experimentPref4]: null, // null because it was not initially set. + }], + "finishStartup should record original values of the prefs initExperimentPrefs changed", + ); + }, +);