Last Comment Bug 655451 - Poison frame crash [@ nsMathMLFrame::GetAttribute] with <frameset>
: Poison frame crash [@ nsMathMLFrame::GetAttribute] with <frameset>
Status: VERIFIED FIXED
[sg:dos (critical w/out frame-poisoni...
: crash, regression, testcase
Product: Core
Classification: Components
Component: MathML (show other bugs)
: Trunk
: All All
: -- critical (vote)
: mozilla7
Assigned To: Karl Tomlinson (ni?:karlt)
:
Mentors:
Depends on:
Blocks: stirdom randomstyles PoisonFrameCrash 569124
  Show dependency treegraph
 
Reported: 2011-05-06 23:02 PDT by Jesse Ruderman
Modified: 2012-02-09 12:44 PST (History)
8 users (show)
karlt: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed
+
fixed
unaffected
unaffected


Attachments
testcase (crashes Firefox when loaded) (480 bytes, application/xhtml+xml)
2011-05-06 23:02 PDT, Jesse Ruderman
no flags Details
stack trace (mac debug) (12.60 KB, text/plain)
2011-05-06 23:03 PDT, Jesse Ruderman
no flags Details
record first of math frame continuations as mstyle ancestor (939 bytes, patch)
2011-05-09 17:37 PDT, Karl Tomlinson (ni?:karlt)
bzbarsky: review+
Details | Diff | Review

Description Jesse Ruderman 2011-05-06 23:02:12 PDT
Created attachment 530806 [details]
testcase (crashes Firefox when loaded)

Found by fuzzing layout/mathml/crashtests/463763-1.xhtml.
Comment 1 Jesse Ruderman 2011-05-06 23:03:50 PDT
Created attachment 530807 [details]
stack trace (mac debug)
Comment 2 Boris Zbarsky [:bz] 2011-05-09 12:03:03 PDT
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) 2011-05-09 13:00:23 PDT
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 [:bz] 2011-05-09 13:07:46 PDT
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 (ni?:karlt) 2011-05-09 16:28:18 PDT
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 (ni?:karlt) 2011-05-09 16:35:37 PDT
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 (ni?:karlt) 2011-05-09 17:37:34 PDT
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.
Comment 8 Karl Tomlinson (ni?:karlt) 2011-05-11 18:53:20 PDT
(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] 2011-05-12 13:31:08 PDT
(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 (ni?:karlt) 2011-05-12 18:31:33 PDT
Backed out bug 569124 on aurora for 5:
http://hg.mozilla.org/releases/mozilla-aurora/rev/f18cccf550ba
Comment 11 :Ehsan Akhgari (busy, don't ask for review please) 2011-05-24 11:08:04 PDT
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 [:bz] 2011-05-27 23:30:31 PDT
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 (ni?:karlt) 2011-05-27 23:38:46 PDT
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 15 Simona B [:simonab] 2011-08-10 08:19:30 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.