Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
131103017:
ASSERTION FAILED: !remainder in WebCore::RenderTableSection::distributeExtraRowSpanHeightToAutoRows (Closed)
|
Created:
6 years, 11 months ago by a.suchit Modified:
6 years, 9 months ago Reviewers:
Julien - ping for review, leviw_travelin_and_unemployed, suchit.agrawal, esprehn, eseidel CC:
blink-reviews, bemjb+rendering_chromium.org, dsinclair, zoltan1, eae+blinkwatch, leviw+renderwatch, jchaffraix+rendering, eae Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionASSERTION FAILED: !remainder in WebCore::RenderTableSection::distributeExtraRowSpanHeightToAutoRows
While distributing the extra rowspan height in rows, result of multiplication
is overflowing in integer and whole method is getting failed due to this. So
typecasted a value to 'long long' and final result of the calculation always
fits in integer range.
BUG=334652
R=jchaffraix@chromium.org
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=169843
Patch Set 1 #Patch Set 2 : Used LayoutUnit for computation. It is just for reference because it does not work. #Patch Set 3 : Updated #
Total comments: 7
Patch Set 4 : Review comments addressed #
Total comments: 4
Patch Set 5 : Added a test case and fixed review comments #
Total comments: 7
Patch Set 6 : Fixed review comments #
Messages
Total messages: 25 (0 generated)
On 2014/01/23 05:19:02, suchit.agrawal wrote: I do not think, typecasting is good way to solve this problem. @Julien, Can you please review it and guide me some other option for it which would be better for blink. Thanks
I don't have a good solution for an overflow triggering the ASSERT. However using LayoutUnit for your layout computation should neuter the nastiness of the overflow as we use saturated arithmetic.
On 2014/02/06 22:55:09, Julien Chaffraix - PST wrote: > I don't have a good solution for an overflow triggering the ASSERT. > > However using LayoutUnit for your layout computation should neuter the nastiness > of the overflow as we use saturated arithmetic. I tried LayoutUnit, But it don't solve this issue. For your reference, I added the patch with LayoutUnit change. But this patch does not solve this issue. Please comment, if I am doing some wrong here.
On 2014/02/19 13:35:00, a.suchit wrote: > On 2014/02/06 22:55:09, Julien Chaffraix - PST wrote: > > I don't have a good solution for an overflow triggering the ASSERT. > > > > However using LayoutUnit for your layout computation should neuter the > nastiness > > of the overflow as we use saturated arithmetic. > > I tried LayoutUnit, But it don't solve this issue. > For your reference, I added the patch with LayoutUnit change. But this patch > does not solve this issue. Please comment, if I am doing some wrong here. LayoutUnit is not solving overflowing issue. So Updated the patch with type-casted with long long.
I tried to fix it by LayoutUnit. But it did not solve this problem. Patch Set : 2 , have a version of LayoutUnit. Please comment on it also if I am not using LayoutUnit correctly. OR Please let me know if there is other better way to solve it Or Patch Set : 3 is OK.
Emil and Levi are layoutunit ninjas.
On 2014/03/05 12:38:15, a.suchit wrote: > I tried to fix it by LayoutUnit. But it did not solve this problem. Patch Set : > 2 , have a version of LayoutUnit. Please comment on it also if I am not using > LayoutUnit correctly. > OR > Please let me know if there is other better way to solve it Or Patch Set : 3 is > OK. Tables are specifically laid out using integers (see https://trac.webkit.org/wiki/LayoutUnit) to match the spec. They're limited to a size of INT_MAX/64.
LayoutUnit has a smaller range than integer (INT_MAX/64). We explicitly don't use LayoutUnit for tables (see https://trac.webkit.org/wiki/LayoutUnit) to match the table spec, which expects whole pixels to be distributed to cells. https://codereview.chromium.org/131103017/diff/100001/Source/core/rendering/R... File Source/core/rendering/RenderTableSection.cpp (right): https://codereview.chromium.org/131103017/diff/100001/Source/core/rendering/R... Source/core/rendering/RenderTableSection.cpp:346: // One of the variable in calculation typecasted with 'long long'. There are some grammar ("getting overflowing"), spelling ("multiplecation"), and capitalization ("So One"), issues with this comment. https://codereview.chromium.org/131103017/diff/100001/Source/core/rendering/R... Source/core/rendering/RenderTableSection.cpp:349: accumulatedPositionIncrease += ((long long)extraRowSpanningHeight * rowsHeight[row - rowIndex]) / totalAutoRowsHeight; I'm not sure if this is the fix we want, but I'd expect it to be extracted into a function instead of duplicated.
https://codereview.chromium.org/131103017/diff/100001/Source/core/rendering/R... File Source/core/rendering/RenderTableSection.cpp (right): https://codereview.chromium.org/131103017/diff/100001/Source/core/rendering/R... Source/core/rendering/RenderTableSection.cpp:349: accumulatedPositionIncrease += ((long long)extraRowSpanningHeight * rowsHeight[row - rowIndex]) / totalAutoRowsHeight; This doesn't solve the problem in general as the int multiplication could still overflow the long long (C/C++ only define a minimum range for the different types). That's why I said that I don't have a good solution for the overflow issue. LayoutUnit was meant as a way to *neuter* the nastiness of an overflow, not solve it. What this just means is that the ASSERT is wrong as written, we should either fix it or remove it (to be replaced by a comment explaining why we don't ASSERT). We should also have a way to mitigate overflows.
https://codereview.chromium.org/131103017/diff/100001/Source/core/rendering/R... File Source/core/rendering/RenderTableSection.cpp (right): https://codereview.chromium.org/131103017/diff/100001/Source/core/rendering/R... Source/core/rendering/RenderTableSection.cpp:346: // One of the variable in calculation typecasted with 'long long'. On 2014/03/05 23:01:43, Levi wrote: > There are some grammar ("getting overflowing"), spelling ("multiplecation"), and > capitalization ("So One"), issues with this comment. Done. https://codereview.chromium.org/131103017/diff/100001/Source/core/rendering/R... Source/core/rendering/RenderTableSection.cpp:349: accumulatedPositionIncrease += ((long long)extraRowSpanningHeight * rowsHeight[row - rowIndex]) / totalAutoRowsHeight; On 2014/03/06 00:29:18, Julien Chaffraix - PST wrote: > This doesn't solve the problem in general as the int multiplication could still > overflow the long long (C/C++ only define a minimum range for the different > types). > > That's why I said that I don't have a good solution for the overflow issue. > LayoutUnit was meant as a way to *neuter* the nastiness of an overflow, not > solve it. > > What this just means is that the ASSERT is wrong as written, we should either > fix it or remove it (to be replaced by a comment explaining why we don't > ASSERT). We should also have a way to mitigate overflows. Removing the assert will just prevent assertion failure. But it will not solve it. Because when such scenario will come than table will not layout properly. Multiplication of 2 integers(4 bytes) would never overflow the long long(8 bytes). So if right now we do not have any better solution to solve this problem then typecast with long long is OK which would not have wrong table layout in such scenarios. https://codereview.chromium.org/131103017/diff/100001/Source/core/rendering/R... Source/core/rendering/RenderTableSection.cpp:349: accumulatedPositionIncrease += ((long long)extraRowSpanningHeight * rowsHeight[row - rowIndex]) / totalAutoRowsHeight; On 2014/03/05 23:01:43, Levi wrote: > I'm not sure if this is the fix we want, but I'd expect it to be extracted into > a function instead of duplicated. Done.
The code is fine, but the change needs a test. https://codereview.chromium.org/131103017/diff/100001/Source/core/rendering/R... File Source/core/rendering/RenderTableSection.cpp (right): https://codereview.chromium.org/131103017/diff/100001/Source/core/rendering/R... Source/core/rendering/RenderTableSection.cpp:349: accumulatedPositionIncrease += ((long long)extraRowSpanningHeight * rowsHeight[row - rowIndex]) / totalAutoRowsHeight; On 2014/03/10 09:37:56, a.suchit wrote: > On 2014/03/06 00:29:18, Julien Chaffraix - PST wrote: > > This doesn't solve the problem in general as the int multiplication could > still > > overflow the long long (C/C++ only define a minimum range for the different > > types). > > > > That's why I said that I don't have a good solution for the overflow issue. > > LayoutUnit was meant as a way to *neuter* the nastiness of an overflow, not > > solve it. > > > > What this just means is that the ASSERT is wrong as written, we should either > > fix it or remove it (to be replaced by a comment explaining why we don't > > ASSERT). We should also have a way to mitigate overflows. > > Removing the assert will just prevent assertion failure. But it will not solve > it. Because when such scenario will come than table will not layout properly. > > Multiplication of 2 integers(4 bytes) would never overflow the long long(8 > bytes). So if right now we do not have any better solution to solve this problem > then typecast with long long is OK which would not have wrong table layout in > such scenarios. You're assuming a certain platform. C allows integer and long long to be of the same size (it's unlikely and I don't know of any platform that do but that's perfectly valid per the standard). https://codereview.chromium.org/131103017/diff/120001/Source/core/rendering/R... File Source/core/rendering/RenderTableSection.cpp (right): https://codereview.chromium.org/131103017/diff/120001/Source/core/rendering/R... Source/core/rendering/RenderTableSection.cpp:335: static int calcIncreasedRowHeight(long long extraHeight, long long rowHeight, long long totalHeight, int& remainder) Let's not abbreviate function names. I don't really like how we mix side effect with return value. I would rather that the code pass |accumulatedPositionIncrease| and modify it and return nothing. https://codereview.chromium.org/131103017/diff/120001/Source/core/rendering/R... Source/core/rendering/RenderTableSection.cpp:337: remainder += (extraHeight * rowHeight) % totalHeight; Per my comment, there should be some ASSERT about the types' sizes. COMPILE_ASSERT(sizeof(long long int) > sizeof(int)); This ensures that we can't overflow in the multiplication and will catch platform that breaks the new code's assumption.
https://codereview.chromium.org/131103017/diff/120001/Source/core/rendering/R... File Source/core/rendering/RenderTableSection.cpp (right): https://codereview.chromium.org/131103017/diff/120001/Source/core/rendering/R... Source/core/rendering/RenderTableSection.cpp:335: static int calcIncreasedRowHeight(long long extraHeight, long long rowHeight, long long totalHeight, int& remainder) On 2014/03/10 18:36:39, Julien Chaffraix - PST wrote: > Let's not abbreviate function names. > > I don't really like how we mix side effect with return value. I would rather > that the code pass |accumulatedPositionIncrease| and modify it and return > nothing. Done. https://codereview.chromium.org/131103017/diff/120001/Source/core/rendering/R... Source/core/rendering/RenderTableSection.cpp:337: remainder += (extraHeight * rowHeight) % totalHeight; On 2014/03/10 18:36:39, Julien Chaffraix - PST wrote: > Per my comment, there should be some ASSERT about the types' sizes. > > COMPILE_ASSERT(sizeof(long long int) > sizeof(int)); > > This ensures that we can't overflow in the multiplication and will catch > platform that breaks the new code's assumption. Done.
On 2014/03/11 12:33:56, a.suchit wrote: > https://codereview.chromium.org/131103017/diff/120001/Source/core/rendering/R... > File Source/core/rendering/RenderTableSection.cpp (right): > > https://codereview.chromium.org/131103017/diff/120001/Source/core/rendering/R... > Source/core/rendering/RenderTableSection.cpp:335: static int > calcIncreasedRowHeight(long long extraHeight, long long rowHeight, long long > totalHeight, int& remainder) > On 2014/03/10 18:36:39, Julien Chaffraix - PST wrote: > > Let's not abbreviate function names. > > > > I don't really like how we mix side effect with return value. I would rather > > that the code pass |accumulatedPositionIncrease| and modify it and return > > nothing. > > Done. > > https://codereview.chromium.org/131103017/diff/120001/Source/core/rendering/R... > Source/core/rendering/RenderTableSection.cpp:337: remainder += (extraHeight * > rowHeight) % totalHeight; > On 2014/03/10 18:36:39, Julien Chaffraix - PST wrote: > > Per my comment, there should be some ASSERT about the types' sizes. > > > > COMPILE_ASSERT(sizeof(long long int) > sizeof(int)); > > > > This ensures that we can't overflow in the multiplication and will catch > > platform that breaks the new code's assumption. > > Done. Please review it. Thanks
On 2014/03/15 02:12:52, a.suchit wrote: > On 2014/03/11 12:33:56, a.suchit wrote: > > > https://codereview.chromium.org/131103017/diff/120001/Source/core/rendering/R... > > File Source/core/rendering/RenderTableSection.cpp (right): > > > > > https://codereview.chromium.org/131103017/diff/120001/Source/core/rendering/R... > > Source/core/rendering/RenderTableSection.cpp:335: static int > > calcIncreasedRowHeight(long long extraHeight, long long rowHeight, long long > > totalHeight, int& remainder) > > On 2014/03/10 18:36:39, Julien Chaffraix - PST wrote: > > > Let's not abbreviate function names. > > > > > > I don't really like how we mix side effect with return value. I would rather > > > that the code pass |accumulatedPositionIncrease| and modify it and return > > > nothing. > > > > Done. > > > > > https://codereview.chromium.org/131103017/diff/120001/Source/core/rendering/R... > > Source/core/rendering/RenderTableSection.cpp:337: remainder += (extraHeight * > > rowHeight) % totalHeight; > > On 2014/03/10 18:36:39, Julien Chaffraix - PST wrote: > > > Per my comment, there should be some ASSERT about the types' sizes. > > > > > > COMPILE_ASSERT(sizeof(long long int) > sizeof(int)); > > > > > > This ensures that we can't overflow in the multiplication and will catch > > > platform that breaks the new code's assumption. > > > > Done. > > Please review it. Thanks Test case added and fixed review comments. Please review this issue. Thanks
On 2014/03/18 10:50:41, a.suchit wrote: > On 2014/03/15 02:12:52, a.suchit wrote: > > On 2014/03/11 12:33:56, a.suchit wrote: > > > > > > https://codereview.chromium.org/131103017/diff/120001/Source/core/rendering/R... > > > File Source/core/rendering/RenderTableSection.cpp (right): > > > > > > > > > https://codereview.chromium.org/131103017/diff/120001/Source/core/rendering/R... > > > Source/core/rendering/RenderTableSection.cpp:335: static int > > > calcIncreasedRowHeight(long long extraHeight, long long rowHeight, long long > > > totalHeight, int& remainder) > > > On 2014/03/10 18:36:39, Julien Chaffraix - PST wrote: > > > > Let's not abbreviate function names. > > > > > > > > I don't really like how we mix side effect with return value. I would > rather > > > > that the code pass |accumulatedPositionIncrease| and modify it and return > > > > nothing. > > > > > > Done. > > > > > > > > > https://codereview.chromium.org/131103017/diff/120001/Source/core/rendering/R... > > > Source/core/rendering/RenderTableSection.cpp:337: remainder += (extraHeight > * > > > rowHeight) % totalHeight; > > > On 2014/03/10 18:36:39, Julien Chaffraix - PST wrote: > > > > Per my comment, there should be some ASSERT about the types' sizes. > > > > > > > > COMPILE_ASSERT(sizeof(long long int) > sizeof(int)); > > > > > > > > This ensures that we can't overflow in the multiplication and will catch > > > > platform that breaks the new code's assumption. > > > > > > Done. > > > > Please review it. Thanks > > Test case added and fixed review comments. > Please review this issue. > Thanks Please review it. Thanks
lgtm https://codereview.chromium.org/131103017/diff/140001/LayoutTests/fast/table/... File LayoutTests/fast/table/table-rowspan-crash-with-huge-pedding-value.html (right): https://codereview.chromium.org/131103017/diff/140001/LayoutTests/fast/table/... LayoutTests/fast/table/table-rowspan-crash-with-huge-pedding-value.html:10: </style> Nit: The indentation is not consistent above (here and the <script> also). https://codereview.chromium.org/131103017/diff/140001/Source/core/rendering/R... File Source/core/rendering/RenderTableSection.cpp (right): https://codereview.chromium.org/131103017/diff/140001/Source/core/rendering/R... Source/core/rendering/RenderTableSection.cpp:331: // Some time multiplication of below 2 values is overflowing from integer. So In correct English: Sometimes the multiplication of the 2 values below will overflow an integer. https://codereview.chromium.org/131103017/diff/140001/Source/core/rendering/R... Source/core/rendering/RenderTableSection.cpp:332: // type of variable in computation are taken as 'long long' in place of int. So we convert the parameters to 'long long' instead of 'int' to avoid the problem in this function. https://codereview.chromium.org/131103017/diff/140001/Source/core/rendering/R... Source/core/rendering/RenderTableSection.cpp:333: // In this computation, only 2 integer variables are multiplied which would not This sentence really doesn't add much IMO.
https://codereview.chromium.org/131103017/diff/140001/LayoutTests/fast/table/... File LayoutTests/fast/table/table-rowspan-crash-with-huge-pedding-value.html (right): https://codereview.chromium.org/131103017/diff/140001/LayoutTests/fast/table/... LayoutTests/fast/table/table-rowspan-crash-with-huge-pedding-value.html:10: </style> On 2014/03/21 04:18:05, Julien Chaffraix - PST wrote: > Nit: The indentation is not consistent above (here and the <script> also). Done. https://codereview.chromium.org/131103017/diff/140001/Source/core/rendering/R... File Source/core/rendering/RenderTableSection.cpp (right): https://codereview.chromium.org/131103017/diff/140001/Source/core/rendering/R... Source/core/rendering/RenderTableSection.cpp:331: // Some time multiplication of below 2 values is overflowing from integer. So On 2014/03/21 04:18:05, Julien Chaffraix - PST wrote: > In correct English: > Sometimes the multiplication of the 2 values below will overflow an integer. Done. https://codereview.chromium.org/131103017/diff/140001/Source/core/rendering/R... Source/core/rendering/RenderTableSection.cpp:332: // type of variable in computation are taken as 'long long' in place of int. On 2014/03/21 04:18:05, Julien Chaffraix - PST wrote: > So we convert the parameters to 'long long' instead of 'int' to avoid the > problem in this function. Done.
The CQ bit was checked by a.suchit@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/a.suchit@samsung.com/131103017/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.blink on win_blink_rel
The CQ bit was checked by a.suchit@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/a.suchit@samsung.com/131103017/160001
Message was sent while issue was closed.
Change committed as 169843 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
