Bug 129148 - ASSERTION FAILED: span >= 1
Summary: ASSERTION FAILED: span >= 1
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Zsolt Borbely
URL:
Keywords:
Depends on:
Blocks: 116980
  Show dependency treegraph
 
Reported: 2014-02-21 04:02 PST by Zsolt Borbely
Modified: 2014-03-10 12:50 PDT (History)
8 users (show)

See Also:


Attachments
Test case (61 bytes, text/html)
2014-02-21 04:02 PST, Zsolt Borbely
no flags Details
Proposed patch (1.33 KB, patch)
2014-02-21 05:10 PST, Zsolt Borbely
no flags Details | Formatted Diff | Diff
Proposed patch (1.35 KB, patch)
2014-02-24 01:01 PST, Zsolt Borbely
no flags Details | Formatted Diff | Diff
Proposed patch (3.27 KB, patch)
2014-02-26 08:33 PST, Zsolt Borbely
kling: review-
kling: commit-queue-
Details | Formatted Diff | Diff
Patch (3.42 KB, patch)
2014-03-05 14:28 PST, Zsolt Borbely
kling: review-
kling: commit-queue-
Details | Formatted Diff | Diff
Patch (3.63 KB, patch)
2014-03-06 06:42 PST, Zsolt Borbely
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zsolt Borbely 2014-02-21 04:02:16 PST
Created attachment 224849 [details]
Test case

The failing test case:

<table>
	<colgroup span="0">
	<th></th>
	</colgroup>
</table>

ASSERTION FAILED: span >= 1
/home/bzsolt/webkit/EflWebKit1/Source/WebCore/rendering/RenderTable.cpp(874) : WebCore::RenderTableCol* WebCore::RenderTable::slowColElement(unsigned int, bool*, bool*) const

Backtrace:

#0  0x00007ffff5ec5f62 in WTFCrash () at /home/bzsolt/webkit/EflWebKit1/Source/WTF/wtf/Assertions.cpp:333
#1  0x00007ffff15d876d in WebCore::RenderTable::slowColElement (this=0x1043970, col=0, startEdge=0x0, endEdge=0x0)
    at /home/bzsolt/webkit/EflWebKit1/Source/WebCore/rendering/RenderTable.cpp:874
#2  0x00007ffff13fdc22 in WebCore::RenderTable::colElement (this=0x1043970, col=0, startEdge=0x0, endEdge=0x0)
    at /home/bzsolt/webkit/EflWebKit1/Source/WebCore/rendering/RenderTable.h:216
#3  0x00007ffff13fdcf6 in WebCore::RenderTableCell::styleOrColLogicalWidth (this=0x107c0c0)
    at /home/bzsolt/webkit/EflWebKit1/Source/WebCore/rendering/RenderTableCell.h:93
#4  0x00007ffff14546f2 in WebCore::RenderBlock::computeIntrinsicLogicalWidths (this=0x107c0c0, minLogicalWidth=..., 
    maxLogicalWidth=...) at /home/bzsolt/webkit/EflWebKit1/Source/WebCore/rendering/RenderBlock.cpp:3869
#5  0x00007ffff1454972 in WebCore::RenderBlock::computePreferredLogicalWidths (this=0x107c0c0)
    at /home/bzsolt/webkit/EflWebKit1/Source/WebCore/rendering/RenderBlock.cpp:3893
#6  0x00007ffff15df0d9 in WebCore::RenderTableCell::computePreferredLogicalWidths (this=0x107c0c0)
    at /home/bzsolt/webkit/EflWebKit1/Source/WebCore/rendering/RenderTableCell.cpp:177
#7  0x00007ffff14a8cc5 in WebCore::RenderBox::minPreferredLogicalWidth (this=0x107c0c0)
    at /home/bzsolt/webkit/EflWebKit1/Source/WebCore/rendering/RenderBox.cpp:956
#8  0x00007ffff13f970d in WebCore::AutoTableLayout::recalcColumn (this=0x1047d40, effCol=0)
    at /home/bzsolt/webkit/EflWebKit1/Source/WebCore/rendering/AutoTableLayout.cpp:76
#9  0x00007ffff13f9fab in WebCore::AutoTableLayout::fullRecalc (this=0x1047d40)
    at /home/bzsolt/webkit/EflWebKit1/Source/WebCore/rendering/AutoTableLayout.cpp:179
#10 0x00007ffff13fa22e in WebCore::AutoTableLayout::computeIntrinsicLogicalWidths (this=0x1047d40, minWidth=..., 
    maxWidth=...) at /home/bzsolt/webkit/EflWebKit1/Source/WebCore/rendering/AutoTableLayout.cpp:214
#11 0x00007ffff15d7e05 in WebCore::RenderTable::computeIntrinsicLogicalWidths (this=0x1043970, minWidth=..., 
    maxWidth=...) at /home/bzsolt/webkit/EflWebKit1/Source/WebCore/rendering/RenderTable.cpp:752
#12 0x00007ffff15d7e6b in WebCore::RenderTable::computePreferredLogicalWidths (this=0x1043970)
    at /home/bzsolt/webkit/EflWebKit1/Source/WebCore/rendering/RenderTable.cpp:761
#13 0x00007ffff14a8d27 in WebCore::RenderBox::maxPreferredLogicalWidth (this=0x1043970)
    at /home/bzsolt/webkit/EflWebKit1/Source/WebCore/rendering/RenderBox.cpp:968
#14 0x00007ffff15d4c4b in WebCore::RenderTable::updateLogicalWidth (this=0x1043970)
    at /home/bzsolt/webkit/EflWebKit1/Source/WebCore/rendering/RenderTable.cpp:292
#15 0x00007ffff15d5b43 in WebCore::RenderTable::layout (this=0x1043970)
    at /home/bzsolt/webkit/EflWebKit1/Source/WebCore/rendering/RenderTable.cpp:431
Comment 1 Zsolt Borbely 2014-02-21 05:10:43 PST
Created attachment 224853 [details]
Proposed patch
Comment 2 Zsolt Borbely 2014-02-24 01:01:28 PST
Created attachment 225033 [details]
Proposed patch
Comment 3 Csaba Osztrogonác 2014-02-25 04:43:02 PST
(In reply to comment #2)
> Created an attachment (id=225033) [details]
> Proposed patch

Please add a test which asserts without the fix and not with it.
Comment 4 Zsolt Borbely 2014-02-26 08:33:57 PST
Created attachment 225258 [details]
Proposed patch
Comment 5 Andreas Kling 2014-03-05 12:58:48 PST
Comment on attachment 225258 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=225258&action=review

r- because the isNull() case is not tested despite ChangeLog mentioning such a test.

> LayoutTests/ChangeLog:8
> +        Added test demonstrates the behavior of colgroup in case of large negative, large positive  and null span values.

The added test is not hitting the null value case.
"value" will be null in parseAttribute() if the attribute is being programmatically removed from the element; e.g element.removeAttribute("colgroup")

> Source/WebCore/html/HTMLTableColElement.cpp:68
> -        m_span = !value.isNull() ? value.toInt() : 1;
> +        m_span = (value.isNull() || !value.toInt()) ? 1 : value.toInt();

This code looks a bit messy. In the common case, you are doing string-to-int conversion twice.
I'd write it like this:

int newSpan = value.toInt();
m_span = newSpan ? newSpan : 1;
Comment 6 Zsolt Borbely 2014-03-05 14:28:51 PST
Created attachment 225914 [details]
Patch
Comment 7 Andreas Kling 2014-03-06 02:30:41 PST
Comment on attachment 225914 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=225914&action=review

> LayoutTests/fast/table/incorrect-colgroup-span-values.html:6
> +      if (window.testRunner)
> +      testRunner.dumpAsText();

The second line here should be indented.

> LayoutTests/fast/table/incorrect-colgroup-span-values.html:12
> +      <colgroup span="">

This is still not testing the null case; it will arrive in parseAttribute() as an empty string, not a null one.
To get null, you need to run some JavaScript that does element.removeAttribute("colgroup")
Comment 8 Zsolt Borbely 2014-03-06 06:42:25 PST
Created attachment 225985 [details]
Patch
Comment 9 Zsolt Borbely 2014-03-06 06:52:24 PST
(In reply to comment #7)
> (From update of attachment 225914 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=225914&action=review
> 
> > LayoutTests/fast/table/incorrect-colgroup-span-values.html:6
> > +      if (window.testRunner)
> > +      testRunner.dumpAsText();
> 
> The second line here should be indented.

Fixed.
> 
> > LayoutTests/fast/table/incorrect-colgroup-span-values.html:12
> > +      <colgroup span="">
> 
> This is still not testing the null case; it will arrive in parseAttribute() as an empty string, not a null one.
> To get null, you need to run some JavaScript that does element.removeAttribute("colgroup")

Sorry about this misunderstanding, when I wrote null I actually meant zero.
I have just extended the test with a null case.
Comment 10 Andreas Kling 2014-03-10 12:18:03 PDT
Comment on attachment 225985 [details]
Patch

r=me
Comment 11 WebKit Commit Bot 2014-03-10 12:50:47 PDT
Comment on attachment 225985 [details]
Patch

Clearing flags on attachment: 225985

Committed r165395: <http://trac.webkit.org/changeset/165395>
Comment 12 WebKit Commit Bot 2014-03-10 12:50:51 PDT
All reviewed patches have been landed.  Closing bug.