1503871 - SVGs not rendered for buttons/icons, making those elements invisible

Status: VERIFIED FIXED

(DevTools :: Debugger, defect)

Description

[:Harald Kirschner :digitarald]

Attachment: image.png

65.0a1 (2018-11-01) (64-bit)

STR: Open Debugger

Step & pause buttons are invisible but hoverable for tooltips.

Comment 1

[Jason Laster [:jlast]]

It looks like these commits introduced the issue

0a08088df7b5 Bug 1501503 Part 2: Test that CORS rejection messages are output for loads triggered from styles. r=ckerschb
a4d0a122a3d1 Bug 1501503: Report cross-origin load failures in more cases. r=ckerschb

Comment 2

[Jason Laster [:jlast]]

Attachment: Screen Shot 2018-11-01 at 10.40.06 AM.png

Comment 4

[Brad Werth [:bradwerth]]

Attachment: Bug 1503871: Make CORS checks trivally pass through a SystemPrincipal as the loading principal. r?ckerschb!

Comment 5

[Oriol Brufau [:Oriol]]

Regressed in https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=7fac836796900045b621a258719127c0805fe4db&tochange=e79a4c8e28669b9a9e15a01535fb80adff870f10

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Brad Werth [:bradwerth] from comment #4)
> Created attachment 9021895 [details]
> Bug 1503871: Make CORS checks trivally pass through a SystemPrincipal as the
> loading principal. r?ckerschb!

Hey Brad, what is the console warning in that particular case? Looking at your patch I am quite curious when it would ever be the case that the triggeringPrincipal is not a systemPrincipal but the loadingPrincipal is.

Comment 7

[Jonathan Kingston [:jkt] he/him]

An example error log looks like: 

> Security Error: Content at resource://devtools/client/debugger/new/dist/debugger.css may not load or link to chrome://devtools/skin/images/debugger/close.svg.

Which sounds to me like devtools/client/debugger/new/dist/debugger.css isn't getting the system principal it needs to load that.

Changing this code to permit loads into system principal documents seems like it will cause security regressions though. Do the mask's need to be chrome:// ?

Comment 8

[Brad Werth [:bradwerth]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #6)
> Hey Brad, what is the console warning in that particular case? Looking at
> your patch I am quite curious when it would ever be the case that the
> triggeringPrincipal is not a systemPrincipal but the loadingPrincipal is.

The first error I get is from a path where this URL is attempting to be loaded with CORS anonymous headers (presumably because of a mask-image): "file:///Users/brad/repos/obj-stylo/dist/NightlyDebug.app/Contents/Resources/browser/chrome/devtools/skin/images/devtools-components/arrow.svg"

In that case, the loading principal is a/the System Principal, and the triggering principal is a ContentPrincipal with the codebase URL "resource://devtools/client/debugger/new/dist/debugger.css"

That triggers a CheckLoadURIError, which reads: Security Error: Content at resource://devtools/client/debugger/new/dist/debugger.css may not load or link to chrome://devtools/skin/images/devtools-components/arrow.svg.

So, it appears we can have cases where the loading principal is a System Principal and the triggering principal is not, which is why my proposed patch makes a difference in this case.

Comment 10

[Brad Werth [:bradwerth]]

(In reply to Jonathan Kingston [:jkt] from comment #7)
> Changing this code to permit loads into system principal documents seems
> like it will cause security regressions though. Do the mask's need to be
> chrome:// ?

The uses in debugger.css[1][2] both seem to use chrome protocol URLs already.

[1][https://searchfox.org/mozilla-central/rev/7c848ac7630df5baf1314b0c03e015683599efb9/devtools/client/debugger/new/dist/debugger.css#341]
[2][https://searchfox.org/mozilla-central/rev/7c848ac7630df5baf1314b0c03e015683599efb9/devtools/client/debugger/new/dist/debugger.css#1099]

Comment 11

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Brad Werth [:bradwerth] from comment #8)
> So, it appears we can have cases where the loading principal is a System
> Principal and the triggering principal is not, which is why my proposed
> patch makes a difference in this case.

As explained in the phab, that change looks to risky and like a potential footgun to me. We need to find something else. I guess what Jonathan was proposing sounds like a plausible path forward - if we can load both the css and svg using resource: URIs the problem should disappear.

Comment 12

[Brad Werth [:bradwerth]]

Attachment: Bug 1503871 Part 1: Change all of the debugger.css SVG mask URLs to use resource scheme. r?jlast!

Comment 13

[Brad Werth [:bradwerth]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #11)
> As explained in the phab, that change looks to risky and like a potential
> footgun to me. We need to find something else. I guess what Jonathan was
> proposing sounds like a plausible path forward - if we can load both the css
> and svg using resource: URIs the problem should disappear.

Understood. Attachment 9023072 [details] switches all of the URLs to use resource:// schemes. This avoids the security errors, but the images still do not appear for a reason I have yet to determine. Stepping through the security check code in all the places where I can think to check, the loads appear to be successful. So this patch may be part of the solution, but it's not the whole solution.

Comment 14

[Brad Werth [:bradwerth]]

(In reply to Jonathan Kingston [:jkt] from comment #7)
> An example error log looks like: 
> 
> > Security Error: Content at resource://devtools/client/debugger/new/dist/debugger.css may not load or link to chrome://devtools/skin/images/debugger/close.svg.
> 
> Which sounds to me like devtools/client/debugger/new/dist/debugger.css isn't
> getting the system principal it needs to load that.

Agreed. The System Principal loads debugger.css, and then when the "mask:" declaration in there is resolved, it's loaded by a content principal for debugger.css, which doesn't have sufficient permissions to load the content into the document. Changing this would require the URLExtraData for the SVG element to be created earlier than MappedAttrParser::ParseMappedAttrValue, when it still has access to the triggering principal used when debugger.css was loaded. Hard to do.

I'll continue to see if I can make the change to "resource://" URLs work, because it will not be easy to change the principal at the time the SVGs are loaded.

Comment 16

[Julian Descottes [:jdescottes]]

(In reply to Brad Werth [:bradwerth] from comment #14)
> (In reply to Jonathan Kingston [:jkt] from comment #7)
> > An example error log looks like: 
> > 
> > > Security Error: Content at resource://devtools/client/debugger/new/dist/debugger.css may not load or link to chrome://devtools/skin/images/debugger/close.svg.
> > 
> > Which sounds to me like devtools/client/debugger/new/dist/debugger.css isn't
> > getting the system principal it needs to load that.
> 
> Agreed. The System Principal loads debugger.css, and then when the "mask:"
> declaration in there is resolved, it's loaded by a content principal for
> debugger.css, which doesn't have sufficient permissions to load the content
> into the document. Changing this would require the URLExtraData for the SVG
> element to be created earlier than MappedAttrParser::ParseMappedAttrValue,
> when it still has access to the triggering principal used when debugger.css
> was loaded. Hard to do.
> 
> I'll continue to see if I can make the change to "resource://" URLs work,
> because it will not be easy to change the principal at the time the SVGs are
> loaded.

I tried to migrate everything CSS from chrome:// to resource:// in the past: Bug 1311541. It stalled because we were unsure about performance and it was no longer needed at some point, but the patches were working so you can look at them as a reference.

It looks like you at least need to:
- add the svgs to the relevant moz.build so that they can be accessible via resource://
- remove them from jar.mn
- change paths from `chrome://devtools/skin/images/` to `resource://devtools/client/themes/images/`

Comment 17

[Brad Werth [:bradwerth]]

Jason tells me he's built a fix. Taking myself off the bug so he can land patches.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/50586a989dc4
Change all of the debugger.css SVG mask URLs to use resource scheme. r=bradwerth

Comment 19

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/50586a989dc4

Comment 20

[Cristian Fogel [:cfogel]]

Confirmed issue with ffx 65.0a1(20181101220058) on Windows10.
Fix verified with 65.0b12 on Windows 10, macOS 10.11, Ubuntu18.04.