Bug 129148 – ASSERTION FAILED: span >= 1

• Component: DOM
• Status: RESOLVED FIXED

[Zsolt Borbely]

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

[Zsolt Borbely]

Created attachment 224853 [details]
Proposed patch

[Zsolt Borbely]

Created attachment 225033 [details]
Proposed patch

[Csaba Osztrogonác]

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

[Zsolt Borbely]

Created attachment 225258 [details]
Proposed patch

[Andreas Kling]

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;

[Zsolt Borbely]

Created attachment 225914 [details]
Patch

[Andreas Kling]

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

[Zsolt Borbely]

Created attachment 225985 [details]
Patch

[Zsolt Borbely]

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

[Andreas Kling]

Comment on attachment 225985 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 225985 [details]
Patch

Clearing flags on attachment: 225985

Committed r165395: <http://trac.webkit.org/changeset/165395>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.