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

Source/core/rendering/RenderTableSection.cpp

 /*
  * Copyright (C) 1997 Martin Jones (mjones@kde.org)
  *           (C) 1997 Torben Weis (weis@kde.org)
  *           (C) 1998 Waldo Bastian (bastian@kde.org)
  *           (C) 1999 Lars Knoll (knoll@kde.org)
  *           (C) 1999 Antti Koivisto (koivisto@kde.org)
  * Copyright (C) 2003, 2004, 2005, 2006, 2008, 2009, 2010 Apple Inc. All rights reserved.
  * Copyright (C) 2006 Alexey Proskuryakov (ap@nypop.com)
  *
  * This library is free software; you can redistribute it and/or
@@ skipping 324 matching lines @@
 
     const unsigned rowSpan = cell->rowSpan();
     const unsigned rowIndex = cell->rowIndex();
     int accumulatedPositionIncrease = 0;
     int remainder = 0;
 
     // Aspect ratios of auto rows should not change otherwise table may look different than user expected.
     // 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
             // is equvalent to divisor then 1 unit increased in row position.
             // Note that this algorithm is biased towards adding more space towards the lower rows.
             if (remainder >= totalAutoRowsHeight) {
                 remainder -= totalAutoRowsHeight;
                 accumulatedPositionIncrease++;
             }
         }
@@ skipping 12 matching lines @@
 
     const unsigned rowSpan = cell->rowSpan();
     const unsigned rowIndex = cell->rowIndex();
     int accumulatedPositionIncrease = 0;
     int remainder = 0;
 
     // Aspect ratios of the rows should not change otherwise table may look different than user expected.
     // 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
             // is equvalent to divisor then 1 unit increased in row position.
             // Note that this algorithm is biased towards adding more space towards the lower rows.
             if (remainder >= totalRemainingRowsHeight) {
                 remainder -= totalRemainingRowsHeight;
                 accumulatedPositionIncrease++;
             }
         }
@@ skipping 1336 matching lines @@
     else
         cellLocation.setX(table()->columnPositions()[effectiveColumn] + horizontalBorderSpacing);
 
     cell->setLogicalLocation(cellLocation);
 
     if (!RuntimeEnabledFeatures::repaintAfterLayoutEnabled())
         view()->addLayoutDelta(oldCellLocation - cell->location());
 }
 
 } // namespace WebCore