ASSERTION FAILED: !remainder in WebCore::RenderTableSection::distributeExtraRowSpanHeightToAutoRows

Source/core/rendering/RenderTableSection.cpp

Index: Source/core/rendering/RenderTableSection.cpp
diff --git a/Source/core/rendering/RenderTableSection.cpp b/Source/core/rendering/RenderTableSection.cpp
index d0e9ba961b9e24556333e02eb4e43e5edbe99931..8b44fed808a82cc01f6a333ee97374c3c1823821 100644
--- a/Source/core/rendering/RenderTableSection.cpp
+++ b/Source/core/rendering/RenderTableSection.cpp
@@ -342,8 +342,12 @@ void RenderTableSection::distributeExtraRowSpanHeightToAutoRows(RenderTableCell*
     // So extra height distributed in auto spanning rows based on their weight in spanning cell.
     for (unsigned row = rowIndex; row < (rowIndex + rowSpan); row++) {
         if (m_grid[row].logicalHeight.isAuto()) {
-            accumulatedPositionIncrease += (extraRowSpanningHeight * rowsHeight[row - rowIndex]) / totalAutoRowsHeight;
-            remainder += (extraRowSpanningHeight * rowsHeight[row - rowIndex]) % totalAutoRowsHeight;
+            // Some time multiplecation of below 2 values is getting overflowing from integer. So
+            // One of the variable in calculation typecasted with 'long long'.

[leviw_travelin_and_unemployed, 2014/03/05 23:01:43]

There are some grammar ("getting overflowing"), spelling ("multiplecation"), and
capitalization ("So One"), issues with this comment.

[a.suchit, 2014/03/10 09:37:56]

Done.

+            // Final result of below calculation would not be more than extraRowSpanningHeight because
+            // 'totalAutoRowsHeight' would be equal or more than auto row height.
+            accumulatedPositionIncrease += ((long long)extraRowSpanningHeight * rowsHeight[row - rowIndex]) / totalAutoRowsHeight;

[leviw_travelin_and_unemployed, 2014/03/05 23:01:43]

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.

[Julien - ping for review, 2014/03/06 00:29:18]

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.

[a.suchit, 2014/03/10 09:37:56]

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.

[a.suchit, 2014/03/10 09:37:56]

Done.

[Julien - ping for review, 2014/03/10 18:36:39]

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

+            remainder += ((long long)extraRowSpanningHeight * rowsHeight[row - rowIndex]) % totalAutoRowsHeight;
 
             // While whole extra spanning height is distributing in auto spanning rows, rational parts remains
             // in every integer division. So accumulating all remainder part in integer division and when total remainder
@@ -376,8 +380,12 @@ void RenderTableSection::distributeExtraRowSpanHeightToRemainingRows(RenderTable
     // So extra height distribution in remaining spanning rows based on their weight in spanning cell.
     for (unsigned row = rowIndex; row < (rowIndex + rowSpan); row++) {
         if (!m_grid[row].logicalHeight.isPercent()) {
-            accumulatedPositionIncrease += (extraRowSpanningHeight * rowsHeight[row - rowIndex]) / totalRemainingRowsHeight;
-            remainder += (extraRowSpanningHeight * rowsHeight[row - rowIndex]) % totalRemainingRowsHeight;
+            // Some time multiplecation of below 2 values is getting overflowing from integer. So
+            // One of the variable in calculation typecasted with 'long long'.
+            // Final result of below calculation would not be more than extraRowSpanningHeight because
+            // 'totalAutoRowsHeight' would be equal or more than auto row height.
+            accumulatedPositionIncrease += ((long long)extraRowSpanningHeight * rowsHeight[row - rowIndex]) / totalRemainingRowsHeight;
+            remainder += ((long long)extraRowSpanningHeight * rowsHeight[row - rowIndex]) % totalRemainingRowsHeight;
 
             // While whole extra spanning height is distributing in remaining spanning rows, rational parts remains
             // in every integer division. So accumulating all remainder part in integer division and when total remainder