Closed Bug 366956 Opened 13 years ago Closed 13 years ago

Crash due to SVG foreignObject reflowing child with constrained height [@ nsIFrame::GetStateBits] [@ IsContinuationPlaceholder] [@ nsLineBox::CachedIsEmpty] [@ nsFrameManager::ReResolveStyleContext]

Categories

(Core :: SVG, defect, critical)

defect
Not set
critical

Tracking

()

VERIFIED FIXED

People

(Reporter: jruderman, Assigned: dbaron)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Keywords: crash, testcase, Whiteboard: [sg:critical][patch] post 1.8-branch)

Crash Data

Attachments

(3 files, 1 obsolete file)

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 :(
Flags: blocking1.9?
Whiteboard: [sg:critical]
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.
Blocks: framedest
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.
Summary: Crash [@ nsIFrame::GetStateBits] [@ IsContinuationPlaceholder] [@ nsLineBox::CachedIsEmpty] → Crash [@ nsIFrame::GetStateBits] [@ IsContinuationPlaceholder] [@ nsLineBox::CachedIsEmpty] [@ nsFrameManager::ReResolveStyleContext]
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).
OS: Mac OS X → All
Hardware: Macintosh → All
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.
Blocks: 364688
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.
Assignee: general → dbaron
Summary: Crash [@ nsIFrame::GetStateBits] [@ IsContinuationPlaceholder] [@ nsLineBox::CachedIsEmpty] [@ nsFrameManager::ReResolveStyleContext] → Crash due to SVG foreignObject reflowing child with constrained height [@ nsIFrame::GetStateBits] [@ IsContinuationPlaceholder] [@ nsLineBox::CachedIsEmpty] [@ nsFrameManager::ReResolveStyleContext]
Attached patch patch (obsolete) — Splinter Review
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.
Attached patch patchSplinter Review
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).
Attachment #252552 - Attachment is obsolete: true
Attachment #252556 - Flags: superreview?(roc)
Attachment #252556 - Flags: review?(roc)
Whiteboard: [sg:critical] → [sg:critical][patch]
Comment on attachment 252556 [details] [diff] [review]
patch

+  reflowState.mComputedHeight = size.width;

size.height, surely?

r+sr with that
Attachment #252556 - Flags: superreview?(roc)
Attachment #252556 - Flags: superreview+
Attachment #252556 - Flags: review?(roc)
Attachment #252556 - Flags: review+
Checked in to trunk.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → VERIFIED
Flags: blocking1.9?
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).
Whiteboard: [sg:critical][patch] → [sg:critical][patch] post 1.8-branch
foreignObject is not implemented there
Group: security
Flags: wanted1.8.1.x-
Flags: in-testsuite?
Depends on: 388255
Crashtest checked in.
Flags: in-testsuite? → in-testsuite+
Crash Signature: [@ nsIFrame::GetStateBits] [@ IsContinuationPlaceholder] [@ nsLineBox::CachedIsEmpty] [@ nsFrameManager::ReResolveStyleContext]
You need to log in before you can comment on or make changes to this bug.