704987 - Ignore the hotfix add-on when warning users about incompatible add-ons before updates

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Comment 1

[Robert Strong (they/them - no direct email)]

Why not just exclude it from the add-ons returned from the incompatible add-on check in the add-ons manager?

Comment 2

[Jeff D]

Why not just keep it up-to date and compatible? Once everything in 692664 (make add-ons compatible by default) lands, this issue basically goes away (and anything fancy could be added onto that framework), and in the meantime it would just be a matter of updating maxVersion occasionally (or setting a sufficiently high maxVersion to last until compatible-by-default is released).

If the concern is that Firefox may tell the user that it might not be compatible before it checks, maybe the issue is really that Firefox should check for new versions of all add-ons before upgrading.

Comment 3

[Dave Townsend [:mossop]]

(In reply to Jeff D from comment #2)
> Why not just keep it up-to date and compatible? Once everything in 692664
> (make add-ons compatible by default) lands, this issue basically goes away
> (and anything fancy could be added onto that framework), and in the meantime
> it would just be a matter of updating maxVersion occasionally (or setting a
> sufficiently high maxVersion to last until compatible-by-default is
> released).

Generally when the new app is released it will make the hotfix redundant so we no longer want it around, if we choose to do that y just letting it become incompatible then we don't want that worrying a user into not upgrading.

> If the concern is that Firefox may tell the user that it might not be
> compatible before it checks, maybe the issue is really that Firefox should
> check for new versions of all add-ons before upgrading.

Firefox already does.

Comment 4

[Jeff D]

In https://bugzilla.mozilla.org/show_bug.cgi?id=694068#c14 Tony mentioned letting the add-on code check the Firefox version. Would that be unduly complicated? If so, sorry: I assumed that was reasonable without checking. If not: why does it make more sense to bolt something into the add-on manager?

Comment 5

[Dave Townsend [:mossop]]

(In reply to Jeff D from comment #4)
> In https://bugzilla.mozilla.org/show_bug.cgi?id=694068#c14 Tony mentioned
> letting the add-on code check the Firefox version. Would that be unduly
> complicated? If so, sorry: I assumed that was reasonable without checking.
> If not: why does it make more sense to bolt something into the add-on
> manager?

It can, and we'll probably do that too.

Comment 6

[Dave Townsend [:mossop]]

Attachment: WIP

(In reply to Robert Strong [:rstrong] (do not email) from comment #1)
> Why not just exclude it from the add-ons returned from the incompatible
> add-on check in the add-ons manager?

Not sure what you're referring to here. Attached is a WIP for what I am thinking here. Would like to get a quick feedback pass before I dig into tests.

Comment 7

[Robert Strong (they/them - no direct email)]

(In reply to Dave Townsend (:Mossop) from comment #6)
> Created attachment 578408 [details] [diff] [review] [diff] [details] [review]
> WIP
> 
> (In reply to Robert Strong [:rstrong] (do not email) from comment #1)
> > Why not just exclude it from the add-ons returned from the incompatible
> > add-on check in the add-ons manager?
> 
> Not sure what you're referring to here. Attached is a WIP for what I am
> thinking here. Would like to get a quick feedback pass before I dig into
> tests.
Always report it as compatible in the add-ons manager. I haven't thought about the implications of doing so all that much and suspect there is a reason for not doing that there.

Comment 8

[Robert Strong (they/them - no direct email)]

Comment on attachment 578408 [details] [diff] [review]
WIP

>diff --git a/browser/base/content/aboutDialog.js b/browser/base/content/aboutDialog.js
>--- a/browser/base/content/aboutDialog.js
>+++ b/browser/base/content/aboutDialog.js
>@@ -373,16 +375,22 @@ appUpdater.prototype =
>       return this;
>     }
>   },
> 
>   /**
>    * Checks the compatibility of add-ons for the application update.
>    */
>   checkAddonCompatibility: function() {
>+    var hotfixID = null;
>+    try {
>+      hotfixID = Services.prefs.getCharPref(PREF_EM_HOTFIX_ID);
nit: just declare hotfixID in the try catch block here and elsewhere
var hotfixID = Services.prefs.getCharPref(PREF_EM_HOTFIX_ID);

>+    }
>+    catch (e) { }
>+
>     var self = this;
>     AddonManager.getAllAddons(function(aAddons) {
>       self.addons = [];
>       self.addonsCheckedCount = 0;
>       aAddons.forEach(function(aAddon) {
>         // Protect against code that overrides the add-ons manager and doesn't
>         // implement the isCompatibleWith or the findUpdates method.
>         if (!("isCompatibleWith" in aAddon) || !("findUpdates" in aAddon)) {
>@@ -397,19 +405,20 @@ appUpdater.prototype =
>         // If an add-on isn't appDisabled and isn't userDisabled then it is
>         // either active now or the user expects it to be active after the
>         // restart. If that is the case and the add-on is not installed by the
>         // application and is not compatible with the new application version
>         // then the user should be warned that the add-on will become
>         // incompatible. If an addon's type equals plugin it is skipped since
>         // checking plugins compatibility information isn't supported and
>         // getting the scope property of a plugin breaks in some environments
>-        // (see bug 566787).
>+        // (see bug 566787). The hotfix add-on is also ignored as it shouldn't
>+        // block the user from upgrading.
>         try {
>-          if (aAddon.type != "plugin" &&
>+          if (aAddon.id != hotfixID && aAddon.type != "plugin" &&
I'd prefer to keep the type != "plugin" check as the first check here and elsewhere since that can break tests

Comment 9

[Dave Townsend [:mossop]]

Attachment: patch rev 1

Basically the same patch but includes a hotfix add-on in the updater chrome tests. Normally this would cause most tests to fail but with the pref set they all pass.

Comment 10

[Robert Strong (they/them - no direct email)]

Comment on attachment 582024 [details] [diff] [review]
patch rev 1

nits in comment #8 haven't been responded to in this patch so minusing

Comment 11

[Dave Townsend [:mossop]]

Attachment: patch rev 2

Actually addresses the comments this time.

Comment 12

[Robert Strong (they/them - no direct email)]

Comment on attachment 582177 [details] [diff] [review]
patch rev 2

>diff --git a/toolkit/mozapps/extensions/content/update.js b/toolkit/mozapps/extensions/content/update.js
>--- a/toolkit/mozapps/extensions/content/update.js
>+++ b/toolkit/mozapps/extensions/content/update.js
>@@ -37,16 +37,18 @@
> # ***** END LICENSE BLOCK *****
> 
> // This UI is only opened from the Extension Manager when the app is upgraded.
> 
> "use strict";
> 
> const PREF_UPDATE_EXTENSIONS_ENABLED            = "extensions.update.enabled";
> const PREF_XPINSTALL_ENABLED                    = "xpinstall.enabled";
>+const PREF_EM_HOTFIX_ID                         = "extensions.hotfix.id";
>+
nit: no reason for the extra line

r=me as long as you verified that the test fails without the pref set and all of the chrome tests pass.

Comment 13

[Dave Townsend [:mossop]]

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

Comment 14

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/e7cd9a56b5da

Comment 15

[Dave Townsend [:mossop]]

Comment on attachment 582177 [details] [diff] [review]
patch rev 2

[Approval Request Comment]
User impact if declined: If we were to ship a long-lived hotfix that went incompatible users may be warned and put off upgrading from Firefox 10.
Testing completed (on m-c, etc.): Completed testing on nightly/aurora
Risk to taking this patch (and alternatives if risky): Fairly low risk and has tests

Comment 16

[Alex Keybl [:akeybl]]

Comment on attachment 582177 [details] [diff] [review]
patch rev 2

[Triage Comment]
We'd like to test the add-on hotfix work with our beta population next week. This is required for that testing. The risk here is low, and the benefits great (if we're able to prevent a chemspill).

Comment 17

[Dave Townsend [:mossop]]

https://hg.mozilla.org/releases/mozilla-beta/rev/9b872892f924

Comment 18

[Robert Strong (they/them - no direct email)]

Application update no longer warns users about incompatible add-ons so bug 1300523 is not relevant to this bug.