940625 - SVG image cache should invalidate when the SVG depends on the system theme and the theme changes

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[alex_mayorga]

Attachment: Screen capture of the issue.

Steps:
Switch theme to "Windows Classic"

Result:
Tabs and app tabs show a shade of blue on the left and right borders.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Ah - I think this is the result of us caching the tab shape for performance reasons...

Note that this requires one to switch themes while Firefox is open - that often results in weird artifacts and things, so I don't think this is 100% out of character. Restarting Firefox should resolve the issue.

I don't think it's really worth trying to flush the cache or anything when the system changes themes. I'm going to say this is a WONTFIX unless there's any serious objection.

Comment 2

[alex_mayorga]

IE11 didn't glitch, so I'd say this should be a valid bug if only for parity sake.

Comment 3

[alex_mayorga]

Attachment: Screen capture of the issue when turning Aero back on.

The issue also exist when turning Aero back on, but the blueish hue is inverted.

Comment 4

[:Gijs (he/him)]

(In reply to alex_mayorga from comment #2)
> IE11 didn't glitch, so I'd say this should be a valid bug if only for parity
> sake.

Even for parity bugs, we can still decide to WONTFIX, and I think Mike's decision here is correct. I don't think we should forego the caching here just to get rid of a bug that pretty much nobody will ever see.

There are far worse bugs with switching OS themes while Firefox is running. It's just not something we've really ever supported very well, and I don't think this bug is worth fixing.

Comment 5

[Mike Kaply [:mkaply]]

Is there no event/message you get from the operating system that allows you to clear the cache?

Comment 6

[alex_mayorga]

(In reply to Mike Kaply (:mkaply) from comment #5)
> Is there no event/message you get from the operating system that allows you
> to clear the cache?

There's WM_THEMECHANGED message[1] it seems but seems this will be WONTFIX anyway =(

1 http://msdn.microsoft.com/en-us/library/windows/desktop/ms632650%28v=vs.85%29.aspx

Comment 7

[alex_mayorga]

Quoting Elbart from http://forums.mozillazine.org/viewtopic.php?p=13335635#p13335635 as it seems to contain valuable info:

"I'd run into this on a daily basis, as I'm using a tool (Aerofoil) which switches to the Basic-theme when the laptop is running in battery-mode. i.e. http://superuser.com/questions/63574/windows-7-aero-theme-when-on-battery

HPs, Dells and maybe other have similar features.

And yes, seems to be the same bug. Last good 2013-11-18, first bad 2013-11-19.
And they're already using WM_THEMECHANGED: http://mxr.mozilla.org/mozilla-central/source/widget/windows/nsWindow.cpp?rev=89f9304ff4ba#4604"

Comment 8

[:Gijs (he/him)]

(In reply to alex_mayorga from comment #6)
> (In reply to Mike Kaply (:mkaply) from comment #5)
> > Is there no event/message you get from the operating system that allows you
> > to clear the cache?
> 
> There's WM_THEMECHANGED message[1] it seems but seems this will be WONTFIX
> anyway =(
> 
> 1
> http://msdn.microsoft.com/en-us/library/windows/desktop/ms632650%28v=vs.
> 85%29.aspx

The problem is that that message isn't propagated to frontend/JS. A patch would need to deal with that issue. :-(

Comment 10

[Matthew N. [:MattN]]

(In reply to :Gijs Kruitbosch from comment #8)
> The problem is that that message isn't propagated to frontend/JS. A patch
> would need to deal with that issue. :-(

Layout code can invalidate the SVG image cache when that message occurs. The front-end shouldn't need to know about it. I thought I filed a bug on this when I fixed bug 921038 as I remember making a testcase. I can provide one but don't have time at the moment. An example is at https://hg.mozilla.org/mozilla-central/diff/e771168640a7/browser/themes/shared/tab-selected.svg#l1.42

Comment 12

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 13

[Jonathan Watt [:jwatt]]

Attachment: patch

According to Try, on OS X 10.8, nsPresContext::ThemeChangedInternal calls DiscardAllSurfacesForVectorImages before the surface cache has been initialized, so here's a patch tweaked to check for that.

Comment 14

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8443084 [details] [diff] [review]
patch

Review of attachment 8443084 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for fixing this, Jonathan!

::: image/src/SurfaceCache.h
@@ +174,5 @@
> +   * Evicts all caches surfaces that are for vector images from ths cache.
> +   * This is useful when the app theme changes, since vector images may be
> +   * dependent on the theme (on system colors).
> +   */
> +  static void DiscardAllSurfacesForVectorImages();

I'm not sure we gain much over just calling DiscardAll. Obviously they do exactly the same thing right now, but even in the future when they won't, this is such a rare event that I think it's OK to flush the whole cache. I'd prefer to avoid adding extra machinery for this case until we have evidence that it matters. (That may well happen, but I think there's a decent possibility it won't.)

r+ with DiscardAllSurfacesForVectorImages removed. (If you feel strongly that I'm wrong, though, leave it in. I don't feel so strongly that I'd r- the patch over it.)

Comment 15

[Jonathan Watt [:jwatt]]

Comment on attachment 8443084 [details] [diff] [review]
patch

https://hg.mozilla.org/integration/mozilla-inbound/rev/723f393bbf44

Comment 16

[Jonathan Watt [:jwatt]]

Meh, and a follow-up for build bustage due to DiscardAll not being callable before sInstance is set like DiscardAllSurfacesForVectorImages was:

https://hg.mozilla.org/integration/mozilla-inbound/rev/8e069ea10ef1

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/723f393bbf44
https://hg.mozilla.org/mozilla-central/rev/8e069ea10ef1