317544 - Crash [@ VerifyContextParent() line 816] involving MathML

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Bob Clary [:bc] (inactive)]

Automated StirDOM testing on WinXP with today's FF trunk:

http://golem.ph.utexas.edu/~distler/blog/archives/000539.html 
parameters: 187,217,44,181

+	providerFrame	0x00000000
	providerIsChild	0x00000000
+	aPresContext	0x033d0ce8
+	aFrame	0x0365610c
+	aContext	0xdddddddd
+	aParentContext	0x00000000
+	actualParentContext	0x0364d8e4


VerifyContextParent(nsPresContext * 0x033d0ce8, nsIFrame * 0x0365610c, nsStyleContext * 0xdddddddd, nsStyleContext * 0x00000000) line 816 + 21 bytes
VerifyStyleTree(nsPresContext * 0x033d0ce8, nsIFrame * 0x0365610c, nsStyleContext * 0x00000000) line 856 + 19 bytes
VerifyStyleTree(nsPresContext * 0x033d0ce8, nsIFrame * 0x0364b954, nsStyleContext * 0x00000000) line 874 + 15 bytes
VerifyStyleTree(nsPresContext * 0x033d0ce8, nsIFrame * 0x036506f8, nsStyleContext * 0x00000000) line 881 + 15 bytes
VerifyStyleTree(nsPresContext * 0x033d0ce8, nsIFrame * 0x035e43a0, nsStyleContext * 0x00000000) line 881 + 15 bytes
VerifyStyleTree(nsPresContext * 0x033d0ce8, nsIFrame * 0x035f808c, nsStyleContext * 0x00000000) line 881 + 15 bytes
VerifyStyleTree(nsPresContext * 0x033d0ce8, nsIFrame * 0x03656ec4, nsStyleContext * 0x00000000) line 881 + 15 bytes
VerifyStyleTree(nsPresContext * 0x033d0ce8, nsIFrame * 0x035f8280, nsStyleContext * 0x00000000) line 881 + 15 bytes
VerifyStyleTree(nsPresContext * 0x033d0ce8, nsIFrame * 0x035ec1b8, nsStyleContext * 0x00000000) line 881 + 15 bytes
VerifyStyleTree(nsPresContext * 0x033d0ce8, nsIFrame * 0x035e5680, nsStyleContext * 0x034ec3e8) line 881 + 15 bytes
nsFrameManager::DebugVerifyStyleTree(nsIFrame * 0x035e5680) line 909 + 22 bytes
nsMathMLContainerFrame::PropagateScriptStyleFor(nsIFrame * 0x035e8c8c, int 0x00000000) line 673
nsMathMLContainerFrame::ReResolveScriptStyle(nsMathMLContainerFrame * const 0x035e8cc4, int 0x00000000) line 111 + 16 bytes
nsMathMLContainerFrame::ReLayoutChildren(nsIFrame * 0x035e5680) line 910
nsMathMLmathBlockFrame::RemoveFrame(nsMathMLmathBlockFrame * const 0x035e5680, nsIAtom * 0x00ea0f70, nsIFrame * 0x0365610c) line 399 + 9 bytes
nsFrameManager::RemoveFrame(nsIFrame * 0x035e5680, nsIAtom * 0x00ea0f70, nsIFrame * 0x0365610c) line 705
DeletingFrameSubtree(nsPresContext * 0x033d0ce8, nsIPresShell * 0x034d10c0, nsFrameManager * 0x034d10dc, nsIFrame * 0x00000000) line 9734
nsCSSFrameConstructor::ContentRemoved(nsIContent * 0x035aeeb8, nsIContent * 0x035af138, int 0x00000000, int 0x00000000) line 9883 + 27 bytes
PresShell::ContentRemoved(nsIDocument * 0x034446e0, nsIContent * 0x035aeeb8, nsIContent * 0x035af138, int 0x00000000) line 5185
nsDocument::ContentRemoved(nsIContent * 0x035aeeb8, nsIContent * 0x035af138, int 0x00000000) line 2329
nsHTMLDocument::ContentRemoved(nsIContent * 0x035aeeb8, nsIContent * 0x035af138, int 0x00000000) line 1175
doRemoveChildAt(unsigned int 0x00000000, int 0x00000001, nsIContent * 0x035af138, nsIContent * 0x035aeeb8, nsIDocument * 0x034446e0, nsAttrAndChildArray & {...}) line 2958
nsGenericElement::RemoveChildAt(unsigned int 0x00000000, int 0x00000001) line 2833 + 42 bytes
nsGenericElement::RemoveChild(nsGenericElement * const 0x035aeeb8, nsIDOMNode * 0x035af154, nsIDOMNode * * 0x0012ea4c) line 3713 + 17 bytes
nsXMLElement::RemoveChild(nsXMLElement * const 0x035aeeb8, nsIDOMNode * 0x035af154, nsIDOMNode * * 0x0012ea4c) line 63 + 20 bytes
nsGenericElement::doInsertBefore(nsIDOMNode * 0x035af154, nsIDOMNode * 0x00000000, nsIContent * 0x035de858, nsIDocument * 0x034446e0, nsAttrAndChildArray & {...}, nsIDOMNode * * 0x0012ec90) line 3458 + 63 bytes
nsGenericElement::InsertBefore(nsGenericElement * const 0x035de858, nsIDOMNode * 0x035af154, nsIDOMNode * 0x00000000, nsIDOMNode * * 0x0012ec90) line 3012 + 37 bytes
nsHTMLDivElement::InsertBefore(nsHTMLDivElement * const 0x035de858, nsIDOMNode * 0x035af154, nsIDOMNode * 0x00000000, nsIDOMNode * * 0x0012ec90) line 56 + 24 bytes
nsGenericElement::AppendChild(nsGenericElement * const 0x035de858, nsIDOMNode * 0x035af154, nsIDOMNode * * 0x0012ec90) line 510
nsHTMLDivElement::AppendChild(nsHTMLDivElement * const 0x035de858, nsIDOMNode * 0x035af154, nsIDOMNode * * 0x0012ec90) line 56 + 20 bytes
XPTC_InvokeByIndex(nsISupports * 0x035de874, unsigned int 0x00000012, unsigned int 0x00000002, nsXPTCVariant * 0x0012ec80) line 102
XPCWrappedNative::CallMethod(XPCCallContext & {...}, XPCWrappedNative::CallMode CALL_METHOD) line 2139 + 43 bytes
XPC_WN_CallMethod(JSContext * 0x032a8de8, JSObject * 0x02d43ff8, unsigned int 0x00000001, long * 0x038db108, long * 0x0012ef54) line 1444 + 14 bytes
js_Invoke(JSContext * 0x032a8de8, unsigned int 0x00000001, unsigned int 0x00000000) line 1211 + 23 bytes
js_Interpret(JSContext * 0x032a8de8, unsigned char * 0x034dbfea, long * 0x0012fa14) line 3753 + 15 bytes
js_Invoke(JSContext * 0x032a8de8, unsigned int 0x00000001, unsigned int 0x00000002) line 1231 + 19 bytes
js_InternalInvoke(JSContext * 0x032a8de8, JSObject * 0x02bd9e38, long 0x01a3c338, unsigned int 0x00000000, unsigned int 0x00000001, long * 0x03833e10, long * 0x0012fb94) line 1308 + 20 bytes
JS_CallFunctionValue(JSContext * 0x032a8de8, JSObject * 0x02bd9e38, long 0x01a3c338, unsigned int 0x00000001, long * 0x03833e10, long * 0x0012fb94) line 4157 + 31 bytes
nsJSContext::CallEventHandler(JSObject * 0x02bd9e38, JSObject * 0x01a3c338, unsigned int 0x00000001, long * 0x03833e10, long * 0x0012fb94) line 1422 + 33 bytes
nsGlobalWindow::RunTimeout(nsTimeout * 0x03826f30) line 6219
nsGlobalWindow::TimerCallback(nsITimer * 0x03826fa8, void * 0x03826f30) line 6577
nsTimerImpl::Fire() line 400 + 17 bytes
nsTimerManager::FireNextIdleTimer(nsTimerManager * const 0x01a3f308) line 636
nsAppShell::Run(nsAppShell * const 0x00f72a08) line 142
nsAppStartup::Run(nsAppStartup * const 0x00f72968) line 161 + 26 bytes
XRE_main(int 0x00000004, char * * 0x003f6d28, const nsXREAppData * 0x0042101c kAppData) line 2289 + 35 bytes
main(int 0x00000004, char * * 0x003f6d28) line 61 + 18 bytes
mainCRTStartup() line 338 + 17 bytes

Comment 1

[Mike Schroepfer]

Roger -can you take a look?

Comment 2

[Jesse Ruderman]

Confirmed with a trunk debug build on Mac.  I get a similar stack.

Comment 3

[rbs]

Attachment: patch

With the patch in bug 309120 and this CSS patch, this bug and bug 317545 are also fixed. 

I once suggested this CSS in bug 307826, but it was not well received because of its poor performance. I don't know any other easy way to say that MathML doesn't allow floating/positioning.

Comment 4

[Boris Zbarsky [:bzbarsky]]

rbs, why is that needed here?  Why do we actually crash?

Again, if we have a block child of mathml, there is absolutely no reason to prevent floating on its descendants.

Comment 5

[rbs]

> rbs, why is that needed here?  Why do we actually crash?

Some float related things. A block reflow state isn't been propagated. I could probably get it to work, but it seems clumsy here.

> Again, if we have a block child of mathml, there is absolutely no reason to
> prevent floating on its descendants.

That isn't MathML. You are redefining what MathML is. We already talked about that.

Comment 6

[Boris Zbarsky [:bzbarsky]]

All the same, the patch as written would cause noticeable performance issues any time that stylesheet is loaded.  If we want to do something like that, would it suffice to hack it on the frame constructor level (eg push null absolute and float containing blocks any time we process the kids of a MathML frame)?

Comment 7

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

Attachment: Alternative patch

So this patch takes an alternative approach. What i'm trying to do is to prevent floating if there is no block reflowstate which is the case when mathml reflows foreign frames. Ideally I would want to do the same for positioned frames, but i'm still trying to figure that out. However this patch alone seems to work as well as attachment 206671 [details] [diff] [review].

One thing i'm worried about is that this might cause us to leak the floating frame since it is never placed. Or will the placeholder take care of that during teardown?

Comment 8

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

Err.. cancel that, my patch still crashes on this bug, though not bug 317545. Looking into it.

Comment 9

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

Attachment: Better approach

This should make us not float descendants of mathml frames. It does this by making sure that we never have a float-containing-block while constructing frames for descendants of mathml.

The patch includes a very minimal implementation of IsFrameOfType. It's by no means complete, but rather something we can add to incrementally as we need.

Would be good if we could compleatly remove the rule in mathml.css since I think it won't work when xbl is involved. But we can do that in a second patch.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 207573 [details] [diff] [review]
Better approach

Yeah, I like this.

Comment 11

[Daniel Veditz [:dveditz]]

Need this landed on the trunk if it's going to make the branches. Minusing for .0.1 because getting it in doesn't seem like it's going to happen, but will consider approving a tested patch if it happens soon.

Comment 12

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

Comment on attachment 207573 [details] [diff] [review]
Better approach

Could you add a comment above GetAbsoluteContainingBlock and GetFloatContainingBlock in nsCSSFrameConstructor.h that those two functions are designed to restore state that would have been created on the stack in cases where we start frame construction from a non-root element, and therefore they must match the PushFloatContainingBlock / PushAbsoluteContainingBlock calls that we do elsewhere.  (And perhaps a similar comment around the definitions of PushFloatContainingBlock and PushAbsoluteContainingBlock.)

I'm not entirely happy with the idea that things with style data that say they're a float just don't float.  The only case that already pushes a null float containing block this is svg:foreignObject, which is currently #ifdef-ed out.  What have you done to ensure that we don't get confused in this case.  I'm particularly worried about callers of nsStyleDisplay::IsFloating that assume that a true result means that the frame is out-of-flow.

Comment 13

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

It should also be documented somewhere what pushing a null float containing block means.  I don't really even know, and I'm rather surprised that it doesn't crash.

Comment 14

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

So David and I talked about that this might not be the best solution given the number of places where we call IsFloating(). The alternative approach I'm going to try is to strategically place block frames in strategic places to maintain all invariants we're expecting.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Note that places that call IsFloating() might already have issues in XUL documents and such...

Comment 16

[Boris Zbarsky [:bzbarsky]]

But yeah, for branch it may be better to not push null.  :(

Comment 17

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

So I looked at the approach in attachment 207573 [details] [diff] [review] some more (since the wrap-in-block approach is quite hard, we can't simply always wrap all children in a single block, sometimes we need separate blocks for each child, such as for mfrac).

I thought about going with attachment 207573 [details] [diff] [review], but making it so that if nsFrameConstructorState::AddChild gets a frame that should be floated but there is no floating-container we kill the frame rather then making it not float.

However, there are already quite a few places where we push a null floating or absolute container. So I'm afraid that we would be killing a few more frames then just for mathml.

But since we are already pushing null containers so often I'm wondering if it isn't safe to do it for mathml too. So I looked at all callers of IsFloating() (there are actually very few) and it seems like all but two should be safe. And the last two might be safe too, but I don't know enough to tell. The two callers are:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/tables/nsTableOuterFrame.cpp&rev=3.288&mark=1901#1890
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/tables/nsTableRowFrame.cpp&rev=3.372&mark=1030#1020

Would be great if someone could look to see if these could deal with IsFloating() frames that don't actually float.

Comment 18

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

Where else do we push null containers other than the ifdef-ed-out case for svg:foreignObject?

Comment 19

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

We do it in the following cases:

1) XUL effectivly does it by saying that no xul frames can be floated. So you can
   have xul frames that are IsFloating() without being floating.

2) When the root frame is a table frame.

3) In ConstructRootFrame when calling BeginBuildingScrollFrame

4) In CreateContinuingTableFrame uses a null floating containiner

Comment 20

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

Attachment: Updated with dbarons comments

Me and dbaron talked about how to do this and concluded that this patch might be the best we can do for now. Placing extra frames around content as described in comment 14 is how mathml used to deal with foregin frames and it turned out to be RealHard to get to work.

Comment 21

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

Comment on attachment 208888 [details] [diff] [review]
Updated with dbarons comments

>Index: base/nsCSSFrameConstructor.cpp
>+  // logic in GetFloatContainingBlock.
>   void PushAbsoluteContainingBlock(nsIFrame* aNewAbsoluteContainingBlock,
>                                    nsFrameConstructorSaveState& aSaveState);

Comment should refer to GetAbsoluteContainingBlock.

>   // Function to push the existing float containing block state and
>-  // create a new scope
>+  // create a new scope. Code that uses this function should get matching
>+  // logic in GetFloatContainingBlock.
>+  // Pushing a null float containing block forbids any frames from being
>+  // floated until a new float containing block is pushed.

Could you add an XXX comment that we ought to get rid of the use of null float containing blocks?

>+    nsFrameConstructorSaveState saveState;
>+    aState.PushFloatContainingBlock(nsnull, saveState, PR_FALSE, PR_FALSE);

Could you comment here why you're doing this:  to forbid floating within MathML.

>Index: generic/nsIFrame.h
>+  virtual PRBool IsFrameOfType(PRUint32 aFlags) const
>+  {
>+    return PR_FALSE;
>+  }

Should be !aFlags, as I said in bug 310436 comment 16.

sr=dbaron

Comment 22

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

Checked in on trunk, once this has baked there for a while we should consider taking this for the branches too

Comment 23

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

Attachment: Final version

This is what was checked in. Note that it depends on the patch in bug 310436

Comment 24

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

marking fixed since it should be fixed on trunk

Comment 25

[Jesse Ruderman]

*** Bug 317545 has been marked as a duplicate of this bug. ***

Comment 26

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

Comment on attachment 209640 [details] [diff] [review]
Final version

Asking for approval for branches. This has been on trunk for a while and hasn't caused any regressions.

Comment 27

[Daniel Veditz [:dveditz]]

Comment on attachment 209640 [details] [diff] [review]
Final version

approved for 1.8.0 branch, a=dveditz

Comment 28

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

Comment on attachment 209640 [details] [diff] [review]
Final version

Is there still time to take this on the old branches? If so I think we should.

I'll also have to include some parts of the previous attachment in this bug (with dbarons commends addressed of course), since I can't rely on bug 310436 to provide them.

Comment 29

[Dave Liebreich [:davel]]

Marking [rft-dl] (ready for testing in Firefox 1.5.0.2 release candidates).  Test case info is in comment 0 (fuzz testing tool url and parameters)

Comment 30

[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 that might be related (bug 329044).

Comment 31

[Tim Riley [:timr]]

Comment on attachment 209640 [details] [diff] [review]
Final version

Not considered blocking (at least not nominated)  Too late for 1.0.8/1.7.13 as these were closed for non-blockers a while ago. "?"ing for 1.0.9.

Comment 32

[Boris Zbarsky [:bzbarsky]]

sicking, did this ever land on the 1.8 branch?

Comment 33

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

The patch does fix probably-exploitable crashes. But if it's too late for 1.0.8 lets try for 1.0.9

Comment 34

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

> sicking, did this ever land on the 1.8 branch?

No, i havn't gone through and landed my fuzz patches for 1.8.1 yet.

Comment 35

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

This needs to land on the 1.8 branch ASAP.

Comment 36

[Boris Zbarsky [:bzbarsky]]

This patch seems to depend on the patch for bug 307826...

Comment 37

[Mike Schroepfer]

Please land for 1.8.1

Comment 38

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

checked in on the 1.8 branch

Comment 39

[Bob Clary [:bc] (inactive)]

testing with an early stirdom (see url)

1.8 20060823 linux nightly crash tb22483714. 
1.8 20060824 linux nightly crash tb22484775.
@IncrementalReflow::AddCommand() mozilla/layout/base/nsPresShell.cpp, line 817]

1.8 20060824 winxp nightly crash TB22484873Z
@IncrementalReflow::AddCommand  mozilla/layout/base/nsPresShell.cpp, line 938]
1.8 20060823 winxp debug crash.
@nsIFrame::GetStateBits()  Line 817 (null |this|)
lots of 
###!!! ASSERTION: dangling frame without a content node: 'content', file f:/work
/mozilla/builds/ff/2.0/mozilla/layout/mathml/base/src/nsMathMLFrame.cpp, line 19
followed by 
###!!! ASSERTION: null target frame: 'mTargetFrame != nsnull', file f:/work/mozi
lla/builds/ff/2.0/mozilla/layout/generic/nsHTMLReflowCommand.cpp, line 94
###!!! ASSERTION: reflow command with no target: 'frame != nsnull', file f:/work
/mozilla/builds/ff/2.0/mozilla/layout/base/nsPresShell.cpp, line 929

1.9 20060824 linux nightly no crash
1.9 20060824 linux debug no crash
1.9 20060824 winxp nightly no crash
1.9 20060824 winxp debug no crash

interesting asserts though.

verified fixed 1.9

Comment 40

[Boris Zbarsky [:bzbarsky]]

> ###!!! ASSERTION: null target frame: 'mTargetFrame != nsnull', file

Hmm....  what about on the reflow branch, where reflow commands are gone?

Comment 41

[Bob Clary [:bc] (inactive)]

(In reply to comment #40)
> > ###!!! ASSERTION: null target frame: 'mTargetFrame != nsnull', file
> 
> Hmm....  what about on the reflow branch, where reflow commands are gone?
> 

I don't see that assertion on windows but crash with a deleted |this| in nsStyleContext::AddRef()  

Comment 42

[Boris Zbarsky [:bzbarsky]]

Is there a bug on that crash?

Comment 43

[Boris Zbarsky [:bzbarsky]]

So what exactly landed on branch?  I'm getting merge conflicts trying to merge various things to branch, because apparently the branch version of this patch was somewhat different?

Comment 44

[Johnny Stenback (:jst)]

Clearing stale requests.