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
Created attachment 224853 [details] Proposed patch
Created attachment 225033 [details] Proposed patch
(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.
Created attachment 225258 [details] Proposed patch
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;
Created attachment 225914 [details] Patch
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")
Created attachment 225985 [details] Patch
(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 on attachment 225985 [details] Patch r=me
Comment on attachment 225985 [details] Patch Clearing flags on attachment: 225985 Committed r165395: <http://trac.webkit.org/changeset/165395>
All reviewed patches have been landed. Closing bug.