Closed Bug 341382 Opened 14 years ago Closed 14 years ago

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

Categories

(Core :: Layout, defect, critical)

x86
All
defect
Not set
critical

Tracking

()

VERIFIED FIXED

People

(Reporter: martijn.martijn, Assigned: mats)

References

Details

(Keywords: crash, regression, testcase)

Crash Data

Attachments

(6 files, 3 obsolete files)

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.
Attached file Testcase
Assignee: nobody → mats.palmgren
OS: Windows XP → All
Attached file Testcase #3
Attached file 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...
Attached patch wip1 (obsolete) — Splinter Review
|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...
Attached file Frame dump #2
This is the resulting frame tree for "Testcase #3" with the "wip1" patch.
Attached patch Patch rev. 1 (obsolete) — Splinter Review
Attached patch Patch rev. 1 (diff -w) (obsolete) — Splinter Review
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.
Attachment #225531 - Attachment is obsolete: true
Attachment #225787 - Flags: review?(bzbarsky)
Summary: Crash [@ DoDeletingFrameSubtree] with position:fixed and display: table-caption → [FIX] Crash [@ DoDeletingFrameSubtree] with position:fixed and display: table-caption
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).
Attachment #225787 - Flags: superreview+
Attachment #225787 - Flags: review?(bzbarsky)
Attachment #225787 - Flags: review+
(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)
Attachment #225785 - Attachment is obsolete: true
Attachment #225787 - Attachment is obsolete: true
Attachment #227367 - Flags: superreview?(bzbarsky)
Attachment #227367 - Flags: review?(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!
Attachment #227367 - Flags: superreview?(bzbarsky)
Attachment #227367 - Flags: superreview+
Attachment #227367 - Flags: review?(bzbarsky)
Attachment #227367 - Flags: review+
Filed bug 343048 and added comments to nsTableFrame.cpp referring to it.

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

-> FIXED
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060629 Minefield/3.0a1
Status: RESOLVED → VERIFIED
Flags: in-testsuite?
Crash Signature: [@ DoDeletingFrameSubtree]
Add crashtests:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a1b9d63c8482
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.