[css-grid] Fix crash when using auto repeat for indefinite widths

third_party/WebKit/Source/core/layout/LayoutGrid.cpp

 /*
  * Copyright (C) 2011 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
  *    notice, this list of conditions and the following disclaimer in the
@@ skipping 425 matching lines @@
         // may trigger relayout).
         LayoutState state(*this, locationOffset());
 
         LayoutSize previousSize = size();
 
         updateLogicalWidth();
         bool logicalHeightWasIndefinite = computeContentLogicalHeight(MainOrPreferredSize, style()->logicalHeight(), LayoutUnit(-1)) == LayoutUnit(-1);
 
         TextAutosizer::LayoutScope textAutosizerLayoutScope(this, &layoutScope);
 
-        placeItemsOnGrid();
+        placeItemsOnGrid(TrackSizing);
 
         GridSizingData sizingData(gridColumnCount(), gridRowCount());
 
         // 1- First, the track sizing algorithm is used to resolve the sizes of the grid columns.
         // At this point the logical width is always definite as the above call to updateLogicalWidth()
         // properly resolves intrinsic sizes. We cannot do the same for heights though because many code
         // paths inside updateLogicalHeight() require a previous call to setLogicalHeight() to resolve
         // heights properly (like for positioned items for example).
         LayoutUnit availableSpaceForColumns = availableLogicalWidth();
         computeTrackSizesForDirection(ForColumns, sizingData, availableSpaceForColumns);
@@ skipping 47 matching lines @@
 {
     if (span <= 1)
         return LayoutUnit();
 
     const Length& trackGap = direction == ForColumns ? styleRef().gridColumnGap() : styleRef().gridRowGap();
     return valueForLength(trackGap, LayoutUnit()) * (span - 1);
 }
 
 void LayoutGrid::computeIntrinsicLogicalWidths(LayoutUnit& minLogicalWidth, LayoutUnit& maxLogicalWidth) const
 {
-    const_cast<LayoutGrid*>(this)->placeItemsOnGrid();
+    // TODO(svillar): check the comment in clearGridDataAndMarkAsDirty() to remove these two const_cast.
+    const_cast<LayoutGrid*>(this)->placeItemsOnGrid(IntrinsicSizeComputation);
 
     GridSizingData sizingData(gridColumnCount(), gridRowCount());
     sizingData.freeSpaceForDirection(ForColumns) = LayoutUnit();
     sizingData.sizingOperation = IntrinsicSizeComputation;
     const_cast<LayoutGrid*>(this)->computeUsedBreadthOfGridTracks(ForColumns, sizingData, minLogicalWidth, maxLogicalWidth);
 
     LayoutUnit totalGuttersSize = guttersSize(ForColumns, sizingData.columnTracks.size());
     minLogicalWidth += totalGuttersSize;
     maxLogicalWidth += totalGuttersSize;
 
     LayoutUnit scrollbarWidth = LayoutUnit(scrollbarLogicalWidth());
     minLogicalWidth += scrollbarWidth;
     maxLogicalWidth += scrollbarWidth;
+
+    // Mark the grid as dirty as we need to recompute the auto repeat columns during layout.
+    if (m_autoRepeatColumns)
+        clearGridDataAndMarkAsDirty();
 }
 
 void LayoutGrid::computeIntrinsicLogicalHeight(GridSizingData& sizingData)
 {
     ASSERT(tracksAreWiderThanMinTrackBreadth(ForColumns, sizingData));
     sizingData.freeSpaceForDirection(ForRows) = LayoutUnit();
     sizingData.sizingOperation = IntrinsicSizeComputation;
     computeUsedBreadthOfGridTracks(ForRows, sizingData, m_minContentHeight, m_maxContentHeight);
 
     LayoutUnit totalGuttersSize = guttersSize(ForRows, gridRowCount());
@@ skipping 839 matching lines @@
 
     // Provided the grid container does not have a definite size or max-size in the relevant axis,
     // if the min size is definite then the number of repetitions is the largest possible positive
     // integer that fulfills that minimum requirement.
     if (needsToFulfillMinimumSize)
         ++repetitions;
 
     return repetitions;
 }
 
-void LayoutGrid::placeItemsOnGrid()
+void LayoutGrid::placeItemsOnGrid(SizingOperation inlineAxisSizingOperation)

[jfernandez, 2016/07/11 12:52:22]

Why we need to know whether is inline or block axis ? And if we do need, I don't
think just a variable name is enough.  As far as I understand the code below, we
are processing logic for both axis, so I don't think there is any relation
between axis and the SizingOperation value.

[svillar, 2016/07/11 14:18:43]

Well I tried to make a bit explicit that the sizing operation would be just used
for inline axis, but you're right that the sizing operation has nothing to do
with axis. I'll rename it.

 {
     if (!m_gridIsDirty)
         return;
 
     ASSERT(m_gridItemArea.isEmpty());
 
-    m_autoRepeatColumns = computeAutoRepeatTracksCount(ForColumns);
+    if (inlineAxisSizingOperation == IntrinsicSizeComputation)
+        m_autoRepeatColumns = styleRef().gridAutoRepeatColumns().isEmpty() ? 0 : 1;

[jfernandez, 2016/07/11 12:52:22]

Why 1 ? Hadn't we avoid that kind of harcoded 1 values in other cases ?

[svillar, 2016/07/11 14:18:43]

Because that's what the spec says
(https://drafts.csswg.org/css-grid/#auto-repeat). Basically if you don't have
definite sizes then you must default to 1 repetition.

+    else
+        m_autoRepeatColumns = computeAutoRepeatTracksCount(ForColumns);
     m_autoRepeatRows = computeAutoRepeatTracksCount(ForRows);
 
     populateExplicitGridAndOrderIterator();
 
     // We clear the dirty bit here as the grid sizes have been updated.
     m_gridIsDirty = false;
 
     Vector<LayoutBox*> autoMajorAxisAutoGridItems;
     Vector<LayoutBox*> specifiedMajorAxisAutoGridItems;
     m_hasAnyOrthogonalChild = false;
@@ skipping 203 matching lines @@
 GridTrackSizingDirection LayoutGrid::autoPlacementMajorAxisDirection() const
 {
     return style()->isGridAutoFlowDirectionColumn() ? ForColumns : ForRows;
 }
 
 GridTrackSizingDirection LayoutGrid::autoPlacementMinorAxisDirection() const
 {
     return style()->isGridAutoFlowDirectionColumn() ? ForRows : ForColumns;
 }
 
+void LayoutGrid::clearGridDataAndMarkAsDirty() const
+{

[Manuel Rego, 2016/07/11 11:10:57]

Maybe add a similar if to the one in dirtyGrid():
  if (m_gridIsDirty)
    return;

[svillar, 2016/07/11 11:48:00]

I think I'm adding an ASSERT there.

+    // TODO(svillar): The track sizing algorithm is used for preferredLogicalWidths
+    // computations. This means that some attributes of this class will be modified during that
+    // process. We should make the algorithm work against some interface that would allow us not to
+    // have to change these attributes while computing the intrinsic sizes.
+    m_grid.resize(0);
+    m_gridItemArea.clear();
+    m_gridItemsOverflowingGridArea.resize(0);
+    m_gridItemsIndexesMap.clear();
+    m_autoRepeatColumns = 0;
+    m_autoRepeatRows = 0;
+    m_gridIsDirty = true;
+}
+
 void LayoutGrid::dirtyGrid()

[Manuel Rego, 2016/07/11 11:10:57]

Shouldn't we rename this method?

We've now:
* dirtyGrid()
vs
* clearGridDataAndMarkAsDirty()

Hard to know which one we should call.

[svillar, 2016/07/11 11:48:00]

The reason why I added a new method is because dirtyGrid() does one extra thing
which is to flip the needsLayout flag. I cannot do that during the intrinsic
size computation as is forbidden (there are asserts to test that).

 {
     if (m_gridIsDirty)
         return;
 
     // Even if this could be redundant, it could be seen as a defensive strategy against
     // style changes events happening during the layout phase or even while the painting process
     // is still ongoing.
     // Forcing a new layout for the Grid layout would cancel any ongoing painting and ensure
     // the grid and its children are correctly laid out according to the new style rules.
     setNeedsLayout(LayoutInvalidationReason::GridChanged);
 
-    m_grid.resize(0);
-    m_gridItemArea.clear();
-    m_gridItemsOverflowingGridArea.resize(0);
-    m_gridItemsIndexesMap.clear();
-    m_gridIsDirty = true;
+    clearGridDataAndMarkAsDirty();
 }
 
 static const StyleContentAlignmentData& normalValueBehavior()
 {
     static const StyleContentAlignmentData normalBehavior = {ContentPositionNormal, ContentDistributionStretch};
     return normalBehavior;
 }
 
 void LayoutGrid::applyStretchAlignmentToTracksIfNeeded(GridTrackSizingDirection direction, GridSizingData& sizingData)
 {
@@ skipping 718 matching lines @@
     return isOrthogonalChild(child) ? childLocation.transposedPoint() : childLocation;
 }
 
 void LayoutGrid::paintChildren(const PaintInfo& paintInfo, const LayoutPoint& paintOffset) const
 {
     if (!m_gridItemArea.isEmpty())
         GridPainter(*this).paintChildren(paintInfo, paintOffset);
 }
 
 } // namespace blink