Assertion failures from a 2-cell table, with "padding: 50%" on each cell

RESOLVED FIXED

Status

()

Core
Layout: Tables
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

Trunk
x86
All
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

11 years ago
Created attachment 269443 [details]
testcase

Branching part of Bug 385132, using my simpler testcase that only triggers 2 of the 4 assertions in that bug.

Assertion failures are:

###!!! ASSERTION: assigned width smaller than min: 'col_width >=
colFrame->GetMinCoord()', file
/Users/jruderman/trunk/mozilla/layout/tables/BasicTableLayoutStrategy.cpp, line
960

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


Note that any padding value >= 50% triggers the assertions.
(Assignee)

Comment 1

11 years ago
Created attachment 269715 [details] [diff] [review]
patch (handle eStyleUnit_Auto/nscoord_MAX in GetWidthInfo)

This patch seems to fix the problem.  Not sure if it's the right way to go, but it's one solution.

The bug boils down to this:
  1. nsLayoutUtils.cpp:AddPercents() gets called, with aPercent == 2 * the padding value (because there's padding on both sides).  For padding > 50%, this triggers it to fall into the case return nscoord_MAX. (a really big number)
  2. This value gets bubbled up to prefCoord at the beginning of  BasicTableLayoutStrategy.cpp:GetWidthInfo(). 
  3. For auto-height, this "bad" prefCoord value is not overridden in this function, and it gets used in constructing the CellWidthInfo on the last line.  Going up one level, this gets stored as the mPrefCoord for some column frame.
 *** Fast forward a bit...
  4. In BasicTableLayoutStrategy::ComputeColumnWidths, at around line 774, we use the value of colFrame->GetPrefCoord().  In this case, that value is nscoord_MAX, and it seems to be used without being sanity checked.  Consequently, we end up getting computations that are way too big and overflow.

My patch intercepts the problem at step #3 above, before we store nscoord_MAX as mPrefCoord for a column frame.

Note: This patch also fixes both testcases on Bug 385132.
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Attachment #269715 - Flags: review?(dbaron)
(Assignee)

Comment 2

11 years ago
Here are references on mxr, for the code I mentioned above:
  1. AddPercents() is at:
http://mxr.mozilla.org/seamonkey/source/layout/base/nsLayoutUtils.cpp#1222
  2. GetWidthInfo() is at:
http://mxr.mozilla.org/seamonkey/source/layout/tables/BasicTableLayoutStrategy.cpp#105
The value of prefCoord is set at line 113.
  3. prefCoord gets used to construct a CellWidthInfo at
http://mxr.mozilla.org/seamonkey/source/layout/tables/BasicTableLayoutStrategy.cpp#223
  4. prefCoord gets used in ComputeColumnWidths at
http://mxr.mozilla.org/seamonkey/source/layout/tables/BasicTableLayoutStrategy.cpp#768
 (lines 768-769)
Comment on attachment 269715 [details] [diff] [review]
patch (handle eStyleUnit_Auto/nscoord_MAX in GetWidthInfo)

The idea of returning nscoord_MAX was that things should behave as though the pref width of that cell, and thus that column, were infinitely large, so we shouldn't try to prevent it from reaching the columns.
Attachment #269715 - Flags: review?(dbaron) → review-
(Assignee)

Updated

11 years ago
Attachment #269715 - Attachment is obsolete: true
(Assignee)

Updated

11 years ago
Attachment #269715 - Attachment is patch: true
(Assignee)

Comment 5

11 years ago
I posted an nscoord_MAX-checking patch on bug 367673, and it covers this bug as
well.
Depends on: 367673
(Assignee)

Comment 6

11 years ago
Fixed by checkin for bug 367673.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Depends on: 368573
Flags: in-testsuite?
(Assignee)

Comment 7

11 years ago
Created attachment 282456 [details] [diff] [review]
reftest for assertion
Attachment #282456 - Flags: review?(roc)
(Assignee)

Updated

11 years ago
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.