Closed Bug 1176703 Opened 9 years ago Closed 9 years ago

Theme overrides don't work in safe mode and cause missing/wrong images

Categories

(Toolkit :: Themes, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla42
Tracking Status
firefox40 + verified
firefox41 + verified
firefox42 --- verified

People

(Reporter: elbart, Assigned: Gijs)

References

Details

(Keywords: regression)

Attachments

(2 files)

Attached image normal_-_safemode.png
m-c:
 2:40.59 LOG: MainThread Bisector INFO Last good revision: 7d4ab4a9febd (2015-06-07)
 2:40.59 LOG: MainThread Bisector INFO First bad revision: 4700d1cdf489 (2015-06-08)
 2:40.59 LOG: MainThread Bisector INFO Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=7d4ab4a9febd&tochange=4700d1cdf489

m-i:
 1:47.54 LOG: MainThread Bisector INFO Last good revision: ab9471275a90
 1:47.55 LOG: MainThread Bisector INFO First bad revision: 9ef529a9a02b
 1:47.55 LOG: MainThread Bisector INFO Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=ab9471275a90&tochange=9ef529a9a02b

fx-team:
 5:34.95 LOG: MainThread Bisector INFO Last good revision: 653299500d3c
 5:34.96 LOG: MainThread Bisector INFO First bad revision: 39821d952adc
 5:34.96 LOG: MainThread Bisector INFO Pushlog:
https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=653299500d3c&tochange=39821d952adc

39821d952adc	Gijs Kruitbosch — Bug 1150417 - use theme manifest for overrides, r=glandium,dao
264fb3d4d4c6	Gijs Kruitbosch — Bug 1170207 - allow overrides of chrome://../skin/ URIs with other chrome://../skin/ URIs within skin manifests, r=bsmedberg
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression
Summary: Navbar-icons in safemode have different color to normal mode → Theme overrides don't work in safe mode and cause missing/wrong images
[Tracking Requested - why for this release]:
This will cause missing icons in e.g. about:addons
I don't really understand why the default theme manifest is no longer read in this case, when it is clearly still the theme that's in use... Dave, do you know?

As a minimal fix for 40 we could move the overrides for images that do not otherwise exist back to the regular manifest file, or back the change out if we can't find a way of fixing this.
Flags: needinfo?(dtownsend)
(In reply to :Gijs Kruitbosch from comment #2)
> I don't really understand why the default theme manifest is no longer read
> in this case, when it is clearly still the theme that's in use... Dave, do
> you know?

Ugh right, because we don't load extension manifests when in safe mode.
Flags: needinfo?(dtownsend)
Adding a tracking flag. We should not be messing up the icons in safe mode.
Benjamin, where does the code live that adds these manifests and checks for safe mode? I've been looking for this last week but I couldn't find it. Would changing those to make an exception for the default theme be doable, or should we consider reverting the decision to move the overrides from the main manifest into the extension manifest? (if we leave the overrides-allowed-in-themes thing in, at least themes can workaround by using overrides themselves rather than packaging extra files in the right places, I guess...)
Flags: needinfo?(benjamin)
The code which loads theme directories is here:
http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsXREDirProvider.cpp#644

I don't know how this interacts with safe mode. I suspect it might be through this check: http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm#7106
Flags: needinfo?(benjamin)
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #6)
> I don't know how this interacts with safe mode. I suspect it might be
> through this check:
> http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/
> internal/XPIProvider.jsm#7106

Nope, right here: http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsXREDirProvider.cpp#623
Thanks to both of you!
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Flags: needinfo?(gijskruitbosch+bugs)
Bug 1176703 - load default theme manifest in safe mode, r?bsmedberg
Attachment #8630369 - Flags: review?(benjamin)
https://reviewboard.mozilla.org/r/12711/#review11383

::: toolkit/xre/nsXREDirProvider.cpp:657
(Diff revision 1)
> +    }

This makes assumptions about the default theme's ID that I think we'll forget down the road. I think it would work to just only load the ThemeDirs from the ini and make sure that the value of general.skins.selectedSkin is ignored in safe mode (I assume it is already but let's check)
(In reply to Dave Townsend [:mossop] (PTO till July 21st) from comment #10)
> https://reviewboard.mozilla.org/r/12711/#review11383
> 
> ::: toolkit/xre/nsXREDirProvider.cpp:657
> (Diff revision 1)
> > +    }
> 
> This makes assumptions about the default theme's ID that I think we'll
> forget down the road.

FWIW, I thought about this, but we make this assumption in a lot of places, including automated tests and our packaging manifests, which is why this seemed acceptable...

> I think it would work to just only load the ThemeDirs
> from the ini and make sure that the value of general.skins.selectedSkin is
> ignored in safe mode (I assume it is already but let's check)

... but this does sound neater. I can look at this later today, I hope. Setting ni so I don't forget...
Flags: needinfo?(gijskruitbosch+bugs)
https://reviewboard.mozilla.org/r/12711/#review11383

> This makes assumptions about the default theme's ID that I think we'll forget down the road. I think it would work to just only load the ThemeDirs from the ini and make sure that the value of general.skins.selectedSkin is ignored in safe mode (I assume it is already but let's check)

I'm similarly concerned, although I'll accept this if we can't find a more elegant solution. Waiting on your investigations later today before marking anything.
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #12)
> https://reviewboard.mozilla.org/r/12711/#review11383
> 
> > This makes assumptions about the default theme's ID that I think we'll forget down the road. I think it would work to just only load the ThemeDirs from the ini and make sure that the value of general.skins.selectedSkin is ignored in safe mode (I assume it is already but let's check)
> 
> I'm similarly concerned, although I'll accept this if we can't find a more
> elegant solution. Waiting on your investigations later today before marking
> anything.

So, if I do this:

1) lightweight themes remain applied in safe mode
2) full themes don't remain applied *but* the same problem we're trying to fix here reoccurs, in that the overrides from the default theme aren't being applied, if your selected theme isn't the default theme (yeah... not sure what's happening there).

So it seems like if we want to go this route we need more work in the add-on manager / lwtheme code. I can do that, but it will take more time and I'm worried about running out of time for 40.

Benjamin, thoughts?

(my test patch: https://pastebin.mozilla.org/8839005 )
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to :Gijs Kruitbosch from comment #13)
> (In reply to Benjamin Smedberg  [:bsmedberg] from comment #12)
> > https://reviewboard.mozilla.org/r/12711/#review11383
> > 
> > > This makes assumptions about the default theme's ID that I think we'll forget down the road. I think it would work to just only load the ThemeDirs from the ini and make sure that the value of general.skins.selectedSkin is ignored in safe mode (I assume it is already but let's check)
> > 
> > I'm similarly concerned, although I'll accept this if we can't find a more
> > elegant solution. Waiting on your investigations later today before marking
> > anything.
> 
> So, if I do this:
> 
> 1) lightweight themes remain applied in safe mode
> 2) full themes don't remain applied *but* the same problem we're trying to
> fix here reoccurs, in that the overrides from the default theme aren't being
> applied, if your selected theme isn't the default theme (yeah... not sure
> what's happening there).
> 
> So it seems like if we want to go this route we need more work in the add-on
> manager / lwtheme code. I can do that, but it will take more time and I'm
> worried about running out of time for 40.
> 
> Benjamin, thoughts?
> 
> (my test patch: https://pastebin.mozilla.org/8839005 )
Flags: needinfo?(benjamin)
Comment on attachment 8630369 [details]
MozReview Request: Bug 1176703 - load default theme manifest in safe mode, r?bsmedberg

This patch is simpler, and simple is better here. I'm hoping to get rid of heavy themes anyway since that is a necessary part of kill-XUL, and then the problem will just go away.
Flags: needinfo?(benjamin)
Attachment #8630369 - Flags: review?(benjamin) → review+
Comment on attachment 8630369 [details]
MozReview Request: Bug 1176703 - load default theme manifest in safe mode, r?bsmedberg

Approval Request Comment
[Feature/regressing bug #]: bug 1150417 / windows theme work
[User impact if declined]: missing/wrong images in safe mode
[Describe test coverage new/current, TreeHerder]: nope...
[Risks and why]: reasonably low, change specific to how we deal with the default theme's manifest in safe mode
[String/UUID change made/needed]: nope
Attachment #8630369 - Flags: approval-mozilla-beta?
Attachment #8630369 - Flags: approval-mozilla-aurora?
Comment on attachment 8630369 [details]
MozReview Request: Bug 1176703 - load default theme manifest in safe mode, r?bsmedberg

We want the safe mode to be safe! Taking it.
Attachment #8630369 - Flags: approval-mozilla-beta?
Attachment #8630369 - Flags: approval-mozilla-beta+
Attachment #8630369 - Flags: approval-mozilla-aurora?
Attachment #8630369 - Flags: approval-mozilla-aurora+
Flags: qe-verify+
Confirming the fix for:
- latest 42.0a1 Nightly, build ID: 20150715095506;
- latest 41.0a2 Aurora, build ID: 20150715095519;
- Firefox 40.0b4, build ID: 20150713153304.
Blocks: 1189918
You need to log in before you can comment on or make changes to this bug.