Default theme is not installed causing theme overrides not to load, causing a broken UI

RESOLVED FIXED in Firefox 45

Status

()

Toolkit
Add-ons Manager
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: Arthur K., Assigned: Gijs)

Tracking

({regression})

40 Branch
mozilla45
regression
Points:
---

Firefox Tracking Flags

(firefox39 unaffected, firefox40 affected, firefox41 affected, firefox42 affected, firefox45 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(6 attachments)

(Reporter)

Description

2 years ago
Created attachment 8643875 [details]
bad-tab-X-behavior.jpg

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:39.0) Gecko/20100101 Firefox/39.0 SeaMonkey/2.36
Build ID: 20150708210945

Steps to reproduce:

I hovered my mouse over the close (x) button on a tab.


Actual results:

The round background circle is cut off on a mouse hover event and is split in two opposite semicircle on mouse downpress. See image for an example. Pretty ugly regression IMHO.


Expected results:

On mouse hover, the background circle should be fully visible. On mouse downpress, it should split into inverted semicircles.
Arthur, is this problem a recent regression or only happen intermittently? Do you have any add-ons that might affect the width of your tabs?

With which version of Firefox did you see the problem? Your User-Agent in comment 0 says you are running "Firefox/39.0 SeaMonkey/2.36". But Firefox 39 on my Windows 8.1 VM has different style tab close buttons than your screenshot. Mine are red squares on mouse hover, not round. They're round in Nightly 42, though. I was not able to reproduce the problem with Firefox 39 or Nightly 42 on my Windows 8.1 VM.
Component: Untriaged → Tabbed Browser
(Reporter)

Comment 2

2 years ago
(In reply to Chris Peterson [:cpeterson] from comment #1)
> Arthur, is this problem a recent regression or only happen intermittently?
> Do you have any add-ons that might affect the width of your tabs?

I think this is recent. I don't remember seeing this on 39 and earlier but I will test with them to be sure and try to narrow down where it first appeared. No add-ons.


> With which version of Firefox did you see the problem?
FF40 RC build2.


> Your User-Agent in comment 0 says you are running "Firefox/39.0 SeaMonkey/2.36".
> But Firefox 39 on my Windows 8.1 VM has different style tab close buttons than
> your screenshot. Mine are red squares on mouse hover, not round. They're round
> in Nightly 42, though. I was not able to reproduce the problem with Firefox 39
> or Nightly 42 on my Windows 8.1 VM.

I have two machines and one has the red squares as you see on your FF while the other---the problematic one---has the tabs as they appear in the image I've attached. Not sure why that is though. Theme? I've never changed the theme or any such behavior. The difference between the two is one has a Quadro NVS 160M (latest drivers) and on this PC it shows a red square close button. The other uses Intel HD4000 graphics (latest drivers).

I'll revert back to 39, 38 and 37 to see what happens and try to report back in due time.
(Reporter)

Comment 3

2 years ago
This is for sure a FF40 regression. 39, 38.0.5 and 37.0.2 don't exhibit this behavior.
(Reporter)

Comment 4

2 years ago
> This is for sure a FF40 regression. 39, 38.0.5 and 37.0.2 don't exhibit this
> behavior.

If it helps, this regression first appears in 40.0b7. I tested 40b1 through b6 and they were fine.
(In reply to Arthur K. from comment #4)
> If it helps, this regression first appears in 40.0b7. I tested 40b1 through
> b6 and they were fine.

Thanks! Identifying that regression range is a huge help towards finding and fixing this bug. Here is the list of changes between 40.0b6 and 40.0b7:

https://hg.mozilla.org/releases/mozilla-beta/pushloghtml?fromchange=678bf02b41b0&tochange=d3bcdf6eba95

These three changes, in particular, look suspicious:

* Bug 1173728 - Remove border between tab strip and the navigation toolbar on Windows 10
* Bug 1173725 - Title bar and tab strip should have a darker background color on Windows 10
* Bug 1173729 - Update generic close icon and new tab button icon on Windows 10

@ Jared or Dão, does this bug look like a regression from bug 1173729?
Blocks: 1173729
Status: UNCONFIRMED → NEW
status-firefox39: --- → unaffected
status-firefox40: --- → affected
status-firefox41: --- → affected
status-firefox42: --- → affected
Ever confirmed: true
Flags: needinfo?(jaws)
Flags: needinfo?(dao)
Keywords: regression
Chris, since you confirmed this and marked 40, 41 and 42 as affected, have you reproduced this with all these versions?
Flags: needinfo?(dao)
No. I was not able to reproduce the problem (in my Windows VM). I just sent the status flags to reflect the information Arthur reported. Please feel free to clear the status flags if you prefer to wait until you have confirmed the problem. :)
Status: NEW → UNCONFIRMED
Ever confirmed: false
(Reporter)

Comment 8

2 years ago
I was able to repro on the HD4000 system running Win 7 x64. The NVS 160M didn't exhibit the issue. Is there someone who has that setup who can test?

Comment 9

2 years ago
You're not supposed to get that close icon style on Windows 7. Must be something related to the file overrides.
Are you using a non-default theme? This very much looks like a problem specific to your configuration. Can you reproduce in a new profile?
Flags: needinfo?(jaws) → needinfo?(thee.chicago.wolf)
(Reporter)

Comment 11

2 years ago
(In reply to Justin Dolske [:Dolske] from comment #10)
> Are you using a non-default theme? This very much looks like a problem
> specific to your configuration. Can you reproduce in a new profile?

No this is default vanilla theme. As I mention in comment 4, it only first appears in 40b7. 40b6 and earlier behave correctly. This was tested on two machine in my office. It further repro's on Intel HD4000 hardware. On my home machine with NVidia as primary VGA, it does not repro as was the case mention in comment 8 (against a NVS 160M versus an HD4000). So while I don't was to make any assertions, it's looking like something in the Intel driver (I'm using 15.33.38.64.4252 WHQL) doesn't gel with chances references by Chris in comment 5.
Flags: needinfo?(thee.chicago.wolf)
And did it reproduce with a new profile?
Flags: needinfo?(thee.chicago.wolf)
(Reporter)

Comment 13

2 years ago
(In reply to Justin Dolske [:Dolske] from comment #12)
> And did it reproduce with a new profile?

Yup.
Flags: needinfo?(thee.chicago.wolf)
(Reporter)

Comment 14

2 years ago
Created attachment 8649260 [details]
VM2012R2.jpg

Wanted to note that I just installed 40.0.2 for the first time on one of my VM servers (2012 R2) and it is displaying the tab button close icon as a round (X) as it should. Depressing the mouse on it also displays correctly. See the attchment (VM2012R2.jpg)

Comment 15

2 years ago
(In reply to Arthur K. from comment #14)
> Created attachment 8649260 [details]
> VM2012R2.jpg
> 
> Wanted to note that I just installed 40.0.2 for the first time on one of my
> VM servers (2012 R2) and it is displaying the tab button close icon as a
> round (X) as it should. Depressing the mouse on it also displays correctly.
> See the attchment (VM2012R2.jpg)

This is expected as 2012 R2 is based on Windows 8. But you shouldn't be seeing the new image on Windows 7.

Updated

2 years ago
See Also: → bug 1206049

Comment 16

2 years ago
Can you upload a screenshot of the appearance tab in the add-on manager on the affected profile ?
Flags: needinfo?(thee.chicago.wolf)
(Reporter)

Comment 17

2 years ago
(In reply to Tim Nguyen [:ntim] from comment #16)
> Can you upload a screenshot of the appearance tab in the add-on manager on
> the affected profile ?

I'll get a screen grab in about 1hr.
Flags: needinfo?(thee.chicago.wolf)
(Reporter)

Comment 18

2 years ago
Created attachment 8666844 [details]
screengrab of Add-ons > Appearance tab

(In reply to Tim Nguyen [:ntim] from comment #16)
> Can you upload a screenshot of the appearance tab in the add-on manager on
> the affected profile ?

Sorry for the delay. Got side tracked. Here's the screen grab of the Appearance tab.
(Reporter)

Comment 19

2 years ago
Same thing on FF 41.0 as well btw.

Comment 20

2 years ago
(In reply to Arthur K. from comment #18)
> Created attachment 8666844 [details]
> screengrab of Add-ons > Appearance tab
> 
> (In reply to Tim Nguyen [:ntim] from comment #16)
> > Can you upload a screenshot of the appearance tab in the add-on manager on
> > the affected profile ?
> 
> Sorry for the delay. Got side tracked. Here's the screen grab of the
> Appearance tab.

Thanks for the screenshot ! Looks like this is similar to bug 1206049, except the default theme isn't actually installed.
Summary: The tab close button improperly displays the background when mouse is hovered over it or when the left mouse button is depressed on it → Default theme is not installed causing theme overrides not to load, causing a broken UI
(Reporter)

Comment 21

2 years ago
(In reply to Tim Nguyen [:ntim] from comment #20)
> (In reply to Arthur K. from comment #18)
> > Created attachment 8666844 [details]
> > screengrab of Add-ons > Appearance tab
> > 
> > (In reply to Tim Nguyen [:ntim] from comment #16)
> > > Can you upload a screenshot of the appearance tab in the add-on manager on
> > > the affected profile ?
> > 
> > Sorry for the delay. Got side tracked. Here's the screen grab of the
> > Appearance tab.
> 
> Thanks for the screenshot ! Looks like this is similar to bug 1206049,
> except the default theme isn't actually installed.

Where should I look for the default theme? I'd like to try and transplant the file/folder over to the affected profile to see if it fixes things.
(Assignee)

Comment 22

2 years ago
(In reply to Arthur K. from comment #21)
> (In reply to Tim Nguyen [:ntim] from comment #20)
> > (In reply to Arthur K. from comment #18)
> > > Created attachment 8666844 [details]
> > > screengrab of Add-ons > Appearance tab
> > > 
> > > (In reply to Tim Nguyen [:ntim] from comment #16)
> > > > Can you upload a screenshot of the appearance tab in the add-on manager on
> > > > the affected profile ?
> > > 
> > > Sorry for the delay. Got side tracked. Here's the screen grab of the
> > > Appearance tab.
> > 
> > Thanks for the screenshot ! Looks like this is similar to bug 1206049,
> > except the default theme isn't actually installed.
> 
> Where should I look for the default theme? I'd like to try and transplant
> the file/folder over to the affected profile to see if it fixes things.

The default profile is in the application directory. It shouldn't be possible to remove it, and you can't copy it from a different profile in the way you suggest.

What are the contents of the browser\extensions subdirectory of your application directory ( something like C:\Program Files\Mozilla Firefox\browser\extensions\ ) ? If there are any directories in there, what are their contents?

In your profile, can you find the extensions.json file and attach it to this bug?
Flags: needinfo?(thee.chicago.wolf)
(Reporter)

Comment 23

2 years ago
Created attachment 8666911 [details]
extensions.json

(In reply to :Gijs Kruitbosch from comment #22)
> (In reply to Arthur K. from comment #21)
> > (In reply to Tim Nguyen [:ntim] from comment #20)
> > > (In reply to Arthur K. from comment #18)
> > > > Created attachment 8666844 [details]
> > > > screengrab of Add-ons > Appearance tab
> > > > 
> > > > (In reply to Tim Nguyen [:ntim] from comment #16)
> > > > > Can you upload a screenshot of the appearance tab in the add-on manager on
> > > > > the affected profile ?
> > > > 
> > > > Sorry for the delay. Got side tracked. Here's the screen grab of the
> > > > Appearance tab.
> > > 
> > > Thanks for the screenshot ! Looks like this is similar to bug 1206049,
> > > except the default theme isn't actually installed.
> > 
> > Where should I look for the default theme? I'd like to try and transplant
> > the file/folder over to the affected profile to see if it fixes things.
> 
> The default profile is in the application directory. It shouldn't be
> possible to remove it, and you can't copy it from a different profile in the
> way you suggest.
> 
> What are the contents of the browser\extensions subdirectory of your
> application directory ( something like C:\Program Files\Mozilla
> Firefox\browser\extensions\ ) ? If there are any directories in there, what
> are their contents?

Only a folder tilted {972ce4c6-7e08-4474-a285-3208198ce6fd} exists there with chrome.manifest, icon.png, and install.rdf inside it.
 
> In your profile, can you find the extensions.json file and attach it to this
> bug?

Done.
Flags: needinfo?(thee.chicago.wolf)
(Assignee)

Comment 24

2 years ago
Dave, is there anything we can glean from that extensions.json beyond "err, that's borked..." ?
Flags: needinfo?(dtownsend)
(In reply to :Gijs Kruitbosch from comment #24)
> Dave, is there anything we can glean from that extensions.json beyond "err,
> that's borked..." ?

Not really, looks like extension discovery is broken somehow. One thing to try, enable extensions.logging.enabled in about:config then exit Firefox, delete the extensions.json file then start Firefox. Then open the browser console and there should be a bunch of addons.* messages showing what it did on startup, copy the log into this bug report then see if the theme is showing up in the add-ons manager.
Flags: needinfo?(dtownsend)
(Reporter)

Comment 26

2 years ago
(In reply to Dave Townsend [:mossop] from comment #25)
> (In reply to :Gijs Kruitbosch from comment #24)
> > Dave, is there anything we can glean from that extensions.json beyond "err,
> > that's borked..." ?
> 
> Not really, looks like extension discovery is broken somehow. One thing to
> try, enable extensions.logging.enabled in about:config then exit Firefox,
> delete the extensions.json file then start Firefox. Then open the browser
> console and there should be a bunch of addons.* messages showing what it did
> on startup, copy the log into this bug report then see if the theme is
> showing up in the add-ons manager.

Doing this, it does not create a new extensions.json file no matter what I do.
(In reply to Arthur K. from comment #26)
> (In reply to Dave Townsend [:mossop] from comment #25)
> > (In reply to :Gijs Kruitbosch from comment #24)
> > > Dave, is there anything we can glean from that extensions.json beyond "err,
> > > that's borked..." ?
> > 
> > Not really, looks like extension discovery is broken somehow. One thing to
> > try, enable extensions.logging.enabled in about:config then exit Firefox,
> > delete the extensions.json file then start Firefox. Then open the browser
> > console and there should be a bunch of addons.* messages showing what it did
> > on startup, copy the log into this bug report then see if the theme is
> > showing up in the add-ons manager.
> 
> Doing this, it does not create a new extensions.json file no matter what I
> do.

Ok, but what about the log?
(Reporter)

Comment 28

2 years ago
(In reply to Dave Townsend [:mossop] from comment #27)
> (In reply to Arthur K. from comment #26)
> > (In reply to Dave Townsend [:mossop] from comment #25)
> > > (In reply to :Gijs Kruitbosch from comment #24)
> > > > Dave, is there anything we can glean from that extensions.json beyond "err,
> > > > that's borked..." ?
> > > 
> > > Not really, looks like extension discovery is broken somehow. One thing to
> > > try, enable extensions.logging.enabled in about:config then exit Firefox,
> > > delete the extensions.json file then start Firefox. Then open the browser
> > > console and there should be a bunch of addons.* messages showing what it did
> > > on startup, copy the log into this bug report then see if the theme is
> > > showing up in the add-ons manager.
> > 
> > Doing this, it does not create a new extensions.json file no matter what I
> > do.
> 
> Ok, but what about the log?

Let me know which log and I'll attach it.
(Reporter)

Comment 29

2 years ago
(In reply to Dave Townsend [:mossop] from comment #27)
> (In reply to Arthur K. from comment #26)
> > (In reply to Dave Townsend [:mossop] from comment #25)
> > > (In reply to :Gijs Kruitbosch from comment #24)
> > > > Dave, is there anything we can glean from that extensions.json beyond "err,
> > > > that's borked..." ?
> > > 
> > > Not really, looks like extension discovery is broken somehow. One thing to
> > > try, enable extensions.logging.enabled in about:config then exit Firefox,
> > > delete the extensions.json file then start Firefox. Then open the browser
> > > console and there should be a bunch of addons.* messages showing what it did
> > > on startup, copy the log into this bug report then see if the theme is
> > > showing up in the add-ons manager.
> > 
> > Doing this, it does not create a new extensions.json file no matter what I
> > do.
> 
> Ok, but what about the log?

For crying out loud, never mind, I need to clean my contacts. =) I'm not presently on the affected machine but will be in the AM. I'll check the console log at that time.
(Reporter)

Comment 30

2 years ago
Created attachment 8667275 [details]
error-console.jpg

(In reply to Dave Townsend [:mossop] from comment #27)
> (In reply to Arthur K. from comment #26)
> > (In reply to Dave Townsend [:mossop] from comment #25)
> > > (In reply to :Gijs Kruitbosch from comment #24)
> > > > Dave, is there anything we can glean from that extensions.json beyond "err,
> > > > that's borked..." ?
> > > 
> > > Not really, looks like extension discovery is broken somehow. One thing to
> > > try, enable extensions.logging.enabled in about:config then exit Firefox,
> > > delete the extensions.json file then start Firefox. Then open the browser
> > > console and there should be a bunch of addons.* messages showing what it did
> > > on startup, copy the log into this bug report then see if the theme is
> > > showing up in the add-ons manager.
> > 
> > Doing this, it does not create a new extensions.json file no matter what I
> > do.
> 
> Ok, but what about the log?

Well, here's what's showing in the browser console (see attached JPG). I also have a bit more to add to the mystery and I am not sure if it also might have something to do with bug 1193625. Nonetheless, let me explain. I have been using a .cfg file with many lockPref setting in it for the better part of a decade and is similar to what Chris Ilias wrote up here: http://ilias.ca/blog/2005/03/locking-mozilla-firefox-settings/.

Within my all.js located in Program Files (x86)\Mozilla Firefox\defaults\prefs, I use pref("general.config.filename", "mozilla.cfg");. With it set as such, a new extensions.json is *not* being created upon browser start. However, using #pref("general.config.filename", "mozilla.cfg"); to disable this pref, a new extensions.json *is* created upon startup and the X's in the corners of tabs are now proper and not as shown in bad-tab-X-behavior.jpg.

So this leads me to believe something that I have lockPref'ed in my mozilla.cfg file is, perhaps, taking precedence over the defaults created in a new extensions.json if it does not exist. Still, it's odd that it works on in 40.0b7.

If I had to guess, one of these lockPrefs could be conflicting with whatever regression went into 40.0b8 and with the combination of pref("general.config.filename", "mozilla.cfg"); is causing some bustage?

lockPref("experiments.activeExperiment", false);
lockPref("extensions.blocklist.enabled", false);
lockPref("extensions.blocklist.pingCountTotal", -1);
lockPref("extensions.blocklist.pingCountVersion", 2);
lockPref("extensions.bootstrappedAddons", "{}");
lockPref("extensions.databaseSchema", 17);
lockPref("extensions.autoDisableScopes", 0);
lockPref("extensions.enabledAddons", "%7B972ce4c6-7e08-4474-a285-3208198ce6fd%7D:41.0");
lockPref("extensions.enabledScopes", 1);
lockPref("extensions.getAddons.browseAddons", "");
lockPref("extensions.getAddons.databaseSchema", 5);
lockPref("extensions.getAddons.maxResults", 0);
lockPref("extensions.getAddons.recommended.browseURL", "");
lockPref("extensions.getAddons.recommended.url", "");
lockPref("extensions.getAddons.search.url", "");
lockPref("extensions.getAddons.showPane", false);
lockPref("extensions.getMoreExtensionsURL", "");
lockPref("extensions.getMorePluginsURL", "");
lockPref("extensions.getMoreThemesURL", "");
lockPref("extensions.hideInstallButton", true);
lockPref("extensions.lastAppVersion", "41.0");
lockPref("extensions.lastPlatformVersion", "41.0");
lockPref("extensions.pendingOperations", false);
lockPref("extensions.recommended.browseURL", "");
lockPref("extensions.recommended.url", "");
lockPref("extensions.search.browseURL", "")
lockPref("extensions.search.showPane", false);
lockPref("extensions.search.url", "");
lockPref("extensions.shownSelectionUI", true);
lockPref("extensions.ui.dictionary.hidden", true);
lockPref("extensions.ui.lastCategory", "");
lockPref("extensions.ui.locale.hidden", true);
lockPref("extensions.update.autoUpdateDefault", false);
lockPref("extensions.update.enabled", false);
lockPref("extensions.update.notifyUser", false);
lockPref("extensions.update.url", "");
lockPref("extensions.xpiState", "{}");
(Reporter)

Comment 31

2 years ago
(In reply to Arthur K. from comment #30)
> Still, it's odd that it works on in 40.0b7.

Sorry, this was supposed to say 40.0b6, not b7. The regression first appears in 40.0b7.
(Reporter)

Comment 32

2 years ago
I can confirm that removing lockPref("extensions.enabledScopes", 1); from my autoconfig fixes the issue.
(Reporter)

Comment 33

2 years ago
I investigated a few earlier autoconfig versions and my first use of lockPref("extensions.enabledScopes", 1) began with FF35. Some Google searches detailed using lockPref("extensions.enabledScopes", 15). In testing it just now, it did not cause any issue with my autoconfig file. So was lockPref("extensions.enabledScopes", 1) just an error on my part? Did it used to do anything?
(Assignee)

Comment 34

2 years ago
(In reply to Arthur K. from comment #33)
> I investigated a few earlier autoconfig versions and my first use of
> lockPref("extensions.enabledScopes", 1) began with FF35. Some Google
> searches detailed using lockPref("extensions.enabledScopes", 15). In testing
> it just now, it did not cause any issue with my autoconfig file. So was
> lockPref("extensions.enabledScopes", 1) just an error on my part? Did it
> used to do anything?

It's an OR of the enabled install scopes:

https://dxr.mozilla.org/mozilla-central/rev/79a5b2968d01512470eb6c25d6638d8b9565575e/toolkit/mozapps/extensions/AddonManager.jsm#2934-2944

so this meant only extensions installed in your profile were considered installed, which presumably explains why the default theme, which we ship as part of the application, was not present, and therefore not enabled, and therefore stuff broke.

I think that means we can mark this INVALID? Unless... can we still install things at application level beyond what ships with Firefox, Dave? And if not, can we make that scope always be allowed/present/whatever?
Flags: needinfo?(dtownsend)
(Reporter)

Comment 35

2 years ago
(In reply to :Gijs Kruitbosch from comment #34)
> I think that means we can mark this INVALID? Unless... can we still install
> things at application level beyond what ships with Firefox, Dave? And if
> not, can we make that scope always be allowed/present/whatever?

I would imagine yes to the closing with INVALID but with the caveat that I might not be the only person who experienced this (much less dug deep enough to figure out where this broke). It still doesn't explain the bustage that happened between 40.0b6 and 40.0b7. Meaning, what invalidated lockPref("extensions.enabledScopes", 1) as an acceptable value between 40.0b6 and 40.0b7?
(Assignee)

Comment 36

2 years ago
(In reply to Arthur K. from comment #35)
> (In reply to :Gijs Kruitbosch from comment #34)
> > I think that means we can mark this INVALID? Unless... can we still install
> > things at application level beyond what ships with Firefox, Dave? And if
> > not, can we make that scope always be allowed/present/whatever?
> 
> I would imagine yes to the closing with INVALID but with the caveat that I
> might not be the only person who experienced this (much less dug deep enough
> to figure out where this broke). It still doesn't explain the bustage that
> happened between 40.0b6 and 40.0b7. Meaning, what invalidated
> lockPref("extensions.enabledScopes", 1) as an acceptable value between
> 40.0b6 and 40.0b7?

We started actually relying on the default theme being enabled and its manifest being used.

Updated

2 years ago
No longer blocks: 1173729
Component: Tabbed Browser → Add-ons Manager
Product: Firefox → Toolkit
(In reply to :Gijs Kruitbosch from comment #34)
> (In reply to Arthur K. from comment #33)
> > I investigated a few earlier autoconfig versions and my first use of
> > lockPref("extensions.enabledScopes", 1) began with FF35. Some Google
> > searches detailed using lockPref("extensions.enabledScopes", 15). In testing
> > it just now, it did not cause any issue with my autoconfig file. So was
> > lockPref("extensions.enabledScopes", 1) just an error on my part? Did it
> > used to do anything?
> 
> It's an OR of the enabled install scopes:
> 
> https://dxr.mozilla.org/mozilla-central/rev/
> 79a5b2968d01512470eb6c25d6638d8b9565575e/toolkit/mozapps/extensions/
> AddonManager.jsm#2934-2944
> 
> so this meant only extensions installed in your profile were considered
> installed, which presumably explains why the default theme, which we ship as
> part of the application, was not present, and therefore not enabled, and
> therefore stuff broke.
> 
> I think that means we can mark this INVALID? Unless... can we still install
> things at application level beyond what ships with Firefox, Dave? And if
> not, can we make that scope always be allowed/present/whatever?

If we're in a state where Firefox physically breaks without the default theme present then we should probably make this scope always present.
Flags: needinfo?(dtownsend)
(Assignee)

Comment 38

2 years ago
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d0515cdcb86
(Assignee)

Comment 39

2 years ago
Created attachment 8678871 [details]
MozReview Request: Bug 1191468 - always load app dir add-ons irrespective of enabledScopes, r?Mossop

Bug 1191468 - ensure we always load add-ons from the application scope, r?Mossop
Attachment #8678871 - Flags: review?(dtownsend)
(Assignee)

Updated

2 years ago
Assignee: nobody → gijskruitbosch+bugs
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Comment on attachment 8678871 [details]
MozReview Request: Bug 1191468 - always load app dir add-ons irrespective of enabledScopes, r?Mossop

https://reviewboard.mozilla.org/r/23271/#review20801

I'm not terribly surprised to see that this is failing a lot of xpcshell tests. Some of them are probably easy fixes, some of the theme specific ones are probably hard :(

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:2463
(Diff revision 1)
> +                          AddonManager.SCOPE_APPLICATION;

I think it will be clearer to just remove the location we're interested in from the if block below.
Attachment #8678871 - Flags: review?(dtownsend)
(Assignee)

Comment 41

2 years ago
(In reply to Dave Townsend [:mossop] from comment #40)
> Comment on attachment 8678871 [details]
> MozReview Request: Bug 1191468 - ensure we always load add-ons from the
> application scope, r?Mossop
> 
> https://reviewboard.mozilla.org/r/23271/#review20801
> 
> I'm not terribly surprised to see that this is failing a lot of xpcshell
> tests. Some of them are probably easy fixes, some of the theme specific ones
> are probably hard :(

I looked at this for a bit. It looks doable, though tedious, but I have a more serious concern: it seems quite some of these tests are meant to verify things work when there are 0 add-ons. We can make it ignore the app add-ons in its own checks, but that would mean that the tests stop noticing if things *did* break without add-ons... which sounds like a problem to me.

I would prefer, though it makes me deeply uncomfortable, to make the add-on manager force-on the application directory add-ons if and only if we're not running as part of test infrastructure.

Either that or wontfixing this bug, I guess. Which of the 3 options do you prefer?
Flags: needinfo?(dtownsend)
(In reply to :Gijs Kruitbosch from comment #41)
> (In reply to Dave Townsend [:mossop] from comment #40)
> > Comment on attachment 8678871 [details]
> > MozReview Request: Bug 1191468 - ensure we always load add-ons from the
> > application scope, r?Mossop
> > 
> > https://reviewboard.mozilla.org/r/23271/#review20801
> > 
> > I'm not terribly surprised to see that this is failing a lot of xpcshell
> > tests. Some of them are probably easy fixes, some of the theme specific ones
> > are probably hard :(
> 
> I looked at this for a bit. It looks doable, though tedious, but I have a
> more serious concern: it seems quite some of these tests are meant to verify
> things work when there are 0 add-ons. We can make it ignore the app add-ons
> in its own checks, but that would mean that the tests stop noticing if
> things *did* break without add-ons... which sounds like a problem to me.
> 
> I would prefer, though it makes me deeply uncomfortable, to make the add-on
> manager force-on the application directory add-ons if and only if we're not
> running as part of test infrastructure.
> 
> Either that or wontfixing this bug, I guess. Which of the 3 options do you
> prefer?

I ran some stats, around 0.0006% of users are affected by this bug. Given that I certainly favour wontfixing over the work for the other two options. But I think there is a relatively straightforward way we can fix this and that is by making it possible for tests to override where the add-ons manager looks for the application extensions. We already do this for the other locations by overriding the directory provider service but we can't really override the location of "XCurProcD" without breaking a lot of stuff, so either we need to create a new directory provider location that can be overridden and switch to that or we need to create some new JSM that acts as a shim for the directory provider and that tests can affect easily and use that. The latter is probably a bit more work to think about but could be generally useful elsewhere so I leave it up to you to decide what you prefer to do.
Flags: needinfo?(dtownsend)
(Reporter)

Comment 43

2 years ago
(In reply to Dave Townsend [:mossop] from comment #42)
> I ran some stats, around 0.0006% of users are affected by this bug. Given
> that I certainly favour wontfixing over the work for the other two options.

0.0006% is pretty darn small. Shall I close as wontfix?
(Assignee)

Comment 44

2 years ago
(In reply to Dave Townsend [:mossop] from comment #42)
> (In reply to :Gijs Kruitbosch from comment #41)
> > (In reply to Dave Townsend [:mossop] from comment #40)
> > > Comment on attachment 8678871 [details]
> > > MozReview Request: Bug 1191468 - ensure we always load add-ons from the
> > > application scope, r?Mossop
> > > 
> > > https://reviewboard.mozilla.org/r/23271/#review20801
> > > 
> > > I'm not terribly surprised to see that this is failing a lot of xpcshell
> > > tests. Some of them are probably easy fixes, some of the theme specific ones
> > > are probably hard :(
> > 
> > I looked at this for a bit. It looks doable, though tedious, but I have a
> > more serious concern: it seems quite some of these tests are meant to verify
> > things work when there are 0 add-ons. We can make it ignore the app add-ons
> > in its own checks, but that would mean that the tests stop noticing if
> > things *did* break without add-ons... which sounds like a problem to me.
> > 
> > I would prefer, though it makes me deeply uncomfortable, to make the add-on
> > manager force-on the application directory add-ons if and only if we're not
> > running as part of test infrastructure.
> > 
> > Either that or wontfixing this bug, I guess. Which of the 3 options do you
> > prefer?
> 
> I ran some stats, around 0.0006% of users are affected by this bug. Given
> that I certainly favour wontfixing over the work for the other two options.
> But I think there is a relatively straightforward way we can fix this and
> that is by making it possible for tests to override where the add-ons
> manager looks for the application extensions. We already do this for the
> other locations by overriding the directory provider service but we can't
> really override the location of "XCurProcD" without breaking a lot of stuff,
> so either we need to create a new directory provider location that can be
> overridden and switch to that or we need to create some new JSM that acts as
> a shim for the directory provider and that tests can affect easily and use
> that. The latter is probably a bit more work to think about but could be
> generally useful elsewhere so I leave it up to you to decide what you prefer
> to do.

I personally do not have the time to write the new JSM you suggest. I don't know that we really support supplanting XPCOM services at runtime, which means we'd likely need to actually make consumers use some kind of intermediary for the XPCOM directory service, which sounds like touching a lot of lines again (and some minor indirection for always using the intermediary). Maybe I'm misunderstanding your suggestion?

For that and the 0.0006% of users, I would suggest wontfix. The one other thing I can think of is adding a one-time migration that fixes the enabledScopes to include applications - except that I suspect a large number of these people are enterprises, and so that won't work anyway because the pref will be locked.
Flags: needinfo?(dtownsend)
(In reply to :Gijs Kruitbosch from comment #44)
> (In reply to Dave Townsend [:mossop] from comment #42)
> > (In reply to :Gijs Kruitbosch from comment #41)
> > > (In reply to Dave Townsend [:mossop] from comment #40)
> > > > Comment on attachment 8678871 [details]
> > > > MozReview Request: Bug 1191468 - ensure we always load add-ons from the
> > > > application scope, r?Mossop
> > > > 
> > > > https://reviewboard.mozilla.org/r/23271/#review20801
> > > > 
> > > > I'm not terribly surprised to see that this is failing a lot of xpcshell
> > > > tests. Some of them are probably easy fixes, some of the theme specific ones
> > > > are probably hard :(
> > > 
> > > I looked at this for a bit. It looks doable, though tedious, but I have a
> > > more serious concern: it seems quite some of these tests are meant to verify
> > > things work when there are 0 add-ons. We can make it ignore the app add-ons
> > > in its own checks, but that would mean that the tests stop noticing if
> > > things *did* break without add-ons... which sounds like a problem to me.
> > > 
> > > I would prefer, though it makes me deeply uncomfortable, to make the add-on
> > > manager force-on the application directory add-ons if and only if we're not
> > > running as part of test infrastructure.
> > > 
> > > Either that or wontfixing this bug, I guess. Which of the 3 options do you
> > > prefer?
> > 
> > I ran some stats, around 0.0006% of users are affected by this bug. Given
> > that I certainly favour wontfixing over the work for the other two options.
> > But I think there is a relatively straightforward way we can fix this and
> > that is by making it possible for tests to override where the add-ons
> > manager looks for the application extensions. We already do this for the
> > other locations by overriding the directory provider service but we can't
> > really override the location of "XCurProcD" without breaking a lot of stuff,
> > so either we need to create a new directory provider location that can be
> > overridden and switch to that or we need to create some new JSM that acts as
> > a shim for the directory provider and that tests can affect easily and use
> > that. The latter is probably a bit more work to think about but could be
> > generally useful elsewhere so I leave it up to you to decide what you prefer
> > to do.
> 
> I personally do not have the time to write the new JSM you suggest. I don't
> know that we really support supplanting XPCOM services at runtime, which
> means we'd likely need to actually make consumers use some kind of
> intermediary for the XPCOM directory service, which sounds like touching a
> lot of lines again (and some minor indirection for always using the
> intermediary). Maybe I'm misunderstanding your suggestion?

We can override directories in the directory service in tests, for example https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/xpcshell/test_migrate2.js#109

The JSM alternative I was thinking of would probably be pretty straightforward but yeah, more to think about in terms of getting the API right.

> For that and the 0.0006% of users, I would suggest wontfix. The one other
> thing I can think of is adding a one-time migration that fixes the
> enabledScopes to include applications - except that I suspect a large number
> of these people are enterprises, and so that won't work anyway because the
> pref will be locked.

Hrm now you mention this I realise that telemetry doesn't report pref values if they are the default and that's how an enterprise might well set this pref so my numbers may not be accurate :(
Flags: needinfo?(dtownsend)
(Assignee)

Comment 46

2 years ago
(In reply to Dave Townsend [:mossop] from comment #45)
> We can override directories in the directory service in tests, for example
> https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/
> test/xpcshell/test_migrate2.js#109

When playing around with this in the browser console, and looking at the directory service code, this might not work reliably for XCurProcD anyway because it is cached as soon as it's requested - so if you add a provider for it at runtime, it just never gets called.

> The JSM alternative I was thinking of would probably be pretty
> straightforward but yeah, more to think about in terms of getting the API
> right.

But we'd need to make XPIProvider call that JSM instead of the directory service, right? Or do you think there is some way we can actually replace the directory service it sees when run as part of tests?

I think a shortcut here would be to stick a simple getter on XPIProvider that we could call from the test? Or are there other downsides to that?

(ni for these 2 questions)

> > For that and the 0.0006% of users, I would suggest wontfix. The one other
> > thing I can think of is adding a one-time migration that fixes the
> > enabledScopes to include applications - except that I suspect a large number
> > of these people are enterprises, and so that won't work anyway because the
> > pref will be locked.
> 
> Hrm now you mention this I realise that telemetry doesn't report pref values
> if they are the default and that's how an enterprise might well set this
> pref so my numbers may not be accurate :(

Then it sounds like we should still fix this. :-(
Flags: needinfo?(dtownsend)
(In reply to :Gijs Kruitbosch from comment #46)
> (In reply to Dave Townsend [:mossop] from comment #45)
> > We can override directories in the directory service in tests, for example
> > https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/
> > test/xpcshell/test_migrate2.js#109
> 
> When playing around with this in the browser console, and looking at the
> directory service code, this might not work reliably for XCurProcD anyway
> because it is cached as soon as it's requested - so if you add a provider
> for it at runtime, it just never gets called.

Right so as I said we'd need to create a new location that the default directory provider returns the same thing as XCurProcD but tests could override. Bug 1207287 has an example of that.
 
> > The JSM alternative I was thinking of would probably be pretty
> > straightforward but yeah, more to think about in terms of getting the API
> > right.
> 
> But we'd need to make XPIProvider call that JSM instead of the directory
> service, right? Or do you think there is some way we can actually replace
> the directory service it sees when run as part of tests?

Yes, I'd just have XPIProvider call the JSM to get the location instead of the directory service. Basically this is just a hack around doing the above without having to mess around in C++ and xpcom land I guess.

> I think a shortcut here would be to stick a simple getter on XPIProvider
> that we could call from the test? Or are there other downsides to that?

A getter for what? I'm not sure what you're suggesting here.
Flags: needinfo?(dtownsend)
(Assignee)

Comment 48

2 years ago
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=9abaaa078a4c

because running xpcshell tests on windows myself is apparently madness. (bug 1225462)
(Assignee)

Updated

2 years ago
Attachment #8678871 - Attachment description: MozReview Request: Bug 1191468 - ensure we always load add-ons from the application scope, r?Mossop → MozReview Request: Bug 1191468 - always load app dir add-ons irrespective of enabledScopes, r?Mossop
Attachment #8678871 - Flags: review?(dtownsend)
(Assignee)

Comment 49

2 years ago
Comment on attachment 8678871 [details]
MozReview Request: Bug 1191468 - always load app dir add-ons irrespective of enabledScopes, r?Mossop

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23271/diff/1-2/
(Assignee)

Comment 50

2 years ago
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=a7ea4fbb0cf4
Comment on attachment 8678871 [details]
MozReview Request: Bug 1191468 - always load app dir add-ons irrespective of enabledScopes, r?Mossop

https://reviewboard.mozilla.org/r/23271/#review23031

Looks good to me, can you have :froydnj sign-off on the xpcom bit though.
Attachment #8678871 - Flags: review?(dtownsend) → review+
(Assignee)

Updated

a year ago
Attachment #8678871 - Flags: review?(nfroyd)
Comment on attachment 8678871 [details]
MozReview Request: Bug 1191468 - always load app dir add-ons irrespective of enabledScopes, r?Mossop

https://reviewboard.mozilla.org/r/23271/#review23105

::: xpcom/build/nsXULAppAPI.h:127
(Diff revision 2)
> + * A directory service key which specifies the location for app dir add-ons.
> + */
> +#define XRE_ADDON_APP_DIR "XREAddonAppDir"

Should we document this as a synonym to XCurProcD by default?  I sort of understand the rationale for this after skimming through the bug, but it might be good to provide some hint here for future ?
Attachment #8678871 - Flags: review?(nfroyd) → review+

Comment 53

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/6144806b5c13
(Assignee)

Comment 54

a year ago
(In reply to Nathan Froyd [:froydnj] from comment #52)
> Comment on attachment 8678871 [details]
> MozReview Request: Bug 1191468 - always load app dir add-ons irrespective of
> enabledScopes, r?Mossop
> 
> https://reviewboard.mozilla.org/r/23271/#review23105
> 
> ::: xpcom/build/nsXULAppAPI.h:127
> (Diff revision 2)
> > + * A directory service key which specifies the location for app dir add-ons.
> > + */
> > +#define XRE_ADDON_APP_DIR "XREAddonAppDir"
> 
> Should we document this as a synonym to XCurProcD by default?  I sort of
> understand the rationale for this after skimming through the bug, but it
> might be good to provide some hint here for future ?

Done in the code. I looked at MDN and I don't think anything there needs updating.

Comment 55

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6144806b5c13
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.