Closed Bug 655451 Opened 14 years ago Closed 13 years ago

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

Categories

(Core :: MathML, defect)

defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla7
Tracking Status
firefox5 + fixed
firefox6 + fixed
status1.9.2 --- unaffected
status1.9.1 --- unaffected

People

(Reporter: jruderman, Assigned: karlt)

References

(Blocks 1 open bug)

Details

(Keywords: crash, regression, testcase, Whiteboard: [sg:dos (critical w/out frame-poisoning)])

Crash Data

Attachments

(3 files)

Found by fuzzing layout/mathml/crashtests/463763-1.xhtml.
Attached file stack trace (mac debug) (deleted) —
OS: Windows 7 → All
Hardware: x86 → All
Whiteboard: [sg:dos (critical w/out frame-poisoning)]
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....
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;
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.
Blocks: 569124
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" > > > > > >
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.
(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.
Attachment #531217 - Flags: review?(bzbarsky)
Assignee: nobody → karlt
Status: NEW → ASSIGNED
(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.
(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.
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 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.
Attachment #531217 - Flags: review?(bzbarsky) → review+
Thanks. I'm intending to check in the test with the fix (now), as this only affects nightly (and it will be fixed there).
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
Crash Signature: [@ nsMathMLFrame::GetAttribute]
Group: core-security
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: