Closed Bug 1412714 Opened 2 years ago Closed 2 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)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox57 --- wontfix
firefox58 --- fixed

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.
It seems to only happen in chrome. Non-chrome XUL file doesn't trigger this assertion.
The rule in question is an XBL rule, and it seems we do have a rule map for XBL styleset.
status-firefox57=wontfix unless someone thinks this bug should block 57
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+
Oh, and can we add a testcase?
(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.
(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, ...
(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 :/
Assignee: nobody → xidorn+moz
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/dfa093fe1e15
Don't clone inner of XBL stylesheet in Servo. r=bz
https://hg.mozilla.org/mozilla-central/rev/dfa093fe1e15
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
See Also: → 1420757
You need to log in before you can comment on or make changes to this bug.