Closed Bug 488388 Opened 11 years ago Closed 11 years ago

[FIX]"ASSERTION: Bad aPrevFrame" and more with colgroup

Categories

(Core :: Layout: Tables, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: jruderman, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, regression, testcase)

Attachments

(2 files)

Attached file testcase
###!!! ASSERTION: Bad aPrevFrame: 'col != aPrevFrame', file /Users/jruderman/central/layout/tables/nsTableColGroupFrame.cpp, line 270

###!!! ASSERTION: prev sibling has different parent: 'aFrameList->GetParent() == aPrevSibling->GetParent()', file /Users/jruderman/central/layout/generic/nsFrameList.cpp, line 229

I think this can lead to many additional assertions and crashes.  Regression from within the last week, I think.
Attached patch Proposed fixSplinter Review
This is a regression from bug 487449 (checked in earlier today).  I had checked for asserts in frame constructor, but missed this one...

In any case, the key is that the new ContentAppended code will get the last frame child of the parent and call InsertFrames() with that as aPrevFrame.  This is really equivalent to calling AppendFrames() in a sane world, but wasn't for colgroups.  This patch makes it be equivalent again.

With this patch the testcase still asserts about widths (twice), but it used to do that before too.

Bernd, any idea how I can write a testcase for this?  Removing the anon col creates an anon colgroup, as far as I can tell (using the <table> as the content node), and the new cols were ending up inside that colgroup, not the original one, because the frame pointer aPrevFrame pointed to on entry to InsertFrames got reused by the new colgroup's anonymous child col.  At least in some cases... 

If I could just write a testcase here that doesn't have the width asserts, I suppose I could use that as the crashtest....
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #372773 - Flags: superreview?(roc)
Attachment #372773 - Flags: review?(bernd_mozilla)
Blocks: 487449
Summary: "ASSERTION: Bad aPrevFrame" wand more with colgroup → [FIX]"ASSERTION: Bad aPrevFrame" wand more with colgroup
Attachment #372773 - Flags: superreview?(roc) → superreview+
Just add it as an "asserts(2)" crashtest?
So:

  asserts(2) load 488488-1.html

?
the other asserts are covered by bug 413091
I wouldn't bother with the asserts(2) annotation.  I have checking of them disabled until we have them turned on on a tinderbox, and when we do that we'll have to annotate all the manifests (and then drive the numbers down).
I will fix the other bug its easy I have fixed that before (6 years ago)
Attachment #372773 - Flags: review?(bernd_mozilla) → review+
Pushed http://hg.mozilla.org/mozilla-central/rev/178916d78891 with crashtest
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Summary: [FIX]"ASSERTION: Bad aPrevFrame" wand more with colgroup → [FIX]"ASSERTION: Bad aPrevFrame" and more with colgroup
You need to log in before you can comment on or make changes to this bug.