1403465 - stylo: Assertion failure: aStyle->GetPseudo() && !aStyle->IsAnonBox(), at include/mozilla/ServoStyleContext.h:74

Status: RESOLVED FIXED

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

Description

[Bob Clary [:bc] (inactive)]

Attachment: Windows 10 Debug Log Nightly/58

1. https://www.mathjax.org/

2. Assertion failure: aStyle->GetPseudo() && !aStyle->IsAnonBox(), at z:/build/build/src/obj-firefox/dist/include/mozilla/ServoStyleContext.h:74
#01: mozilla::ServoStyleContext::SetCachedLazyPseudoStyle(mozilla::ServoStyleContext *) [layout/style/ServoStyleContext.h:74]
#02: mozilla::ServoStyleSet::ResolvePseudoElementStyle(mozilla::dom::Element *,mozilla::CSSPseudoElementType,mozilla::ServoStyleContext *,mozilla::dom::Element *) [layout/style/ServoStyleSet.cpp:596]
#03: mozilla::StyleSetHandle::Ptr::ResolvePseudoElementStyle(mozilla::dom::Element *,mozilla::CSSPseudoElementType,nsStyleContext *,mozilla::dom::Element *) [layout/style/StyleSetHandleInlines.h:139]
#04: nsMathMLFrame::ResolveMathMLCharStyle(nsPresContext *,nsIContent *,nsStyleContext *,nsMathMLChar *) [layout/mathml/nsMathMLFrame.cpp:105]
#05: nsMathMLmoFrame::ProcessTextData() [layout/mathml/nsMathMLmoFrame.cpp:184]
#06: nsMathMLmoFrame::InheritAutomaticData(nsIFrame *) [layout/mathml/nsMathMLmoFrame.cpp:913]
#07: nsMathMLContainerFrame::RebuildAutomaticDataForChildren(nsIFrame *) [layout/mathml/nsMathMLContainerFrame.cpp:661]
#08: nsMathMLmoFrame::MarkIntrinsicISizesDirty() [layout/mathml/nsMathMLmoFrame.cpp:1026]

Windows, Linux Nightly/58, Beta/57

Reproduced on Fedora Nightly, Beta.

This site crashed/panicked in different ways that have been fixed and now exposed this assertion.

Comment 1

[Bobby Holley (:bholley)]

So, the good news is that this turns out to be a reproducible testcase for bug 1402285, which is great. We should reduce it and turn it into a crashtest.

The bad news is that the fix in bug 1402285 was really just wallpaper. To be clear, what's happening is:
(1) nsMathMLFrame::ResolveMathMLCharStyle invokes ResolvePseudoElementStyle on an unstyled element.
(2) We somehow have servo data but not actual styles, so cacheable gets set to true (i.e. the fix from bug 1402285 doesn't apply).
(3) Servo_ResolvePseudoStyle hits the "FIXME(bholley): We should assert against this" case, and returns the default CV [1].
(4) The default CVs don't have the appropriate pseudo set up, so we assert when trying to cache them. More to the point, sticking the default CVs into one of the linked list caches is likely to break things, since those CVs are shared system-wide.

There are various other band-aids we can add here, but now that we have a testcase, we should really just figure out why nsMathMLFrame::ResolveMathMLCharStyle is operating on unstyled elements, and fix that. Additionally, we should probably also stop returning the default CVs from Servo_ResolvePseudoStyle, given the problematic interactions with caching described above.

I'm busy with the heap corruption stuff, would be great if someone can take this.

[1] http://searchfox.org/mozilla-central/rev/3dbb47302e114219c53e99ebaf50c5cb727358ab/servo/ports/geckolib/glue.rs#1781

Comment 2

[Emilio Cobos Álvarez (:emilio)]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #1)
> (2) We somehow have servo data but not actual styles, so cacheable gets set
> to true (i.e. the fix from bug 1402285 doesn't apply).

This looks extremely unlikely, given all the stuff should be styled by the time it happens... If it really is this way, I really want to know how / why.

Comment 3

[Bobby Holley (:bholley)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #2)
> (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #1)
> > (2) We somehow have servo data but not actual styles, so cacheable gets set
> > to true (i.e. the fix from bug 1402285 doesn't apply).
> 
> This looks extremely unlikely, given all the stuff should be styled by the
> time it happens... If it really is this way, I really want to know how / why.

From stepping around in debugger, this is definitely what is happening.

Ryan is going to start investigating today, and then hand off to Emilio.

Comment 4

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Crashtest reduced from page

Here's a crashtest reduced from the page that appears to trigger the same stack for me.

I didn't have time to investigate the cause, but hopefully this will help :emilio!

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1403465 - Crashtest.

Review commit: https://reviewboard.mozilla.org/r/184528/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/184528/

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Attachment: style: Avoid creating element data in Servo_ResolvePseudoStyle.

The reason the patch in bug 1402285 doesn't work is that we call this function
multiple times with the same element. This fixes it.

Review commit: https://reviewboard.mozilla.org/r/184530/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/184530/

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8913108 [details]
Bug 1403465 - Crashtest.

https://reviewboard.mozilla.org/r/184528/#review189662

Comment 8

[Emilio Cobos Álvarez (:emilio)]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #4)
> Created attachment 8913007 [details] [diff] [review]
> Crashtest reduced from page
> 
> Here's a crashtest reduced from the page that appears to trigger the same
> stack for me.
> 
> I didn't have time to investigate the cause, but hopefully this will help
> :emilio!

Thanks so much for the crashtest, made it a lot easier! \o/

Comment 9

[Emilio Cobos Álvarez (:emilio)]

And to be clear, this is still a bug in the MathML code, I'm trying a patch to fix it, but doesn't need to block this. Filed bug 1403865.

Comment 10

[Bobby Holley (:bholley)]

Comment on attachment 8913109 [details]
style: Avoid creating element data in Servo_ResolvePseudoStyle.

https://reviewboard.mozilla.org/r/184530/#review189852

Comment 11

[Bobby Holley (:bholley)]

I very much appreciate the low-risk fix here. :-)

Comment 12

[Emilio Cobos Álvarez (:emilio)]

https://github.com/servo/servo/pull/18666

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8654953bc112
Crashtest. r=emilio

Comment 14

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/8654953bc112

Comment 15

[Sylvestre Ledru [:Sylvestre]]

I guess we would like to uplift that to 57, right?

Comment 16

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8913109 [details]
style: Avoid creating element data in Servo_ResolvePseudoStyle.

Approval Request Comment
[Feature/Bug causing the regression]: Various (bug 1402285, bug 1401317), though the underlying issue is longstanding.
[User impact if declined]: wrong global state.
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: no
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: not risky
[Why is the change risky/not risky?]: just be more careful to avoid incorrectly think we can cache stuff.
[String changes made/needed]: none

Comment 17

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8913109 [details]
style: Avoid creating element data in Servo_ResolvePseudoStyle.

Fix an assert, taking it.
Should be in 57b5

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/5a5d0e596c1d
https://hg.mozilla.org/releases/mozilla-beta/rev/3c2c7b00d6ab

Comment 19

[Andrei Vaida [:avaida]]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #16)
> [Is this code covered by automated tests?]: yes
> [Has the fix been verified in Nightly?]: no
> [Needs manual test from QE? If yes, steps to reproduce]: no

Setting qe-verify- based on Emilio's assessment on manual testing needs and the fact that this fix has automated coverage.