Closed Bug 1220903 Opened 4 years ago Closed 4 years ago

Make chrome-only CSS properties really usable in chrome code

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox44 --- fixed
firefox45 --- fixed
b2g-v2.5 --- fixed

People

(Reporter: xidorn, Assigned: xidorn)

References

Details

Attachments

(1 file)

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.
Bug 1220903 - Give a stylesheet the system principal if its loader has.
Attachment #8682334 - Flags: review?(bzbarsky)
Assignee: nobody → quanxunzhen
Blocks: 1220907
Attachment #8682334 - Flags: review?(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 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/
Attachment #8682334 - Attachment description: MozReview Request: Bug 1220903 - Give a stylesheet the system principal if its loader has. → MozReview Request: Bug 1220903 - Allow style to access chrome-only properties if it is loaded via chrome protocol.
Attachment #8682334 - Flags: review?(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.
Attachment #8682334 - Flags: review?(bzbarsky)
(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).
> 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?
Flags: needinfo?(quanxunzhen)
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
Flags: needinfo?(quanxunzhen)
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
Attachment #8682334 - Flags: review+
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 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
Attachment #8682334 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/4f0b5a4695f3
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
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!
Flags: needinfo?(quanxunzhen)
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.
Flags: needinfo?(quanxunzhen)
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.
Attachment #8682334 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(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!
I can, after the patch in this bug gets landed.
You need to log in before you can comment on or make changes to this bug.