Bug 1907649 - Unenroll if prefFlips feature cannot set prefs r=chumphreys

If the prefFlips feature receives a configuration that conflicts with the local
pref store (e.g., a pref has a default branch value of one type and the feature
configuration contains that same pref with a value of a different type), then
the call to set the pref will throw. We now catch those errors and trigger
unenrollment with reason `prefFlips-failed`.

Differential Revision: https://phabricator.services.mozilla.com/D217502
This commit is contained in:
Beth Rennie 2024-07-24 18:11:23 +00:00
Родитель 35ed43c92f
Коммит a5f539c99d
5 изменённых файлов: 621 добавлений и 54 удалений

Просмотреть файл

@ -2,7 +2,10 @@
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */
import { PrefFlipsFeature } from "resource://nimbus/lib/PrefFlipsFeature.sys.mjs";
import {
PrefFlipsFeature,
REASON_PREFFLIPS_FAILED,
} from "resource://nimbus/lib/PrefFlipsFeature.sys.mjs";
const lazy = {};
@ -713,6 +716,8 @@ export class _ExperimentManager {
changedPref = undefined,
duringRestore = false,
conflictingSlug = undefined,
prefName = undefined,
prefType = undefined,
} = {}
) {
const { slug } = enrollment;
@ -744,7 +749,8 @@ export class _ExperimentManager {
typeof changedPref !== "undefined"
? { changedPref: changedPref.name }
: {},
typeof conflictingSlug !== "undefined" ? { conflictingSlug } : {}
typeof conflictingSlug !== "undefined" ? { conflictingSlug } : {},
reason === REASON_PREFFLIPS_FAILED ? { prefType, prefName } : {}
)
);
// Sent Glean event equivalent
@ -760,6 +766,12 @@ export class _ExperimentManager {
: {},
typeof conflictingSlug !== "undefined"
? { conflicting_slug: conflictingSlug }
: {},
reason === REASON_PREFFLIPS_FAILED
? {
pref_type: prefType,
pref_name: prefName,
}
: {}
)
);

Просмотреть файл

@ -10,6 +10,7 @@ ChromeUtils.defineESModuleGetters(lazy, {
});
const FEATURE_ID = "prefFlips";
export const REASON_PREFFLIPS_FAILED = "prefFlips-failed";
export class PrefFlipsFeature {
#initialized;
@ -40,53 +41,76 @@ export class PrefFlipsFeature {
const prefs = lazy.NimbusFeatures[FEATURE_ID].getVariable("prefs") ?? {};
for (const [pref, details] of this._prefs.entries()) {
if (Object.hasOwn(prefs, pref)) {
// The pref may have changed.
const newDetails = prefs[pref];
try {
for (const [pref, details] of this._prefs.entries()) {
if (Object.hasOwn(prefs, pref)) {
// The pref may have changed.
const newDetails = prefs[pref];
if (
newDetails.branch !== details.branch ||
newDetails.value !== details.value
) {
this._updatePref(pref, newDetails);
if (
newDetails.branch !== details.branch ||
newDetails.value !== details.value
) {
this._updatePref({
pref,
branch: newDetails.branch,
value: newDetails.value,
slug: activeEnrollment.slug,
});
}
} else {
// The pref is no longer controlled by us.
this._unregisterPref(pref);
}
} else {
// The pref is no longer controlled by us.
this._unregisterPref(pref);
}
} catch (e) {
if (e instanceof PrefFlipsFailedError) {
this.#updating = false;
this._unenrollForFailure(activeEnrollment, e.pref);
return;
}
}
for (const [pref, { branch, value }] of Object.entries(prefs)) {
const known = this._prefs.get(pref);
if (known) {
// We have already processed this pref.
continue;
}
const setPref = this.manager._prefs.get(pref);
if (setPref) {
const toUnenroll = Array.from(setPref.slugs.values()).map(slug =>
this.manager.store.get(slug)
);
if (toUnenroll.length === 2 && !toUnenroll[0].isRollout) {
toUnenroll.reverse();
try {
for (const [pref, { branch, value }] of Object.entries(prefs)) {
const known = this._prefs.get(pref);
if (known) {
// We have already processed this pref.
continue;
}
for (const enrollment of toUnenroll) {
this.manager._unenroll(enrollment, {
reason: "prefFlips-conflict",
conflictingSlug: activeEnrollment.slug,
});
}
}
const setPref = this.manager._prefs.get(pref);
if (setPref) {
const toUnenroll = Array.from(setPref.slugs.values()).map(slug =>
this.manager.store.get(slug)
);
this._registerPref(
pref,
{ branch, value },
lazy.PrefUtils.getPref(pref, { branch })
);
if (toUnenroll.length === 2 && !toUnenroll[0].isRollout) {
toUnenroll.reverse();
}
for (const enrollment of toUnenroll) {
this.manager._unenroll(enrollment, {
reason: "prefFlips-conflict",
conflictingSlug: activeEnrollment.slug,
});
}
}
this._registerPref({
pref,
branch,
value,
originalValue: lazy.PrefUtils.getPref(pref, { branch }),
slug: activeEnrollment.slug,
});
}
} catch (e) {
this.#updating = false;
this._unenrollForFailure(activeEnrollment, e.pref);
return;
}
// If this is new enrollment, we need to cache the original values of prefs
@ -126,12 +150,23 @@ export class PrefFlipsFeature {
const featureValue = activeEnrollment.branch.features.find(
fc => fc.featureId === FEATURE_ID
).value;
for (const [pref, entry] of Object.entries(featureValue.prefs)) {
this._registerPref(
pref,
entry,
activeEnrollment.prefFlips.originalValues[pref]
);
try {
for (const [pref, { branch, value }] of Object.entries(
featureValue.prefs
)) {
this._registerPref({
pref,
branch,
value,
originalValue: activeEnrollment.prefFlips.originalValues[pref],
slug: activeEnrollment.slug,
});
}
} catch (e) {
if (e instanceof PrefFlipsFailedError) {
this._unenrollForFailure(activeEnrollment, e.pref);
return;
}
}
}
@ -142,7 +177,7 @@ export class PrefFlipsFeature {
this.#initialized = true;
}
_registerPref(pref, { branch, value }, originalValue) {
_registerPref({ pref, branch, value, originalValue, slug }) {
const observer = (_aSubject, _aTopic, aData) => {
// This observer will be called for changes to `name` as well as any
// other pref that begins with `name.`, so we have to filter to
@ -164,14 +199,20 @@ export class PrefFlipsFeature {
originalValue,
value: value ?? null,
observer,
slug,
};
lazy.PrefUtils.setPref(pref, value ?? null, { branch });
try {
lazy.PrefUtils.setPref(pref, value ?? null, { branch });
} catch (e) {
throw new PrefFlipsFailedError(pref);
}
Services.prefs.addObserver(pref, observer);
this._prefs.set(pref, entry);
}
_updatePref(pref, { branch, value }) {
_updatePref({ pref, branch, value, slug }) {
const entry = this._prefs.get(pref);
if (!entry) {
return;
@ -182,6 +223,9 @@ export class PrefFlipsFeature {
let originalValue = entry.originalValue;
if (entry.branch !== branch) {
// Restore the value on the previous branch.
//
// Because we were able to set the pref, it must have the same type as the
// originalValue, so this will also succeed.
lazy.PrefUtils.setPref(pref, entry.originalValue, {
branch: entry.branch,
});
@ -193,9 +237,14 @@ export class PrefFlipsFeature {
branch,
value,
originalValue,
slug,
});
lazy.PrefUtils.setPref(pref, value, { branch });
try {
lazy.PrefUtils.setPref(pref, value, { branch });
} catch (e) {
throw new PrefFlipsFailedError(pref);
}
Services.prefs.addObserver(pref, entry.observer);
}
@ -318,4 +367,39 @@ export class PrefFlipsFeature {
}
return "user";
}
_unenrollForFailure(enrollment, pref) {
const rawType = Services.prefs.getPrefType(pref);
let prefType = "invalid";
switch (rawType) {
case Ci.nsIPrefBranch.PREF_BOOL:
prefType = "bool";
break;
case Ci.nsIPrefBranch.PREF_STRING:
prefType = "string";
break;
case Ci.nsIPrefBranch.PREF_INT:
prefType = "int";
break;
}
this.manager._unenroll(enrollment, {
reason: REASON_PREFFLIPS_FAILED,
prefName: pref,
prefType,
});
}
}
/**
* Thrown when the prefFlips feature fails to set a pref.
*/
class PrefFlipsFailedError extends Error {
constructor(pref, value) {
super(`The Nimbus prefFlips feature failed to set ${pref}=${value}`);
this.pref = pref;
}
}

Просмотреть файл

@ -98,16 +98,27 @@ nimbus_events:
description: >
If reason == "prefFlips-conflict", the slug of the conflicting
experiment that caused the unenrollment.
pref_name:
type: string
description: >
If reason == "prefFlips-failed", the name of the pref that failed to set.
pref_type:
type: string
description: >
If reason == "prefFlips-failed", The type of the existing pref value
(one of "bool", "string", "int", or "invalid").
bugs:
- https://bugzilla.mozilla.org/show_bug.cgi?id=1773563
- https://bugzilla.mozilla.org/show_bug.cgi?id=1781953
- https://bugzilla.mozilla.org/show_bug.cgi?id=1843126
- https://bugzilla.mozilla.org/show_bug.cgi?id=1896718
- https://bugzilla.mozilla.org/show_bug.cgi?id=1907649
data_reviews:
- https://bugzilla.mozilla.org/show_bug.cgi?id=1773563
- https://bugzilla.mozilla.org/show_bug.cgi?id=1781953
- https://bugzilla.mozilla.org/show_bug.cgi?id=1843126
- https://bugzilla.mozilla.org/show_bug.cgi?id=1896718
- https://bugzilla.mozilla.org/show_bug.cgi?id=1907649
data_sensitivity:
- technical
notification_emails:

Просмотреть файл

@ -2215,7 +2215,459 @@ add_task(async function test_prefFlips_restore_unenroll() {
"pref unset after unenrollment"
);
await assertEmptyStore(manager.store);
await assertEmptyStore(manager.store, { cleanup: true });
sandbox.restore();
});
add_task(async function test_prefFlips_failed() {
const PREF = "test.pref.please.ignore";
Services.fog.testResetFOG();
Services.telemetry.snapshotEvents(
Ci.nsITelemetry.DATASET_PRERELEASE_CHANNELS,
/* clear = */ true
);
Services.prefs.getDefaultBranch(null).setStringPref(PREF, "test-value");
const sandbox = sinon.createSandbox();
const manager = ExperimentFakes.manager();
sandbox.stub(ExperimentAPI, "_manager").get(() => manager);
sandbox.stub(ExperimentAPI, "_store").get(() => manager.store);
await manager.onStartup();
const recipe = ExperimentFakes.recipe("prefFlips-test", {
branches: [
{
...ExperimentFakes.recipe.branches[0],
features: [
{
featureId: FEATURE_ID,
value: {
prefs: {
[PREF]: { branch: "user", value: 123 },
},
},
},
],
},
],
bucketConfig: {
...ExperimentFakes.recipe.bucketConfig,
count: 1000,
},
});
await manager.enroll(recipe);
const enrollment = manager.store.get(recipe.slug);
Assert.ok(!enrollment.active, "Experiment should not be active");
Assert.equal(Services.prefs.getStringPref(PREF), "test-value");
TelemetryTestUtils.assertEvents(
[
{
value: recipe.slug,
extra: {
reason: "prefFlips-failed",
prefName: PREF,
prefType: "string",
},
},
],
{
category: "normandy",
object: "nimbus_experiment",
method: "unenroll",
}
);
Assert.deepEqual(
Glean.nimbusEvents.unenrollment.testGetValue().map(event => ({
reason: event.extra.reason,
experiment: event.extra.experiment,
pref_name: event.extra.pref_name,
pref_type: event.extra.pref_type,
})),
[
{
reason: "prefFlips-failed",
experiment: recipe.slug,
pref_name: PREF,
pref_type: "string",
},
]
);
Services.prefs.deleteBranch(PREF);
await assertEmptyStore(manager.store);
});
add_task(async function test_prefFlips_failed_multiple_prefs() {
const GOOD_PREF = "test.pref.please.ignore";
const BAD_PREF = "this.one.too";
Services.fog.testResetFOG();
Services.telemetry.snapshotEvents(
Ci.nsITelemetry.DATASET_PRERELEASE_CHANNELS,
/* clear = */ true
);
Services.prefs.getDefaultBranch(null).setStringPref(BAD_PREF, "test-value");
const sandbox = sinon.createSandbox();
const manager = ExperimentFakes.manager();
sandbox.stub(ExperimentAPI, "_manager").get(() => manager);
sandbox.stub(ExperimentAPI, "_store").get(() => manager.store);
const setPrefSpy = sandbox.spy(PrefUtils, "setPref");
await manager.onStartup();
const recipe = ExperimentFakes.recipe("prefFlips-test", {
branches: [
{
...ExperimentFakes.recipe.branches[0],
features: [
{
featureId: FEATURE_ID,
value: {
prefs: {
[GOOD_PREF]: { branch: USER, value: 123 },
[BAD_PREF]: { branch: USER, value: 123 },
},
},
},
],
},
],
bucketConfig: {
...ExperimentFakes.recipe.bucketConfig,
count: 1000,
},
});
await manager.enroll(recipe);
const enrollment = manager.store.get(recipe.slug);
Assert.ok(!enrollment.active, "Experiment should not be active");
Assert.deepEqual(
setPrefSpy.getCall(0).args,
[GOOD_PREF, 123, { branch: USER }],
`should set ${GOOD_PREF}`
);
Assert.deepEqual(
setPrefSpy.getCall(1).args,
[BAD_PREF, 123, { branch: USER }],
`should have attempted to set ${BAD_PREF}`
);
Assert.ok(
typeof setPrefSpy.getCall(1).exception !== "undefined",
`Attempting to set ${BAD_PREF} threw`
);
Assert.deepEqual(
setPrefSpy.getCall(2).args,
[GOOD_PREF, null, { branch: USER }],
`should reset ${GOOD_PREF}`
);
Assert.equal(
setPrefSpy.callCount,
3,
"should have 3 calls to PrefUtils.setPref"
);
Assert.ok(
!Services.prefs.prefHasUserValue(GOOD_PREF),
`${GOOD_PREF} should not be set`
);
Assert.equal(Services.prefs.getStringPref(BAD_PREF), "test-value");
Services.prefs.deleteBranch(GOOD_PREF);
Services.prefs.deleteBranch(BAD_PREF);
await assertEmptyStore(manager.store);
sandbox.reset();
});
add_task(async function test_prefFlips_failed_experiment_and_rollout() {
const ROLLOUT = "rollout";
const EXPERIMENT = "experiment";
const PREFS = {
[ROLLOUT]: "test.nimbus.prefs.rollout",
[EXPERIMENT]: "test.nimbus.prefs.experiment",
};
const VALUES = {
[ROLLOUT]: "rollout-value",
[EXPERIMENT]: "experiment-value",
};
const BOGUS_VALUE = 123;
const TEST_CASES = [
{
name: "Enrolling in an experiment and then a rollout with errors",
setPrefsBefore: {
[PREFS[ROLLOUT]]: { defaultBranchValue: BOGUS_VALUE },
},
enrollmentOrder: [EXPERIMENT, ROLLOUT],
expectedEnrollments: [EXPERIMENT, ROLLOUT],
expectedUnenrollments: [],
expectedPrefs: {
[PREFS[EXPERIMENT]]: VALUES[EXPERIMENT],
[PREFS[ROLLOUT]]: BOGUS_VALUE,
},
},
{
name: "Enrolling in a rollout and then an experiment with errors",
setPrefsBefore: {
[PREFS[EXPERIMENT]]: { defaultBranchValue: BOGUS_VALUE },
},
enrollmentOrder: [ROLLOUT, EXPERIMENT],
expectedEnrollments: [ROLLOUT],
expectedUnenrollments: [EXPERIMENT],
expectedPrefs: {
[PREFS[ROLLOUT]]: VALUES[ROLLOUT],
[PREFS[EXPERIMENT]]: BOGUS_VALUE,
},
},
];
const FEATURE_VALUES = {
[EXPERIMENT]: {
prefs: {
[PREFS[EXPERIMENT]]: {
value: VALUES[EXPERIMENT],
branch: USER,
},
},
},
[ROLLOUT]: {
prefs: {
[PREFS[ROLLOUT]]: {
value: VALUES[ROLLOUT],
branch: USER,
},
},
},
};
for (const [i, { name, ...testCase }] of TEST_CASES.entries()) {
info(`Running test case ${i}: ${name}`);
const {
setPrefsBefore,
enrollmentOrder,
expectedEnrollments,
expectedUnenrollments,
expectedPrefs,
} = testCase;
const sandbox = sinon.createSandbox();
const manager = ExperimentFakes.manager();
sandbox.stub(ExperimentAPI, "_manager").get(() => manager);
sandbox.stub(ExperimentAPI, "_store").get(() => manager.store);
await manager.onStartup();
info("Setting initial values of prefs...");
setPrefs(setPrefsBefore);
info("Enrolling...");
for (const slug of enrollmentOrder) {
await ExperimentFakes.enrollWithFeatureConfig(
{
featureId: FEATURE_ID,
value: FEATURE_VALUES[slug],
},
{
manager,
slug,
isRollout: slug === ROLLOUT,
}
);
}
info("Checking expected enrollments...");
for (const slug of expectedEnrollments) {
const enrollment = manager.store.get(slug);
Assert.ok(enrollment.active, "The enrollment is active.");
}
info("Checking expected unenrollments...");
for (const slug of expectedUnenrollments) {
const enrollment = manager.store.get(slug);
Assert.ok(!enrollment.active, "The enrollment is no longer active.");
}
info("Checking expected prefs...");
checkExpectedPrefs(expectedPrefs);
info("Unenrolling...");
if (expectedEnrollments.includes(ROLLOUT)) {
manager.unenroll(ROLLOUT, "test-cleanup");
}
if (expectedEnrollments.includes(EXPERIMENT)) {
manager.unenroll(EXPERIMENT, "test-cleanup");
}
info("Cleaning up...");
Services.prefs.deleteBranch(PREFS[ROLLOUT]);
Services.prefs.deleteBranch(PREFS[EXPERIMENT]);
await assertEmptyStore(manager.store);
assertNoObservers(manager);
sandbox.restore();
}
});
add_task(async function test_prefFlips_update_failure() {
const sandbox = sinon.createSandbox();
const manager = ExperimentFakes.manager();
sandbox.stub(ExperimentAPI, "_manager").get(() => manager);
sandbox.stub(ExperimentAPI, "_store").get(() => manager.store);
await manager.onStartup();
PrefUtils.setPref("pref.one", "default-value", { branch: DEFAULT });
PrefUtils.setPref("pref.two", "default-value", { branch: DEFAULT });
const doCleanup = await ExperimentFakes.enrollWithFeatureConfig(
{
featureId: FEATURE_ID,
value: {
prefs: {
"pref.one": { value: "one", branch: USER },
"pref.two": { value: "two", branch: USER },
},
},
},
{ manager, isRollout: true, slug: "rollout" }
);
Assert.equal(Services.prefs.getStringPref("pref.one"), "one");
Assert.equal(Services.prefs.getStringPref("pref.two"), "two");
await ExperimentFakes.enrollWithFeatureConfig(
{
featureId: FEATURE_ID,
value: {
prefs: {
"pref.one": { value: "experiment-value", branch: USER },
"pref.two": { value: 2, branch: USER },
},
},
},
{ manager, slug: "experiment" }
);
const rolloutEnrollment = manager.store.get("rollout");
const experimentEnrollment = manager.store.get("experiment");
Assert.ok(rolloutEnrollment.active, "Rollout is active");
Assert.ok(!experimentEnrollment.active, "Experiment is inactive");
Assert.equal(experimentEnrollment.unenrollReason, "prefFlips-failed");
Assert.equal(Services.prefs.getStringPref("pref.one"), "one");
Assert.equal(Services.prefs.getStringPref("pref.two"), "two");
Services.prefs.deleteBranch("pref.one");
Services.prefs.deleteBranch("pref.two");
doCleanup();
await assertEmptyStore(manager.store);
assertNoObservers(manager);
sandbox.restore();
});
// Test the case where an experiment sets a default branch pref, but the user
// changed their user.js between restarts.
add_task(async function test_prefFlips_restore_failure() {
const PREF = "foo.bar.baz";
const recipe = ExperimentFakes.recipe("prefFlips-test", {
branches: [
{
...ExperimentFakes.recipe.branches[0],
features: [
{
featureId: FEATURE_ID,
value: {
prefs: {
[PREF]: {
branch: DEFAULT,
value: "recipe-value",
},
},
},
},
],
},
],
bucketConfig: {
...ExperimentFakes.recipe.bucketConfig,
count: 1000,
},
});
{
const prevEnrollment = {
slug: recipe.slug,
branch: recipe.branches[0],
active: true,
experimentType: "nimbus",
userFacingName: recipe.userFacingName,
userFacingDescription: recipe.userFacingDescription,
featureIds: recipe.featureIds,
isRollout: recipe.isRollout,
localizations: recipe.localizations,
source: "rs-loader",
prefFlips: {
originalValues: {
[PREF]: "original-value",
},
},
};
const store = ExperimentFakes.store();
await store.init();
await store.ready();
store.set(prevEnrollment.slug, prevEnrollment);
store._store.saveSoon();
await store._store.finalize();
}
Services.prefs.setIntPref(PREF, 123);
const sandbox = sinon.createSandbox();
const manager = ExperimentFakes.manager();
sandbox.stub(ExperimentAPI, "_manager").get(() => manager);
sandbox.stub(ExperimentAPI, "_store").get(() => manager.store);
await manager.onStartup();
const enrollment = manager.store.get(recipe.slug);
Assert.ok(!enrollment.active, "Enrollment should be inactive");
Assert.equal(enrollment.unenrollReason, "prefFlips-failed");
Assert.ok(
!Services.prefs.prefHasDefaultValue(PREF),
"pref has no default value"
);
Assert.equal(Services.prefs.getIntPref(PREF), 123, "pref value unchanged");
await assertEmptyStore(manager.store, { cleanup: true });
assertNoObservers(manager);
Services.prefs.deleteBranch(PREF);
});

Просмотреть файл

@ -816,8 +816,16 @@ normandy:
conflictingSlug: >
For nimbus_experiment, if reason == "prefFlips-conflict", the conflicting
experiment that caused the unenrollment.
bug_numbers: [1443560, 1843126, 1896718]
notification_emails: ["normandy-notifications@mozilla.com"]
prefName: >
For nimbus_experiment, if reason == "prefFlips-failed", the name of the
pref that failed to set.
prefType: >
For nimbus_experiment, if reason = "prefFlips-failed", the type of the
existing pref value (one of "bool", "string", "int", or "invalid").
bug_numbers: [1443560, 1843126, 1896718, 1907649]
notification_emails:
- "normandy-notifications@mozilla.com"
- "nimbus-team@mozilla.com"
products:
- "firefox"
- "fennec"