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

RESOLVED FIXED

Status

()

Core
MathML
--
critical
RESOLVED FIXED
13 years ago
11 years ago

People

(Reporter: Jesse Ruderman, Assigned: rbs)

Tracking

(Blocks: 1 bug, {crash, verified1.8.0.2, verified1.8.1})

Trunk
PowerPC
Mac OS X
crash, verified1.8.0.2, verified1.8.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8b5 -
blocking1.8.0.1 -
blocking1.8.0.2 +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?] uses freed memory [rft-dl], crash signature)

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

13 years ago
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).
(Reporter)

Updated

13 years ago
Flags: blocking1.8b5?
Whiteboard: [sg:fix]
(Reporter)

Comment 3

13 years ago
Created attachment 196624 [details]
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

Updated

13 years ago
Flags: blocking1.8b5? → blocking1.8b5+
(Reporter)

Comment 4

13 years ago
I saw crashes at nsCSSFrameConstructor::GetFloatContainingBlock and 
PresShell::ContentRemoved while attempting to reduce the testcase.
(Reporter)

Comment 6

13 years ago
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

13 years ago
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.
Flags: blocking1.8b5+ → blocking1.8b5-
(Reporter)

Comment 8

13 years ago
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.
Summary: StirDOM/MathML crash [@ nsCSSFrameConstructor::ContentAppended] → StirDOM/MathML crash [@ nsCSSFrameConstructor::ContentAppended] [@ nsCachedStyleData::GetStyleData] [@ nsCSSFrameConstructor::GetFloatContainingBlock]
(Reporter)

Comment 9

13 years ago
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.
Summary: StirDOM/MathML crash [@ nsCSSFrameConstructor::ContentAppended] [@ nsCachedStyleData::GetStyleData] [@ nsCSSFrameConstructor::GetFloatContainingBlock] → StirDOM/MathML crash [@ nsCSSFrameConstructor::ContentAppended] [@ nsCachedStyleData::GetStyleData] [@ nsCSSFrameConstructor::GetFloatContainingBlock] [@ PresShell::ContentRemoved]
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?

Updated

13 years ago
Flags: blocking1.8.0.1?
(Reporter)

Updated

13 years ago
Blocks: 317549

Comment 12

13 years ago
Roger - will you have a chance to look at this further?
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?

Whiteboard: [sg:fix] → [sg:critical?] uses freed memory
(Reporter)

Comment 15

13 years ago
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.
(Assignee)

Comment 16

13 years ago
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.
Attachment #206561 - Flags: superreview?(dbaron)
Attachment #206561 - Flags: review?(dbaron)
(Assignee)

Comment 17

13 years ago
> 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).
(Assignee)

Comment 18

13 years ago
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.)
(Assignee)

Comment 19

13 years ago
Spoke too soon, bug 317546 is alive and kicking under a different stack trace.
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 = &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.

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?
(Assignee)

Comment 21

13 years ago
> 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.
(Assignee)

Comment 22

13 years ago
Created attachment 206846 [details] [diff] [review]
iteration
Attachment #206561 - Attachment is obsolete: true
Attachment #206846 - Flags: superreview?(dbaron)
Attachment #206846 - Flags: review?(dbaron)
Attachment #206561 - Flags: superreview?(dbaron)
Attachment #206561 - Flags: review?(dbaron)
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.
Attachment #206846 - Flags: superreview?(dbaron)
Attachment #206846 - Flags: superreview+
Attachment #206846 - Flags: review?(dbaron)
Attachment #206846 - Flags: review+
(Assignee)

Comment 24

13 years ago
Checked in, with the change to pass unconstrained available width and
height in the linelayout as suggested.
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
dbaron: you reviewed this, is it appropriate and safe enough for 1.8.0.1? 1.8.0.2 instead?
Keywords: qawanted
Flags: blocking1.8.0.2?
Flags: blocking1.8.0.1?
Flags: blocking1.8.0.1-
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.
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.
Depends on: 317546
Flags: blocking1.8.0.2? → blocking1.8.0.2+

Comment 28

12 years ago
dveditz-  do you still need qawanted?  Did you want to verify the fix on the trunk or something else?
(Assignee)

Comment 29

12 years ago
Created attachment 212251 [details] [diff] [review]
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.
Attachment #212251 - Flags: approval-branch-1.8.1?
(Assignee)

Updated

12 years ago
Attachment #212251 - Flags: approval-branch-1.8.1? → approval-branch-1.8.1?(dbaron)
Attachment #212251 - Flags: approval-branch-1.8.1?(dbaron) → approval-branch-1.8.1+
(Assignee)

Comment 30

12 years ago
Checked in the 1.8 branch.
Keywords: fixed1.8.1
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.
per private email, Jonas will add test cases when he returns from w3c meeting this week
Comment on attachment 212251 [details] [diff] [review]
patch in sync with the 1.8 branch

a=dveditz for 1.8.0 branch
Attachment #212251 - Flags: approval1.8.0.2+
Fix checked into 1.8.0 branch
Keywords: fixed1.8.0.2
marking [tcn-dl], indicating we need more information before verifying this bug in a Firefox 1.5.0. candidate build.  Jonas?
Whiteboard: [sg:critical?] uses freed memory → [sg:critical?] uses freed memory [tcn-dl]
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)
Whiteboard: [sg:critical?] uses freed memory [tcn-dl] → [sg:critical?] uses freed memory [rft-dl]

Comment 37

12 years ago
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).
Keywords: fixed1.8.0.2 → verified1.8.0.2

Updated

12 years ago
Flags: in-testsuite+

Updated

12 years ago
Keywords: qawanted

Comment 38

12 years ago
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
Keywords: fixed1.8.1 → verified1.8.1
Summary: StirDOM/MathML crash [@ nsCSSFrameConstructor::ContentAppended] [@ nsCachedStyleData::GetStyleData] [@ nsCSSFrameConstructor::GetFloatContainingBlock] [@ PresShell::ContentRemoved] → MathML crash [@ nsCSSFrameConstructor::ContentAppended] [@ nsCachedStyleData::GetStyleData] [@ nsCSSFrameConstructor::GetFloatContainingBlock] [@ PresShell::ContentRemoved]
comment 38 looks like bug 366493
Group: security

Updated

11 years ago
Flags: in-testsuite+ → in-testsuite?
Crash Signature: [@ nsCSSFrameConstructor::ContentAppended] [@ nsCachedStyleData::GetStyleData] [@ nsCSSFrameConstructor::GetFloatContainingBlock] [@ PresShell::ContentRemoved]
You need to log in before you can comment on or make changes to this bug.