Closed Bug 1321754 Opened 8 years ago Closed 7 years ago

stylo: UA style sheets parsed with author features should be added at the UA level

Categories

(Core :: CSS Parsing and Computation, defect, P1)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: heycam, Assigned: TYLin)

References

Details

Attachments

(8 files, 1 obsolete file)

Bug 1321754 - Part 2: Pass SheetType to Servo FFI functions that create sheets.
58 bytes, text/x-review-board-request
xidorn
: review+
Details
Use SheetType to determine Servo Stylesheet origin, not SheetParsingMode.
58 bytes, text/x-review-board-request
emilio
: review+
Details
Update regen.py.
58 bytes, text/x-review-board-request
emilio
: review+
Details
Regenerate bindings.
58 bytes, text/x-review-board-request
Details
Bug 1321754 - Part 1: Record on StyleSheet objects whether they are UA, user or document style sheets.
58 bytes, text/x-review-board-request
dbaron
: review-
Details
Bug 1321754 Part 1 - Add an enum value to SheetParsingMode for agent sheets that use no unsafe rules.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Define scrollbar-thumb
1.39 KB, patch
Details | Diff | Splinter Review
Bug 1321754 Part 2 - Update reftest and crashtest expectations for stylo.
59 bytes, text/x-review-board-request
heycam
: review+
Details 
Since bug 1293809, we no longer apply document-level style sheets to native anonymous content.  However, some UA sheets are parsed with document level features enabled, rather than agent level features.  This is done just to limit the chance of accidentally adding "unsafe" rules, I guess, but it has the effect of those sheets being added as document-level sheets in the Stylist.

The scrollbars sheet is one such sheet, so this is causing scrollbars not to work.
Comment on attachment 8816863 [details]
Use SheetType to determine Servo Stylesheet origin, not SheetParsingMode.

https://reviewboard.mozilla.org/r/97394/#review97670
Attachment #8816863 - Flags: review?(ecoal95) → review+
Comment on attachment 8816864 [details]
Update regen.py.

https://reviewboard.mozilla.org/r/97396/#review97672
Attachment #8816864 - Flags: review?(ecoal95) → review+
I'm not sure whether part 1 is the right way to go. Redirecting to dbaron. Actually, how do scrollbars work on Firefox if it doesn't work when its style sheet is treated as doc-level one?
Attachment #8816866 - Flags: review?(xidorn+moz) → review?(dbaron)
Having a sheet type in addition to parsing mode everywhere doesn't look sensible to me... What would we lose if we just make every sheet use the parsing mode they desire?
I don't know if there are places other than those UA sheets in nsLayoutStylesheetCache where we use a parsing mode that doesn't match the sheet type.  I will look.

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #8)
> Actually, how do scrollbars work on Firefox if it doesn't work when its
> style sheet is treated as doc-level one?

It's not treated that way in Gecko, just with Stylo.
Comment on attachment 8816862 [details]
Bug 1321754 - Part 2: Pass SheetType to Servo FFI functions that create sheets.

https://reviewboard.mozilla.org/r/97392/#review97800

This patch makes sense to me. I wonder whether we can just replace parsing mode with sheet type. I have no idea how it is useful to have two things basically for the same goal.
Attachment #8816862 - Flags: review?(xidorn+moz) → review+
(In reply to Cameron McCormack (:heycam) from comment #0)
> This is done just to
> limit the chance of accidentally adding "unsafe" rules, I guess, but it has
> the effect of those sheets being added as document-level sheets in the
> Stylist.

Are you sure this wasn't just an accident?
Flags: needinfo?(cam)
I think so.  The comment above the LoadSheet calls that use a more restricted parsing mode but for an agent sheet say things like "this sheet doesn't need unsafe rules".
Flags: needinfo?(cam)
A try push shows no failures when I align the sheet type and parsing mode for the sheets loaded by nsLayoutStylesheetCache, so maybe we indeed don't need to record both of these:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=71ab03448022ca8920d7463479fd97470486b503&group_state=expanded
Bobby pointed out that in the WebExtensions world, we probably want to restrict extensions from adding style sheets with unsafe rules, but still want to allow them to be added in the user agent level.  So that probably indicates we still want to keep the concepts of sheet level and parsing mode separate.
See Also: → 1291376
As a record, I think we in general shouldn't allow anything to touch user agent sheet if we don't allow it to use any unsafe rules. Basically, important user agent level rule should be considered unsafe to change.

Extensions should either be restricted to use at most the user style level, or be allowed to use whatever they want at their own risk of breaking the browser, since allowing them to use user agent level could be dangerous enough.
I had trouble searching for a definitive answer, but Rob Wu told me that WebExtensions don't allow you to load a style sheet at anything other than the document level, so at some point in the future when we no longer have XPCOM addons this problem might indeed be solved for us.  (Although it is still possible system / go faster addons would want to load sheets at different levels.)

Searching on DXR, there do seem to be many addons that are passing AGENT_SHEET to style sheet service methods.  I did wonder whether we could just silently treat AGENT_SHEET as USER_SHEET, but thinking some more I am concerned that addons rely on (accidentally or deliberately) the relative specificity of rules in their sheets and Firefox's sheets.

I think for now we should do the safest thing for addon compatibility and keep the concepts of "level a sheet is added at" and "features available when parsing the sheet" separate.  As a followup, I think we can change Servo so that the Origin is not recorded on a sheet itself, but instead something that the Stylist keeps track of.  That way we can create a sheet with a given parsing mode and be able to add to the Stylist at an arbitrary cascade level.
I believe that style sheet service does use agent level parsing mode for style sheets with type AGENT_SHEET, and some of our tests may also depend on that behavior.

Addons are not encouraged to do that, though.
So I'm still not happy about having 2 different values for the cascade level, one of which is sometimes wrong in order to prevent allowing unsafe rules?

What about, instead:
 * having a single cascade level enum that's always correct
 * having a flags field that (today) has just one flag, which is whether to allow unsafe rules
 * having an MOZ_RELEASE_ASSERT that the unsafe rules flag is only set when the cascade level is UA sheet
?
Flags: needinfo?(cam)
Comment on attachment 8816866 [details]
Bug 1321754 - Part 1: Record on StyleSheet objects whether they are UA, user or document style sheets.

https://reviewboard.mozilla.org/r/97402/#review103582

see comment 20
Attachment #8816866 - Flags: review?(dbaron) → review-
Priority: -- → P1
Assignee: nobody → cam
Cameron, Per comment 20, are your patches in this bug still relevant? Just trying to figure out whether I should keep this bug on your plate (on the grounds that you're partway through it) or pass it along to someone else (on the grounds that you've got a lot on your plate).
They probably need changing a fair bit, so it's fine for someone else to take this.
Flags: needinfo?(cam)
Assignee: cam → nobody
Assignee: nobody → tlin
Re comment 20:

scrollbars.css is the only sheet created in nsLayoutStylesheetCache.cpp which is parsed as author level, but later added as agent level in [1]. Instead of adding a bool flag to allow unsafe rules, which might be of no use other than agent sheet, and will need to modify a lot of function signatures. We can probably add a new enum value to SheetParsingMode called eAgentSheetNoUnsafeRulesFeatures so that it can be parsed as author level in gecko (nsCSSParser::AgentRulesEnabled() will exclude it), but servo can recognize it as agent level sheet when the sheet is created.

I'll upload a patch for review later.

[1] http://searchfox.org/mozilla-central/rev/7419b368156a6efa24777b21b0e5706be89a9c2f/layout/base/nsDocumentViewer.cpp#2326
Comment on attachment 8852384 [details]
Bug 1321754 Part 1 - Add an enum value to SheetParsingMode for agent sheets that use no unsafe rules.

https://reviewboard.mozilla.org/r/124628/#review127520

::: layout/style/SheetParsingMode.h:40
(Diff revision 1)
>    eAuthorSheetFeatures = 0,
>    eUserSheetFeatures,
> -  eAgentSheetFeatures
> +  eAgentSheetFeatures,
> +  eAgentSheetNoUnsafeRulesFeatures,

Nit: regarding the name, I interpret the names like "eAuthorSheetFeatures" as "the features that are available in author sheets".  So to me, "eAgentSheetNoUnsafeRulesFeatures" doesn't read well to me.  How about "eSafeAgentSheetFeatures"?

::: layout/style/nsCSSParser.cpp
(Diff revision 1)
> -  bool UserRulesEnabled() const {
> -    return mParsingMode == css::eAgentSheetFeatures ||
> -           mParsingMode == css::eUserSheetFeatures;
> -  }

By the way, it looks like the intention was to limit @-moz-document rules to user sheets and UA sheets, in bug 1035091, but that never got fixed and landed.

It might be worth mentioning above, in the comment for SheetParsingMode, that there aren't any features currently that are available in user sheets that are not also available in author sheets.
Attachment #8852384 - Flags: review?(cam) → review+
Comment on attachment 8852384 [details]
Bug 1321754 Part 1 - Add an enum value to SheetParsingMode for agent sheets that use no unsafe rules.

https://reviewboard.mozilla.org/r/124628/#review127520

> Nit: regarding the name, I interpret the names like "eAuthorSheetFeatures" as "the features that are available in author sheets".  So to me, "eAgentSheetNoUnsafeRulesFeatures" doesn't read well to me.  How about "eSafeAgentSheetFeatures"?

`eSafeAgentSheetFeatures` sounds good to me.

> By the way, it looks like the intention was to limit @-moz-document rules to user sheets and UA sheets, in bug 1035091, but that never got fixed and landed.
> 
> It might be worth mentioning above, in the comment for SheetParsingMode, that there aren't any features currently that are available in user sheets that are not also available in author sheets.

Added a comment to mention that `eUserSheetFeatures` aren't currently in use.
Pushed by tlin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/aa46d54ab74b
Add an enum value to SheetParsingMode for agent sheets that use no unsafe rules. r=heycam
I've tried to mark some reftest failure related to scrollbars. However, with my patch, some intermittent bugs such as bug 1279822 become nearly perma-orange. It's also unknown to me why tc-R(C) leaks. Need to investigate further.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9585f7ec57fbe1125f3e4eed853fb8ac4d3e2739
Blocks: 1321769
Just checking in - how close are we on this? bz thinks it will fix a lot of test failures.
Sorry, I haven't getting my patch closer to land yet. Comment 32 still applies, and the number of failures it causes are more than what it fixed (such as leaks and reftest failures unrelated to scrollbars).

https://treeherder.mozilla.org/#/jobs?repo=try&revision=109bf8fbc8ff0123ff40fecb4d6e23486c269f29
Flags: needinfo?(tlin)
I did a try run with attachment 8852384 [details] (and fix for bug 1358754) but removing all -moz-appearance in xulscrollbars.css:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f1f8e572a7e9b58e469e80c462f54a07df92cde8

Most of failures in comment 32 have gone. That means we can mark 'fails' for them with bug number (bug 1349651).
Remaining failures are:

1) 582146-1.html | assertion count 4 is more than expected 1 to 3
2) 323386-1.html | assertion count 1 is more than expected 0 assertions
3) leak on crashtests

1) raises currently 2 assertions on gecko (for the reference image), whereas we got 1 assertion without 8852384 on stylo, now we get 2 assertions that are exactly the same. That's really good!
2) Likewise; On gecko we get 1 assertion.
3) is the final issue here in this bug.  Someone holds 'scrollbar-thumb' atom;
  2 dynamic atom(s) with non-zero refcount: [System Principal],scrollbar-thumb: 'nonZeroRefcountAtomsCount == 0', file /home/worker/workspace/build/src/xpcom/ds/nsAtomTable.cpp, line 421
I can reproduce the crashtest leak locally on Linux by running "./mach crashtest layout/base/crashtests/1343937.html"
By removing the -moz-binding [1] in xulscrollbars.css, there's no leak after running the test in comment 36. I don't know what's wrong with -moz-binding yet.

[1] http://searchfox.org/mozilla-central/rev/876c7dd30586f9c6f9c99ef7444f2d73c7acfe7c/toolkit/themes/windows/global/xulscrollbars.css#16
I did compare a leak string (scrollbar-thumb) and non-leak string (scrollbar-up-top);

http://searchfox.org/mozilla-central/search?q=scrollbar-up-top&case=true&path=
http://searchfox.org/mozilla-central/search?q=%22scrollbar-thumb%22&case=true&path=

Both use cases look the same to me, but scrollbar-up-top is defined in nsGkAtomList.h and atom_macros.rs. So I tried attaching patch, then scrollbar-thumb is not leaked any more. 
Honestly I don't really understand our atom string handling, so I am not sure this is a right thing to do.

Anyway, we still have another leak '[System Principal]'.
So, it seems we're leaking non-static atoms? That's kind of worrying.
I am not really sure.  But if we leaked non-static atoms, I think we should have been seeing more leak failures on try or other places.
Two possibilities: we are leaking specifically because this is scrollbar content (maybe we are missing clearing out their styles in ServoStyleSet::BeginShutdown?); or this is something specific to UA style sheets (whose lifetime is a bit different from document style sheets) and it's the only dynamic atom that UA sheets end up creating.
FWIW here is a list of elements (element's name) iterated over in ServoRestyleManager::ClearServoDataFromSubtree (which is called from ServoStyleSet::BeginShutdown) when I run the leak test (1343937.html).  It seems to me that scrollbar things are correctly processed in the function.

HTML
HEAD
BODY
scrollbar
xul:scrollbarbutton
xul:scrollbarbutton
xul:slider
xul:thumb
xul:scrollbarbutton
xul:scrollbarbutton
scrollbar
xul:scrollbarbutton
xul:scrollbarbutton
xul:slider
xul:thumb
xul:scrollbarbutton
xul:scrollbarbutton
scrollcorner
HTML
HEAD
BODY
OPTGROUP
scrollbar
xul:scrollbarbutton
xul:scrollbarbutton
xul:slider
xul:thumb
xul:scrollbarbutton
xul:scrollbarbutton
scrollbar
xul:scrollbarbutton
xul:scrollbarbutton
xul:slider
xul:thumb
xul:scrollbarbutton
xul:scrollbarbutton
scrollcorner

Also the leaked atoms come from optgroup element[1].

[1] http://searchfox.org/mozilla-central/source/layout/base/crashtests/1343937.html#11
I think we can land Ting's patch with disabling the leaked crash test on stylo, and we can investigate the leak later because it's the only one test case that causes the leak.  After we landed the patch, if we met more leaks caused by scrollbars, it might be easier to investigate than now. Just a thought.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #43)
> I think we can land Ting's patch with disabling the leaked crash test on
> stylo, and we can investigate the leak later because it's the only one test
> case that causes the leak. 

Oh, I am not convinced that "it's the only one". There might be other test cases though.
Hiro, I agree! I could skip those leak crashtests and land my patch first, and investigate further. Simple page like "data:text/html,<div style="width: 200%; height: 20px; background: blue"></div>" doesn't leak.
I am guessing the leak is somewhat related to pseudo element and position: fixed. 
Simplified test case;
<style>
.c3::before {
  position: fixed;
  overflow-x: hidden;
  content: 'before';
}
</style>
<div class='c3'></div>
Landing now and fixing the leak in a follow-up sounds great to me.
I think the leak has gone on today's central. I guess Emilio's pseudo fixes fixed the leak!!!
Hooray! The leak is gone after I rebased. (Should be fixed by bug 1331047 perhaps?) 

Anyway, since Shing just pushed bug 1351548 to delete reftest-stylo.list on mozilla-inbound, I'll update my the reftest expectations on top of that.
Comment on attachment 8862771 [details]
Bug 1321754 Part 2 - Update reftest and crashtest expectations for stylo.

https://reviewboard.mozilla.org/r/134646/#review137616

::: layout/reftests/bugs/reftest.list:986
(Diff revision 1)
>  == 413982.html 413982-ref.html
>  == 414123.xhtml 414123-ref.xhtml
>  == 414638.html 414638-ref.html
>  fails-if(stylo) == 414851-1.html 414851-1-ref.html
>  fails-if(stylo) == 416106-1.xhtml 416106-1-ref.xhtml
> -== 416752-1.html 416752-1-ref.html
> +fails-if(sytlo) == 416752-1.html 416752-1-ref.html

*stylo
Attachment #8862771 - Flags: review?(cam) → review+
Blocks: 1361235
Attachment #8862770 - Attachment is obsolete: true
Pushed by tlin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c3646caa4ff9
Part 1 - Add an enum value to SheetParsingMode for agent sheets that use no unsafe rules. r=heycam
https://hg.mozilla.org/integration/autoland/rev/7c117b558b21
Part 2 - Update reftest and crashtest expectations for stylo. r=heycam
https://hg.mozilla.org/mozilla-central/rev/c3646caa4ff9
https://hg.mozilla.org/mozilla-central/rev/7c117b558b21
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Depends on: 1384446
No longer depends on: 1384446
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: