413180 - "ASSERTION: should not be zero if we haven't allocated all pref percent" with colspan, percentage width

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect)

Description

[Matthew Cline]

If you go to http://tvtropes.org/pmwiki/pmwiki.php/Main/WikiSandbox and click "Edit", you'll get the assertion:

###!!! ASSERTION: should not be zero if we haven't allocated all pref percent: '(!spanHasNonPctPref || nonPctTotalPrefWidth != 0) && nonPctColCount != 0', file ../../../layout/tables/BasicTableLayoutStrategy.cpp, line 585

You'll also get the assertion when you click on the "Send" button.

Comment 1

[Matthew Cline]

Attachment: gdb stack trace

Huh, uploading an attachment during bug creation doesn't seem to work.

Comment 2

[Jesse Ruderman]

Attachment: reduced testcase

Comment 3

[Jesse Ruderman]

I can reproduce on Mac.

I'm guessing this is another regression from bug 368504.

Comment 4

[Daniel Holbert [:dholbert]]

I also see this assertion when loading the URL
  http://en.wikipedia.org/wiki/Chronic_fatigue_syndrome
in a trunk checkout from today.

Comment 5

[Daniel Holbert [:dholbert]]

I also see this assertion on attachment 303417 [details] from bug 413286.  Specifically, the tables under the headings "Spanning cell has [low|high] percent width".

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: fix w/ crashtest

Here's a fix, with the reduced testcase attached as a crashtest.

PATCH SUMMARY: When we've distributed all the colspan's percent-width, return immediately, rather than continuing to loop across trailing zero-width columns.

EXPLANATION:
Basically, the issue was with situation with:
  - we're distributing a colspan's %-width proportional to the spanned columns' pref width
  - there are empty (zero-pref-width) columns at the end
 
When we're looping and we get to those empty columns at the end, we'd fail the assertion, because nonPctTotalPrefWidth would be zero, since the only columns left are empty.

This patch fixes the reduced testcase and all URLs that have been mentioned as triggering this assertion.

Comment 7

[Daniel Holbert [:dholbert]]

FWIW:  It might not be immediately obvious why we hit this assertion on the reduced testcase, since it looks like all of its cells have zero width, so we shouldn't be dealing with nonPctTotalPrefWidth in the first place.

BUT, the reason we hit the assertion is this: The first column actually has a small nonzero pref width, because of the table's cellpadding/cellspacing.  The second column, however, has prefWidth == 0, since no cell is explicitly created there.  So we've got a trailing empty column after a nonempty column, which makes us fail the assertion as described in comment 6.

Comment 8

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 304312 [details] [diff] [review]
fix w/ crashtest

(please nominate for approval only after you get your review)

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 304312 [details] [diff] [review]
fix w/ crashtest

Why not just fix the assertion to allow for "|| aSpanPrefPct == 0.0f"?

Also, while you're there, allocatedPct could be moved inside the if, and not initialized.

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 304312 [details] [diff] [review]
fix w/ crashtest

Er, never mind.  nonPctTotalPrefWidth or nonPctColCount could also be zero at this point.  So yeah, this patch is correct.

You might actually want to phrase the assertion as a ?:, though.  And you should probably move allocatedPct into the if and not initialize it.

So r+sr=dbaron with that.

Comment 11

[Daniel Holbert [:dholbert]]

Attachment: fix v2 (addressed review comments)

> You might actually want to phrase the assertion as a ?:, though.

Yup, that makes much more sense.

> And you should probably move allocatedPct into the if and not initialize it.

Done.

Comment 12

[Daniel Holbert [:dholbert]]

Justification for requesting approval on patch:
 - Avoids divide-by-zero
 - Fixes assertion failure

Comment 13

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 304892 [details] [diff] [review]
fix v2 (addressed review comments)

a=beltzner for 1.9

Comment 14

[Daniel Holbert [:dholbert]]

fix v2 checked in.

Checking in BasicTableLayoutStrategy.cpp;
/cvsroot/mozilla/layout/tables/BasicTableLayoutStrategy.cpp,v  <--  BasicTableLayoutStrategy.cpp
new revision: 3.267; previous revision: 3.266
done
RCS file: /cvsroot/mozilla/layout/tables/crashtests/413180-1.html,v
done
Checking in crashtests/413180-1.html;
/cvsroot/mozilla/layout/tables/crashtests/413180-1.html,v  <--  413180-1.html
initial revision: 1.1
done
Checking in crashtests/crashtests.list;
/cvsroot/mozilla/layout/tables/crashtests/crashtests.list,v  <--  crashtests.list
new revision: 1.34; previous revision: 1.33
done