Testcase 1: ###!!! ASSERTION: frame not in line: 'line->Contains(aDeletedFrame)', file /Users/admin/trunk/mozilla/layout/generic/nsBlockFrame.cpp, line 5067 Crash [@ nsIFrame::GetStateBits] dereferencing 0xddddde01. Testcase 2: ###!!! ASSERTION: frame not dirty: 'aFrame->GetStateBits() & (NS_FRAME_IS_DIRTY | NS_FRAME_HAS_DIRTY_CHILDREN)', file /Users/admin/trunk/mozilla/layout/base/nsPresShell.cpp, line 3319 (This assertion also appeared in bug 364685.) Null deref crash [@ nsLineBox::CachedIsEmpty]. The only difference between the two testcases is a setTimeout vs. a direct function call. The testcase is bigger than I'd like (2535 bytes), and I have no idea whether this is an SVG bug, a MathML bug, or a generic layout bug :(

FWIW, I hit "ASSERTION: Some frame destructors were not called" (the assertion from bug 334514) while trying to make these reduced testcases. I saved a copy of the messy intermediate testcase that triggered that assertion and will retest it once this bug is fixed.

The above information is all with debug builds. I tested in a nightly just now, and got a crash [@ nsFrameManager::ReResolveStyleContext] with 0 on top with testcase 1. Testcase 2 has the same effect in nightly as in debug.

So on that first testcase, when I hit the assert we have: (gdb) p aDeletedFrame $7 = (nsBlockFrame *) 0x88fd8dc Looking at the frame tree, the relevant part looks like: SVGForeignObject(foreignObject)(1)@0x88e6a88 {0,210,350,350} [state=00041000] [content=0x88ca890] [sc=0x88e6964]< Block(foreignObject)(1)@0x88e6c08 [view=0x8920858] {0,0,7000,7000} [state=02cc2000] sc=0x88e6b5c(i=1,b=0) pst=:-moz-svg-foreign-content< line 0x88e7cd0: count=1 state=inline,dirty,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x4301] {0,0,0,0} < Text(0)@0x88e4e00[0,4,T] {0,0,0,0} [state=51100000] sc=0x88e6c78 pst=:-moz-non-element< "\n \n" > > Overflow-list< Text(2)@0x88fd620[0,0,F] next=0x88fd8dc {0,0,0,0} [state=00000402] sc=0x88e6c78 pst=:-moz-non-element< "" > Block(div)(-1)@0x88fd8dc next=0x88e7c90 {0,0,0,0} [state=00040402] sc=0x88e6d54(i=0,b=0)< > Text(1)@0x88e7c90[0,0,F] {0,0,0,0} [state=00000402] sc=0x88e6c78 pst=:-moz-non-element< "" > > > > So we fail to actually remove aDeletedFrame from the tree, and then crash because we call a method on that deleted frame. The real problem, as I see it, is that nsSVGForeignObjectFrame::DoReflow sets a constrained available height on the reflow state for the block frame inside it. So in nsBlockReflowState::nsBlockReflowState we end up thinking we're paginated, and behave accordingly, but the caller ignores the reflow status (and thus the fact that there's overflow to be dealt with). Not sure what the right behavior here is, but I suspect the right thing is to use an unconstrained avail height in nsSVGForeignObjectFrame::DoReflow... except maybe when printing (and then fix printing).

SVG doesn't really support pagination (in our sense) so I think foreignobject should just not constrain the height.

That would probably fix bug 364688 too.

Doing that is not enough -- the block inside the foreignObject is a reflow root, so gets reflown directly from the presshell, with a constrained available height, since that's how reflow roots work. Perhaps the nsSVGForeignObjectFrame should be the reflow root or something?

(In reply to comment #8) > Perhaps the nsSVGForeignObjectFrame should be the reflow root or something? Agreed, although that would require that we give it a useful Reflow method.

Sure. Reflow() could just call into the existing DoReflow(), I bet... Possibly after asserting that the avail size being passed in matches the current size. Though do we need to worry about restyles on the foreignobject? Those should reflow something like the <svg>, no?

(In reply to comment #10) > Though do we need to worry about restyles on the foreignobject? Those should > reflow something like the <svg>, no? SVG doesn't actually need reflow, so it's fine, I think. What we have to worry about are restyles on things that contain the foreignObject that require reflowing it (bug 328829).

Critical security bugs must have owners. If you can't work on this bug please help us find another active owner for it.

This fixes the crashes (although there are still some assertions, probably unrelated). I think I want to try fixing the XXX comment I added in the pres shell, though. My assertions also caught some frames that weren't setting their overflow area.

This version continues using the child as the reflow root. Note that I'm setting overflow in svg.css basically to avoid the assertion about overflow area. I think the SVG code was already doing such clipping, although I haven't actually tested (and probably should).

Comment on attachment 252556 [details] [diff] [review] patch + reflowState.mComputedHeight = size.width; size.height, surely? r+sr with that

Checked in to trunk.

Verified fixed, using the latest trunk build, no crashes with the testcases, using that. I can see it crash with a 2007-01-24 trunk build.

I don't see these crashes on the 1.8 branch (Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.4pre) Gecko/20070322 BonEcho/2.0.0.4pre).

foreignObject is not implemented there

Crashtest checked in.