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

RESOLVED FIXED in mozilla1.9beta4

Status

()

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

People

(Reporter: Matthew Cline, Assigned: dholbert)

Tracking

({assertion, regression, testcase})

Trunk
mozilla1.9beta4
assertion, regression, testcase
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

10 years ago
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.
(Reporter)

Comment 1

10 years ago
Created attachment 298037 [details]
gdb stack trace

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

Comment 2

10 years ago
Created attachment 298050 [details]
reduced testcase

Comment 3

10 years ago
I can reproduce on Mac.

I'm guessing this is another regression from bug 368504.
Blocks: 368504
Keywords: regression, testcase
OS: Linux → All
Hardware: PC → All
Summary: should not be zero if we haven't allocated all pref percent → "ASSERTION: should not be zero if we haven't allocated all pref percent" with colspan, percentage width
I also see this assertion when loading the URL
  http://en.wikipedia.org/wiki/Chronic_fatigue_syndrome
in a trunk checkout from today.
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".
Created attachment 304312 [details] [diff] [review]
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.
Attachment #304312 - Flags: superreview?(dbaron)
Attachment #304312 - Flags: review?(dbaron)
Attachment #304312 - Flags: approval1.9?
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.
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Comment on attachment 304312 [details] [diff] [review]
fix w/ crashtest

(please nominate for approval only after you get your review)
Attachment #304312 - Flags: approval1.9?
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.
Attachment #304312 - Flags: superreview?(dbaron)
Attachment #304312 - Flags: superreview-
Attachment #304312 - Flags: review?(dbaron)
Attachment #304312 - Flags: review-
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.
Attachment #304312 - Flags: superreview-
Attachment #304312 - Flags: superreview+
Attachment #304312 - Flags: review-
Attachment #304312 - Flags: review+
Target Milestone: --- → mozilla1.9beta4
Created attachment 304892 [details] [diff] [review]
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.
Attachment #304312 - Attachment is obsolete: true
Attachment #304892 - Flags: approval1.9?
Justification for requesting approval on patch:
 - Avoids divide-by-zero
 - Fixes assertion failure
Comment on attachment 304892 [details] [diff] [review]
fix v2 (addressed review comments)

a=beltzner for 1.9
Attachment #304892 - Flags: approval1.9? → approval1.9+
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
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.