Closed
Bug 655451
Opened 14 years ago
Closed 13 years ago
Poison frame crash [@ nsMathMLFrame::GetAttribute] with <frameset>
Categories
(Core :: MathML, defect)
Core
MathML
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.
Reporter | ||
Comment 1•14 years ago
|
||
Reporter | ||
Updated•14 years ago
|
OS: Windows 7 → All
Hardware: x86 → All
Reporter | ||
Updated•14 years ago
|
tracking-firefox6:
--- → ?
Whiteboard: [sg:dos (critical w/out frame-poisoning)]
Comment 2•14 years ago
|
||
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•14 years ago
|
||
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•14 years ago
|
||
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.
Assignee | ||
Comment 5•14 years ago
|
||
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"
> >
> >
> >
Assignee | ||
Comment 6•14 years ago
|
||
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.
tracking-firefox5:
--- → ?
Assignee | ||
Comment 7•14 years ago
|
||
(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 | ||
Updated•14 years ago
|
Assignee: nobody → karlt
Status: NEW → ASSIGNED
Assignee | ||
Comment 8•14 years ago
|
||
(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•14 years ago
|
||
(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.
status-firefox5:
--- → affected
Keywords: regression
Assignee | ||
Comment 10•14 years ago
|
||
Backed out bug 569124 on aurora for 5:
http://hg.mozilla.org/releases/mozilla-aurora/rev/f18cccf550ba
Updated•14 years ago
|
status1.9.1:
--- → unaffected
status1.9.2:
--- → unaffected
Comment 11•14 years ago
|
||
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•14 years ago
|
||
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+
Assignee | ||
Comment 13•14 years ago
|
||
Thanks. I'm intending to check in the test with the fix (now), as this only affects nightly (and it will be fixed there).
Assignee | ||
Comment 14•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/8d50e4db7828
http://hg.mozilla.org/mozilla-central/rev/3efdb67c20e7
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
status-firefox6:
--- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
Updated•13 years ago
|
Crash Signature: [@ nsMathMLFrame::GetAttribute]
Updated•13 years ago
|
Group: core-security
Comment 15•13 years ago
|
||
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.
Description
•