307277 - crash [@ DoDeletingFrameSubtree]

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Jesse Ruderman]

Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/20050906
Firefox/1.6a1

Testcase crashes while status bar says "900".

Filing as security-sensitive because the unsimplified testcase uses code from
bug 306663.

Comment 1

[Jesse Ruderman]

Attachment: testcase (not reduced)

Comment 2

[Jesse Ruderman]

TB9097333E

Comment 3

[Mats Palmgren (inactive)]

The problem is that we have a float with a wrong parent (or it's
on the wrong frame's float list). Like this:

Block 1
   Block 2      <---
<Float-list>       |
  Frame::mParent ---

Then we do ContentRemoved() which tries to remove the frame from its parent:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/base/nsCSSFrameConstructor.cpp&rev=1.1126&root=/cvsroot&mark=9752-9753#9732

which ends up doing just Destroy() since it can't find the frame in 'mFloats':
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/generic/nsBlockFrame.cpp&rev=3.737&root=/cvsroot&mark=5378,5410-5412#5377

Later when we do DoDeletingFrameSubtree() on Block 1 we will find the
deleted float in its 'mFloats'...

So, a pretty simple wallpaper for this bug is look at the ancestors' 'mFloats'
in nsBlockFrame::RemoveFloat() and remove it from there (and assert).
This is in case we can't find the real bug in nsCSSFrameConstructor...

So far, I have tracked down the bad call that inserts the float to here:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/base/nsCSSFrameConstructor.cpp&rev=1.1126&root=/cvsroot&mark=1416,1444-1446#1415

Maybe we should assert (in nsBlockFrame) that we have a correct parent
pointer when an out-of-flow frame is added so we will see these errors earlier...

Comment 4

[Mats Palmgren (inactive)]

FWIW:

0xbfffc7e4::nsFrameConstructorState float CB 0x8a60334
[...]
Created placeholderFrame=0x8ccde08 for aFrame=0x8ccddb4
  0xbfffb6b4::nsFrameConstructorState float CB 0x8cce310
MoveChildrenTo 1 blockFrame=0x8cce310 GetFloatContainingBlock(blockFrame)=0x8cce310
AdjustFloatParentPtrs 0x8ccba5c -> 0x8cce310  (placeholder 0x8ccde08, float
0x8ccddb4)
  0xbfffb6b4::~nsFrameConstructorState float CB 0x8cce310
MoveChildrenTo 1 DONE
ProcessFrameInsertions: state=0xbfffc7e4,
containingBlock(0x8ccba5c)->SetInitialChildList firstNewFrame=0x8ccddb4
uh oh, GetFloatContainingBlock(firstNewFrame->GetParent())=0x8cce310 !=
containingBlock=0x8ccba5c
SetInitialChildList: float list for 0x8ccba5c:
###!!! ASSERTION: wrong parent: 'f->GetParent() == me', file nsBlockFrame.cpp,
line 2038
Break: at file nsBlockFrame.cpp, line 2038


So, we have a nested nsFrameConstructorState, in the inner we do
AdjustFloatParentPtrs() then we pop and in the outer we do
aFrameItems.containingBlock->SetInitialChildList() on the old CB.
Maybe we need to update 'containingBlock' in parent states when we do
AdjustFloatParentPtrs() or maybe just call GetFloatContainingBlock() again
in the outer state so we add to the right frame?

Is the following comment relevant here?
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/base/nsCSSFrameConstructor.cpp&rev=1.1126&root=/cvsroot&mark=12972-12978#12965

Comment 5

[Boris Zbarsky [:bzbarsky]]

What it sounds like is that we should move the still-pending floats whose
parents we adjust from the "outer" frame constructor state to the inner "one".

Mats, do you think you can do that, or should I go for it?

Comment 6

[Boris Zbarsky [:bzbarsky]]

> Is the following comment relevant here?

Yes, indeed.

So the calls to MoveChildrenTo from nsCSSFrameConstructor::ConstructInline
should have the invariant that the only floats they mess with are ones that are
on the float list of ConstructInline's aState.  The other calls to
MoveChildrenTo are from SplitToContainingBlock, and those do no reparenting (and
that method is about to die on trunk anyway).

So I think what I suggest in comment 5 should work -- every float placeholder we
hit in MoveChildrenTo when aState is non-null should correspond (in the same
order, even!) to the list of floats in ConstructInline's state.  We should
probably pass that list in in addition to the state itself so that
MoveChildrenTo can remove frames from it and move them to the "inner" state instead.

Comment 7

[Mats Palmgren (inactive)]

OK, I will make an attempt at that...

Comment 8

[Mats Palmgren (inactive)]

Yes, that seems to work fine, it hasn't crashed in 15 minutes now... (it crashed
instantly before). I'm seeing quite a bit of this assertion though:

###!!! ASSERTION: uh oh. the block we need to reframe has no parent!: 'Error',
file nsCSSFrameConstructor.cpp, line 13335

and once or twice, this one:

###!!! ASSERTION: no last inline frame: 'PR_FALSE', file
nsCSSFrameConstructor.cpp, line 8922

Comment 9

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 196391 [details] [diff] [review]
Patch rev. 1

>Index: layout/base/nsCSSFrameConstructor.cpp

>+      // Move the float from the outer state to the inner when reparenting, bug 307277.
>+      if (outOfFlowFrame->GetParent() != parent) {

Is this test ever false here?  I don't see how it could be, but maybe I'm
missing something?

If you replace that test with an assert that the two are in fact not equal,
does that assert trigger in this testcase?  If it doesn't, I think we should go
with the assert instead of the test.

Other than that, looks great.  Thanks for doing this!

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 196391 [details] [diff] [review]
Patch rev. 1

>Index: layout/base/nsCSSFrameConstructor.cpp

>+      // Move the float from the outer state to the inner when reparenting, bug 307277.
>+      if (outOfFlowFrame->GetParent() != parent) {

Is this test ever false here?  I don't see how it could be, but maybe I'm
missing something?

If you replace that test with an assert that the two are in fact not equal,
does that assert trigger in this testcase?  If it doesn't, I think we should go
with the assert instead of the test.

Other than that, looks great.  Thanks for doing this!

Comment 12

[Mats Palmgren (inactive)]

Attachment: Patch rev. 2

(only changes in AdjustFloatParentPtrs() compared to rev. 1)

The comment "Update the parent pointer for outOfFlowFrame *if* it's ..."
lead me to believe we could have some floats already on the inner CB.
The assertion agrees with you that this is not the case ;-)

Other than that, I added a PRECONDITION and removed a null-check on
'outOfFlowFrame' and a couple of formatting changes.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 196421 [details] [diff] [review]
Patch rev. 2

>Index: layout/base/nsCSSFrameConstructor.cpp
> AdjustFloatParentPtrs(nsIFrame*                aFrame,
>+      // Update the parent pointer for outOfFlowFrame since it's

s/it's/its/

>+        // recursion of the SplitToContainingBlock will propagate the bit.

Note that this comment has changed on trunk; the patch will need to be merged
accordingly... (and a branch version of the patch will be needed).

>@@ -13374,34 +13406,34 @@ nsCSSFrameConstructor::SplitToContaining

This method is gone on trunk (but the branch version of the patch will need
this change).

r+sr=bzbarsky.

Comment 14

[Boris Zbarsky [:bzbarsky]]

This looks like it fixes bug 266971 too, no?

Comment 15

[Mats Palmgren (inactive)]

Attachment: Patch rev.2, final trunk version

Checked in to trunk at 2005-09-20 16:00 PDT

Comment 16

[Mats Palmgren (inactive)]

Comment on attachment 196421 [details] [diff] [review]
Patch rev. 2

Re comment 13:
> (and a branch version of the patch will be needed)

Boris, this patch applies cleanly to the branch, did you think
other adjustments were needed? (besides the code comments)

Comment 17

[Boris Zbarsky [:bzbarsky]]

No, just the code comments.  All I meant is that the trunk version wouldn't
apply to branch, so the code comments would need to be addressed there separately.

Comment 18

[Mats Palmgren (inactive)]

Comment on attachment 196421 [details] [diff] [review]
Patch rev. 2

Checked in to MOZILLA_1_8_BRANCH at 2005-09-21 15:15 PDT

-> FIXED

Comment 19

[Bob Clary [:bc] (inactive)]

no crash with the testcase in firefox 1.5 rc2 winxp/linux. Can someone with a Mac verify this an mark verified1.8?

Comment 20

[Jesse Ruderman]

Verified fixed on the Gecko 1.8 branch: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8) Gecko/20051108 Firefox/1.5.