Once a day along with the normal update checks we should load the update.rdf for a specific add-on ID to see if a compatible version is available. If so it will be downloaded and installed allowing us to stream hotfixes to users without shipping entirely new builds of Firefox. We should maintain the last installed version of the add-on in a pref to ensure we only install newer versions.

As discussed at a meeting, product wants this feature and it has been given the green light.

Slightly more involved than I had expected because I had to pull the URL escaping code out to be more public, this isn't a bad thing I think though. Basically just checks for an update (if the add-on isn't already installed) and compares the available version to the last installed version. If it is newer it downloads and installs it, all tied off the app update prefs.

Comment on attachment 571807 [details] [diff] [review] patch rev 1 Review of attachment 571807 [details] [diff] [review]: ----------------------------------------------------------------- I assume there will be followups/dependent bugs for the various details listed on the wiki. Having it never show up as incompatible, for instance. The wiki contradicts itself whether hotfixes should/can uninstall itself or be uninstallable, so not sure what the intended behaviour is there. Also, if a hotfix can be uninstalled, is it useful to have the last hotfix version be added as a crash reporter annotation? Presumably there could be cases where uninstalling a hotfix won't always completely revert everything back to how it was before the hotfix was installed (thinking of hotfixes that do a one-time data-migration fix, or something similar). What's meant to happen when app.update.auto is false? Right now, there would be no way to get a hotfix if you don't already have one. Conversely, a hotfix will always get updated, regardless of that pref (or any other app-update pref). What's meant to happen if a user sets an installed hotfix to not auto-update by default? (As they can with every other extension.) ::: toolkit/mozapps/extensions/AddonManager.jsm @@ +56,5 @@ > +const PREF_EM_HOTFIX_LASTVERSION = "extensions.hotfix.lastVersion"; > +const PREF_MATCH_OS_LOCALE = "intl.locale.matchOS"; > +const PREF_SELECTED_LOCALE = "general.useragent.locale"; > + > +const REQ_VERSION = 2; Since you're moving this anyway, would be good to have a better name. UPDATE_REQ_VERSION ? @@ +651,5 @@ > */ > backgroundUpdateCheck: function AMI_backgroundUpdateCheck() { > + let checkHotfix = Services.prefs.getBoolPref(PREF_APP_UPDATE_ENABLED) && > + Services.prefs.getBoolPref(PREF_APP_UPDATE_AUTO) && > + Services.prefs.getPrefType(PREF_EM_HOTFIX_ID) == Ci.nsIPrefBranch.PREF_STRING; Not sure I like having the hotfix stuff in AddonManager.jsm, but then having a HotfixProvider presents it's own difficulties :\ (and equally weird interactions with XPIProvider) @@ +653,5 @@ > + let checkHotfix = Services.prefs.getBoolPref(PREF_APP_UPDATE_ENABLED) && > + Services.prefs.getBoolPref(PREF_APP_UPDATE_AUTO) && > + Services.prefs.getPrefType(PREF_EM_HOTFIX_ID) == Ci.nsIPrefBranch.PREF_STRING; > + > + if (checkHotfix) { Feels like this if-block should be moved to below the early return (not for any practical gain, but for code readability/flow). @@ +659,5 @@ > + var hotfixVersion = "0"; > + try { > + hotfixVersion = Services.prefs.getCharPref(PREF_EM_HOTFIX_LASTVERSION); > + } > + catch (e) { } hotfixVersion is only ever used if a hotfix isn't currently installed, so this could be moved below, to where it will always be useful. @@ +676,5 @@ > Services.obs.notifyObservers(null, "addons-background-update-complete", null); > } > > let scope = {}; > Components.utils.import("resource://gre/modules/AddonRepository.jsm", scope); AddonRepository isn't used for anything hotfix-specific, so this could also only be imported when checkAddons==true. @@ +689,4 @@ > > + if (checkAddons) { > + pendingUpdates++; > + scope.AddonRepository.repopulateCache(ids, notifyComplete); Is it useful for AddonRepository to get & cache metadata for hotfixes? Will there ever be any useful metadata for hotfixes? @@ +693,4 @@ > > + pendingUpdates += aAddons.length; > + > + aAddons.forEach(function BUC_forEachCallback(aAddon) { aAddons will include any installed hotfix, and then update it, regardless of any app-update prefs. Need to filter-out any installed hotfix here, I think. @@ +694,5 @@ > + pendingUpdates += aAddons.length; > + > + aAddons.forEach(function BUC_forEachCallback(aAddon) { > + // Check all add-ons for updates so that any compatibility updates will > + // be applied Nit: full-stop. @@ +711,5 @@ > + }); > + } > + > + // If the hotfix add-on isn't already installed then look for an > + // available version of it Nit: full-stop. @@ +715,5 @@ > + // available version of it > + if (checkHotfix && ids.indexOf(hotfixID) == -1) { > + pendingUpdates++; > + > + let url = Services.prefs.getCharPref(PREF_EM_UPDATE_URL); Think this should use a different pref, so the URL can potentially be different. Hotfixes are managed (by whatever Organization is behind an application) quite differently from normal addon updates, so it's not unreasonable to serve their updates from a different URL. @@ +717,5 @@ > + pendingUpdates++; > + > + let url = Services.prefs.getCharPref(PREF_EM_UPDATE_URL); > + > + // Build the URI from a fake add-on data Nit: full-stop. @@ +735,5 @@ > + return; > + } > + > + // If the available version isn't newer than the last installed > + // version then ignore it Nit: full-stop. @@ +745,5 @@ > + LOG("Downloading hotfix version " + update.version); > + AddonManager.getInstallForURL(update.updateURL, function(aInstall) { > + aInstall.addListener({ > + onInstallEnded: function(aInstall) { > + // Remember the last successfully installed version Nit: full-stop. @@ +752,5 @@ > + }, > + > + onInstallCancelled: function(aInstall) { > + // Revert to the previous version if the installation was > + // cancelled Nit: full-stop.

(In reply to Blair McBride (:Unfocused) from comment #3) > Comment on attachment 571807 [details] [diff] [review] [diff] [details] [review] > patch rev 1 > > Review of attachment 571807 [details] [diff] [review] [diff] [details] [review]: > ----------------------------------------------------------------- > > I assume there will be followups/dependent bugs for the various details > listed on the wiki. Having it never show up as incompatible, for instance. > > The wiki contradicts itself whether hotfixes should/can uninstall itself or > be uninstallable, so not sure what the intended behaviour is there. This is my bad, dave and I discussed the fact that we don't want to get in an infinite install loop with an add-on uninstalling itself but I think we added the installed version pref to deal with that case. > > Also, if a hotfix can be uninstalled, is it useful to have the last hotfix > version be added as a crash reporter annotation? Presumably there could be > cases where uninstalling a hotfix won't always completely revert everything > back to how it was before the hotfix was installed (thinking of hotfixes > that do a one-time data-migration fix, or something similar). Useful yes. Required, I don't think so. We also discussed appending a string to the version number in some way but I don't really want to do that. > What's meant to happen when app.update.auto is false? Right now, there would > be no way to get a hotfix if you don't already have one. I think that's fine, though I would prefer it to be decoupled from any app update prefs and couple it to the add-on prefs instead (like a normal add-on). Conversely, a > hotfix will always get updated, regardless of that pref (or any other > app-update pref). Sounds fine to me. > > What's meant to happen if a user sets an installed hotfix to not auto-update > by default? (As they can with every other extension.) No auto-update. We want it to behave just like a normal add-on, which cuts down risk, testing, and development time.

(In reply to Blair McBride (:Unfocused) from comment #3) > Comment on attachment 571807 [details] [diff] [review] [diff] [details] [review] > patch rev 1 > > Review of attachment 571807 [details] [diff] [review] [diff] [details] [review]: > ----------------------------------------------------------------- > > I assume there will be followups/dependent bugs for the various details > listed on the wiki. Having it never show up as incompatible, for instance. I think the goal here is that the user would never be warned off an update because of an incompatible hotfix. I'll file a bug to address that in the update service. > The wiki contradicts itself whether hotfixes should/can uninstall itself or > be uninstallable, so not sure what the intended behaviour is there. We just want this to behave like a normal extension, the user can uninstall if they choose but the majority of the time the hotfix will be a restartless add-on that uninstalls itself immediately so this issue will likely never come up. > Also, if a hotfix can be uninstalled, is it useful to have the last hotfix > version be added as a crash reporter annotation? Presumably there could be > cases where uninstalling a hotfix won't always completely revert everything > back to how it was before the hotfix was installed (thinking of hotfixes > that do a one-time data-migration fix, or something similar). I'll file this in a follow-up, it's nice to have. > What's meant to happen when app.update.auto is false? Right now, there would > be no way to get a hotfix if you don't already have one. Conversely, a > hotfix will always get updated, regardless of that pref (or any other > app-update pref). I see hotfixes as slimline ways to update Firefox so it feels like their installation should be controlled by the app update prefs, I spoke with legneato on IRC and convinced him this wasn't really any extra work which is all he was worried about. I'll make it so the hotfix isn't auto-updated, I think it'll clean things up a bit. > > + if (checkAddons) { > > + pendingUpdates++; > > + scope.AddonRepository.repopulateCache(ids, notifyComplete); > > Is it useful for AddonRepository to get & cache metadata for hotfixes? Will > there ever be any useful metadata for hotfixes? Probably not much use nor any harm I think.

Hopefully this addresses all the concerns.

Comment on attachment 573599 [details] [diff] [review] patch rev 2 Review of attachment 573599 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/mozapps/extensions/AddonManager.jsm @@ +682,5 @@ > + this.getAllAddons(function getAddonsCallback(aAddons) { > + // If there is a known hotfix then exclude it from the list of add-ons to update. > + if (hotfixID) > + aAddons = aAddons.filter(function(aAddon) aAddon.id != hotfixID); > + var ids = [a.id for each (a in aAddons)]; I know it's never going to be thousands of items, but I still cringe at looping over this twice. Will leave it up to you to decide whether it's worth changing this to filter inside the array comprehension, and later on in BUC_forEachCallback() @@ +735,5 @@ > + pendingUpdates++; > + Components.utils.import("resource://gre/modules/AddonUpdateChecker.jsm"); > + AddonUpdateChecker.checkForUpdates(hotfixID, "extension", null, url, { > + onUpdateCheckComplete: function(aUpdates) { > + let update = AddonUpdateChecker.getNewestCompatibleUpdate(aUpdates); This will ignore Addon.applyBackgroundUpdates, since you're not checking AddonManager.shouldAutoUpdate() - according to comment 4, it should obey that setting. However, it don't think it should, since that setting will get wiped every time there's a hotfix that uninstalls itself, or an app-update uninstalls it (and thus this won't be an update of the addon anyway). Instead, I think there should be an exception for hotfixes in the frontend, that just hides the auto-update part of the UI. Am happy for that to be a followup. @@ +754,5 @@ > + aInstall.addListener({ > + onInstallEnded: function(aInstall) { > + // Remember the last successfully installed version. > + Services.prefs.setCharPref(PREF_EM_HOTFIX_LASTVERSION, > + aInstall.version); Since a hotfix acts like a normal addon, a user can make it check for updates from the UI, in which case this pref won't get updated for the new version. So I think you'll have to move this to a global InstallListener that's registered on startup.

I wonder if the locale-detection code is right. Should this rather use the updater locale rather than the selected one, possibly from a langpack? Also, should we be concerned about partner builds here?

(In reply to Axel Hecht [:Pike] from comment #8) > I wonder if the locale-detection code is right. Should this rather use the > updater locale rather than the selected one, possibly from a langpack? That code is exactly what we've used since Firefox 2 era IIRC. If we think it's wrong then we should file a new bug to change it, I wouldn't want to do that here. > Also, should we be concerned about partner builds here? Can you elaborate on that?

(In reply to Dave Townsend (:Mossop) from comment #9) > (In reply to Axel Hecht [:Pike] from comment #8) > > I wonder if the locale-detection code is right. Should this rather use the > > updater locale rather than the selected one, possibly from a langpack? > > That code is exactly what we've used since Firefox 2 era IIRC. If we think > it's wrong then we should file a new bug to change it, I wouldn't want to do > that here. The updater looks at update.locale in the application dir, and not at the selected locale, and this looks like we're patching the installed build in a sense. > > Also, should we be concerned about partner builds here? > > Can you elaborate on that? I wonder if a hotfix might not be compatible with a partner build, or something like that.

(In reply to Axel Hecht [:Pike] from comment #10) > (In reply to Dave Townsend (:Mossop) from comment #9) > > (In reply to Axel Hecht [:Pike] from comment #8) > > > I wonder if the locale-detection code is right. Should this rather use the > > > updater locale rather than the selected one, possibly from a langpack? > > > > That code is exactly what we've used since Firefox 2 era IIRC. If we think > > it's wrong then we should file a new bug to change it, I wouldn't want to do > > that here. > > The updater looks at update.locale in the application dir, and not at the > selected locale, and this looks like we're patching the installed build in a > sense. The locale is only used in the AMO update request. I don't think we actually use it to make any determination about what hotfix to deliver. > > > Also, should we be concerned about partner builds here? > > > > Can you elaborate on that? > > I wonder if a hotfix might not be compatible with a partner build, or > something like that. Possible, we should certainly consider that when creating the hotfixes.

So, let's reset a little bit, apologies for muddying the water. We just need a dumb secure pipe to send bits down to users. We're not doing anything fancy like sending one version of a hotfix to some locale, or not sending it to partner repacks, etc. Any of that logic will be handled in the hotfix add-on itself. Dave and I discussed if the add-on should respect add-on or Firefox update settings, and he convinced me that it should follow Firefox update settings (Dave, feel free to chime in if I got it wrong). So comment 4 is now wrong. The original idea was to make this patch so low touch that it would make it into the tree quickly and require minimal testing. This is why we allow users to uninstall it, disable it, not receive it if their prefs are set in a certain way, etc....the more we act like a normal add-on, the less change and testing there needs to be. I naively thought we could just append the id into an array we were looping over when checking for add-on updates...yay for being a project manager :-)

Is it acceptable behavior for the hotfix add-on to be updated according to its per-add-on settings, and include the logic to only apply changes when app.update.auto is true? (This would result in optional "download hotfixes but don't install" and "update except hotfixes" functionality. Manually applying downloading hotfixes could also be an option.) It might be marginally more complicated for a small handful of users, but it seems like the most low-key design for the developer to implement: no hacking around with the add-on manager just to let the hotfix dodge updates in a nonstandard manner. Also, will each release be bundled with a unique hotfix add-on, so that different hotfixes can be easily published for different firefox versions? Or is there a reason for the same add-on to carry over from one Firefox release to the next?

(In reply to Jeff D from comment #13) [...] > Also, will each release be bundled with a unique hotfix add-on, so that > different hotfixes can be easily published for different firefox versions? > Or is there a reason for the same add-on to carry over from one Firefox > release to the next? Wouldn't it be simpler to have a single add-on ID (possibly with different versions as needed) and let the add-on code and/or the minVersion,maxVersion logic do any necessary checking, also to avoid applying the same changes twice to the same installation?

(In reply to Tony Mechelynck [:tonymec] from comment #14) > (In reply to Jeff D from comment #13) > [...] > > Also, will each release be bundled with a unique hotfix add-on, so that > > different hotfixes can be easily published for different firefox versions? > > Or is there a reason for the same add-on to carry over from one Firefox > > release to the next? > > Wouldn't it be simpler to have a single add-on ID (possibly with different > versions as needed) and let the add-on code and/or the minVersion,maxVersion > logic do any necessary checking, also to avoid applying the same changes > twice to the same installation? Yes, this.

(In reply to Tony Mechelynck [:tonymec] from comment #14) > (In reply to Jeff D from comment #13) > [...] > > Also, will each release be bundled with a unique hotfix add-on, so that > > different hotfixes can be easily published for different firefox versions? > > Or is there a reason for the same add-on to carry over from one Firefox > > release to the next? > > Wouldn't it be simpler to have a single add-on ID (possibly with different > versions as needed) and let the add-on code and/or the minVersion,maxVersion > logic do any necessary checking, also to avoid applying the same changes > twice to the same installation? The update request sends the application version, and the server (AMO) can figure out what hotfix (if any) to send based on that. This is how the normal addon update check currently works for AMO - it only sends one item back, which is the most recent version it thinks is compatible (this is done to reduce the amount of data sent over the wire).

Yeah, comment #14 is how this works soooo...we're all in agreement ;-)

Now that my secondary question has been answered 4 times... anyone want to weigh in on whether it's acceptable for the add-on to simply check app.update.auto before applying changes, instead of requiring special treatment from the add-on manager?

(In reply to Blair McBride (:Unfocused) from comment #7) > Comment on attachment 573599 [details] [diff] [review] [diff] [details] [review] > patch rev 2 > > Review of attachment 573599 [details] [diff] [review] [diff] [details] [review]: > ----------------------------------------------------------------- > > ::: toolkit/mozapps/extensions/AddonManager.jsm > @@ +682,5 @@ > > + this.getAllAddons(function getAddonsCallback(aAddons) { > > + // If there is a known hotfix then exclude it from the list of add-ons to update. > > + if (hotfixID) > > + aAddons = aAddons.filter(function(aAddon) aAddon.id != hotfixID); > > + var ids = [a.id for each (a in aAddons)]; > > I know it's never going to be thousands of items, but I still cringe at > looping over this twice. Will leave it up to you to decide whether it's > worth changing this to filter inside the array comprehension, and later on > in BUC_forEachCallback() Fixed. I love array comprehensions, if only I could remember all their features! > @@ +735,5 @@ > > + pendingUpdates++; > > + Components.utils.import("resource://gre/modules/AddonUpdateChecker.jsm"); > > + AddonUpdateChecker.checkForUpdates(hotfixID, "extension", null, url, { > > + onUpdateCheckComplete: function(aUpdates) { > > + let update = AddonUpdateChecker.getNewestCompatibleUpdate(aUpdates); > > This will ignore Addon.applyBackgroundUpdates, since you're not checking > AddonManager.shouldAutoUpdate() - according to comment 4, it should obey > that setting. However, it don't think it should, since that setting will get > wiped every time there's a hotfix that uninstalls itself, or an app-update > uninstalls it (and thus this won't be an update of the addon anyway). > > Instead, I think there should be an exception for hotfixes in the frontend, > that just hides the auto-update part of the UI. Am happy for that to be a > followup. Filed bug 706292, I don't think we'd block releasing this feature on having it though so I'm not marking it as blocking this bug. > @@ +754,5 @@ > > + aInstall.addListener({ > > + onInstallEnded: function(aInstall) { > > + // Remember the last successfully installed version. > > + Services.prefs.setCharPref(PREF_EM_HOTFIX_LASTVERSION, > > + aInstall.version); > > Since a hotfix acts like a normal addon, a user can make it check for > updates from the UI, in which case this pref won't get updated for the new > version. So I think you'll have to move this to a global InstallListener > that's registered on startup. I don't think this is really worth it. In the worst case we just end up installing the same hotfix a second time. The hotfixes are going to have to cope with that anyway since the user could uninstall the hotfix and then re-install it manually. So I'm leaving this as-is for now.

Only the one change mentioned here and some unbitrotting. Even if you get to land your stuff that bitrots this then I'd still like to get this form reviewed as it should apply pretty cleanly to the branches. There are also now two dependent bugs which will be getting fixes before we can ship this in a release.

Comment on attachment 577769 [details] [diff] [review] patch rev 3 Review of attachment 577769 [details] [diff] [review]: ----------------------------------------------------------------- Good to go, just one fixup since you changed to using an array comprehension. ::: toolkit/mozapps/extensions/AddonManager.jsm @@ +690,2 @@ > > + aAddons.forEach(function BUC_forEachCallback(aAddon) { Need to filter out the hotfix here, since you're no longer doing it above.

Aside from handling bitrot there are a couple of minor changes here. Firstly I actually get the value of the hotfix ID and if it is the empty string won't check for hotfixes, this makes it easier to disable hotfix checking in tests. Secondly I had to push pendingUpdates back up again, when there are no XPI add-ons at all, getAllAddons, repopulateCache and updateAddonRepositoryData all call their callbacks sychronously so it was possible to get to notifyComplete before the code even worked down to the checkHotfix block. Otherwise this should be all the same as before.

(In reply to Dave Townsend (:Mossop) from comment #22) > Secondly I had to push pendingUpdates back up again, when there are no XPI > add-ons at all, getAllAddons, repopulateCache and updateAddonRepositoryData > all call their callbacks sychronously so it was possible to get to > notifyComplete before the code even worked down to the checkHotfix block. That deserves a comment, I think - otherwise some patch is going to eventually move it back again...

On inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/51926af07f68

\o/

Comment on attachment 578766 [details] [diff] [review] patch rev 4 [Approval Request Comment] Testing completed (on m-c, etc.): Been tested on aurora/nightly for a while though we haven't actually shipped hotfixes yet Risk to taking this patch (and alternatives if risky): Should be pretty low risk code, it doesn't really touch the rest of the code around it.

Comment on attachment 578766 [details] [diff] [review] patch rev 4 [triage comment] We're going to bring this up for beta for Fx10 so that we can test it and potentially ship it. The code is self contained and should be ok to ship if no issues are found.