The extension compatibility dialog is showing up for some new profiles

VERIFIED FIXED in mozilla2.0b5

Status

()

Toolkit
Add-ons Manager
VERIFIED FIXED
7 years ago
7 years ago

People

(Reporter: mossop, Assigned: mossop)

Tracking

Trunk
mozilla2.0b5
Points:
---
Bug Flags:
in-testsuite +
in-litmus -

Firefox Tracking Flags

(blocking2.0 beta5+)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

7 years ago
The dialog is only meant to appear when starting a new version of the application with an existing profile
(Assignee)

Updated

7 years ago
blocking2.0: --- → beta5+
(Assignee)

Updated

7 years ago
Assignee: nobody → dtownsend
(Assignee)

Updated

7 years ago
Blocks: 557956
I wonder if this is due to extensions outside of the profile?
(Assignee)

Comment 2

7 years ago
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
(Assignee)

Comment 3

7 years ago
Created attachment 467218 [details] [diff] [review]
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.
Attachment #467218 - Flags: review?(robert.bugzilla)
(Assignee)

Updated

7 years ago
Status: NEW → ASSIGNED
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);
Attachment #467218 - Flags: review?(robert.bugzilla) → review+
(Assignee)

Comment 5

7 years ago
Created attachment 467619 [details] [diff] [review]
patch rev 2

Updated to use the tri-state idea we talked about today. Probably needs a quick re-skim.
Attachment #467218 - Attachment is obsolete: true
Attachment #467619 - Flags: review?(robert.bugzilla)
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 on attachment 467619 [details] [diff] [review]
patch rev 2

minusing to make sure Mossop sees the question in comment #6
Attachment #467619 - Flags: review?(robert.bugzilla) → review-
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.
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.
(Assignee)

Comment 10

7 years ago
(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?
(Assignee)

Comment 11

7 years ago
Created attachment 468333 [details] [diff] [review]
patch rev 3

Moves the pref to a constant
Attachment #467619 - Attachment is obsolete: true
Attachment #468333 - Flags: review?(robert.bugzilla)
(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 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.
Attachment #468333 - Flags: review?(robert.bugzilla) → review+
(Assignee)

Comment 14

7 years ago
Landed: http://hg.mozilla.org/mozilla-central/rev/6ea408281158
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b5
(Assignee)

Comment 15

7 years ago
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.
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?
Status: RESOLVED → VERIFIED
Flags: in-testsuite?
Flags: in-litmus?
(Assignee)

Comment 17

7 years ago
I think that this case is covered just fine by the automated test
Flags: in-testsuite?
Flags: in-testsuite+
Flags: in-litmus?
Flags: in-litmus-
You need to log in before you can comment on or make changes to this bug.