588440 - The extension compatibility dialog is showing up for some new profiles

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Dave Townsend [:mossop]]

The dialog is only meant to appear when starting a new version of the application with an existing profile

Comment 1

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

I wonder if this is due to extensions outside of the profile?

Comment 2

[Dave Townsend [:mossop]]

Yeah you'll only get it if there are extensions outside the application directory, but it's due to a mistake on my part, fix coming soon

Comment 3

[Dave Townsend [:mossop]]

Attachment: patch rev 1

I had been thinking that aAppChanged was only true for upgrades and not for new profiles (i.e. that it matches the cases that checkForMismatches would run in the old code) but it is actually true even for new profiles. This adds a new argument to provider's startup to flag when the profile is brand new and just avoids opening the dialog then and tests it.

Comment 4

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

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

>diff --git a/toolkit/mozapps/extensions/AddonManager.jsm b/toolkit/mozapps/extensions/AddonManager.jsm
>--- a/toolkit/mozapps/extensions/AddonManager.jsm
>+++ b/toolkit/mozapps/extensions/AddonManager.jsm
>@@ -173,20 +173,23 @@ var AddonManagerInternal = {
>   startup: function AMI_startup() {
>     if (this.started)
>       return;
> 
>     this.installListeners = [];
>     this.addonListeners = [];
> 
>     let appChanged = true;
>+    let newProfile = true;
> 
>     try {
>       appChanged = Services.appinfo.version !=
>                    Services.prefs.getCharPref("extensions.lastAppVersion");
>+      // If the preference existed then the profile isn't new
>+      newProfile = false;
>     }
>     catch (e) { }
Seems like it would be simpler to just set appChanged to false in the catch and set the extensions.lastAppVersion pref here. I'm ok with this though if you prefer having the additional param in case you think other providers might find it useful.

> 
>     if (appChanged) {
>       LOG("Application has been upgraded");
>       Services.prefs.setCharPref("extensions.lastAppVersion",
>                                  Services.appinfo.version);
>     }

>diff --git a/toolkit/mozapps/extensions/XPIProvider.jsm b/toolkit/mozapps/extensions/XPIProvider.jsm
>--- a/toolkit/mozapps/extensions/XPIProvider.jsm
>+++ b/toolkit/mozapps/extensions/XPIProvider.jsm
>@@ -1000,17 +1000,17 @@ var XPIProvider = {
>   startupChanges: {
>     // Add-ons that became disabled for compatibility reasons
>     appDisabled: []
>   },
> 
>   /**
>    * Starts the XPI provider initializes the install locations and prefs.
>    */
>-  startup: function XPI_startup(aAppChanged) {
>+  startup: function XPI_startup(aAppChanged, aNewProfile) {
Please add the params to the function header.

>     LOG("startup");
>     this.installs = [];
>     this.installLocations = [];
>     this.installLocationsByName = {};
> 
>     function addDirectoryInstallLocation(aName, aKey, aPaths, aScope, aLocked) {
>       try {
>         var dir = FileUtils.getDir(aKey, aPaths);

Comment 5

[Dave Townsend [:mossop]]

Attachment: patch rev 2

Updated to use the tri-state idea we talked about today. Probably needs a quick re-skim.

Comment 6

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

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

You might want to make extensions.lastAppVersion a const alongside PREF_EM_UPDATE_ENABLED.

It doesn't look like extensions.disabledAddons will be updated. It isn't clear to me that it is actually used anywhere except in one test.
http://tinyurl.com/2bkrqoo
http://tinyurl.com/29gas32

Looks good otherwise and just need an explanation regarding extensions.disabledAddons.

Comment 7

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

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

minusing to make sure Mossop sees the question in comment #6

Comment 8

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

btw: I don't recall if extensions.disabledAddons was added only for tests and I am fine if it makes sense to have it only for testing as long as there is a comment to this affect.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Just as a note: we also see this problem when running an incompatible version of Mozmill with the latest nightly build. The UI blocks the tests and causes a hang of our test-run.

Comment 10

[Dave Townsend [:mossop]]

(In reply to comment #6)
> Comment on attachment 467619 [details] [diff] [review]
> patch rev 2
> 
> You might want to make extensions.lastAppVersion a const alongside
> PREF_EM_UPDATE_ENABLED.

Done

> It doesn't look like extensions.disabledAddons will be updated. It isn't clear
> to me that it is actually used anywhere except in one test.
> http://tinyurl.com/2bkrqoo
> http://tinyurl.com/29gas32
> 
> Looks good otherwise and just need an explanation regarding
> extensions.disabledAddons.

extensions.disabledAddons is used by Fennec and is available for other apps. If they set extensions.showMismatchUI to false then now UI is shown but they can look at the pref to see if any extensions became incompatible and notify the user through their own custom UI. Why do you say it won't get updated?

Comment 11

[Dave Townsend [:mossop]]

Attachment: patch rev 3

Moves the pref to a constant

Comment 12

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

(In reply to comment #10)
>...
> > It doesn't look like extensions.disabledAddons will be updated. It isn't clear
> > to me that it is actually used anywhere except in one test.
> > http://tinyurl.com/2bkrqoo
> > http://tinyurl.com/29gas32
> > 
> > Looks good otherwise and just need an explanation regarding
> > extensions.disabledAddons.
> 
> extensions.disabledAddons is used by Fennec and is available for other apps. If
> they set extensions.showMismatchUI to false then now UI is shown but they can
> look at the pref to see if any extensions became incompatible and notify the
> user through their own custom UI. Why do you say it won't get updated?
Makes sense. I was concerned that the list of disabled add-ons in this pref won't always reflect the actual addons that are disabled. In reality this pref isn't suppose to always reflect the disabled add-ons which is what threw me.

Comment 13

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

Comment on attachment 468333 [details] [diff] [review]
patch rev 3

Wouldn't hurt to add a comment about what extensions.disabledAddons is used for especially since an mozilla-central mxr search doesn't show any consumers.

Comment 14

[Dave Townsend [:mossop]]

Landed: http://hg.mozilla.org/mozilla-central/rev/6ea408281158

Comment 15

[Dave Townsend [:mossop]]

Bustage fix landed: http://hg.mozilla.org/mozilla-central/rev/d2ad8d1fede6, seems I lost some of the changes when I made the pref name a constant.

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b5pre) Gecko/20100824 Minefield/4.0b5pre by using an outdated version of Mozmill and a fresh profile for the test-run.

Dave, what about tests? Do we need a manual one?

Comment 17

[Dave Townsend [:mossop]]

I think that this case is covered just fine by the automated test