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).

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.

I'm not sure I follow either the comment or the fix. Can you explain?

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.

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.

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).

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

Comment on attachment 373057 [details] [diff] [review] revised patch r+sr=dbaron. Could you add the test as a crashtest as well?

>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