309120 - MathML crash [@ nsCSSFrameConstructor::ContentAppended] [@ nsCachedStyleData::GetStyleData] [@ nsCSSFrameConstructor::GetFloatContainingBlock] [@ PresShell::ContentRemoved]

Status: RESOLVED FIXED

(Core :: MathML, defect)

Description

[Jesse Ruderman]

Please leave as security-sensitive until bug 306663 is made public (because the
testcase is not simplified) and a Firefox release containing a fix for this bug
has gone out (because this crash looks exploitable).

Comment 3

[Jesse Ruderman]

Attachment: apple crash report with stack trace

This crash report is from today's Firefox trunk nightly.  I get a similar stack
with today's Firefox nightly from the 1.8 branch.

Note the 0x02542618 at the top of the stack and the EXC_BAD_INSTRUCTION
exception, indicating that Firefox is probably trying to execute data.

First few lines:
0   <<00000000>>	0x02542618 0 + 39069208
1   nsCSSFrameConstructor::ContentAppended(nsIContent*, int) + 1588
2   PresShell::ContentAppended(nsIDocument*, nsIContent*, int) + 60
3   nsDocument::ContentAppended(nsIContent*, int) + 116
4   nsGenericElement::InsertChildAt(nsIContent*, unsigned, int) + 504
5   nsGenericElement::InsertBefore(nsIDOMNode*, nsIDOMNode*, nsIDOMNode**) +
1640
6   _XPTC_InvokeByIndex + 216

Comment 4

[Jesse Ruderman]

I saw crashes at nsCSSFrameConstructor::GetFloatContainingBlock and 
PresShell::ContentRemoved while attempting to reduce the testcase.

Comment 6

[Jesse Ruderman]

rbs, can you tell what's going on without having a reduced testcase?  If not,
please try to help me figure out how to create a useful reduced testcase -- for
example, should I try to create a minimal testcase for a particular assertion
failure instead of the crash, or is there a debug option I can use to make it
crash earlier or more reliably?

Comment 7

[Asa Dotzler [:asa]]

If you come up with a very safe fix in the next couple of days, please request
approval for the patch and we'll evaluate.

Comment 8

[Jesse Ruderman]

I now see crashes at nsCachedStyleData::GetStyleData for #1 and #3 in the zip
file instead of the original crashes.  #2 still crashes at
nsCSSFrameConstructor::GetFloatContainingBlock.  I am using a debug build from a
few hours ago.

Comment 9

[Jesse Ruderman]

The previous comment is slightly incorrect.  The crashes using an opt build
(today's nightly) are the same as they used to be.  The crashes using a debug
build are different.

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Jesse, what about if you serialize the DOM before the final crashing command, and then apply the final command to the resulting document. Does that produce the same crash?

Comment 12

[Mike Schroepfer]

Roger - will you have a chance to look at this further?

Comment 13

[Daniel Veditz [:dveditz]]

The testcase claiming the nsCSSFrameConstructor::ContentAppended crash now crashes using a deleted object at nsCachedStyleData::GetStyleData for me (winxp). I've seen that crash in at least one of the other fuzz crashes.

Second testcase still crashes in nsCSSFrameConstructor::GetFloatContainingBlock. The initial aFrame looks OK, but the fourth GetParent() (on one run, anyway) returns a deleted object. If it matters, all of the blocks are 0x0.

The third testcase is no longer PresShell::ContentRemoved but another nsCachedStyleData::GetStyleData identical to the first. Maybe this is a new one masking these older problems?

Comment 15

[Jesse Ruderman]

The crashes in this bug went away when I applied the patch to my Mac trunk debug build, as did the crashes in bug 317549 :)

The instructions in bug 317544 and bug 317545 lead to crashes in nsBlockReflowState::AddFloat with the status bar counter around 1000 to 1300, sooner than they used to crash (before this patch) :(

Bug 317546 was not affected.

Comment 16

[rbs]

Comment on attachment 206561 [details] [diff] [review]
proposed patch

dbaron, I would like to land this one so that it can bake while I continue to invetsigate the other bugs.

Comment 17

[rbs]

> The instructions in bug 317544 and bug 317545 lead to crashes

I just put attachment 206671 [details] [diff] [review] in bug 317544. It fixes both of those bugs (with the patch of this bug).

Comment 18

[rbs]

Bug 317546 also seems to go away with the CSS fixup.

(The perf is different. I can sense how my system is working much harder.)

Comment 19

[rbs]

Spoke too soon, bug 317546 is alive and kicking under a different stack trace.

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 206561 [details] [diff] [review]
proposed patch

Eventually I suspect you might want this for more than just nsInlineFrames.  Can nsTextFrames ever end up inside MathML?  Or do you not want to support that?  Maybe nsImageFrame too?  But that need not happen in this bug.

>Index: nsMathMLContainerFrame.cpp
>     for (frame = mParent; frame; frame = frame->GetParent()) {
>+      frame->AddStateBits(NS_FRAME_IS_DIRTY | NS_FRAME_HAS_DIRTY_CHILDREN);

This will never walk up to the root frame, right?

I don't completely understand what this bits mean today (or at least I have trouble remembering it when I manage to understand it), so if both bits are really necessary on all these frames, so be it -- but I should remember to clean this up on the reflow branch.

>+nsMathMLContainerFrame::ReflowChild

>+  aDesiredSize.width = aDesiredSize.height = 0;
>+  aDesiredSize.ascent = aDesiredSize.descent = 0;
>+  aDesiredSize.mBoundingMetrics.Clear();

We have assertions to make sure that all child frame reflow initializes this stuff.  Why are these changes needed?

>+  aDesiredSize.mFlags |= NS_REFLOW_CALC_BOUNDING_METRICS;

Wow.  Should this be set by something else, or is this really needed?

>+nsMathMLContainerFrame::ReflowForeignChild

>+  nsHTMLReflowState rs(aPresContext, aReflowState, aChildFrame, availSize);
>+  nsLineLayout ll(aPresContext, aReflowState.mSpaceManager, aReflowState.parentReflowState,
>+                  aDesiredSize.mComputeMEW);

Do you need to call ll.Init?

>+  ll.BeginLineReflow(0, 0, availSize.width, availSize.height, PR_FALSE, PR_FALSE);
>+  rs.mLineLayout = ≪
>+  PRBool pushedFrame;
>+  ll.ReflowFrame(aChildFrame, aStatus, &aDesiredSize, pushedFrame);
>+  ll.EndLineReflow();

What if pushedFrame is true, which means the entire frame was pushed to a new line (which can't happen for the first frame on a line unless there's a float next to it)?  Do you instead want to pass unconstrained available width and height and force the parent to wrap the MathML if it's too big?  Then again, maybe pushedFrame can't happen -- if you're passing in a clean space manager (are you?  should you be?) and the initial position on the line is 0 (which it is), then it can't, and you should assert that it doesn't.

But perhaps more importantly, what happens if the inline frame is split?  That may be a better argument for passing in unconstrained sizes.

Have you tested large inline content that could potentially wrap?  (And try it in the presence of floats as well, with a piece of inline content whose first word doesn't fit next to the float.)

>+  // make up the bounding metrics from the reflow metrics.
>+  aDesiredSize.mBoundingMetrics.ascent = aDesiredSize.ascent;
>+  aDesiredSize.mBoundingMetrics.descent = aDesiredSize.descent;
>+  aDesiredSize.mBoundingMetrics.width = aDesiredSize.width;
>+  aDesiredSize.mBoundingMetrics.rightBearing = aDesiredSize.width;

I'm not sure if it makes more sense to use this or some combination with the combined area.

>Index: nsMathMLmactionFrame.cpp

>     nsReflowReason reason = aReflowState.reason;
>-    if (mWasRestyled) {
>+    if (childFrame->GetStateBits() & NS_FRAME_FIRST_REFLOW)
>+      reason = eReflowReason_Initial;
>+    else if (mWasRestyled) {
>       mWasRestyled = PR_FALSE;

Why should this be an if-else rather than two ifs?

Comment 21

[rbs]

> Eventually I suspect you might want this for more than just nsInlineFrames. 
> Can nsTextFrames ever end up inside MathML?  Or do you not want to support
> that?  Maybe nsImageFrame too?  But that need not happen in this bug.

That's why I am calling it ReflowForeignChild in case I might need it for more than nsInlineFrames, but so far the need hasn't arise.

In fact, elements such as <html:img> and even <html:input> already work. They allow for some really cool (non-standards) demos. C.f.:
http://www.mozilla.org/projects/mathml/demo/basics.xhtml

Also, text elements are already rendered by nsTextFrames (as children of nsMathMLTokenFrames). That's why hit-testing half-way in a string, selection, CJK/i18n and the other goodies of nsTextFrames are also shared by MathML without much bloat to re-invent things. The special things are mainly the stretchy characters (surrounding parentheses, curly brackets, square-roots, etc) that are handled by nsMathMLChars and made to stretch high or wide depending of their context, as described here:
http://www.mozilla.org/projects/mathml/fonts/encoding/

> >Index: nsMathMLContainerFrame.cpp
> >     for (frame = mParent; frame; frame = frame->GetParent()) {
> >+      frame->AddStateBits(NS_FRAME_IS_DIRTY | NS_FRAME_HAS_DIRTY_CHILDREN);
>
> This will never walk up to the root frame, right?

It at least stops at the <math> or at the first non-MathML frame:
   if (mEmbellishData.coreFrame) {
     nsEmbellishData embellishData;
     for (frame = mParent; frame; frame = frame->GetParent()) {
+      frame->AddStateBits(NS_FRAME_IS_DIRTY | NS_FRAME_HAS_DIRTY_CHILDREN);
       GetEmbellishDataFrom(frame, embellishData);
       if (embellishData.coreFrame != mEmbellishData.coreFrame)
         break;

Indeed, <math> or a non-MathML frame will have a nil |embellishData.coreFrame|,  which will then be different from |mEmbellishData.coreFrame|, otherwise we wouldn't be there in the first place.

> I don't completely understand what this bits mean today (or at least I have
> trouble remembering it when I manage to understand it), so if both bits are
> really necessary on all these frames, so be it -- but I should remember to
> clean this up on the reflow branch.

I don't do incremental reflows in MathML and the bits are not critical for MathML and can be adjusted as you see fit for the reflow branch when you get there. (All that is of interest is to steer the reflow reason for those non-MathML frames that munge it.)

===========
As far as I can tell, the only useful place I could see for the dirty-children  bit was to coalesce reflow events, in conjunction with ReflowDirtyChild. Consider:

block
  inline-child1
  inline-child2

Then, something happens in child1, it marks itself dirty and calls its block parent's ReflowDirtyChild(). The block sets its dirty-children bit and fires a reflow event.

Next, something happens in child2, which marks itself dirty and calls its block parent's ReflowDirtyChild(). The block sees that it already has a dirty-children bit, and so does not fire a reflow event.

Next, the block gets its reflow, and looks for dirty lines to clean up in the usual way.

Apparently, this reduced reflow events from hundreds down to a handful. With some CVS archeology, I saw: 

"nisheeth: Reflow commands are now coalesced by block and inline frames. This  fixes bug 985 in which we now generate 6 reflow commands instead of 257."

It predates the reflow root/path. So it needs to re-evaluated in that respect.

(Arguably in the case of MathML, for example, the denominator of a fraction may  not have to be reflowed if only the numerator has changed. But usually, it is a little "a" over "b", so it seems a bit academic and premature when still trying to make MathML a commodity.)

> >+nsMathMLContainerFrame::ReflowChild
>
> >+  aDesiredSize.width = aDesiredSize.height = 0;
> >+  aDesiredSize.ascent = aDesiredSize.descent = 0;
> >+  aDesiredSize.mBoundingMetrics.Clear();
>
> We have assertions to make sure that all child frame reflow initializes this
> stuff.  Why are these changes needed?

Paranoia, I suppose... It makes me feel secure to know that the frame will collapse to nothingness in case of an early return in the callee due to an unforseen issue. In particular, nsLineLayout::ReflowFrame does not set the metrics when the reflow fails.

> >+  aDesiredSize.mFlags |= NS_REFLOW_CALC_BOUNDING_METRICS;
>
> Wow.  Should this be set by something else, or is this really needed?

Yep, it was forced under my throat because the bit is killed by nsLineLayout::ReflowFrame. I had to restore it because I am calling ReflowChild() in a |while (childFrame)| loop with possibly hybrid children, using a common temporary "metrics" variable declared outside the loop (a cleaner fix would be to declare the metrics variable inside the loop. I shy away from doing that because it would lead to a bigger patch):

nsLineLayout::ReflowFrame()
  [...]
  nsHTMLReflowMetrics metrics(mComputeMaxElementWidth);
  [...]
  if (aMetrics) {
    *aMetrics = metrics;
  }
  [...]
}

> >+  nsLineLayout ll(aPresContext, aReflowState.mSpaceManager, aReflowState.parentReflowState,
> >+                  aDesiredSize.mComputeMEW);
>
> Do you need to call ll.Init?

No, it is not needed because all I would pass there would be nil, which is what the constructor assigned by default.

> >+  ll.BeginLineReflow(0, 0, availSize.width, availSize.height, PR_FALSE, PR_FALSE);
> >+  rs.mLineLayout = &ll;
> >+  PRBool pushedFrame;
> >+  ll.ReflowFrame(aChildFrame, aStatus, &aDesiredSize, pushedFrame);
> >+  ll.EndLineReflow();
>
> What if pushedFrame is true, which means the entire frame was pushed to a new
> line (which can't happen for the first frame on a line unless there's a float
> next to it)?  Do you instead want to pass unconstrained available width and
> height and force the parent to wrap the MathML if it's too big?  Then again,
> maybe pushedFrame can't happen -- if you're passing in a clean space manager
> (are you?  should you be?) and the initial position on the line is 0 (which it
> is), then it can't, and you should assert that it doesn't.
>
> But perhaps more importantly, what happens if the inline frame is split?  That
> may be a better argument for passing in unconstrained sizes.

It can't (for some of the reasons you alluded above). It also shouldn't... Otherwise that will open a Pandora box with a whole raft of unresolved issues - like what happens if a numerator of a fraction has a square-root with its content that gets split... I am going to simply assert as suggested. The horizontal scrollbar is a trusted friend in such cases...

> Have you tested large inline content that could potentially wrap?  (And try it
> in the presence of floats as well, with a piece of inline content whose first
> word doesn't fit next to the float.)

I just tested. It behaves like what happens with normal (non-MathML) content with <nobr>...<nobr>.

> >+  // make up the bounding metrics from the reflow metrics.
> >+  aDesiredSize.mBoundingMetrics.ascent = aDesiredSize.ascent;
> >+  aDesiredSize.mBoundingMetrics.descent = aDesiredSize.descent;
> >+  aDesiredSize.mBoundingMetrics.width = aDesiredSize.width;
> >+  aDesiredSize.mBoundingMetrics.rightBearing = aDesiredSize.width;
>
> I'm not sure if it makes more sense to use this or some combination with the
> combined area.

They are being set to fallback values there because it is a non-MathML child. They are otherwise different.

> >Index: nsMathMLmactionFrame.cpp
>
> >     nsReflowReason reason = aReflowState.reason;
> >-    if (mWasRestyled) {
> >+    if (childFrame->GetStateBits() & NS_FRAME_FIRST_REFLOW)
> >+      reason = eReflowReason_Initial;
> >+    else if (mWasRestyled) {
> >       mWasRestyled = PR_FALSE;
>
> Why should this be an if-else rather than two ifs?

The two ifs are unlikely. I am also okay to let the initial-reflow be the winner if it hasn't happen yet.

Comment 22

[rbs]

Attachment: iteration

Comment 23

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 206846 [details] [diff] [review]
iteration

OK.  I still don't see why you want to pass available width/height through if you don't want the child to wrap (and it seems like doing so could be dangerous in that it could cause it to wrap), but r+sr=dbaron.

Comment 24

[rbs]

Checked in, with the change to pass unconstrained available width and
height in the linelayout as suggested.

Comment 25

[Daniel Veditz [:dveditz]]

dbaron: you reviewed this, is it appropriate and safe enough for 1.8.0.1? 1.8.0.2 instead?

Comment 26

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I'm not confident that this didn't exacerbate other problems, such as bug 317544.  I do find it a bit scary, but I'm having trouble keeping track of everything here, so I could be confused.

Comment 27

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

The patch certainly did introduce crashers in the case when floats are used inside mathml. However that crashed before too and now it crashes using a nullpointer dereference rather then access to deleted memory.

Comment 28

[Tim Riley [:timr]]

dveditz-  do you still need qawanted?  Did you want to verify the fix on the trunk or something else?

Comment 29

[rbs]

Attachment: patch in sync with the 1.8 branch

Note: I will be off next week due to a trip and so will be unable to check in if I don't do it today. I have sync'ed the patch with the 1.8 branch so that 1.8 drivers could check it in directly if I don't do it before the trip.

Comment 30

[rbs]

Checked in the 1.8 branch.

Comment 31

[Dave Liebreich [:davel]]

asking jonas to attach test cases of floats used inside mathml (see comment 27).  I think we can remove the qawanted when such testcases are attached.

Comment 32

[Dave Liebreich [:davel]]

per private email, Jonas will add test cases when he returns from w3c meeting this week

Comment 33

[Daniel Veditz [:dveditz]]

Comment on attachment 212251 [details] [diff] [review]
patch in sync with the 1.8 branch

a=dveditz for 1.8.0 branch

Comment 34

[Daniel Veditz [:dveditz]]

Fix checked into 1.8.0 branch

Comment 35

[Dave Liebreich [:davel]]

marking [tcn-dl], indicating we need more information before verifying this bug in a Firefox 1.5.0. candidate build.  Jonas?

Comment 36

[Dave Liebreich [:davel]]

as per jonas, testcase in bug 317544 covers the crash mentioned in comment 27). Marking [rft-dl] (ready for testing in Firefox 1.5.0.2 release candidates)

Comment 37

[Jay Patel [:jay]]

v.fixed on 1.8.0 branch with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.1) Gecko/20060301 Firefox/1.5.0.1, no crashes with the stack signatures in this bug, BUT a new crash with the 7% reduced testcase (bug 329044).

Comment 38

[Bob Clary [:bc] (inactive)]

ff2b2 debug crash with null |this|

>	gklayout.dll!nsIFrame::GetStateBits()  Line 817 + 0xa bytes	C++
 	gklayout.dll!IncrementalReflow::AddCommand(nsPresContext * aPresContext=0x0456fda8, nsHTMLReflowCommand * aCommand=0x047a67b0)  Line 938 + 0x8 bytes	C++
 	gklayout.dll!PresShell::ProcessReflowCommands(int aInterruptible=1)  Line 6896 + 0x1c bytes	C++

but no deleted reference I can see. verified fixed 1.8

Comment 39

[Daniel Veditz [:dveditz]]

comment 38 looks like bug 366493