Closed Bug 1440782 Opened 6 years ago Closed 6 years ago

Add preference-rollout internal action

Categories

(Firefox :: Normandy Client, enhancement, P1)

enhancement

Tracking

()

VERIFIED FIXED
Firefox 61
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- unaffected
firefox60 --- unaffected
firefox61 blocking verified

People

(Reporter: mythmon, Assigned: mythmon)

References

Details

Attachments

(3 files)

This bug covers adding a new internal action (see bug 1436129) "preference-rollout". Briefly, preference rollout changes a set of prefs in a permanent way. That is, it does not automatically rollback when the recipe is disabled.

There should be a separate rollback action in case we want to undo a preference rollout.

Preference roll-out is not intended to be used for experiments. It should not show up in about:studies, though it may be desirable for it to show up in about:support.

For more details, see this blueprint document: https://docs.google.com/document/d/1qFCOscDiMGVDcBYtznzgPNCWzOhaxC4X5VGt3LTjZY0/edit#heading=h.eoqr3si0tn8z
It needs to show up in telemetry somehow, otherwise you can't tell if people who are experiencing problems have the pref flipped.
> It needs to show up in telemetry somehow, otherwise you can't tell if people who are experiencing problems have the pref flipped.

Definitely. This wasn't captured well in the blueprint from comment 0, but I've added it now.

Sunah, will it work if we re-use the same experiment annotation we use for preference studies, with a unique type? Today we have type=exp and type=exp-highpop. This would probably add type=rollout. Does that sound fine for you? We would likely be setting this preference for upwards of 100% of Firefox users at times.
Flags: needinfo?(ssuh)
Yes, that would be fine on our end -- we ignore any unknown types in the `telemetry-cohorts` dataset and the `experiments` dataset so that works well for us.

Will the user ever be unenrolled, or will they be tagged in `environment/experiments` forever? An alternative is to add these pref values to `environment/userPrefs`
Flags: needinfo?(ssuh)
> Will the user ever be unenrolled, or will they be tagged in `environment/experiments` forever? An alternative is to add these pref values to `environment/userPrefs`

I'm not sure yet. I think that users won't be tagged forever, as eventually a version of Firefox should be released that has a default value that matches the rollout. Of course, if a manual rollback happens, then the user would be unenrolled early, but this should be an exceptional case. However, until either the update or the rollback happen, I would expect users to be tagged for a long time. I wouldn't be surprised if a tag like this lasts for about 3 versions.

Is `environments/userPrefs` appropriate if the preference change is happening on the default branch?

How does telemetry handle system add-ons? I expect a similar approach may be useful here, since preference rollouts are actually expected to replace many simple system add-ons.
System addons are listed with the other active addons, so there's precedent for having this data even though it should be the same across the userbase. I don't think there's any issue with adding these with the new type, besides slight regret on the naming of the "experiments" block :)
Priority: P3 → P1
Assignee: nobody → mcooper
Comment on attachment 8965467 [details]
Bug 1440782 Part 2 - Add preference-rollout action to Normandy

https://reviewboard.mozilla.org/r/234218/#review239844


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/components/enterprisepolicies/tests/browser/browser_policies_validate_and_parse_API.js:366
(Diff revision 1)
> +  is(parsed, 1, "Parsed correctly");
> +  ok(valid && parsed === 1, "Parsed number value correctly");
> +
> +  [valid, parsed] = PoliciesValidator.validateAndParseParameters([1, 2, 3], schema);
> +  ok(valid, "Array is valid");
> +  Assert.deepEqual(parsed, [1,2, 3], "Parsed correctly");

Error: A space is required after ','. [eslint: comma-spacing]
Comment on attachment 8965467 [details]
Bug 1440782 Part 2 - Add preference-rollout action to Normandy

This is a rough draft of preference rollout. It is missing a few key things, detailed below. I'll be on PTO tomorrow and all next week, so I would like to get early feedback that I can incorporate when I get back into a final version of this patch. Time is a bit short for this, since we really want to land this in Firefox 61 to support rollout of important features. The window for that is rapidly closing.

To summarize, QA and Relman are aware of this plan and the timing. It is certainly less than ideal, but I think it may work.

Gijs: I'm hoping that by putting this up for review, we can shorten the review time when I finish this patch and get it in the hands of QA more quickly. I'm planning to have the final version of this patch ready by EOD Wednesday, April 18th, and hopefully get it reviewed and landed by EOD Friday, April 20th. Does that timeline sound ok to you?

Things that still need work (that I know of):

* There are several TODO comments for small things that need finished
* There are no tests for preference-rollout
* None of the Telemetry integration is written
* preference-rollback, the pair action that goes with this, is entirely missing. The blueprint doc has more details as to how it would work.
* I'm borrowing the JSON schema validator from Policy Engine. That probably isn't kosher since that is in browser/ and Normandy is in toolkit/. I need to look into this more.
Attachment #8965467 - Flags: feedback?(gijskruitbosch+bugs)
Felipe: In the above patch, I'm using the JSON schema validator you implemented for Policy Engine. I also made a couple changes to bring it more in line with the JSON schema spec. Specifically, I added support for the types of values to be an array. Do you think that's ok? Do you think it is ok for Normandy to use it? Should we do something different here?
Flags: needinfo?(felipc)
Comment on attachment 8965467 [details]
Bug 1440782 Part 2 - Add preference-rollout action to Normandy

Reviewboard cleared the f? flag. Putting it back.
Attachment #8965467 - Flags: feedback?(gijskruitbosch+bugs)
Status: NEW → ASSIGNED
Comment on attachment 8965467 [details]
Bug 1440782 Part 2 - Add preference-rollout action to Normandy

https://reviewboard.mozilla.org/r/234218/#review240126

On the whole this seems like it's heading in the right direction. I'm not sure about finishing this all by April 20 if you're away... but we do have a bit more time than that within 61.

One of the main pieces of feedback from my perspective is that PrefUtils seems like it's reimplementing Preferences.jsm, and that we're over-rotating on the exact type of the preference. I think we should just infer the type from the rolled-out value (or the pref type of the extant pref, for pref getting) everywhere. If the type of the rolled-out value is wrong, that's going to cause issues regardless, and adding type information everywhere doesn't really help that.

One of the other things, looking at the blueprint, is that it says this stuff must show up in telemetry somehow. How are we planning to do that? I'm asking in particular because this is going to be modifying the default branch prefs, I don't think changes will show up in about:support because Firefox won't "know" that the pref has changed. I suspect the same will be true for telemetry environment (but I could be wrong about this).

Which brings me to my next point: the blueprint in parts ("In the case of user-branch preferences, store the old value so we can undo the change") implies that we can do user-branch rollouts, but AFAICT the code is hardcoded to use the default branch. Is that right? Then maybe the doc needs updating? The code comments ("The value the preference would have on the default branch if this study wasn't active") also are a bit confusing (this isn't a study, in a sense) and don't match the document ("In the case of default-branch preferences, the original value doesn't need to be stored, since unenrollment is simply not setting it again."). I assume all of this just means reconciling all the different bits (doc, code, comments) to agree with each other, so shouldn't be major, but I think felipe knows from experience that dealing with issues with rollouts after they happen is a giant pain in the ... so having the docs, comments and code all agree is going to be very important when, almost inevitably, some of this might not work perfectly because we mess up somewhere, somehow, sometime.

Going to let felipe deal with the schema thing, but I expect the answer is "move (parts of) the schema validator to toolkit" (ideally in a separate cset from the rest of this implementation :-) ).

::: toolkit/components/normandy/actions/BaseAction.jsm:125
(Diff revision 2)
>      try {
> -      this._finalize();
> +      await this._finalize();
>      } catch (err) {
>        status = Uptake.ACTION_POST_EXECUTION_ERROR;
> -      this.log.info(`Could not run postExecution hook for ${this.name}: ${err.message}`);
> +      err.message = `Could not run postExecution hook for ${this.name}: ${err.message}`;
> +      Cu.reportError(err);

I don't really mind either way, but out of interest, why the switch to Cu.reportError?

::: toolkit/components/normandy/actions/ConsoleLogAction.jsm:1
(Diff revision 2)
> +/* This Source Code Form is subject to the terms of the Mozilla Public

I assume this file is moved with no changes from `ConsoleLog.jsm`, but hg doesn't know it is. Please fix before landing.

::: toolkit/components/normandy/actions/PreferenceRolloutAction.jsm:30
(Diff revision 2)
> +    const existingManagedPrefs = new Set();
> +    for (const rollout of await PreferenceRollouts.getAllActive()) {
> +      if (rollout.slug === args.slug) {
> +        continue;
> +      }
> +      for (const prefSpec of rollout.preferences) {
> +        existingManagedPrefs.add(prefSpec.preferenceName);
> +      }
> +    }

A comment before this set of code would be quite helpful. Something like:

```js
// First determine which preferences we're already managing.
// New recipes are not allowed to manage already-managed preferences.
```

::: toolkit/components/normandy/actions/PreferenceRolloutAction.jsm:61
(Diff revision 2)
> +      preferences: args.preferences.map(({preferenceName, preferenceType, value}) => ({
> +        preferenceName,
> +        preferenceType,
> +        value,
> +        previousValue: PrefUtils.getPref("default", preferenceName, preferenceType),
> +      })),

Slightly simpler:

```js
(prefObj => Object.assign({previousValue: ...}, prefObj))
```

::: toolkit/components/normandy/actions/PreferenceRolloutAction.jsm:71
(Diff revision 2)
> +      const oldPrefs = new Map();
> +      const newPrefs = new Map();
> +      for (const prefSpec of existingRollout.preferences) {
> +        oldPrefs.set(prefSpec.preferenceName, prefSpec);
> +      }

Nit: `const prefNames = new Set(<whatever>.preferences.map(p => p.preferenceName));`

for both of these maps, I think, and then in the 2 loops, just iterate over `<whatever>` (ie the objects, sans 'key') and check if `preferenceName` occurs in the other `Set` with `.has()` .

::: toolkit/components/normandy/actions/PreferenceRolloutAction.jsm:93
(Diff revision 2)
> +        if (!oldPrefs.has(key)) {
> +          anyChanged = true;
> +          PrefUtils.setPref("default", preferenceName, preferenceType, value);
> +        }
> +      }
> +      if (anyChanged) {

I wonder if, to keep this block + method easier to grasp, it'd make sense to put the "have any of the prefs changed") determination in its own method. I know I'm a curmudgeon, but a 50-line if/else block seems a bit much to me, and the root of the pref comparison looks to me like it can be separated out cleanly.

::: toolkit/components/normandy/actions/PreferenceRolloutAction.jsm:101
(Diff revision 2)
> +        } else {
> +          Cu.reportError(new Error(`Updated pref rollout in unexpected state: ${existingRollout.state}`));
> +        }

So rolled back rollouts would hit this, wouldn't they? That seems suboptimal... Are we just keeping this until rollback is implemented, or something?

Nit: also, maybe this should be a switch statement given it's comparing the same stuff? :-)

::: toolkit/components/normandy/actions/PreferenceRolloutAction.jsm:111
(Diff revision 2)
> +      for (const {preferenceType, preferenceName, value} of args.preferences) {
> +        PrefUtils.setPref("default", preferenceName, preferenceType, value);
> +      }

I wonder if there are ever prefs where we should avoid this and only set them on startup. Then again, I guess that would cause grief for all the poor mac users who never restart. Damned if you do/don't... but perhaps worth documenting that we will set prefs at runtime, too, not just at startup.

::: toolkit/components/normandy/actions/PreferenceRolloutAction.jsm:122
(Diff revision 2)
> +    }
> +
> +  }
> +
> +  async _finalize() {
> +    await PreferenceRollouts.saveStartupPrefs();

Hypothetically, what happens if we run an action that adds a rollout, and Firefox shuts down before we run `_finalize` (either gracefully or otherwise) ?

::: toolkit/components/normandy/lib/PrefUtils.jsm:41
(Diff revision 2)
> +   * @param {string} type
> +   *   One of "string", "boolean", or "integer"
> +   * @param {string|boolean|integer|null} [default]
> +   *   The value to return if the preference does not exist. Defaults to null.
> +   */
> +  getPref(branchName, pref, type, defaultValue = null) {

What does passing the pref type add here?

It seems to me the consumers of the studies would be much simpler if they just set the pref to the expected value and inferred the type from that expected value.

Here, you're already calling `branch.getPrefType`, so we might as well use that information to get the pref, rather than passing it along everywhere?

::: toolkit/components/normandy/lib/PreferenceRollouts.jsm:91
(Diff revision 2)
> +
> +  /**
> +   * Update the rollout database with changes that happened during early startup.
> +   * @param {object} rolloutPrefsChanged Map from pref name to previous pref value
> +   */
> +  async recordOriginalValues(studyPrefsChanged) {

Nit: update argument name

::: toolkit/components/normandy/lib/PreferenceRollouts.jsm:104
(Diff revision 2)
> +          if (prefSpec.previousValue !== builtInDefault) {
> +            prefSpec.previousValue = builtInDefault;
> +            changed = true;
> +          }

Sorry, can you elaborate on why this is necessary / a good idea, maybe in a code comment? I *think* this is just syncing the 'default' value pre-rollout into the `previousValue` property. But would be good to be sure I'm understanding this correctly.

::: toolkit/components/normandy/lib/PreferenceRollouts.jsm:156
(Diff revision 2)
> +  /**
> +   * Test whether there is a rollout in storage with the given slug.
> +   * @param {string} slug
> +   * @returns {boolean}
> +   */
> +  async has(recipeId) {
> +    const db = await getDatabase();
> +    const study = await getStore(db).get(recipeId);
> +    return !!study;
> +  },
> +
> +  /**
> +   * Get a rollout by slug
> +   * @param {string} slug
> +   */
> +  async get(slug) {
> +    const db = await getDatabase();
> +    return getStore(db).get(slug);
> +  },

Finding this slightly confusing - I assume it's a copy-paste issue and the `has` method has a `slug` argument, too, and that it'd be less confusing if `s/study/rollout/` .

::: toolkit/components/normandy/test/browser/browser_Normandy.js:6
(Diff revision 2)
>  "use strict";
>  
>  ChromeUtils.import("resource://normandy/Normandy.jsm", this);
>  ChromeUtils.import("resource://normandy/lib/AddonStudies.jsm", this);
>  ChromeUtils.import("resource://normandy/lib/PreferenceExperiments.jsm", this);
> +ChromeUtils.import("resource://normandy/lib/PreferenceRollouts.jsm", this);

As you already said, it'd be good to have actual tests for the functionality in PreferenceRollouts.jsm.
Comment on attachment 8965467 [details]
Bug 1440782 Part 2 - Add preference-rollout action to Normandy

Note that for the telemetry work, I hope we can quickly find a resolution for bug 1450690.
Attachment #8965467 - Flags: feedback?(gijskruitbosch+bugs) → feedback+
(In reply to Michael Cooper [:mythmon] from comment #9)
> Felipe: In the above patch, I'm using the JSON schema validator you
> implemented for Policy Engine. I also made a couple changes to bring it more
> in line with the JSON schema spec. Specifically, I added support for the
> types of values to be an array. Do you think that's ok? Do you think it is
> ok for Normandy to use it? Should we do something different here?

Yeah, that's ok. It's fine to use it. We should probably move it to a different folder and call it something else than "Policies Validator".

However, I didn't see where you actually used it in the code. I saw your changes and the added tests, but not how the validator is being used.

One thing that I suggest doing: I thought about this before, and just didn't implement it because it didn't turn out to be necessary.. But I think it would be fine to make the "array" type to accept single values and treat them as arrays too. This would simplify things in that you wouldn't need to specify things like:

type: "number" or "array of numbers"

And then the validator would automatically convert a single item to an array with that item, which also simplifies the caller from dealing with the parsed value.


A couple of things I should mention about this validator so that you are aware of them:

- it has been criticized as it doesn't adhere to the json-schema spec because the spec doesn't allow custom types to be defined, like I did with "URL", "URLorEmpty", "origin" etc.  I think that's very handy so I really don't want to get rid of that functionality

- It automatically converts strings to nsIURIs for those types. (and I'm planning to change that to be URL objects instead of nsIURI)

- it doesn't support the "pattern" string matching (I see that you have that in parts of your code, so are you using another validator currently?)

- the boolean type accepts 0 and 1 (as numbers), to support data coming from the windows registry
Flags: needinfo?(felipc)
See Also: → 1451033
Comment on attachment 8965467 [details]
Bug 1440782 Part 2 - Add preference-rollout action to Normandy

https://reviewboard.mozilla.org/r/234218/#review240126

PrefUtils.jsm is mostly making common a bunch of things that Normandy was already doing, mostly in preference studies. I chatted about PrefUtils.jsm vs Preference.jsm a bit in #fx-team, and the feeling I got was that Preferences.jsm was maybe sort of deprecated. That was just a feeling though, and maybe it is fine to use. One thing Preferences.jsm doesn't do is make dealing with both the user and default branches easy, which PrefUtils.jsm does. I think you're right about the types not being substantially helpful though, I can remove that.

The plan to include this in telemetry is to re-use the "experiments" block of the Telemetry main ping (note, not "Telemetry Experiments" which is going away). Normandy already uses this to record preference studies and add-on studies, and Sunah, who has been the primary consumer of that field signed off on using it for preference rollout in comment 5, as long as we follow the same procesures we have for other parts of Normandy.

You're right about it not showing up in about:support though. That's something I think will not make it into Fx61, unfortunately. It is definitely something I want to do soon though.

Good catch about user-branch rollouts in the blueprint. During design, the idea of a user-branch rollout was considered for a while, and then dropped. I guess we didn't do a good enough job editing the blueprint after that. There will be better source of documentation in the future. Rob Helmer is going to be helping us whip the docs into shape soon. For now I'm going to focus on getting the code working.
(In reply to Michael Cooper [:mythmon] from comment #15)
> PrefUtils.jsm is mostly making common a bunch of things that Normandy was
> already doing, mostly in preference studies. I chatted about PrefUtils.jsm
> vs Preference.jsm a bit in #fx-team, and the feeling I got was that
> Preferences.jsm was maybe sort of deprecated.

Yes, Preferences.jsm sort of *is* deprecated, but that's because you shouldn't need it, so reimplementing it in a different jsm would also be frowned upon. If the pref service doesn't have good enough primitives then that's where we should make changes, ideally. That's not to say you can't have a utility method or two that takes some repetitive logic out of callsites, but it felt like it was a bit more than that in the previous commit.

You haven't asked for review on this cset and it's still labeled WIP in the commit message so I'm holding off looking at it again, please flag me if/when you need me to have another pass.
Felipe: Thanks for the comments. I agree that moving the validator to another directory makes sense. I'll have a patch with that soon.

(In reply to :Felipe Gomes (needinfo me!) from comment #14)
> However, I didn't see where you actually used it in the code. I saw your
> changes and the added tests, but not how the validator is being used.

The actual usage was added in bug 1440777, in toolkit/components/normandy/actions/BaseAction.jsm.

> One thing that I suggest doing: I thought about this before, and just didn't
> implement it because it didn't turn out to be necessary.. But I think it
> would be fine to make the "array" type to accept single values and treat
> them as arrays too. This would simplify things in that you wouldn't need to
> specify things like:
> 
> type: "number" or "array of numbers"
> 
> And then the validator would automatically convert a single item to an array
> with that item, which also simplifies the caller from dealing with the
> parsed value.

This isn't something I need, so I'm going to leave it out for now. I also generally shy away from "value or array of value" type APIs. I've had trouble with them, and tend to prefer stronger type guarantees.
 
> A couple of things I should mention about this validator so that you are
> aware of them:
> 
> - it has been criticized as it doesn't adhere to the json-schema spec
> because the spec doesn't allow custom types to be defined, like I did with
> "URL", "URLorEmpty", "origin" etc.  I think that's very handy so I really
> don't want to get rid of that functionality
> 
> - It automatically converts strings to nsIURIs for those types. (and I'm
> planning to change that to be URL objects instead of nsIURI)
> 
> - it doesn't support the "pattern" string matching (I see that you have that
> in parts of your code, so are you using another validator currently?)
> 
> - the boolean type accepts 0 and 1 (as numbers), to support data coming from
> the windows registry

These are useful to know. I don't think that any of them will impact Normandy much, except the pattern validator. That's something I can revisit in the future. We are indeed using these schemas in another place too: we use them on the Normandy API server and in the Normandy admin site.

I'll note these in my patch, so that future users of the file will be aware of them too. Hopefully.
Comment on attachment 8965467 [details]
Bug 1440782 Part 2 - Add preference-rollout action to Normandy

https://reviewboard.mozilla.org/r/234218/#review240126

> I don't really mind either way, but out of interest, why the switch to Cu.reportError?

First of all, this is an error condition, so log.info isn't really the right thing to be using, and should be atleast log.error. Second, using Cu.reportError integrates well with the error reporting tools that Osmose has been experimenting with. log.error also calls Cu.reportError, but with less helpful information. Calling it directly like this provides the best debugging support.

> Slightly simpler:
> 
> ```js
> (prefObj => Object.assign({previousValue: ...}, prefObj))
> ```

That reads much nicer, but is slightly different. It will include all fields on `prefObj`, whereas the existing version only gets the named prefs. Considering this is being written to disk, I'd prefer to keep the more explicit, limited version.

> Nit: `const prefNames = new Set(<whatever>.preferences.map(p => p.preferenceName));`
> 
> for both of these maps, I think, and then in the 2 loops, just iterate over `<whatever>` (ie the objects, sans 'key') and check if `preferenceName` occurs in the other `Set` with `.has()` .

I tried to use Set here, and eventually had to go back to a Map to get enough information to handle updates.

> I wonder if, to keep this block + method easier to grasp, it'd make sense to put the "have any of the prefs changed") determination in its own method. I know I'm a curmudgeon, but a 50-line if/else block seems a bit much to me, and the root of the pref comparison looks to me like it can be separated out cleanly.

I tried to extract this, and had trouble getting a clean separation. Mostly the issue is that while checking if prefs changed, I also act on which prefs changed. I could split that pref-writing in order to move the "have any of the prefs changed" question to an isolated function, but that ends up with a net increase in lines of code and complexity. I'll definitely keep this in mind as I work through this code, but I'm not hopeful.

Perhaps moving making both the new creation and update steps dedicated functions would help. It wouldn't reduce the complexity of each branch, but it might help to put it in a specific place.

> So rolled back rollouts would hit this, wouldn't they? That seems suboptimal... Are we just keeping this until rollback is implemented, or something?
> 
> Nit: also, maybe this should be a switch statement given it's comparing the same stuff? :-)

Making this a switch statement is a good idea, thanks. And yes, rollbacks wouldn't be compatible with this, but I didn't want to write much rollback-specific code until I thought more about rollbacks, which I'll be doing soon. That's why I left out the rollback case.

> I wonder if there are ever prefs where we should avoid this and only set them on startup. Then again, I guess that would cause grief for all the poor mac users who never restart. Damned if you do/don't... but perhaps worth documenting that we will set prefs at runtime, too, not just at startup.

I think that in general, preferences managed by Normandy need to be robust to being changed at any time. Mostly this is because even "at startup", Normandy is setting prefs at runtime, just very early in runtime. I think this the best we can do in a generic sense with the current pref system. Definitely something that should be called out in the docs.

> Hypothetically, what happens if we run an action that adds a rollout, and Firefox shuts down before we run `_finalize` (either gracefully or otherwise) ?

There is another call to saveStartupPrefs during a graceful shutdown. The window between `_run` and `_finalize` being called is pretty narrow. In theory if Firefox shuts down non-gracefully in that narrow window, Normandy in general will be in an inconsistent state, but should restore itself the next time it runs (currently 24 hours later, soon 6 hours). I don't think there is much we can do about "what if the system crashes during a write" except to just try again another time.

> What does passing the pref type add here?
> 
> It seems to me the consumers of the studies would be much simpler if they just set the pref to the expected value and inferred the type from that expected value.
> 
> Here, you're already calling `branch.getPrefType`, so we might as well use that information to get the pref, rather than passing it along everywhere?

Most of the code here is copying what PreferenceStudies.jsm did. I don't really recall why that system used explicit pref types. Thinking through it more, I don't think they are needed in this case, and can probably be removed.

> Sorry, can you elaborate on why this is necessary / a good idea, maybe in a code comment? I *think* this is just syncing the 'default' value pre-rollout into the `previousValue` property. But would be good to be sure I'm understanding this correctly.

I've added a brief comment here. The reason for this is that when a rollout ends (for whatever reason), the preference values need to be set back to the builtin defaults. It isn't possible to fetch the builtin default if we've written to the default branch, so storing it here makes sure we use the right value. The reason this needs done every time Firefox starts is that the defaults can change during a recipe's lifetime. This is something we ran into with preference *studies*. We ended a recipe, and users got a bad value for one session because Normandy used an obsolete value for the rollback.
Gijs: The preference-rollout action is now ready for review. I've covered most of your feedback from before, and all of the todos I mentioned previously except for rollback.

I plan to write rollback tomorrow in a 3rd changeset. I think that will make things easier for you to review, and for me to incorporate feedback about the first two changesets. If you think another way would be better, I'm open to that too.
Comment on attachment 8965467 [details]
Bug 1440782 Part 2 - Add preference-rollout action to Normandy

https://reviewboard.mozilla.org/r/234218/#review243728

r=me but I do think I noticed a bug, and there's quite a few comments below. Anyway, once that's all fixed this LGTM.

::: toolkit/components/normandy/Normandy.jsm:62
(Diff revision 5)
>        this.finishInit();
>      }
>    },
>  
>    async finishInit() {
> +    await PreferenceRollouts.recordOriginalValues(studyPrefsChanged);

This looks like it should pass `rolloutPrefsChanged` ? I'm mildly worried that this didn't fail any tests or eslint (along the lines of "variable X is never read"), though if it's hard to test for that then let's not. Maybe in an xpcshell test you could set this up to stub out `migrateShieldPrefs` and manually send the `UI_AVAILABLE_NOTIFICATION`, and verify that the rollout and experiment prefs' `recordOriginalValues` methods are called with the right data. Or something.

::: toolkit/components/normandy/Normandy.jsm:112
(Diff revision 5)
>  
>      await RecipeRunner.init();
>      Services.obs.notifyObservers(null, SHIELD_INIT_NOTIFICATION);
>    },
>  
>    async uninit() {

Uh, so, I should have realized this before now. But the callsite for Normandy.uninit() is a sync method, and doesn't await the result of uninit() at all. Right now, I imagine all of the async stuff here finishes before we manage to shutdown, but I guess the correct thing to do would be to ensure we add a shutdown blocker (see https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/AsyncShutdown.jsm ) that gets resolved when we've finished saving state. Can you file a follow-up for this?

::: toolkit/components/normandy/Normandy.jsm:255
(Diff revision 5)
> +  initExperimentPrefs() {
> +    studyPrefsChanged = {};
> +    const defaultBranch = Services.prefs.getDefaultBranch("");
> +    const experimentBranch = Services.prefs.getBranch(STARTUP_EXPERIMENT_PREFS_BRANCH);
> +
> +    for (const prefName of experimentBranch.getChildList("")) {
> +      try {
> +        studyPrefsChanged[prefName] = this.applyStartupPref(experimentBranch, defaultBranch, prefName);
> +      } catch (e) {
> +        Cu.reportError(e);
> +      }
> +    }
> +  },
> +
> +  initRolloutPrefs() {
> +    rolloutPrefsChanged = {};
> +    const defaultBranch = Services.prefs.getDefaultBranch("");
> +    const rolloutBranch = Services.prefs.getBranch(STARTUP_ROLLOUT_PREFS_BRANCH);
> +
> +    for (const prefName of rolloutBranch.getChildList("")) {
> +      try {
> +        rolloutPrefsChanged[prefName] = this.applyStartupPref(rolloutBranch, defaultBranch, prefName);
> +      } catch (e) {
> +        Cu.reportError(e);
>        }
>      }

These methods are identical except for the object on which we save prefs and the branch from which we get them. Could we just make this one method and pass those 2 things as arguments?

Also, nit - could we make the default branch thing a lazy getter at the top of the file (`gDefaultBranch` or whatever) ?

::: toolkit/components/normandy/actions/PreferenceRolloutAction.jsm:32
(Diff revision 5)
> +    // First determine which preferences are already being managed, to avoid
> +    // conflicts between recipes.
> +    const existingManagedPrefs = new Set();
> +    for (const rollout of await PreferenceRollouts.getAllActive()) {
> +      if (rollout.slug === args.slug) {
> +        continue;
> +      }
> +      for (const prefSpec of rollout.preferences) {
> +        existingManagedPrefs.add(prefSpec.preferenceName);
> +      }
> +    }
> +
> +    for (const prefSpec of args.preferences) {
> +      if (existingManagedPrefs.has(prefSpec.preferenceName)) {
> +        TelemetryEvents.sendEvent(
> +          "enrollFailed",
> +          "preference_rollout",
> +          args.slug,
> +          {reason: "conflict", preference: prefSpec.preferenceName},
> +        );
> +        throw new Error(`Cannot start rollout ${args.slug}. Preference ${prefSpec.preferenceName} is already managed.`);
> +      }
> +      const existingPrefType = Services.prefs.getPrefType(prefSpec.preferenceName);
> +      const rolloutPrefType = PREFERENCE_TYPE_MAP[typeof prefSpec.value];
> +
> +      if (existingPrefType !== Services.prefs.PREF_INVALID && existingPrefType !== rolloutPrefType) {
> +        TelemetryEvents.sendEvent(
> +          "enrollFailed",
> +          "preference_rollout",
> +          args.slug,
> +          {reason: "invalid type", pref: prefSpec.preferenceName},
> +        );
> +        throw new Error(
> +          `Cannot start rollout "${args.slug}" on "${prefSpec.prefName}". ` +
> +          `Existing preference is of type ${existingPrefType}, but rollout ` +
> +          `specifies type ${rolloutPrefType}`
> +        );
> +      }
> +    }

These 2 loops, too, could be factored out into a helper, something like

```js
// This will throw if any of the prefs are already managed.
this._verifyAllPrefsAreUnManaged(slug, args.preferences);
```

or something like that.

::: toolkit/components/normandy/actions/PreferenceRolloutAction.jsm:84
(Diff revision 5)
> +      })),
> +    };
> +
> +    const existingRollout = await PreferenceRollouts.get(args.slug);
> +    if (existingRollout) {
> +      let anyChanged = false;

I don't think I was very clear - I understand that this is also making changes using PrefUtils. However, everything between this declaration of `anyChanged` and the `if (anyChanged)` bit 30 lines later could be its own function, right? As far as I can tell, neither `oldPrefSpecs` or `newPrefSpecs` are used after the 2 loops, and there are no other variables declared outside the loop, so it should be straightforward to factor that out to its own function. So something like:

```js
let anyPrefsChanged = this.updatePrefsForExistingRollout(existingRollout, newRollout);
```

Here and in the other comment about moving loops out into helper functions, I'm basically trying to get the total size of the `_run` method down from the current ~125 lines to something more manageable, to make the code easier to read because it can then fit on the screen reasonably well, and it's clearer what sequence of steps the `_run` method follows (first check if we're not conflicting with other recipes' managed prefs, then for existing recipes, update pref storage bits from any previous versions of the recipe and write to the DB if necessary; for new ones, fetch current pref values, save them, add the new rollout to the DB, and then set the prefs).

::: toolkit/components/normandy/lib/PrefUtils.jsm:10
(Diff revision 5)
> +function getBranch(branchName) {
> +  if (branchName === "user") {
> +    return Services.prefs;
> +  } else if (branchName === "default") {
> +    return Services.prefs.getDefaultBranch("");
> +  }
> +  throw new Error(`Unexpected branch name ${branchName}`);
> +}

Nit: could just have a mapping object:

```js
const kPrefBranches = {
  user: Services.prefs,
  default: Services.prefs.getDefaultBranch(""),
};
```

and in the get/set methods just use `kPrefBranches[branchName]` . If we end up with the wrong pref branch key worse things have gone wrong anyway (and using the resulting `undefined` prop will still throw). :-)

::: toolkit/components/normandy/lib/PreferenceRollouts.jsm:101
(Diff revision 5)
> +
> +      // Count the number of preferences in this rollout that are now redundant.
> +      let prefMatchingDefaultCount = 0;
> +
> +      for (const prefSpec of rollout.preferences) {
> +        if (prefSpec.preferenceName in originalPreferences) {

When would this be false? Shouldn't it never be false? Or could it be false for preferences that have no default value, or something?

::: toolkit/components/normandy/lib/PreferenceRollouts.jsm:104
(Diff revision 5)
> +
> +      for (const prefSpec of rollout.preferences) {
> +        if (prefSpec.preferenceName in originalPreferences) {
> +          const builtInDefault = originalPreferences[prefSpec.preferenceName];
> +          if (prefSpec.value === builtInDefault) {
> +            prefMatchingDefaultCount += 1;

Nit: `++`

::: toolkit/components/normandy/lib/PreferenceRollouts.jsm:155
(Diff revision 5)
> +      try {
> +        await testFunction(...args);
> +      } finally {
> +        db = await getDatabase();
> +        await getStore(db).clear();
> +        await Promise.all(oldData.map(d => getStore(db).add(d)));

Could we ensure this happens in 1 DB synchronous loop where we get the store once, instead of N separate transactions for N items in `oldData` ?

::: toolkit/components/normandy/lib/TelemetryEvents.jsm:26
(Diff revision 5)
>        },
>  
>        enroll_failure: {
>          methods: ["enrollFailed"],
> -        objects: ["addon_study"],
> -        extra_keys: ["reason"],
> +        objects: ["addon_study", "preference_rollout"],
> +        extra_keys: ["reason", "preference"],

I assume these are optional and it's OK that the addon_study and preference_rollout each only provide 1 of these?

::: toolkit/components/normandy/test/browser/browser.ini:28
(Diff revision 5)
>  [browser_LogManager.js]
>  [browser_Normandy.js]
>  [browser_NormandyDriver.js]
>  [browser_PreferenceExperiments.js]
>  [browser_RecipeRunner.js]
> +[browser_PreferenceRollouts.js]

Nit: sorting - this entry should be before RecipeRunner and after PreferenceExperiments.

::: toolkit/components/normandy/test/browser/browser_PreferenceRollouts.js:57
(Diff revision 5)
> +    Assert.deepEqual(
> +      new Set(storedRollouts),
> +      new Set([rollout1, rollout2]),
> +      "getAll should return every stored rollout.",
> +    );

The use of `Set` here means we wouldn't detect if we returned the same thing more than once. Is this just to avoid issues with the arrays being mis-sorted ? Maybe just call .sort() on both arrays - that should guarantee the ordering is the same, right?

(All of this is pretty academic, of course, given the current implementation, but the use of Set struck me as peculiar, so...)

::: toolkit/components/normandy/test/browser/browser_PreferenceRollouts.js:98
(Diff revision 5)
> +    await PreferenceRollouts.closeDB();
> +    const openSpy = sinon.spy(IndexedDB, "open");
> +    sinon.assert.notCalled(openSpy);
> +
> +    try {
> +      // Using studies at all should open the database, but only once.

Nit: 'studies' should probably be 'rollouts'?

::: toolkit/components/normandy/test/browser/browser_PreferenceRollouts.js:103
(Diff revision 5)
> +      // close can be called multiple times
> +      await PreferenceRollouts.closeDB();
> +      await PreferenceRollouts.closeDB();
> +      sinon.assert.calledOnce(openSpy);

I'm being confused reading this - I take it that sinon doesn't reset its called counter for methods once you assert? That is, `closeDB` shouldn't open the DB, right?

::: toolkit/components/normandy/test/browser/browser_PreferenceRollouts.js:111
(Diff revision 5)
> +      sinon.assert.calledOnce(openSpy);
> +
> +      // After being closed, new operations cause the database to be opened again, but only once
> +      await PreferenceRollouts.has("foo");
> +      await PreferenceRollouts.get("foo");
> +      sinon.assert.calledTwice(openSpy);

Here too I was a bit puzzled that the comment says we open the DB once, but we assert we called open twice...

::: toolkit/components/normandy/test/browser/browser_actions_PreferenceRolloutAction.js:97
(Diff revision 5)
> +    is(Services.prefs.getIntPref("test.pref1"), 1, "pref1 should be set");
> +    is(Services.prefs.getIntPref("test.pref2"), 1, "pref2 should be set");

Should we verify these are set on the default branch?

::: toolkit/components/normandy/test/browser/browser_actions_PreferenceRolloutAction.js:164
(Diff revision 5)
> +    Services.prefs.getDefaultBranch("").deleteBranch("test.pref2");
> +    Services.prefs.getDefaultBranch("").deleteBranch("test.pref3");
> +  },
> +);
> +
> +// Test that a graduated rollout can be ungraduated

So this is interesting... when do we clean up recipes once they've graduated? That is, how do we avoid a scenario like this:

1. user on version X gets recipe A that changes pref to value P
2. user upgrades to version Y and the pref changes to P anyway so we graduate A
3. user upgrades to version Z where the pref changes to Q, but now the recipe gets ungraduated and sets it back to P again.

Maybe this is just meant to be dealt with by not serving that recipe to users on version Z or something? Either way, we should make sure that these rollouts don't become permanent keep-the-pref-at-this-value thingies that stop us from changing default pref values...

::: toolkit/components/normandy/test/browser/browser_actions_PreferenceRolloutAction.js:296
(Diff revision 5)
> +// Test when the wrong value type is given, the recipe is not applied
> +decorate_task(
> +  PreferenceRollouts.withTestMock,
> +  withStub(TelemetryEvents, "sendEvent"),
> +  async function simple_recipe_enrollment(sendEventStub) {
> +    Services.prefs.setCharPref("test.pref", "not an int");

Can we also have a test for the case where the value lives on the default branch, and one where the value exists on the default branch already, testing that we get the correct previous value?
Attachment #8965467 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8968971 [details]
Bug 1440782 Part 1 - Move PolicyEngine's JSON schema validator to toolkit

https://reviewboard.mozilla.org/r/237664/#review243876

r+ with the comment change we talked about on irc
Attachment #8968971 - Flags: review?(felipc) → review+
See Also: → 1455464
Comment on attachment 8965467 [details]
Bug 1440782 Part 2 - Add preference-rollout action to Normandy

https://reviewboard.mozilla.org/r/234218/#review243728

> Uh, so, I should have realized this before now. But the callsite for Normandy.uninit() is a sync method, and doesn't await the result of uninit() at all. Right now, I imagine all of the async stuff here finishes before we manage to shutdown, but I guess the correct thing to do would be to ensure we add a shutdown blocker (see https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/AsyncShutdown.jsm ) that gets resolved when we've finished saving state. Can you file a follow-up for this?

Oops. Wish we noticed that earlier. I filed bug 1455464.

> When would this be false? Shouldn't it never be false? Or could it be false for preferences that have no default value, or something?

You're right, this shouldn't ever be false, except maybe after a weird ungraceful shutdown. Even then I think it will be fine. This came about because PreferenceExperiments (which this code was based on) loops over all studies, not just the active ones. Since only the actives rollouts are considered, this check is vestigial.

> I assume these are optional and it's OK that the addon_study and preference_rollout each only provide 1 of these?

Yes. The same pattern is used in the enroll action: preference experiments have a branch, and add-on experiments have an addon ID.

> I'm being confused reading this - I take it that sinon doesn't reset its called counter for methods once you assert? That is, `closeDB` shouldn't open the DB, right?

Yeah, sinon only resets counts when asked explicitly. I can reset the counts to make the comments and the code read better together though.

> Here too I was a bit puzzled that the comment says we open the DB once, but we assert we called open twice...

Like you guessed in the previous comment, sinon's counts are cumulative. So this is really "once before, and now once more". Either way, I can change this to be clearer.

> So this is interesting... when do we clean up recipes once they've graduated? That is, how do we avoid a scenario like this:
> 
> 1. user on version X gets recipe A that changes pref to value P
> 2. user upgrades to version Y and the pref changes to P anyway so we graduate A
> 3. user upgrades to version Z where the pref changes to Q, but now the recipe gets ungraduated and sets it back to P again.
> 
> Maybe this is just meant to be dealt with by not serving that recipe to users on version Z or something? Either way, we should make sure that these rollouts don't become permanent keep-the-pref-at-this-value thingies that stop us from changing default pref values...

The main goal behind this design decision is to make it easy to predict what Normandy will do with a given recipe. In it's current form, an active recipe on the server will assert a value for a pref for any clients it is targetting (unless it has also been rolled back). This is easy to predict.

The workflow I'm intending to propose and document is that recipe A would target only version X, and version Y and Z would no longer match the recipe. The value would persist in Y as a graduated value, and the rollout won't affect Z because a graduated experiment has no affect.

I also don't plan to ever remove graduated rollouts from the database, partially because I can't think of a reasonable time to do that. Mostly though it is because I would like a record of what pref rollout actions were ever taken.
Comment on attachment 8969468 [details]
Bug 1440782 Part 3 - Add preference-rollback action to Normandy

https://reviewboard.mozilla.org/r/238224/#review244152

::: toolkit/components/normandy/actions/PreferenceRollbackAction.jsm:27
(Diff revision 1)
> +    if (!rollout) {
> +      TelemetryEvents.sendEvent("unenrollFailure", "preference_rollout", rolloutSlug, {"reason": "rollout missing"});
> +      throw new Error(`Cannot rollback ${rolloutSlug}: no rollout found with that slug`);
> +    }

Is it likely that this will happen? It seems to me that if we first release a recipe that does a pref rollout, and then later one that does a rollback, profiles that are too new (and/or only ran on more recent versions of Firefox) might not have the original. A telemetry event and warning seems fine, but is throwing the right thing here? Should we just return early more quietly?

::: toolkit/components/normandy/test/browser/browser_actions_PreferenceRollbackAction.js:120
(Diff revision 1)
> +    // Cleanup
> +    Services.prefs.getDefaultBranch("").deleteBranch("test.pref");
> +  },
> +);
> +
> +// Test that a rollback without a matching rollout raises an error

Can we also have a test that rollbacks don't alter user prefs, and a test that running a rollback for a rollout that has already been rolled back works as expected?

::: toolkit/components/normandy/test/browser/browser_actions_PreferenceRollbackAction.js:137
(Diff revision 1)
> +    Assert.deepEqual(
> +      reportRecipeStub.args,
> +      [[recipe.id, Uptake.RECIPE_EXECUTION_ERROR]],
> +      "recipe should be reported as succesful",

Err, this is a bit confusing - it checks for an execution error and then the message says the recipe should be reported as successful?
Attachment #8969468 - Flags: review?(gijskruitbosch+bugs) → review+
Pushed by mcooper@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8350c61c629e
Part 1 - Move PolicyEngine's JSON schema validator to toolkit r=Felipe
https://hg.mozilla.org/integration/autoland/rev/c760daa73210
Part 2 - Add preference-rollout action to Normandy r=Gijs
https://hg.mozilla.org/integration/autoland/rev/cba1a84a758f
Part 3 - Add preference-rollback action to Normandy r=Gijs
Flags: qe-verify+
Based on the feature sign-off (green) https://drive.google.com/open?id=1dMl2icDaT4d3fWxH88mRJilmkrRKlH4cyAS0tHOqbn0, marking this issue as verified.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Blocks: 1574577
No longer blocks: 1574577
You need to log in before you can comment on or make changes to this bug.