Last Comment Bug 704987 - Ignore the hotfix add-on when warning users about incompatible add-ons before updates
: Ignore the hotfix add-on when warning users about incompatible add-ons before...
Status: RESOLVED FIXED
:
Product: Toolkit
Classification: Components
Component: Application Update (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla11
Assigned To: Dave Townsend [:mossop]
:
Mentors:
Depends on:
Blocks: 694068
  Show dependency treegraph
 
Reported: 2011-11-23 14:29 PST by Dave Townsend [:mossop]
Modified: 2012-01-10 16:00 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
WIP (11.01 KB, patch)
2011-12-01 14:51 PST, Dave Townsend [:mossop]
robert.strong.bugs: feedback+
Details | Diff | Review
patch rev 1 (14.01 KB, patch)
2011-12-15 10:29 PST, Dave Townsend [:mossop]
robert.strong.bugs: review-
Details | Diff | Review
patch rev 2 (13.93 KB, patch)
2011-12-15 21:45 PST, Dave Townsend [:mossop]
robert.strong.bugs: review+
akeybl: approval‑mozilla‑beta+
Details | Diff | Review

Description Dave Townsend [:mossop] 2011-11-23 14:29:38 PST

    
Comment 1 Robert Strong [:rstrong] (use needinfo to contact me) 2011-11-23 14:55:23 PST
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 2011-11-27 01:51:41 PST
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] 2011-11-27 09:41:40 PST
(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 2011-11-27 11:06:37 PST
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] 2011-11-27 11:13:39 PST
(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] 2011-12-01 14:51:29 PST
Created attachment 578408 [details] [diff] [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.
Comment 7 Robert Strong [:rstrong] (use needinfo to contact me) 2011-12-01 14:53:36 PST
(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 [:rstrong] (use needinfo to contact me) 2011-12-01 15:03:41 PST
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] 2011-12-15 10:29:29 PST
Created attachment 582024 [details] [diff] [review]
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 [:rstrong] (use needinfo to contact me) 2011-12-15 11:41:50 PST
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] 2011-12-15 21:45:56 PST
Created attachment 582177 [details] [diff] [review]
patch rev 2

Actually addresses the comments this time.
Comment 12 Robert Strong [:rstrong] (use needinfo to contact me) 2011-12-16 03:15:36 PST
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] 2011-12-16 12:08:31 PST
On inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/e7cd9a56b5da
Comment 14 Matt Brubeck (:mbrubeck) 2011-12-17 09:20:28 PST
https://hg.mozilla.org/mozilla-central/rev/e7cd9a56b5da
Comment 15 Dave Townsend [:mossop] 2012-01-10 15:47:07 PST
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] 2012-01-10 15:49:52 PST
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] 2012-01-10 16:00:38 PST
https://hg.mozilla.org/releases/mozilla-beta/rev/9b872892f924

Note You need to log in before you can comment on or make changes to this bug.