Closed
Bug 1412714
Opened 7 years ago
Closed 7 years ago
stylo: Asserts with "We should be able to map a raw rule to a rule" when inspecting XUL <toolbar> element
Categories
(Core :: CSS Parsing and Computation, defect)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla58
People
(Reporter: xidorn, Assigned: xidorn)
References
Details
Attachments
(1 file)
Hit this when debugging bug 1412560. Not sure what's going wrong this time, will investigate.
Assignee | ||
Comment 1•7 years ago
|
||
It seems to only happen in chrome. Non-chrome XUL file doesn't trigger this assertion.
Assignee | ||
Comment 2•7 years ago
|
||
The rule in question is an XBL rule, and it seems we do have a rule map for XBL styleset.
Comment hidden (mozreview-request) |
Comment 4•7 years ago
|
||
status-firefox57=wontfix unless someone thinks this bug should block 57
status-firefox57:
--- → wontfix
Comment 5•7 years ago
|
||
mozreview-review |
Comment on attachment 8923276 [details]
Bug 1412714 - Don't clone inner of XBL stylesheet in Servo.
https://reviewboard.mozilla.org/r/194454/#review200790
::: layout/style/StyleSheet.cpp:458
(Diff revision 1)
> + // it, as it may break ServoStyleRuleMap. XBL stylesheets are not
> + // supposed to change anyway.
Can we add asserts when they _do_ get changed, so we notice if this theory doesn't hold?
::: layout/style/StyleSheet.cpp:460
(Diff revision 1)
> return;
> }
> + // If this stylesheet is for XBL with Servo, don't bother cloning
> + // it, as it may break ServoStyleRuleMap. XBL stylesheets are not
> + // supposed to change anyway.
> + // The mDocument check is used as a fast reject path as none of
s/as none of/because no/
Attachment #8923276 -
Flags: review?(bzbarsky) → review+
Comment 6•7 years ago
|
||
Oh, and can we add a testcase?
Assignee | ||
Comment 7•7 years ago
|
||
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #5)
> Comment on attachment 8923276 [details]
> Bug 1412714 - Don't clone inner of XBL stylesheet in Servo.
>
> https://reviewboard.mozilla.org/r/194454/#review200790
>
> ::: layout/style/StyleSheet.cpp:458
> (Diff revision 1)
> > + // it, as it may break ServoStyleRuleMap. XBL stylesheets are not
> > + // supposed to change anyway.
>
> Can we add asserts when they _do_ get changed, so we notice if this theory
> doesn't hold?
This theory is expected to hold, but it doesn't, because XBL stylesheets can be changed if someone is really willing to do, by using inspector. See bug 1412744.
That is somehow broken in both Stylo and Gecko anyway, and in the old style system, it is inconsistent as well. One can always get a style rule of XBL stylesheet from a shared inner in inspector (via calling inDOMUtils::GetCSSStyleRules) because EnsureSafeToHandOutCSSRules() doesn't work into XBL stylesheets and ensure unique of them.
You may argue that we should probably just fix EnsureSafeToHandOutCSSRules() to ensure unique of XBL stylesheets, but I don't think it's worth it, because inDOMUtils don't know whether one really wants to inspect XBL rules, and making all XBL stylesheets unique can lead to unnecessary memory / performance impact in normal case where developers don't care about inner of XBL bindings at all.
So this patch is basically making things more consistent everywhere that we never clone an XBL stylesheet. It would make things broken when one wants to mutate it, but I don't think that's something we really need to worry about... And actually I think we should forbid it somehow.
Assignee | ||
Comment 8•7 years ago
|
||
(In reply to Xidorn Quan [:xidorn] UTC+10 (less responsive Nov 5 ~ Dec 16) from comment #7)
> So this patch is basically making things more consistent everywhere that we
> never clone an XBL stylesheet. It would make things broken when one wants to
> mutate it,
... which has been broken or inconsistent somehow right now in Gecko anyway, ...
Assignee | ||
Comment 9•7 years ago
|
||
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #6)
> Oh, and can we add a testcase?
Hmmm... I have no idea how to write testcase for this... It seems that a naive devtools browser mochitest doesn't crash for this like when I did it on browser window directly :/
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → xidorn+moz
Comment 11•7 years ago
|
||
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/dfa093fe1e15
Don't clone inner of XBL stylesheet in Servo. r=bz
Comment 12•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Updated•7 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•