341382 - [FIX] Crash [@ DoDeletingFrameSubtree] with position:fixed and display: table-caption

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Martijn Wargers (dead)]

See upcoming testcase, which crashes current trunk Mozilla build just after load.

Talkback ID: TB19824647H
DoDeletingFrameSubtree   DoDeletingFrameSubtree   DoDeletingFrameSubtree   DoDeletingFrameSubtree   DeletingFrameSubtree   nsCSSFrameConstructor::ContentRemoved   nsCachedStyleData::Destroy  

This regressed between 2006-01-26 and 2006-01-27:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2006-01-26+09&maxdate=2006-01-27+08&cvsroot=%2Fcvsroot
Probably some sort of regression from bug 310638.
But I don't see the crash happening on 1.8.0.4 branch.

Comment 1

[Martijn Wargers (dead)]

Attachment: Testcase

Comment 2

[Martijn Wargers (dead)]

Attachment: More or less original file from which the testcase is derived

Comment 3

[Mats Palmgren (inactive)]

Attachment: Testcase #3

Comment 4

[Mats Palmgren (inactive)]

Attachment: Frame dump #1

DeletingFrameSubtree is given a bogus frame tree so some frames remains in
the tree that should have been removed, in this case this subtree have a
placeholder to a fixed frame which we do succeed in removing.

In the first DeletingFrameSubtree call for the SPAN (red) we unlink the
placeholder from the out-of-flow, but since we fail to remove the whole
SPAN subtree the next call to DeletingFrameSubtree will get:
###!!! ASSERTION: Null out-of-flow for placeholder?: 'outOfFlow'
and we crash on a null-pointer access on the out-of-flow.

The SPAN has |mParent| set to the TableOuter(body) but it's linked in on
Table(body)'s childlist. This is why we fail to remove it, I think.
But the error is of course how this tree was created in the first place...

Comment 5

[Mats Palmgren (inactive)]

Attachment: wip1

|prevSibling| was calculated based on the insertion point into |parentFrame|
so it's not valid if we adjust the parent.
Bugs like this would be *so* much easier to find if the table code actually
had some assertions. I'll add some that would have detected this case
tomorrow...

Comment 6

[Mats Palmgren (inactive)]

Attachment: Frame dump #2

This is the resulting frame tree for "Testcase #3" with the "wip1" patch.

Comment 7

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

Comment 8

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1 (diff -w)

1. reset the insertion point if it was calculated based on a frame different
   than the outer table frame
2. add more assertions to detect frame construction errors 
3. optimize the list name tests in nsBlockFrame.cpp so that we check
   the common case (nsnull) first.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 225787 [details] [diff] [review]
Patch rev. 1 (diff -w)

>Index: layout/generic/nsFrameList.cpp
>+      NS_ASSERTION(!aParent || aParent == aPrevSibling->GetParent(),
>+                   "prev sibling have different parent");
>       nsIFrame* nextFrame = aPrevSibling->GetNextSibling();
>+      NS_ASSERTION(!nextFrame || !aParent || aParent == nextFrame->GetParent(),
>+                   "next sibling have different parent");

s/have/has/ in both strings.  Why the !aParent check for the next sibling?

>Index: layout/tables/nsTableFrame.cpp
> nsTableFrame::RemoveFrame(nsIAtom*        aListName,
>   if (NS_STYLE_DISPLAY_TABLE_COLUMN_GROUP == display->mDisplay) {
>+    NS_ASSERTION(!aListName || aListName == nsLayoutAtoms::colGroupList,
>+                 "unexpected child list");

Can we just assert aListName == nsLayoutAtoms::colGroupList here?  We're certainly assuming that mColGroups is the thing to mess with...

r+sr=bzbarsky with the nits resolved (either changed, or explained).

Comment 10

[Mats Palmgren (inactive)]

Attachment: Patch rev. 2 (diff -w)

(In reply to comment #9)
> Why the !aParent check for the next sibling?

Many call nsFrameList::InsertFrames() with a null aParent to avoid the
parent assignment at the beginning, I guess because that is already done
in most cases.
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/generic/nsFrameList.cpp&rev=3.40&root=/cvsroot&mark=194-200#187

I changed it to test aFrameList->GetParent() instead which should
cover both cases. I also added the same assertions to
nsFrameList::InsertFrame() and moved the parent assignment earlier
(this method is immideately above InsertFrames() in the link above)
There are no other changes compared to rev. 1.

> 
> >Index: layout/tables/nsTableFrame.cpp
> > nsTableFrame::RemoveFrame(nsIAtom*        aListName,
> >   if (NS_STYLE_DISPLAY_TABLE_COLUMN_GROUP == display->mDisplay) {
> >+    NS_ASSERTION(!aListName || aListName == nsLayoutAtoms::colGroupList,
> >+                 "unexpected child list");
> 
> Can we just assert aListName == nsLayoutAtoms::colGroupList here?  We're
> certainly assuming that mColGroups is the thing to mess with...

I tried that at first but got assertions since the frame constructor removes
col-group frames using nsnull in some cases... when I read the code I got the
impression this is by design.
So I decided to be lenient in this case and accept nsnull or colGroupList but
not anything else. (sorry, I should have explained this before)

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 227367 [details] [diff] [review]
Patch rev. 2 (diff -w)

> when I read the code I got the impression this is by design.

I suspect it's more likely negligence... Worth filing a followup bug on this (and citing the bug in a comment in the table code).

r+sr=bzbarsky.  Thanks for doing this!

Comment 12

[Mats Palmgren (inactive)]

Filed bug 343048 and added comments to nsTableFrame.cpp referring to it.

Checked in at 2006-06-28 19:32 PDT

-> FIXED

Comment 13

[Martijn Wargers (dead)]

Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060629 Minefield/3.0a1

Comment 14

[Mats Palmgren (inactive)]

Add crashtests:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a1b9d63c8482

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a1b9d63c8482