1220903 - Make chrome-only CSS properties really usable in chrome code

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Xidorn Quan [:xidorn] UTC+10]

It seems that, currently style sheets loaded by document never have system principal, even if the document has, which means those style sheets are not qualified to access chrome-only properties at all since bug 1069192.

I'm not quite sure what should be done here. May I give those style sheets system principal? Or simply make the properties available for stylesheets in chrome: scheme? I'd prefer the former, but I have no idea whether it could open some security hole. Since content can load chrome style sheets, I really afraid that the latter approach could make this restriction just meaningless.

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

Attachment: MozReview Request: Bug 1220903 - Allow style to access chrome-only properties if it is loaded via chrome protocol.

Bug 1220903 - Give a stylesheet the system principal if its loader has.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8682334 [details]
MozReview Request: Bug 1220903 - Allow style to access chrome-only properties if it is loaded via chrome protocol.

https://reviewboard.mozilla.org/r/24063/#review21783

::: layout/style/Loader.cpp:346
(Diff revision 1)
> -    mUseSystemPrincipal(false),
> +    mUseSystemPrincipal(nsContentUtils::IsSystemPrincipal(aLoaderPrincipal)),

I'm really not terribly happy with this.  If we have some extension pulling in a stylesheet from the web for some reason (and nothing prevents that right now), this sheet will suddenly be able to do all sorts of things it couldn't do before, like link to arbitrary things on the users hard drive, etc.

Looking at the general history of this stuff, bug 416942 has some discussion about which sheets should or should not have the system principal.

I, personally, would much prefer it if we just made skin chrome packages have the system principal; I never could tell why we don't do that...

But failing that, I would probably be OK with setting mIsChrome to IsSystemPrincipal() or aSheetURI has the URI_IS_UI_RESOURCE flag.  See NS_URIChainHasFlags.

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8682334 [details]
MozReview Request: Bug 1220903 - Allow style to access chrome-only properties if it is loaded via chrome protocol.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24063/diff/1-2/

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8682334 [details]
MozReview Request: Bug 1220903 - Allow style to access chrome-only properties if it is loaded via chrome protocol.

https://reviewboard.mozilla.org/r/24063/#review22411

::: layout/style/nsCSSParser.cpp:1602
(Diff revision 2)
> -  mIsChrome = nsContentUtils::IsSystemPrincipal(aSheetPrincipal);
> +  mIsChrome = dom::IsChromeURI(aSheetURI);

Won't this break user/UA stylesheets, which are not loaded from chrome:// URIs, but _are_ loaded with the system principal?  You really do want the "or" bit from comment 2.

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Boris Zbarsky [:bz] from comment #4)
> Comment on attachment 8682334 [details]
> MozReview Request: Bug 1220903 - Allow style to access chrome-only
> properties if it is loaded via chrome protocol.
> 
> https://reviewboard.mozilla.org/r/24063/#review22411
> 
> ::: layout/style/nsCSSParser.cpp:1602
> (Diff revision 2)
> > -  mIsChrome = nsContentUtils::IsSystemPrincipal(aSheetPrincipal);
> > +  mIsChrome = dom::IsChromeURI(aSheetURI);
> 
> Won't this break user/UA stylesheets, which are not loaded from chrome://
> URIs, but _are_ loaded with the system principal?  You really do want the
> "or" bit from comment 2.

I don't know what would happen for user stylesheets (which actually I'm not much concerned). UA stylesheets won't be broken anyway, because all chrome-only properties are required to be accessible from UA stylesheets (there is an assertion for that).

Comment 6

[Boris Zbarsky [:bzbarsky]]

> UA stylesheets won't be broken anyway, because all chrome-only properties are required to
> be accessible from UA stylesheets (there is an assertion for that).

What makes that work, given that mIsChrome will be false for UA sheets with this patch?

Comment 7

[Xidorn Quan [:xidorn] UTC+10]

This static assertion in nsCSSProps.cpp [1] ensures that all properties with CSS_PROPERTY_ENABLED_IN_CHROME set must also have CSS_PROPERTY_ENABLED_IN_UA_SHEETS set.


[1] https://dxr.mozilla.org/mozilla-central/rev/cc473fe5dc512c450634506f68cbacfb40a06a23/layout/style/nsCSSProps.cpp#50

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8682334 [details]
MozReview Request: Bug 1220903 - Allow style to access chrome-only properties if it is loaded via chrome protocol.

Oh, I see.  So we have a separate check for UA sheet stuff, ok.  r=me

Comment 9

[Xidorn Quan [:xidorn] UTC+10]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4f0b5a4695f34f096c13554c7a8967cc0eb990da
Bug 1220903 - Allow style to access chrome-only properties if it is loaded via chrome protocol. r=bz

Comment 10

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8682334 [details]
MozReview Request: Bug 1220903 - Allow style to access chrome-only properties if it is loaded via chrome protocol.

Approval Request Comment
[Feature/regressing bug #]: bug 1069192
[User impact if declined]: see CSS warnings in the browser console, any may see unwanted shadow for certain windows on OS X (see bug 1219875 comment 0 for some details)
[Describe test coverage new/current, TreeHerder]: fix to browser_parsable_css.js will be landed in bug 1220907
[Risks and why]: low risk, because the change is simple and there is test for it
[String/UUID change made/needed]: n/a

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4f0b5a4695f3

Comment 12

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Xidorn, is there a reason why the test patch from bug 1220907 is not being uplifted to Aurora? Should we do that so we can quickly catch any regressions? Thanks!

Comment 13

[Xidorn Quan [:xidorn] UTC+10]

No particular reason. I just don't think uplifting the test is necessary. That may catch regression in aurora, so if you prefer, you can uplift that as well.

Comment 14

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8682334 [details]
MozReview Request: Bug 1220903 - Allow style to access chrome-only properties if it is loaded via chrome protocol.

This change has been in Nightly for a few days, seems safe to uplift to Aurora44.

Comment 15

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #13)
> No particular reason. I just don't think uplifting the test is necessary.
> That may catch regression in aurora, so if you prefer, you can uplift that
> as well.

I think test-only patches don't need uplift request and can be landed directly. Could you please uplift test-only patch to m-a directly? Thanks!

Comment 16

[Xidorn Quan [:xidorn] UTC+10]

I can, after the patch in this bug gets landed.

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d4caa8fa3d08

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/d4caa8fa3d08