The default bug view has changed. See this FAQ.

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

VERIFIED FIXED in Firefox 5

Status

()

Core
MathML
--
critical
VERIFIED FIXED
6 years ago
5 years ago

People

(Reporter: Jesse Ruderman, Assigned: karlt)

Tracking

(Blocks: 3 bugs, {crash, regression, testcase})

Trunk
mozilla7
crash, regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox5+ fixed, firefox6+ fixed, status1.9.2 unaffected, status1.9.1 unaffected)

Details

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

Attachments

(3 attachments)

(Reporter)

Description

6 years ago
Created attachment 530806 [details]
testcase (crashes Firefox when loaded)

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

Comment 1

6 years ago
Created attachment 530807 [details]
stack trace (mac debug)
(Reporter)

Updated

6 years ago
OS: Windows 7 → All
Hardware: x86 → All
(Reporter)

Updated

6 years ago
tracking-firefox6: --- → ?
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.
(Assignee)

Updated

6 years ago
Blocks: 569124
(Assignee)

Comment 5

6 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

6 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

6 years ago
Created attachment 531217 [details] [diff] [review]
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.
Attachment #531217 - Flags: review?(bzbarsky)
(Assignee)

Updated

6 years ago
Assignee: nobody → karlt
Status: NEW → ASSIGNED
(Assignee)

Comment 8

6 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.
(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
tracking-firefox5: ? → +
tracking-firefox6: ? → +
Keywords: regression
(Assignee)

Comment 10

6 years ago
Backed out bug 569124 on aurora for 5:
http://hg.mozilla.org/releases/mozilla-aurora/rev/f18cccf550ba
status-firefox5: affected → fixed
status1.9.1: --- → unaffected
status1.9.2: --- → unaffected
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+
(Assignee)

Comment 13

6 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

6 years ago
http://hg.mozilla.org/mozilla-central/rev/8d50e4db7828
http://hg.mozilla.org/mozilla-central/rev/3efdb67c20e7
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
status-firefox6: --- → fixed
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.