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

VERIFIED FIXED

Status

()

Core
Layout
--
critical
VERIFIED FIXED
12 years ago
5 years ago

People

(Reporter: Martijn Wargers (dead), Assigned: mats)

Tracking

({crash, regression, testcase})

Trunk
x86
All
crash, regression, testcase
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(crash signature)

Attachments

(6 attachments, 3 obsolete attachments)

(Reporter)

Description

12 years ago
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.
(Reporter)

Comment 1

12 years ago
Created attachment 225428 [details]
Testcase
(Reporter)

Comment 2

12 years ago
Created attachment 225429 [details]
More or less original file from which the testcase is derived
(Assignee)

Updated

12 years ago
Assignee: nobody → mats.palmgren
OS: Windows XP → All
(Assignee)

Comment 3

12 years ago
Created attachment 225527 [details]
Testcase #3
(Assignee)

Comment 4

12 years ago
Created attachment 225529 [details]
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...
(Assignee)

Comment 5

12 years ago
Created attachment 225531 [details] [diff] [review]
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...
(Assignee)

Comment 6

12 years ago
Created attachment 225532 [details]
Frame dump #2

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

Comment 7

12 years ago
Created attachment 225785 [details] [diff] [review]
Patch rev. 1
(Assignee)

Comment 8

12 years ago
Created attachment 225787 [details] [diff] [review]
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.
Attachment #225531 - Attachment is obsolete: true
Attachment #225787 - Flags: review?(bzbarsky)
(Assignee)

Updated

12 years ago
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+
(Assignee)

Comment 10

12 years ago
Created attachment 227367 [details] [diff] [review]
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)
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+
(Assignee)

Comment 12

12 years ago
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
Last Resolved: 12 years ago
Resolution: --- → FIXED
(Reporter)

Comment 13

12 years ago
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
Crash Signature: [@ DoDeletingFrameSubtree]
(Assignee)

Comment 14

5 years ago
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.