[css-tables] Stop row collapsing from subtracting border spacing twice

A collapsed row would subtract border-spacing right away, then also
subtract it later when passing heights to %-height children.

Bug: 855917
Change-Id: I29f5e01035b7f9dc76af337fbd65cc332d50376c
Reviewed-on: https://chromium-review.googlesource.com/1116249Reviewed-by: Morten Stenshorne <mstensho@chromium.org>
Commit-Queue: David Grogan <dgrogan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#571636}

third_party/blink/renderer/core/layout/layout_table.cc

@@ -92,6 +92,8 @@ void LayoutTable::StyleDidChange(StyleDifference diff,
   // In the collapsed border model, there is no cell spacing.
   h_spacing_ = ShouldCollapseBorders() ? 0 : Style()->HorizontalBorderSpacing();
   v_spacing_ = ShouldCollapseBorders() ? 0 : Style()->VerticalBorderSpacing();
+  DCHECK_GE(h_spacing_, 0);
+  DCHECK_GE(v_spacing_, 0);
 
   if (!table_layout_ ||
       Style()->IsFixedTableLayout() != old_fixed_table_layout) {

third_party/blink/renderer/core/layout/layout_table_section.cc

@@ -958,6 +958,7 @@ int LayoutTableSection::CalcRowLogicalHeight() {
       total_collapsed_height += row_collapsed_height_[r];
       // Adjust row position according to the height collapsed so far.
       row_pos_[r + 1] -= total_collapsed_height;
+      DCHECK_GE(row_pos_[r + 1], row_pos_[r]);
     }
   }
 
@@ -1186,13 +1187,21 @@ void LayoutTableSection::LayoutRows() {
     if (LayoutTableRow* row = grid_[r].row) {
       row->SetLogicalLocation(LayoutPoint(0, row_pos_[r]));
       row->SetLogicalWidth(LogicalWidth());
-      LayoutUnit row_logical_height(row_pos_[r + 1] - row_pos_[r] - vspacing);
+      LayoutUnit row_logical_height;
+      // If the row is collapsed then it has 0 height. vspacing was implicitly
+      // removed earlier, when row_pos_[r+1] was set to row_pos[r].
+      if (!RowHasVisibilityCollapse(r)) {
+        row_logical_height =
+            LayoutUnit(row_pos_[r + 1] - row_pos_[r] - vspacing);
+      }
+      DCHECK_GE(row_logical_height, 0);
       if (state.IsPaginated() && r + 1 < total_rows) {
         // If the next row has a pagination strut, we need to subtract it. It
         // should not be included in this row's height.
         if (LayoutTableRow* next_row_object = grid_[r + 1].row)
           row_logical_height -= next_row_object->PaginationStrut();
       }
+      DCHECK_GE(row_logical_height, 0);
       row->SetLogicalHeight(row_logical_height);
       row->UpdateAfterLayout();
     }

third_party/blink/renderer/core/layout/layout_table_section_test.cc

@@ -383,6 +383,22 @@ TEST_F(LayoutTableSectionTest, OverflowingCells) {
   EXPECT_EQ(big_section->FullTableEffectiveColumnSpan(), columns);
 }
 
+TEST_F(LayoutTableSectionTest, RowCollapseNegativeHeightCrash) {
+  // Table % height triggers the heuristic check for relayout of cells at
+  // https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/layout/layout_table_section.cc?rcl=5ea6fa63d8809f990d662182d971facbf557f812&l=1899
+  // Cell child needs a % height to set cell_children_flex at line 1907, which
+  // caused a negative override height to get set at 1929, which DCHECKed.
+  SetBodyInnerHTML(R"HTML(
+    <table style="height:50%">
+      <tr style="visibility:collapse">
+        <td>
+          <div style="height:50%"></div>
+        </td>
+      </tr>
+    </table>
+  )HTML");
+}
+
 }  // anonymous namespace
 
 }  // namespace blink