Closed Bug 317544 Opened 18 years ago Closed 18 years ago

Crash [@ VerifyContextParent() line 816] involving MathML

Categories

(Core :: Layout, defect)

defect
Not set
major

Tracking

()

VERIFIED FIXED

People

(Reporter: bc, Assigned: sicking)

References

()

Details

(Keywords: crash, fixed1.8.1, verified1.8.0.2, Whiteboard: [sg:critical])

Crash Data

Attachments

(1 file, 4 obsolete files)

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
Whiteboard: [sg:investigate]
Flags: blocking1.8.0.1?
Summary: Crash [@ VerifyContextParent() line 816] → Crash [@ VerifyContextParent() line 816] involving MathML
Assignee: nobody → rbs
Roger -can you take a look?
Whiteboard: [sg:investigate] → [sg:critical?] references freed memory
Confirmed with a trunk debug build on Mac.  I get a similar stack.
OS: Windows XP → All
Hardware: PC → All
Attached patch patch (obsolete) — Splinter Review
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.
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.
> 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.
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)?
Attached patch Alternative patch (obsolete) — Splinter Review
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?
Err.. cancel that, my patch still crashes on this bug, though not bug 317545. Looking into it.
Attached patch Better approach (obsolete) — Splinter Review
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.
Attachment #207441 - Attachment is obsolete: true
Attachment #207573 - Flags: superreview?
Attachment #207573 - Flags: review?
Attachment #207573 - Flags: superreview?(dbaron)
Attachment #207573 - Flags: superreview?
Attachment #207573 - Flags: review?(bzbarsky)
Attachment #207573 - Flags: review?
Comment on attachment 207573 [details] [diff] [review]
Better approach

Yeah, I like this.
Attachment #207573 - Flags: review?(bzbarsky) → review+
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.
Flags: blocking1.8.1?
Flags: blocking1.8.0.2?
Flags: blocking1.8.0.1?
Flags: blocking1.8.0.1-
Whiteboard: [sg:critical?] references freed memory → [sg:critical?] need sr=dbaron
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.
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.
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.
Note that places that call IsFloating() might already have issues in XUL documents and such...
But yeah, for branch it may be better to not push null.  :(
Attachment #207573 - Flags: superreview?(dbaron) → superreview-
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.
Where else do we push null containers other than the ifdef-ed-out case for svg:foreignObject?
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
Attached patch Updated with dbarons comments (obsolete) — Splinter Review
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.
Assignee: rbs → bugmail
Attachment #206671 - Attachment is obsolete: true
Attachment #207573 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #208888 - Flags: superreview?
Attachment #208888 - Flags: superreview? → superreview?(dbaron)
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
Attachment #208888 - Flags: superreview?(dbaron) → superreview+
Checked in on trunk, once this has baked there for a while we should consider taking this for the branches too
Attached patch Final versionSplinter Review
This is what was checked in. Note that it depends on the patch in bug 310436
Attachment #208888 - Attachment is obsolete: true
marking fixed since it should be fixed on trunk
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
*** Bug 317545 has been marked as a duplicate of this bug. ***
Whiteboard: [sg:critical?] need sr=dbaron → [sg:critical] uses freed memory, at least in bug 317545 (dup of this)
Flags: blocking1.8.0.2? → blocking1.8.0.2+
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.
Attachment #209640 - Flags: approval1.8.0.2?
Attachment #209640 - Flags: approval-branch-1.8.1?(dbaron)
Attachment #209640 - Flags: approval-branch-1.8.1?(dbaron) → approval-branch-1.8.1+
Comment on attachment 209640 [details] [diff] [review]
Final version

approved for 1.8.0 branch, a=dveditz
Attachment #209640 - Flags: approval1.8.0.2? → approval1.8.0.2+
Depends on: 310436
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.
Attachment #209640 - Flags: approval1.7.13?
Attachment #209640 - Flags: approval-aviary1.0.8?
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)
Whiteboard: [sg:critical] uses freed memory, at least in bug 317545 (dup of this) → [sg:critical] uses freed memory, at least in bug 317545 (dup of this) [rft-dl]
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 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.
Attachment #209640 - Flags: approval1.7.13?
Attachment #209640 - Flags: approval1.7.13-
Attachment #209640 - Flags: approval-aviary1.0.9?
Attachment #209640 - Flags: approval-aviary1.0.8?
Attachment #209640 - Flags: approval-aviary1.0.8-
sicking, did this ever land on the 1.8 branch?
The patch does fix probably-exploitable crashes. But if it's too late for 1.0.8 lets try for 1.0.9
Flags: blocking1.7.14?
Flags: blocking-aviary1.0.9?
> 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.
This needs to land on the 1.8 branch ASAP.
This patch seems to depend on the patch for bug 307826...
Please land for 1.8.1
Flags: blocking1.8.1? → blocking1.8.1+
Whiteboard: [sg:critical] uses freed memory, at least in bug 317545 (dup of this) [rft-dl] → [checkin needed] [sg:critical] uses freed memory, at least in bug 317545 (dup of this) [rft-dl]
checked in on the 1.8 branch
Keywords: fixed1.8.1
Whiteboard: [checkin needed] [sg:critical] uses freed memory, at least in bug 317545 (dup of this) [rft-dl] → [sg:critical] uses freed memory, at least in bug 317545 (dup of this) [rft-dl]
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
Status: RESOLVED → VERIFIED
> ###!!! ASSERTION: null target frame: 'mTargetFrame != nsnull', file

Hmm....  what about on the reflow branch, where reflow commands are gone?
(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()  
Is there a bug on that crash?
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?
Whiteboard: [sg:critical] uses freed memory, at least in bug 317545 (dup of this) [rft-dl] → [sg:critical] contains 306663 testcase
Clearing stale requests.
Flags: blocking1.7.14?
Flags: blocking-aviary1.0.9?
Crash Signature: [@ VerifyContextParent() line 816]
Whiteboard: [sg:critical] contains 306663 testcase → [sg:critical] Embargo until fuzzers public (contains 306663 testcase)
Whiteboard: [sg:critical] Embargo until fuzzers public (contains 306663 testcase) → [sg:critical] Embargo until fuzzers public (URL field history contains 306663 testcase)
Group: core-security → core-security-release
Whiteboard: [sg:critical] Embargo until fuzzers public (URL field history contains 306663 testcase) → [sg:critical]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.