Changeset 277297 in webkit

Timestamp:

May 10, 2021, 3:01:57 PM (5 years ago)

Author:

commit-queue@webkit.org

Message:

Unreviewed, reverting r277222.
​https://bugs.webkit.org/show_bug.cgi?id=225618

WebContent process crashes while visiting
<​http://ign.com|ign.com> (RenderFlexibleBox::layoutFlexItems)

Reverted changeset:

"[css-flexbox] Flex item construction may affect sibling flex
item height computation"
​https://bugs.webkit.org/show_bug.cgi?id=225489
​https://trac.webkit.org/changeset/277222

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/fast/flexbox/flex-column-with-percent-height-descendants-expected.html (deleted)
• LayoutTests/fast/flexbox/flex-column-with-percent-height-descendants.html (deleted)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/rendering/RenderFlexibleBox.cpp (modified) (5 diffs)
• Source/WebCore/rendering/RenderFlexibleBox.h (modified) (2 diffs)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2021-05-10  Commit Queue  <commit-queue@webkit.org>
+
+        Unreviewed, reverting r277222.
+        https://bugs.webkit.org/show_bug.cgi?id=225618
+
+        WebContent process crashes while visiting
+        <http://ign.com|ign.com> (RenderFlexibleBox::layoutFlexItems)
+
+        Reverted changeset:
+
+        "[css-flexbox] Flex item construction may affect sibling flex
+        item height computation"
+        https://bugs.webkit.org/show_bug.cgi?id=225489
+        https://trac.webkit.org/changeset/277222
+
 2021-05-10  Andres Gonzalez  <andresg_22@apple.com>
 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2021-05-10  Commit Queue  <commit-queue@webkit.org>
+
+        Unreviewed, reverting r277222.
+        https://bugs.webkit.org/show_bug.cgi?id=225618
+
+        WebContent process crashes while visiting
+        <http://ign.com|ign.com> (RenderFlexibleBox::layoutFlexItems)
+
+        Reverted changeset:
+
+        "[css-flexbox] Flex item construction may affect sibling flex
+        item height computation"
+        https://bugs.webkit.org/show_bug.cgi?id=225489
+        https://trac.webkit.org/changeset/277222
+
 2021-05-10  Wenson Hsieh  <wenson_hsieh@apple.com>
 

trunk/Source/WebCore/rendering/RenderFlexibleBox.cpp

@@ -269 +269 @@
     m_relaidOutChildren.clear();
     
+    bool oldInLayout = m_inLayout;
+    m_inLayout = true;
+    
     if (recomputeLogicalWidth())
         relayoutChildren = true;
@@ -319 +322 @@
 
     clearNeedsLayout();
+    
+    m_inLayout = oldInLayout;
 }
 
@@ -841 +846 @@
         return false;
     bool definite = child.computePercentageLogicalHeight(flexBasis, updateDescendants).hasValue();
-    if (!m_inFlexItemConstruction && (isHorizontalWritingMode() == child.isHorizontalWritingMode())) {
+    if (m_inLayout && (isHorizontalWritingMode() == child.isHorizontalWritingMode())) {
         // We can reach this code even while we're not laying ourselves out, such
         // as from mainSizeForPercentageResolution.
@@ -961 +966 @@
     // should work off this list of a subset.
     // TODO(cbiesinger): That second part is not yet true.
-    auto allItems = constructFlexItems(relayoutChildren);
+    Vector<FlexItem> allItems;
+    m_orderIterator.first();
+    for (RenderBox* child = m_orderIterator.currentChild(); child; child = m_orderIterator.next()) {
+        if (m_orderIterator.shouldSkipChild(*child)) {
+            // Out-of-flow children are not flex items, so we skip them here.
+            if (child->isOutOfFlowPositioned())
+                prepareChildForPositionedLayout(*child);
+            continue;
+        }
+        allItems.append(constructFlexItem(*child, relayoutChildren));
+    }
+
+    // constructFlexItem() might set the override containing block height so any value cached for definiteness might be incorrect.
+    m_hasDefiniteHeight = SizeDefiniteness::Unknown;
+    
     const LayoutUnit lineBreakLength = mainAxisContentExtent(LayoutUnit::max());
     LayoutUnit gapBetweenItems = computeGap(GapType::BetweenItems);
@@ -1276 +1295 @@
 }
 
-Vector<FlexItem> RenderFlexibleBox::constructFlexItems(bool relayoutChildren)
-{
-    SetForScope<bool> inFlexItemConstruction(m_inFlexItemConstruction, true);
-
-    Vector<FlexItem> flexItems;
-    for (auto* child = m_orderIterator.first(); child; child = m_orderIterator.next()) {
-        if (m_orderIterator.shouldSkipChild(*child)) {
-            // Out-of-flow children are not flex items, so we skip them here.
-            if (child->isOutOfFlowPositioned())
-                prepareChildForPositionedLayout(*child);
-            continue;
-        }
-
-        auto constructFlexItemForChildBox = [&](auto& childBox) {
-            childBox.clearOverridingContentSize();
-            if (childHasIntrinsicMainAxisSize(childBox)) {
-                // If this condition is true, then computeMainAxisExtentForChild will call
-                // child.intrinsicContentLogicalHeight() and child.scrollbarLogicalHeight(),
-                // so if the child has intrinsic min/max/preferred size, run layout on it now to make sure
-                // its logical height and scroll bars are up to date.
-                updateBlockChildDirtyBitsBeforeLayout(relayoutChildren, childBox);
-                // Don't resolve percentages in children. This is especially important for the min-height calculation,
-                // where we want percentages to be treated as auto. For flex-basis itself, this is not a problem because
-                // by definition we have an indefinite flex basis here and thus percentages should not resolve.
-                if (childBox.needsLayout() || !m_intrinsicSizeAlongMainAxis.contains(&childBox)) {
-                    if (isHorizontalWritingMode() == childBox.isHorizontalWritingMode())
-                        childBox.setOverridingContainingBlockContentLogicalHeight(WTF::nullopt);
-                    else
-                        childBox.setOverridingContainingBlockContentLogicalWidth(WTF::nullopt);
-                    childBox.clearOverridingContentSize();
-                    childBox.setChildNeedsLayout(MarkOnlyThis);
-                    childBox.layoutIfNeeded();
-                    cacheChildMainSize(childBox);
-                    childBox.clearOverridingContainingBlockContentSize();
-                }
-            }
-
-            auto borderAndPadding = isHorizontalFlow() ? childBox.horizontalBorderAndPaddingExtent() : childBox.verticalBorderAndPaddingExtent();
-            auto childInnerFlexBaseSize = computeInnerFlexBaseSizeForChild(childBox, borderAndPadding);
-            auto childMinMaxAppliedMainAxisExtent = adjustChildSizeForMinAndMax(childBox, childInnerFlexBaseSize);
-            auto margin = isHorizontalFlow() ? childBox.horizontalMarginExtent() : childBox.verticalMarginExtent();
-            return FlexItem(childBox, childInnerFlexBaseSize, childMinMaxAppliedMainAxisExtent, borderAndPadding, margin);
-        };
-        flexItems.append(constructFlexItemForChildBox(*child));
-    }
-    return flexItems;
+FlexItem RenderFlexibleBox::constructFlexItem(RenderBox& child, bool relayoutChildren)
+{
+    child.clearOverridingContentSize();
+    if (childHasIntrinsicMainAxisSize(child)) {
+        // If this condition is true, then computeMainAxisExtentForChild will call
+        // child.intrinsicContentLogicalHeight() and child.scrollbarLogicalHeight(),
+        // so if the child has intrinsic min/max/preferred size, run layout on it now to make sure
+        // its logical height and scroll bars are up to date.
+        updateBlockChildDirtyBitsBeforeLayout(relayoutChildren, child);
+        // Don't resolve percentages in children. This is especially important for the min-height calculation,
+        // where we want percentages to be treated as auto. For flex-basis itself, this is not a problem because
+        // by definition we have an indefinite flex basis here and thus percentages should not resolve.
+        if (child.needsLayout() || !m_intrinsicSizeAlongMainAxis.contains(&child)) {
+            if (isHorizontalWritingMode() == child.isHorizontalWritingMode())
+                child.setOverridingContainingBlockContentLogicalHeight(WTF::nullopt);
+            else
+                child.setOverridingContainingBlockContentLogicalWidth(WTF::nullopt);
+            child.clearOverridingContentSize();
+            child.setChildNeedsLayout(MarkOnlyThis);
+            child.layoutIfNeeded();
+            cacheChildMainSize(child);
+            child.clearOverridingContainingBlockContentSize();
+        }
+    }
+    
+    LayoutUnit borderAndPadding = isHorizontalFlow() ? child.horizontalBorderAndPaddingExtent() : child.verticalBorderAndPaddingExtent();
+    LayoutUnit childInnerFlexBaseSize = computeInnerFlexBaseSizeForChild(child, borderAndPadding);
+    LayoutUnit childMinMaxAppliedMainAxisExtent = adjustChildSizeForMinAndMax(child, childInnerFlexBaseSize);
+    LayoutUnit margin = isHorizontalFlow() ? child.horizontalMarginExtent() : child.verticalMarginExtent();
+    return FlexItem(child, childInnerFlexBaseSize, childMinMaxAppliedMainAxisExtent, borderAndPadding, margin);
 }
     

trunk/Source/WebCore/rendering/RenderFlexibleBox.h

@@ -176 +176 @@
     LayoutUnit adjustChildSizeForMinAndMax(RenderBox& child, LayoutUnit childSize);
     LayoutUnit adjustChildSizeForAspectRatioCrossAxisMinAndMax(const RenderBox& child, LayoutUnit childSize);
-    Vector<FlexItem> constructFlexItems(bool relayoutChildren);
+    FlexItem constructFlexItem(RenderBox&, bool relayoutChildren);
     
     void freezeInflexibleItems(FlexSign, Vector<FlexItem>& children, LayoutUnit& remainingFreeSpace, double& totalFlexGrow, double& totalFlexShrink, double& totalWeightedFlexShrink);
@@ -221 +221 @@
     // This is SizeIsUnknown outside of layoutBlock()
     SizeDefiniteness m_hasDefiniteHeight { SizeDefiniteness::Unknown };
-    bool m_inFlexItemConstruction { false };
+    bool m_inLayout { false };
     bool m_shouldResetChildLogicalHeightBeforeLayout { false };
 };