655451 - Poison frame crash [@ nsMathMLFrame::GetAttribute] with <frameset>

Status: VERIFIED FIXED

(Core :: MathML, defect)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when loaded)

Found by fuzzing layout/mathml/crashtests/463763-1.xhtml.

Comment 1

[Jesse Ruderman]

Attachment: stack trace (mac debug)

Comment 2

[Boris Zbarsky [:bzbarsky]]

So we crash because nsMathMLFrame::GetAttribute is passed an aMathMLmstyleFrame which is dead.

And that comes from nsMathMLmoFrame::ProcessOperatorData passing in mPresentationData.mstyle.

The problem, as far as I can tell, is that nothing makes sure that mPresentationData.mstyle is sane.  In this case I see us setting it to a nsMathMLmathInlineFrame during initial frame construction, then setting it to an nsInlineFrame which is the next-in-flow of the next-in-flow of the nsMathMLmathInlineFrame when we do a nsMathMLContainerFrame::RebuildAutomaticDataForChildren due to a nsMathMLmoFrame::MarkIntrinsicWidthsDirty call.  Then I bet we shorten the flow from three lines to two lines, the nsInlineFrame we're pointing to dies, and we lose.  I bet we could get this with just some long equations without any framesets involved.

Fred, any idea what mstyle should actually be here?  We need to either keep it updated correctly or reget it every time we need it or something....

Comment 3

[Frédéric Wang (:fredw)]

mPresentationData.mstyle is a pointer to the nearest <mstyle> ancestor. In bug 569124, I've added code to initialize aPresentationData.mstyle with the value of the frame of the <math> element instead of being null. That way, the math element can behave as a <mstyle>. Removing this change prevent the crash to happen:

diff --git a/layout/mathml/nsMathMLFrame.cpp b/layout/mathml/nsMathMLFrame.cpp
--- a/layout/mathml/nsMathMLFrame.cpp
+++ b/layout/mathml/nsMathMLFrame.cpp
@@ -225,16 +225,17 @@ nsMathMLFrame::GetPresentationDataFrom(n
     if (!content)
       break;
 
     if (content->Tag() == nsGkAtoms::math) {
       const nsStyleDisplay* display = frame->GetStyleDisplay();
       if (display->mDisplay == NS_STYLE_DISPLAY_BLOCK) {
         aPresentationData.flags |= NS_MATHML_DISPLAYSTYLE;
       }
+      aPresentationData.mstyle = frame;
       break;

Comment 4

[Boris Zbarsky [:bzbarsky]]

Hmm.  Would it be good enough to initialize with the first-continuation of the ancestor or something?  That should solve this problem of continuations that go away.

Comment 5

[Karl Tomlinson (:karlt)]

Confirming bz's diagnosis in comment 2, at MarkIntrinsicWidthsDirty the frame
tree is:

> Block(body)(3)@0x7fffb67d82d0 [content=0x7fffc6674900] {480,480,82260,114360} [state=0000000000101000] [vis-overflow=0,0,84180,114360] [scr-overflow=0,0,84180,114360] sc=0x7fff8d50e9b8(i=4,b=0)<
>   line 0x7fff8c13f340: count=1 state=inline,dirty,prevmarginclean,not impacted,wrapped,before:nobr,after:nobr[0x161] {0,0,84180,55860} <
>     Inline(math)(0)@0x7fffb67d85c0 next=0x7fff8cf85538 next-continuation=0x7fff8cf85538 {0,54360,84180,1500} [state=0000000000000020] [content=0x7fffd7f35f80] [vis-overflow=0,-54360,84180,55860] [scr-overflow=0,-54360,84180,55860] [sc=0x7fff8cf85988]<
>       Frameset(frameset)(0)@0x7fffb67d8a00 {0,-54360,84180,55380} [content=0x7fffbd8a3e20] [sc=0x7fff8cf85a88]<
>         0x7fffb67d8cb0 BLANK 
>         Frame(frameset)(0)@0x7fffb67d8cb0 {0,0,82260,55380} [content=0x7fffbd8a3e20] [sc=0x7fff8cf85b28]
>       >
>     >
>   >
>   line 0x7fff8cf855a8: count=1 state=inline,dirty,prevmarginclean,not impacted,wrapped,before:nobr,after:nobr[0x161] {0,55860,5760,1500} <
>     Inline(math)(0)@0x7fff8cf85538 next=0x7fff8cf855e8 prev-continuation=0x7fffb67d85c0 next-continuation=0x7fff8cf855e8 {0,55860,5760,1500} [state=0000000000001004] [content=0x7fffd7f35f80] [sc=0x7fff8cf85988]<>
>   >
>   line 0x7fff8cf85658: count=1 state=inline,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x100] {0,57360,84180,55860} <
>     Inline(math)(0)@0x7fff8cf855e8 next=0x7fff8c13f2d0 prev-continuation=0x7fff8cf85538 {0,111720,84180,1500} [state=0000000000000004] [content=0x7fffd7f35f80] [vis-overflow=0,-54360,84180,55860] [scr-overflow=0,-54360,84180,55860] [sc=0x7fff8cf85988]<
>       Frame(mo)(1)@0x7fffb67d8dd0 {0,-54360,84180,55860} [state=0000000000000020] [content=0x7fffc6690380] [sc=0x7fffb67d4458]<
>         Frameset(frameset)(0)@0x7fff8c13f020 {0,0,84180,55380} [content=0x7fffbd8a3ec0] [sc=0x7fffb67d81e0]<
>           0x7fff8c13f1e0 BLANK 
>           Frame(frameset)(0)@0x7fff8c13f1e0 {0,0,0,55380} [content=0x7fffbd8a3ec0] [sc=0x7fff8cf85bc8]
>         >
>       >
>     >
>   >
>   line 0x7fff8cf85718: count=1 state=inline,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x100] {0,113220,540,1140} <
>     Text(1)@0x7fff8c13f2d0 [run=(nil)][0,1,T]  {0,113220,540,1140} [state=0000000090600000] [content=0x7fffc6690780] sc=0x7fff8cf852a8 pst=:-moz-non-element<
>       "x"
>     >
>   >
> >

When we crash at

#1  0x00007ffff5945ae1 in nsMathMLFrame::GetAttribute (aContent=0x7fffc6690380, aMathMLmstyleFrame=0x7fff8cf855e8, aAttributeAtom=0x7fffd7ff2070, aValue=...) at /home/karl/moz/dev/layout/mathml/nsMathMLFrame.cpp:267
#2  0x00007ffff594edab in nsMathMLmoFrame::ProcessOperatorData (this=0x7fffb67d8dd0) at /home/karl/moz/dev/layout/mathml/nsMathMLmoFrame.cpp:369
#3  0x00007ffff595100f in nsMathMLmoFrame::GetIntrinsicWidth (this=0x7fffb67d8dd0, aRenderingContext=0x7fff8d1edc70) at /home/karl/moz/dev/layout/mathml/nsMathMLmoFrame.cpp:1008

The flow has shortened, the last nsInlineFrame (previous ancestor) has gone
and the mo frame is now a descendant of a different nsInlineFrame.

> Block(body)(3)@0x7fffb67d82d0 [content=0x7fffc6674900] {480,480,82260,114360} [state=0000000000101401] [vis-overflow=0,0,84180,114360] [scr-overflow=0,0,84180,114360] sc=0x7fff8d50e9b8(i=2,b=0)<
>   line 0x7fff8c13f340: count=1 state=inline,clean,prevmarginclean,not impacted,wrapped,before:nobr,after:nobr[0x120] {0,0,84180,55860} <
>     Inline(math)(0)@0x7fffb67d85c0 next=0x7fff8cf85538 next-continuation=0x7fff8cf85538 {0,54360,84180,1500} [state=0000000000000020] [content=0x7fffd7f35f80] [vis-overflow=0,-54360,84180,55860] [scr-overflow=0,-54360,84180,55860] [sc=0x7fff8cf85988]<
>       Frameset(frameset)(0)@0x7fffb67d8a00 {0,-54360,84180,55380} [content=0x7fffbd8a3e20] [sc=0x7fff8cf85a88]<
>         0x7fffb67d8cb0 BLANK 
>         Frame(frameset)(0)@0x7fffb67d8cb0 {0,0,82260,55380} [content=0x7fffbd8a3e20] [sc=0x7fff8cf85b28]
>       >
>     >
>   >
>   line 0x7fff8cf855a8: count=2 state=inline,dirty,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x1] {0,55860,5760,1500} <
>     Inline(math)(0)@0x7fff8cf85538 next=0x7fff8c13f2d0 prev-continuation=0x7fffb67d85c0 {0,55860,84180,1500} [state=0000000000000405] [content=0x7fffd7f35f80] [sc=0x7fff8cf85988]<
>       Frame(mo)(1)@0x7fffb67d8dd0 {0,-54360,84180,55860} [state=0000000000000420] [content=0x7fffc6690380] [sc=0x7fffb67d4458]<
>         Frameset(frameset)(0)@0x7fff8c13f020 {0,0,84180,55380} [content=0x7fffbd8a3ec0] [sc=0x7fffb67d81e0]<
>           0x7fff8c13f1e0 BLANK 
>           Frame(frameset)(0)@0x7fff8c13f1e0 {0,0,0,55380} [content=0x7fffbd8a3ec0] [sc=0x7fff8cf85bc8]
>         >
>       >
>     >
>     Text(1)@0x7fff8c13f2d0 [run=0x7fff8cfdf700][0,1,T]  {0,113220,540,1140} [state=00000000c0000000] [content=0x7fffc6690780] sc=0x7fff8cf852a8 pst=:-moz-non-element<
>       "x"
>     >
>   >
> >

Comment 6

[Karl Tomlinson (:karlt)]

Perhaps we should back out bug 569124 on release branches.

Dynamic mathml is unusual, but AIUI this situation could also be hit on window resize.

Comment 7

[Karl Tomlinson (:karlt)]

Attachment: record first of math frame continuations as mstyle ancestor

(In reply to comment #4)
> Would it be good enough to initialize with the first-continuation of
> the ancestor or something?  That should solve this problem of continuations
> that go away.

Yes the first-continuation is fine.  For math elements, this is just a pointer so we can ->GetContent()->GetAttr().  (It is something more fancy for nsIMathMLFrames, which don't have continuations.)

I don't know about "special frames".
I assume GetFirstContinuation() is the ancestor-ish frame that won't go away.

Comment 8

[Karl Tomlinson (:karlt)]

(In reply to comment #6)
> AIUI this situation could also be hit on window resize.

I actually haven't managed to reproduce on resize or zoom.

Comment 9

[Daniel Veditz [:dveditz] out until Nov 10]

(In reply to comment #6)
> Perhaps we should back out bug 569124 on release branches.

So this is a regression in Firefox 5 and doesn't affect Firefox 4 or earlier? If so, yes, we should.

Comment 10

[Karl Tomlinson (:karlt)]

Backed out bug 569124 on aurora for 5:
http://hg.mozilla.org/releases/mozilla-aurora/rev/f18cccf550ba

Comment 11

[(no longer active)]

I have transplanted the backout changeset for this bug into the Aurora merge for Firefox 6: <http://hg.mozilla.org/releases/mozilla-aurora/rev/26d6981b3d6a>

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 531217 [details] [diff] [review]
record first of math frame continuations as mstyle ancestor

r=me.  We should add a test too, once this is ready to be opened up.

Comment 13

[Karl Tomlinson (:karlt)]

Thanks.  I'm intending to check in the test with the fix (now), as this only affects nightly (and it will be fixed there).

Comment 14

[Karl Tomlinson (:karlt)]

http://hg.mozilla.org/mozilla-central/rev/8d50e4db7828
http://hg.mozilla.org/mozilla-central/rev/3efdb67c20e7

Comment 15

[Simona Badau, Desktop QA]

Mozilla/5.0 (Windows NT 6.1; rv:8.0a1) Gecko/20110810 Firefox/8.0a1
Mozilla/5.0 (Windows NT 6.1; rv:6.0) Gecko/20100101 Firefox/6.0

Verified issue using the test case attached in the Description on Windows XP, Windows 7, Mac OS X 10.6 and Ubuntu - Firefox does not crash.

Setting resolution to VERIFIED FIXED.