Closed Bug 404301 Opened 13 years ago Closed 13 years ago

[FIX]"ASSERTION: How did that happen??" with multiple <col> elements

Categories

(Core :: Layout: Tables, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9beta2

People

(Reporter: jruderman, Assigned: bzbarsky)

References

(Blocks 2 open bugs)

Details

(Keywords: assertion, testcase)

Attachments

(2 files)

Attached file testcase
Loading the testcase triggers:

###!!! ASSERTION: How did that happen??: 'col->GetStyleContext() == colFrame->GetStyleContext() && col->GetContent() == colFrame->GetContent()', file /Users/jruderman/trunk/mozilla/layout/tables/nsTableColGroupFrame.cpp, line 326

This is the same assertion as in bug 403249, but this testcase still triggers it even though bug 403249 is fixed.
Attached patch FixSplinter Review
Another longstanding bug caught by that assert.  The issue is that we're putting the newly-inserted column in between the content and spanned column frames for the preceding <col span="2">.  That's bad.

I don't think there's a good way to get the right prev sibling in the frame constructor here, nor do I think it's really needed.  This should work fine.
Attachment #289284 - Flags: superreview?(roc)
Attachment #289284 - Flags: review?(bernd_mozilla)
Assignee: nobody → bzbarsky
OS: Mac OS X → All
Hardware: PC → All
Summary: "ASSERTION: How did that happen??" with multiple <col> elements → [FIX]"ASSERTION: How did that happen??" with multiple <col> elements
Target Milestone: --- → mozilla1.9 M10
Attachment #289284 - Flags: superreview?(roc) → superreview+
Comment on attachment 289284 [details] [diff] [review]
Fix

r+ on the fix,
r- on the comments added,
Fresh from the HTML 4.01 spec "User agents must ignore this attribute if the COLGROUP element contains one or more COL elements."

This means once we have col below a colgroup we have to remove the cols resulting from the span. If the number of columns for the table is then not sufficient we will create columns under the last anonymous colgroup that will be created if the number of columns is too small to fit the cells.
Attachment #289284 - Flags: review?(bernd_mozilla) → review+
> Fresh from the HTML 4.01 spec

Ah, ok.  I'll adjust the comments.  Do we have tests for this, or should I write some?
Comment on attachment 289284 [details] [diff] [review]
Fix

Fixes long-standing correctness bug that is triggering assertions because the frame tree is incorrect.
Attachment #289284 - Flags: approval1.9?
Attachment #289284 - Flags: approval1.9? → approval1.9+
Checked in, with the correctness tests, and with the comments adjusted.  Still need to land the assertion test.
Status: NEW → RESOLVED
Closed: 13 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
I checked in the original testcase as a crashtest to test for the assertion.
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.