Unreviewed, rolling out r167879 and r167942.
authorrego@igalia.com <rego@igalia.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 1 May 2014 00:12:59 +0000 (00:12 +0000)
committerrego@igalia.com <rego@igalia.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 1 May 2014 00:12:59 +0000 (00:12 +0000)
https://bugs.webkit.org/show_bug.cgi?id=132408

OrderIterator changes caused regressions in flexbox (Requested
by rego on #webkit).

We're keeping the new layout test introduced in r167942
(fast/flexbox/order-iterator-crash.html) to avoid similar
regressions in the future.

Reverted changesets:

"OrderIterator refactoring to avoid extra loops"
https://bugs.webkit.org/show_bug.cgi?id=119061
http://trac.webkit.org/changeset/167879

"REGRESSION (r167879): Heap-use-after-free in
WebCore::RenderFlexibleBox"
https://bugs.webkit.org/show_bug.cgi?id=132337
http://trac.webkit.org/changeset/167942

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@168067 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog
Source/WebCore/rendering/OrderIterator.cpp
Source/WebCore/rendering/OrderIterator.h
Source/WebCore/rendering/RenderFlexibleBox.cpp
Source/WebCore/rendering/RenderFlexibleBox.h
Source/WebCore/rendering/RenderGrid.cpp
Source/WebCore/rendering/RenderGrid.h

index c077d57..6c08e34 100644 (file)
@@ -1,3 +1,26 @@
+2014-04-30  Manuel Rego Casasnovas  <rego@igalia.com>
+
+        Unreviewed, rolling out r167879 and r167942.
+        https://bugs.webkit.org/show_bug.cgi?id=132408
+
+        OrderIterator changes caused regressions in flexbox (Requested
+        by rego on #webkit).
+
+        We're keeping the new layout test introduced in r167942
+        (fast/flexbox/order-iterator-crash.html) to avoid similar
+        regressions in the future.
+
+        Reverted changesets:
+
+        "OrderIterator refactoring to avoid extra loops"
+        https://bugs.webkit.org/show_bug.cgi?id=119061
+        http://trac.webkit.org/changeset/167879
+
+        "REGRESSION (r167879): Heap-use-after-free in
+        WebCore::RenderFlexibleBox"
+        https://bugs.webkit.org/show_bug.cgi?id=132337
+        http://trac.webkit.org/changeset/167942
+
 2014-04-30  Enrica Casucci  <enrica@apple.com>
 
         Cursor gets thinner on empty lines.
index 7630136..e09924f 100644 (file)
 #include "config.h"
 #include "OrderIterator.h"
 
-#include "RenderBox.h"
+#include "RenderFlexibleBox.h"
+#include "RenderGrid.h"
 
 namespace WebCore {
 
-RenderBox* OrderIterator::currentChild() const
+static const int cInvalidIndex = -1;
+
+OrderIterator::OrderIterator(RenderBox& containerBox)
+    : m_containerBox(containerBox)
+{
+    reset();
+}
+
+void OrderIterator::setOrderValues(OrderValues&& orderValues)
 {
-    if (m_childrenIndex == m_children.size())
-        return nullptr;
+    reset();
+    m_orderValues = std::move(orderValues);
+    if (m_orderValues.size() < 2)
+        return;
 
-    return m_children[m_childrenIndex].first;
+    std::sort(m_orderValues.begin(), m_orderValues.end());
+    auto nextElement = std::unique(m_orderValues.begin(), m_orderValues.end());
+    m_orderValues.shrinkCapacity(nextElement - m_orderValues.begin());
 }
 
 RenderBox* OrderIterator::first()
 {
-    m_childrenIndex = 0;
-    return currentChild();
+    reset();
+    return next();
 }
 
 RenderBox* OrderIterator::next()
 {
-    ++m_childrenIndex;
-    return currentChild();
-}
+    int endIndex = m_orderValues.size();
+    do {
+        if (m_currentChild) {
+            m_currentChild = m_currentChild->nextSiblingBox();
+            continue;
+        }
 
-void OrderIterator::invalidate()
-{
-    m_children.clear();
-}
+        if (m_orderIndex == endIndex)
+            return nullptr;
 
-static bool compareByOrderValueAndIndex(std::pair<RenderBox*, int> childAndIndex1, std::pair<RenderBox*, int> childAndIndex2)
-{
-    if (childAndIndex1.first->style().order() != childAndIndex2.first->style().order())
-        return childAndIndex1.first->style().order() < childAndIndex2.first->style().order();
-    return childAndIndex1.second < childAndIndex2.second;
-}
+        if (m_orderIndex != cInvalidIndex) {
+            ++m_orderIndex;
+            if (m_orderIndex == endIndex)
+                return nullptr;
+        } else
+            m_orderIndex = 0;
 
-OrderIteratorPopulator::~OrderIteratorPopulator()
-{
-    if (!m_allChildrenHaveDefaultOrderValue)
-        std::sort(m_iterator.m_children.begin(), m_iterator.m_children.end(), compareByOrderValueAndIndex);
+        m_currentChild = m_containerBox.firstChildBox();
+    } while (!m_currentChild || m_currentChild->style().order() != m_orderValues[m_orderIndex]);
+
+    return m_currentChild;
 }
 
-void OrderIteratorPopulator::collectChild(RenderBox& child)
+void OrderIterator::reset()
 {
-    std::pair<RenderBox*, int> childAndIndex = { &child, m_childIndex++ };
-    m_iterator.m_children.append(childAndIndex);
-
-    if (m_allChildrenHaveDefaultOrderValue && child.style().order())
-        m_allChildrenHaveDefaultOrderValue = false;
+    m_currentChild = nullptr;
+    m_orderIndex = cInvalidIndex;
 }
 
-
 } // namespace WebCore
index 27f89bf..e47c850 100644 (file)
@@ -41,39 +41,22 @@ class RenderBox;
 
 class OrderIterator {
 public:
-    friend class OrderIteratorPopulator;
+    OrderIterator(RenderBox&);
 
-    RenderBox* currentChild() const;
+    typedef Vector<int, 1> OrderValues;
+    void setOrderValues(OrderValues&&);
+
+    RenderBox* currentChild() const { return m_currentChild; }
     RenderBox* first();
     RenderBox* next();
 
-    void invalidate();
-
 private:
     void reset();
 
-    Vector<std::pair<RenderBox*, int>> m_children;
-    size_t m_childrenIndex;
-};
-
-class OrderIteratorPopulator {
-public:
-    OrderIteratorPopulator(OrderIterator& iterator)
-        : m_iterator(iterator)
-        , m_childIndex(0)
-        , m_allChildrenHaveDefaultOrderValue(true)
-    {
-        m_iterator.invalidate();
-    }
-
-    ~OrderIteratorPopulator();
-
-    void collectChild(RenderBox&);
-
-private:
-    OrderIterator& m_iterator;
-    size_t m_childIndex;
-    bool m_allChildrenHaveDefaultOrderValue;
+    RenderBox& m_containerBox;
+    RenderBox* m_currentChild;
+    OrderValues m_orderValues;
+    int m_orderIndex;
 };
 
 } // namespace WebCore
index fb41e8c..48521a8 100644 (file)
@@ -68,6 +68,7 @@ struct RenderFlexibleBox::Violation {
 
 RenderFlexibleBox::RenderFlexibleBox(Element& element, PassRef<RenderStyle> style)
     : RenderBlock(element, std::move(style), 0)
+    , m_orderIterator(*this)
     , m_numberOfInFlowChildrenOnFirstLine(-1)
 {
     setChildrenInline(false); // All of our children must be block-level.
@@ -75,6 +76,7 @@ RenderFlexibleBox::RenderFlexibleBox(Element& element, PassRef<RenderStyle> styl
 
 RenderFlexibleBox::RenderFlexibleBox(Document& document, PassRef<RenderStyle> style)
     : RenderBlock(document, std::move(style), 0)
+    , m_orderIterator(*this)
     , m_numberOfInFlowChildrenOnFirstLine(-1)
 {
     setChildrenInline(false); // All of our children must be block-level.
@@ -273,11 +275,13 @@ void RenderFlexibleBox::layoutBlock(bool relayoutChildren, LayoutUnit)
 
     dirtyForLayoutFromPercentageHeightDescendants();
 
-    prepareOrderIteratorAndMargins();
+    Vector<LineContext> lineContexts;
+    OrderIterator::OrderValues orderValues;
+    computeMainAxisPreferredSizes(orderValues);
+    m_orderIterator.setOrderValues(std::move(orderValues));
 
     ChildFrameRects oldChildRects;
     appendChildFrameRects(oldChildRects);
-    Vector<LineContext> lineContexts;
     layoutFlexItems(relayoutChildren, lineContexts);
 
     updateLogicalHeight();
@@ -830,12 +834,16 @@ LayoutUnit RenderFlexibleBox::computeChildMarginValue(const Length& margin)
     return minimumValueForLength(margin, availableSize);
 }
 
-void RenderFlexibleBox::prepareOrderIteratorAndMargins()
+void RenderFlexibleBox::computeMainAxisPreferredSizes(OrderIterator::OrderValues& orderValues)
 {
-    OrderIteratorPopulator populator(m_orderIterator);
+    ASSERT(orderValues.isEmpty());
 
     for (RenderBox* child = firstChildBox(); child; child = child->nextSiblingBox()) {
-        populator.collectChild(*child);
+        // Avoid growing the vector for the common-case default value of 0. This optimizes the most common case which is
+        // one or a few values with the default order 0
+        int order = child->style().order();
+        if (orderValues.isEmpty() || orderValues.last() != order)
+            orderValues.append(order);
 
         if (child->isOutOfFlowPositioned())
             continue;
@@ -1392,10 +1400,4 @@ bool RenderFlexibleBox::isLeftLayoutOverflowAllowed() const
     return isHorizontalFlow();
 }
 
-void RenderFlexibleBox::removeChild(RenderObject& child)
-{
-    RenderBlock::removeChild(child);
-    m_orderIterator.invalidate();
-}
-
 }
index a3594f6..91b547a 100644 (file)
@@ -60,8 +60,6 @@ public:
     bool isTopLayoutOverflowAllowed() const override;
     bool isLeftLayoutOverflowAllowed() const override;
 
-    void removeChild(RenderObject&) override;
-
 protected:
     virtual void computeIntrinsicLogicalWidths(LayoutUnit& minLogicalWidth, LayoutUnit& maxLogicalWidth) const override;
     virtual void computePreferredLogicalWidths() override;
@@ -139,7 +137,7 @@ private:
     LayoutUnit marginBoxAscentForChild(RenderBox&);
 
     LayoutUnit computeChildMarginValue(const Length& margin);
-    void prepareOrderIteratorAndMargins();
+    void computeMainAxisPreferredSizes(OrderIterator::OrderValues&);
     LayoutUnit adjustChildSizeForMinAndMax(RenderBox&, LayoutUnit childSize);
     bool computeNextFlexLine(OrderedFlexItemList& orderedChildren, LayoutUnit& preferredMainAxisExtent, double& totalFlexGrow, double& totalWeightedFlexShrink, LayoutUnit& minMaxAppliedMainAxisExtent, bool& hasInfiniteLineLength);
 
index 9c17b09..ad19103 100644 (file)
@@ -164,6 +164,7 @@ public:
 
 RenderGrid::RenderGrid(Element& element, PassRef<RenderStyle> style)
     : RenderBlock(element, std::move(style), 0)
+    , m_orderIterator(*this)
 {
     // All of our children must be block level.
     setChildrenInline(false);
@@ -698,12 +699,17 @@ void RenderGrid::placeItemsOnGrid()
 
 void RenderGrid::populateExplicitGridAndOrderIterator()
 {
-    OrderIteratorPopulator populator(m_orderIterator);
+    // FIXME: We should find a way to share OrderValues's initialization code with RenderFlexibleBox.
+    OrderIterator::OrderValues orderValues;
     size_t maximumRowIndex = std::max<size_t>(1, explicitGridRowCount());
     size_t maximumColumnIndex = std::max<size_t>(1, explicitGridColumnCount());
 
     for (RenderBox* child = firstChildBox(); child; child = child->nextSiblingBox()) {
-        populator.collectChild(*child);
+        // Avoid growing the vector for the common-case default value of 0. This optimizes the most common case which is
+        // one or a few values with the default order 0
+        int order = child->style().order();
+        if (orderValues.isEmpty() || orderValues.last() != order)
+            orderValues.append(order);
 
         // This function bypasses the cache (cachedGridCoordinate()) as it is used to build it.
         std::unique_ptr<GridSpan> rowPositions = resolveGridPositionsFromStyle(child, ForRows);
@@ -720,6 +726,8 @@ void RenderGrid::populateExplicitGridAndOrderIterator()
     m_grid.grow(maximumRowIndex);
     for (size_t i = 0; i < m_grid.size(); ++i)
         m_grid[i].grow(maximumColumnIndex);
+
+    m_orderIterator.setOrderValues(std::move(orderValues));
 }
 
 void RenderGrid::placeSpecifiedMajorAxisItemsOnGrid(const Vector<RenderBox*>& autoGridItems)
@@ -1186,12 +1194,6 @@ const char* RenderGrid::renderName() const
     return "RenderGrid";
 }
 
-void RenderGrid::removeChild(RenderObject& child)
-{
-    RenderBlock::removeChild(child);
-    m_orderIterator.invalidate();
-}
-
 } // namespace WebCore
 
 #endif /* ENABLE(CSS_GRID_LAYOUT) */
index d1a3e8f..8184ecf 100644 (file)
@@ -58,8 +58,6 @@ public:
     const Vector<LayoutUnit>& columnPositions() const { return m_columnPositions; }
     const Vector<LayoutUnit>& rowPositions() const { return m_rowPositions; }
 
-    void removeChild(RenderObject&) override;
-
 private:
     virtual const char* renderName() const override;
     virtual bool isRenderGrid() const override { return true; }