1613647 - Add ability to graduate addon rollouts

Status: NEW

(Firefox :: Normandy Client, enhancement)

Description

[Rehan Dalal (Inactive)]

We need a way to graduate addon rollouts when the built in version of the addon catches up to the rollout.

After discussions with mixedpuppy in Berlin it was determined that simply uninstalling the addons installed by Normandy is not a good idea as it may result in data loss or other unintended side effects. Some additional changes to AddonManager may be required before we can implement the appropriate behavior.

Comment 1

[Shane Caraveo (:mixedpuppy)]

We're likely going to want something like installStagedAddon[1], but without staging. Specifically if you look in the implementation for that, you'll see where it generates a reason for the uninstall. installStagedAddon also doesn't generate the onInstallStarted/onUninstalling/etc events. Those events are what will kick off some initialization/cleanup in the webextensions code, which I'm pretty sure we want to avoid on an upgrade/downgrade path.

ni?aswan for his input here as well

[1] https://searchfox.org/mozilla-central/rev/3811b11b5773c1dccfe8228bfc7143b10a9a2a99/toolkit/mozapps/extensions/internal/XPIInstall.jsm#3823

Comment 2

[Shane Caraveo (:mixedpuppy)]

Also, for the normandy/balrog locations, I'm wondering if we want to add version checking to the precedence. That way, if the builtin version is => the addon in the normandy/balrog locations, the builtin would win. If we do that...it may just be a file removal to "uninstall" the normandy/balrog xpi.

Comment 3

[Andrew Swan [:aswan]]

ni?aswan for his input here as well

I could use more context. What does "graduate addon rollouts" mean?

Comment 4

[Michael Cooper [:mythmon]]

Graduation is a concept that comes from Normandy. Normandy rollouts are meant as only one step in the process of permanently releasing a feature. Eventually the feature should be turned on by default as a part of Firefox. When that happens, Normandy needs to "stand down" and let the built-in version take precedence. When that happens, we say that the Normandy rollout has graduated into a built-in feature.

In the case of addon rollouts: if a built-in addon has the same ID and a matching or higher version number. When this happens Normandy should (through a process which I am not defining here) allow the built-in addon to take precedence over the Normandy-provided one.

It should still be possible for Normandy to override a built-in addon with one of a higher version. The precedence here is complicated because both systems must sometimes take priority.

Comment 5

[Andrew Swan [:aswan]]

In the case of addon rollouts: if a built-in addon has the same ID and a matching or higher version number. When this happens Normandy should (through a process which I am not defining here) allow the built-in addon to take precedence over the Normandy-provided one.

Sorry, not trying to be obtuse but I don't understand the model here. What is "the built-in addon"? The thing that's implementing a new feature or just something to control enabling the feature? Is the idea that the final state in which the feature is fully enabled entails shipping a built-in addon forever?

Comment 6

[Michael Cooper [:mythmon]]

Good question. You have spotted the detail I left out. For a lot of cases I expect that the feature I described above will be enough, since many features start that start as add-ons first migrate to built-in add-ons before becoming full-fledged features. In that case, the rollout will graduate during that middle step, and will stay graduated when the built-in addon goes away.

However, as you pointed out that, that's not always the case and sometimes we might skip the built-in addon step. Our plan for that is bug 1602912, which will provide an alternate graduation path for features that don't have the expected pattern. It would be a simple, hard coded list of rollouts that no longer are needed. For features that go straight from Normandy-controlled-addon to non-addon-feature, the graduation list will be a simple method to make sure we do the needed cleanup.

I think that the combination of those two approaches ("graduations are roughly permanent" and the graduation list) handle the cases you are worried about. Does that sound right to you?

Comment 7

[Andrew Swan [:aswan]]

I got some information out-of-band about what an "addon rollout" is and while I don't fully understand all the plans, I think I can comment on the addon manager side.

First, unstated here is the plan for adding a new addon location (in the sense of XPIStateLocation inside the addon manager). This is to support the scenario in which some addon is installed via Normandy but a user also manually installs (a different version of?) the same addon. The notion of precedence of locations in the addon manager is supposed to deal with this, but I think there may be some nasty surprises here. See for instance bug 557710 and https://searchfox.org/mozilla-central/rev/3811b11b5773c1dccfe8228bfc7143b10a9a2a99/toolkit/mozapps/extensions/internal/XPIInstall.jsm#1688-1690. Rob was the last one to look closely at this, cc'ing him in case he has anything to add.

Additionally, Shane's comment above is about the fact that if the same addon is installed in two different locations and it is uninstalled from the higher priority location, the version from the lower priority location does take its place but it is treated as an uninstall followed by an install which means that any stored preferences etc. from the addon will be wiped out at this time. There is special code that "fixes" this for the temporary install location by handling the above sequence as an update rather than uninstall-followed-by-install when clearing out temporary addons: https://searchfox.org/mozilla-central/rev/3811b11b5773c1dccfe8228bfc7143b10a9a2a99/toolkit/mozapps/extensions/internal/XPIProvider.jsm#2646-2651. It seems to me that it would be a good idea to generalize this and move it into XPIInstall.uninstallAddon() but 1) that's a chunk of work that needs to be acccounted for and 2) I have no idea if anybody might be depending on the current behavior. I can't easily think of how somebody might be depending on it but people never fail to surprise me in this regard :)

I think this can all be addressed but it will take a decent amount of effort. I would urge the Normandy folks to reconsider the requirement that users are expected to be able to manually install Normandy-managed addons and have everything work smoothly. It would certainly be ideal if it worked that way but are there actually specific scenarios where this is important enough to justify the work that it will take to handle it correctly?

Comment 8

[Michael Cooper [:mythmon]]

I can appreciate your concerns, but none of that has any bearing on graduation, which is the topic of this bug. Graduation is simply the path from when a feature stops being remotely controlled by Normandy and when it starts being built into the binaries downloaded from archive.mozilla.org.

Even if we use the existing addon install location, never allow simultaneous installs in multiple locations, ignore the idea of fall back, etc., we still need to eventually have a plan for when the remote server stops being involved in a feature being enabled. This is a policy of the Normandy system for all rollouts, and isn't related to the install mechanism.

but are there actually specific scenarios where this is important enough to justify the work that it will take to handle it correctly?

In short, yes. In the past 4 months, with the DoH rollout, we have had an add-on that was installed in multiple places in the profile, with multiple versions. We have had complex fallbacks that luckily were unaffected with the uninstall/reinstall flow that currently happens. In order to test these scenarios, QA has needed to install the add-on in the user slot while having it present in another slot, in order to test fallback. There have been users that have installed the add-on manually for various reasons out of our control.

Last year Armagaddon happened. In that case the ability for users to manually install the fix without disrupting the system would have been massively useful. Even though we discouraged it (due to many of the problems you have pointed out, and others), users still tried to do this, which complicated our cleanup efforts. If we could have instructed users to install an add-on to fix the problem (as ironic as that is), it would have helped spread the fix faster, improving user good will. Despite what we may want, we cannot stop users from acquiring these special addons and installing them manually (I suppose we could make a special class of addon that only Normandy could install, but that seems fraught). In the kind of emergency situations that Hotfixes are intended to support, this is even more true.

Having a separate install location means we can go forward confidently, knowing exactly how the system will behave in the face of user intervention, built in addons, and other sources. If we share an install location, it limits our ability to reason about the system during the exact moments when we need clarity the most. I'm not trying to diminish the amount of work that this will take, but my experience with Normandy as a remote deployment tool has taught me that flexibility and segmentation is absolutely vital when dealing with changes from a distance to Firefox.

Comment 9

[Andrew Swan [:aswan]]

(In reply to Michael Cooper [:mythmon] from comment #8)

I can appreciate your concerns, but none of that has any bearing on graduation, which is the topic of this bug.

I was addressing the questions raised in comment 0 and comment 1. If this bug is not the proper place to consider these questions, can you please point to the proper bug?

Comment 10

[Rehan Dalal (Inactive)]

First, unstated here is the plan for adding a new addon location (in the sense of XPIStateLocation inside the addon manager). This is to support the scenario in which some addon is installed via Normandy but a user also manually installs (a different version of?) the same addon.

That is covered in the blocking bug 1575948.