<colgroup> triggers "ASSERTION: Table width is less than the sum of its columns' min widths" and more

RESOLVED FIXED

Status

()

Core
Layout: Tables
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: Jesse Ruderman, Assigned: Bernd)

Tracking

(Blocks: 1 bug, {assertion, regression, testcase})

Trunk
x86
Mac OS X
assertion, regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

119 bytes, application/xhtml+xml
Details
3.65 KB, patch
dbaron
: review+
dbaron
: superreview+
Details | Diff | Splinter Review
(Reporter)

Description

10 years ago
Created attachment 297927 [details]
testcase

Loading this very simple testcase triggers:

###!!! ASSERTION: Table width is less than the sum of its columns' min widths: '!(aWidthType == BTLS_FINAL_WIDTH && aWidth < guess_min)', file /Users/jruderman/trunk/mozilla/layout/tables/BasicTableLayoutStrategy.cpp, line 764

###!!! ASSERTION: didn't subtract all that we added: '(space == 0 || space == nscoord_MAX) && ((l2t == FLEX_PCT_LARGE) ? (-0.001f < basis.f && basis.f < 0.001f) : (basis.c == 0 || basis.c == nscoord_MAX))', file /Users/jruderman/trunk/mozilla/layout/tables/BasicTableLayoutStrategy.cpp, line 991

I'm guessing this is a regression from bug 368504.  The first assertion is new (added in the patch for that bug).
(Reporter)

Comment 1

10 years ago
Of all the reftests and crashtests, only layout/base/crashtests/337268-1.html triggers these assertions.  That testcase is not very similar to this one, so once this bug is fixed, the testcase here should probably go in as a crashtest.
(Assignee)

Updated

9 years ago
Assignee: nobody → bernd_mozilla
Status: NEW → ASSIGNED
(Assignee)

Comment 2

9 years ago
Created attachment 372942 [details] [diff] [review]
patch
(Assignee)

Updated

9 years ago
Attachment #372942 - Flags: superreview?(bzbarsky)
Attachment #372942 - Flags: review?(dholbert)
I'm not sure I follow either the comment or the fix.  Can you explain?
(Assignee)

Comment 4

9 years ago
In several places we add/subtract 2 cellspacings  for example http://mxr.mozilla.org/mozilla-central/source/layout/tables/nsTableFrame.cpp#120
without care if we have cells at all

The assert is caused in particular by http://mxr.mozilla.org/mozilla-central/source/layout/tables/BasicTableLayoutStrategy.cpp#639 where 2 cellspacings are removed. One could also fix that place but I think the supplied patch fits better into the general picture, I am not religous about this.
Attachment #372942 - Flags: review?(dbaron)
How can ColumnHasCellSpacingBefore(0) ever return false?
Ah, I see -- a table with no cells, but a colgroup.

I think we should change nsTableFrame::ColumnHasCellSpacingBefore to return true unconditionally if the index is 0.  Would that fix this bug?
FWIW, I'd agree with comment 6 -- that sounds like a good fix to me.
(Assignee)

Comment 8

9 years ago
I am not sure that this will not have unintended side effects 
see for instance

http://mxr.mozilla.org/mozilla-central/source/layout/tables/BasicTableLayoutStrategy.cpp#738
So the semantics and name of the function ColumnHasCellSpacingBefore are based on the assumption that there's always cell spacing before the first column and after the last column (if there are columns at all), and therefore we don't need a function to check whether there's cell spacing after the last column.

That said, we might want to change the case you point out (plus the matching http://hg.mozilla.org/mozilla-central/annotate/9498df7e5ecd/layout/tables/BasicTableLayoutStrategy.cpp#l938 ) to check whether cells originate in the column rather than using ColumnHasCellSpacingBefore (which isn't really what we mean in that case).
(Assignee)

Comment 10

9 years ago
Created attachment 373057 [details] [diff] [review]
revised patch

this changes ColumnHasCellSpacingBefore so that the 0th column always has a cellspacing before
and it fixes the two callers that need to know if there  are cells originating rather than if there is a cellspacing
Attachment #372942 - Attachment is obsolete: true
Attachment #373057 - Flags: superreview?(dbaron)
Attachment #373057 - Flags: review?(dbaron)
Attachment #372942 - Flags: superreview?(bzbarsky)
Attachment #372942 - Flags: review?(dholbert)
Attachment #372942 - Flags: review?(dbaron)
Attachment #373057 - Flags: superreview?(dbaron)
Attachment #373057 - Flags: superreview+
Attachment #373057 - Flags: review?(dbaron)
Attachment #373057 - Flags: review+
Comment on attachment 373057 [details] [diff] [review]
revised patch

r+sr=dbaron.

Could you add the test as a crashtest as well?
(Assignee)

Comment 12

9 years ago
>Could you add the test as a crashtest as well?

done, I only omitted them in the patch as I thought this is self explaining.

http://hg.mozilla.org/mozilla-central/rev/dccda3f7724b
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.