credit:Kuzzcc bug: http://code.google.com/p/chromium/issues/detail?id=46754 testcase (indented for better readability): <html> <head> <style type="text/css"> table { table-layout: fixed; width: 1px; } </style> </head> <body> <table> <td width="30">ABC</td> <td colspan="65536">DEF</td> </table> </body> </html> span (int) get narrow casted to short in rendertable.cpp (since m_columns[pos].span is short), so 65536 becomes 0 (even though we want to have it greater than 1, see HTMLTableCellElement.cpp - if (attr->name() == colspanAttr) { m_colSpan = max(1, attr->value().toInt()); ) void RenderTable::appendColumn(int span) { // easy case. int pos = m_columns.size(); int newSize = pos + 1; m_columns.grow(newSize); m_columns[pos].span = span; since autospan is 0, we get crash in fixedtablelayout.cpp. if (m_width[i].isAuto()) { int span = m_table->spanOfEffCol(i); int w = remainingWidth * span / autoSpan; This bug is secseverity none for chromium (since it just crashes tab_ and low for safari (since it crashes browser completely). it should be an easy fix since span is used at int at all places, it does make sense to use it an unsigned int in declaration. unsigned part is kept in a good way to keep its value positive.
HTML5 spec also says to have larger values for colspan. so, going from short to int shouldn't be that bad. Also, conversions, one comparison between signed and unsigned int shouldn't be bad since initially we start with colspan b/w 1 and +INT_MAX (m_colSpan = max(1, attr->value().toInt()) and it stays within that range. http://www.whatwg.org/specs/web-apps/current-work/multipage/tabular-data.html#htmltablecellelement interface HTMLTableCellElement : HTMLElement { attribute unsigned long colSpan; attribute unsigned long rowSpan; [PutForwards=value] readonly attribute DOMSettableTokenList headers; readonly attribute long cellIndex; }; I will file a seperate bug to cover some code cleanups, like converting all signed int use of span to unsigned int and remove the width attribute which is no longer used.
Created attachment 59129 [details] Patch
Comment on attachment 59129 [details] Patch Interesting. What's the memory impact of this change?WebCore/rendering/RenderTableSection.cpp:234 + if (cSpan < static_cast<int>(columns[m_cCol].span)) I don't quite understand why this cast is needed. Presumably cSpan is a signed int. Would it be better to make cSpan an unsigned? WebCore/rendering/RenderTable.h:78 + unsigned int span; Usually we just say "unsigned" when we mean "unsigned int"
Thanks a lot Adam for your fast review. WebCore/rendering/RenderTable.h:78 + unsigned int span; Usually we just say "unsigned" when we mean "unsigned int" --- Ok, i will remember this. i like the unsigned int version :) Interesting. What's the memory impact of this change?WebCore/rendering/RenderTableSection.cpp:234 + if (cSpan < static_cast<int>(columns[m_cCol].span)) I don't quite understand why this cast is needed. Presumably cSpan is a signed int. Would it be better to make cSpan an unsigned? ---- this is the only comparison b/w signed and unsigned int span and fails to compile on mac. as i said in comment #1, the cspan and like 30 more places all use span as signed int. The code needs to be cleaned up to change all spans to unsigned ints and non-used width attribute needs to be removed. for now, it is ok, since at input, we restrict it to 1 to +INT_MAX. Me or Cris Neckar might work on this cleanup.
Created attachment 59135 [details] Patch
Comment on attachment 59135 [details] Patch Thanks. We might want to ask jamesr or hyatt to make sure we're not using too much extra memory.
James, Dave, can you please take a look to see if it is ok to use extra memory for colspan in this patch.
I had a chat with Dave and he is fine with this change.
(In reply to comment #8) > I had a chat with Dave and he is fine with this change. Thanks for checking. Looks like this is ready to land.
Committed as http://trac.webkit.org/changeset/61451
<rdar://problem/8152839>
Removing Security since <http://code.google.com/p/chromium/issues/detail?id=46754> is public and this is a divide-by-zero crash.