ASSERTION FAILED in WebCore::RenderFlowThread::getRegionRangeForBox

https://bugs.webkit.org/show_bug.cgi?id=135563

Reviewed by David Hyatt.

Source/WebCore:

The new multi-column code doesn't work correctly when the document contains nested fragmentation
contexts. The problem is the current flow thread concept that can store only one RenderFlowThread
at a time and use it during layout.

The stored flow thread is always correct for regions because named flow threads are absolutley positioned
so every child renderer is contained inside them (with the expcetion of fixed positioned elements which are
treated separately).

For multi-column elements this is no longer the case. An absolutely positioned element inside a static
multi-column element will be contained by a block outside the fragmentation context. It can even be
contained by a different multi-column element in the case of nested flow threads.

The patch below explores a solution that's not based on a current flow thread stored globally. The proposed
patch makes every block to store a pointer to its fragmentation context and a flag that states if this pointer
needs to be updated or not. If the renderer is not a block it will get its flow thread from the containing
block. Once the containing flow thread is requested for the block, the pointer is computed and cached until
invalidated:
- when a subtree is removed from a flow thread
- when the position property of an element inside a flow thread changes

The process is recursive and it doesn't affect elements that are not nested inside a flow thread. If a block
changes position from relative to static, any element that was contained by it can only be contained by an
ancestor of the block. This ancestor will still be outside of any flow thread. This ensures that non-fragmentation
code is not affected from a performance perspective.

The patch affects the results of the performance tests:
- the regions layout tests have a decreased performance raging from 2% to 5-6%
- the regions selection tests have an increased performance raging from 1-2% to 10%
- the multicolumn layout tests (now pending review in b137687) have an increased performance
raging from 1.8% to 5%

Tests: fast/multicol/multicol-all-positioned-crash.html
       fast/multicol/multicol-transform-containing-block.html

* rendering/FlowThreadController.cpp:
(WebCore::FlowThreadController::FlowThreadController):
* rendering/FlowThreadController.h:
(WebCore::FlowThreadController::currentRenderFlowThread): Deleted.
(WebCore::FlowThreadController::setCurrentRenderFlowThread): Deleted.
* rendering/LayoutState.h:
(WebCore::LayoutState::currentRenderFlowThread):
(WebCore::LayoutState::setCurrentRenderFlowThread):
* rendering/RenderBlock.cpp:
(WebCore::RenderBlockRareData::RenderBlockRareData):
(WebCore::RenderBlock::styleWillChange):
(WebCore::RenderBlock::styleDidChange):
(WebCore::RenderBlock::collapseAnonymousBoxChild):
(WebCore::RenderBlock::cachedFlowThreadContainingBlock):
(WebCore::RenderBlock::cachedFlowThreadContainingBlockNeedsUpdate):
(WebCore::RenderBlock::setCachedFlowThreadContainingBlockNeedsUpdate):
(WebCore::RenderBlock::updateCachedFlowThreadContainingBlock):
(WebCore::RenderBlock::locateFlowThreadContainingBlock):
* rendering/RenderBlock.h:
* rendering/RenderBlockFlow.cpp:
(WebCore::RenderBlockFlow::styleWillChange):
* rendering/RenderBox.cpp:
(WebCore::RenderBox::borderBoxRectInRegion):
* rendering/RenderFlowThread.cpp:
(WebCore::RenderFlowThread::layout):
(WebCore::RenderFlowThread::updateAllLayerToRegionMappings):
(WebCore::RenderFlowThread::repaintRectangleInRegions):
(WebCore::CurrentRenderFlowThreadMaintainer::CurrentRenderFlowThreadMaintainer): Deleted.
(WebCore::CurrentRenderFlowThreadMaintainer::~CurrentRenderFlowThreadMaintainer): Deleted.
(WebCore::CurrentRenderFlowThreadDisabler::CurrentRenderFlowThreadDisabler): Deleted.
(WebCore::CurrentRenderFlowThreadDisabler::~CurrentRenderFlowThreadDisabler): Deleted.
* rendering/RenderFlowThread.h:
* rendering/RenderLayer.cpp:
(WebCore::RenderLayer::paintLayer):
(WebCore::RenderLayer::hitTestLayer):
(WebCore::RenderLayer::mapLayerClipRectsToFragmentationLayer):
(WebCore::RenderLayer::calculateClipRects):
* rendering/RenderObject.cpp:
(WebCore::RenderObject::showRegionsInformation):
(WebCore::RenderObject::insertedIntoTree):
(WebCore::RenderObject::removeFromRenderFlowThread):
(WebCore::RenderObject::removeFromRenderFlowThreadIncludingDescendants):
(WebCore::RenderObject::invalidateFlowThreadContainingBlockIncludingDescendants):
(WebCore::RenderObject::currentRenderNamedFlowFragment):
(WebCore::RenderObject::locateFlowThreadContainingBlock):
(WebCore::RenderObject::locateFlowThreadContainingBlockNoCache): Deleted.
(WebCore::RenderObject::removeFromRenderFlowThreadRecursive): Deleted.
* rendering/RenderObject.h:
(WebCore::RenderObject::flowThreadContainingBlock):
* rendering/RenderRegion.cpp:
(WebCore::RenderRegion::computeOverflowFromFlowThread):
* rendering/RenderView.cpp:
(WebCore::RenderView::pushLayoutStateForCurrentFlowThread):
(WebCore::RenderView::popLayoutStateForCurrentFlowThread):
* rendering/RenderView.h:

LayoutTests:

A test verifying that positioned elements inside multi-column containers don't
cause assertions or crashes.

* fast/multicol/multicol-all-positioned-crash-expected.txt: Added.
* fast/multicol/multicol-all-positioned-crash.html: Added.
* fast/multicol/multicol-transform-containing-block-expected.txt: Added.
* fast/multicol/multicol-transform-containing-block.html: Added.


Canonical link: https://commits.webkit.org/155620@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@174761 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog

@@ -1,3 +1,18 @@
+2014-10-15  Andrei Bucur  <abucur@adobe.com>
+
+        ASSERTION  FAILED in WebCore::RenderFlowThread::getRegionRangeForBox
+        https://bugs.webkit.org/show_bug.cgi?id=135563
+
+        Reviewed by David Hyatt.
+
+        A test verifying that positioned elements inside multi-column containers don't
+        cause assertions or crashes.
+
+        * fast/multicol/multicol-all-positioned-crash-expected.txt: Added.
+        * fast/multicol/multicol-all-positioned-crash.html: Added.
+        * fast/multicol/multicol-transform-containing-block-expected.txt: Added.
+        * fast/multicol/multicol-transform-containing-block.html: Added.
+
 2014-10-15  Commit Queue  <commit-queue@webkit.org>
 
         Unreviewed, rolling out r174753.

LayoutTests/fast/multicol/multicol-all-positioned-crash-expected.txt

@@ -0,0 +1 @@
+Test for the bug 135563. It should not crash or assert.

LayoutTests/fast/multicol/multicol-all-positioned-crash.html

@@ -0,0 +1,17 @@
+<html>
+<head>
+<style>
+* {
+    -webkit-columns: 2;
+    position: absolute;
+}
+</style>
+</head>
+<body>
+Test for <a href="https://bugs.webkit.org/show_bug.cgi?id=135563">the bug 135563</a>. It should not crash or assert.
+<script>
+if (window.testRunner)
+    window.testRunner.dumpAsText();
+</script>
+</body>
+</html>

LayoutTests/fast/multicol/multicol-transform-containing-block-expected.txt

@@ -0,0 +1 @@
+Test for b135563. It should not crash or assert.

LayoutTests/fast/multicol/multicol-transform-containing-block.html

@@ -0,0 +1,57 @@
+<!DOCTYPE html>
+<html>
+<style>
+    #pos {
+        position: absolute;
+        top: 10px;
+        left: 10px;
+    }
+
+    .multicol {
+        -webkit-column-count: 2;
+        height: 300px;
+    }
+
+    #transformed {
+        background: red;
+        width: 10px;
+        height: 10px;
+    }
+
+    .update {
+        -webkit-transform: rotate(-10deg);
+    }
+
+    .parent {
+        height: 400px;
+    }
+
+    .container {
+        padding: 50px;
+        margin: 20px;
+    }
+</style>
+<body>
+<div class="multicol parent">
+    <div id="change" class="container">
+        <div class="multicol">
+            <div>Static</div>
+            <div>Static</div>
+            <div>Static</div>
+            <div id="pos">Positioned</div>
+            <div>Static</div>
+            <div>Static</div>
+        </div>
+    </div>
+</div>
+<script type="text/javascript">
+    if (window.testRunner)
+        window.testRunner.dumpAsText();
+
+    document.body.offsetTop;
+    document.getElementById("change").className += " update";
+    document.body.offsetTop;
+    document.body.innerHTML = "Test for b135563. It should not crash or assert.";
+</script>
+</body>
+</html>

Source/WebCore/ChangeLog

@@ -1,3 +1,100 @@
+2014-10-15  Andrei Bucur  <abucur@adobe.com>
+
+        ASSERTION  FAILED in WebCore::RenderFlowThread::getRegionRangeForBox
+        https://bugs.webkit.org/show_bug.cgi?id=135563
+
+        Reviewed by David Hyatt.
+
+        The new multi-column code doesn't work correctly when the document contains nested fragmentation
+        contexts. The problem is the current flow thread concept that can store only one RenderFlowThread
+        at a time and use it during layout.
+
+        The stored flow thread is always correct for regions because named flow threads are absolutley positioned
+        so every child renderer is contained inside them (with the expcetion of fixed positioned elements which are
+        treated separately).
+
+        For multi-column elements this is no longer the case. An absolutely positioned element inside a static
+        multi-column element will be contained by a block outside the fragmentation context. It can even be
+        contained by a different multi-column element in the case of nested flow threads.
+
+        The patch below explores a solution that's not based on a current flow thread stored globally. The proposed
+        patch makes every block to store a pointer to its fragmentation context and a flag that states if this pointer
+        needs to be updated or not. If the renderer is not a block it will get its flow thread from the containing
+        block. Once the containing flow thread is requested for the block, the pointer is computed and cached until
+        invalidated:
+        - when a subtree is removed from a flow thread
+        - when the position property of an element inside a flow thread changes
+
+        The process is recursive and it doesn't affect elements that are not nested inside a flow thread. If a block
+        changes position from relative to static, any element that was contained by it can only be contained by an
+        ancestor of the block. This ancestor will still be outside of any flow thread. This ensures that non-fragmentation
+        code is not affected from a performance perspective.
+
+        The patch affects the results of the performance tests:
+        - the regions layout tests have a decreased performance raging from 2% to 5-6%
+        - the regions selection tests have an increased performance raging from 1-2% to 10%
+        - the multicolumn layout tests (now pending review in b137687) have an increased performance
+        raging from 1.8% to 5%
+
+        Tests: fast/multicol/multicol-all-positioned-crash.html
+               fast/multicol/multicol-transform-containing-block.html
+
+        * rendering/FlowThreadController.cpp:
+        (WebCore::FlowThreadController::FlowThreadController):
+        * rendering/FlowThreadController.h:
+        (WebCore::FlowThreadController::currentRenderFlowThread): Deleted.
+        (WebCore::FlowThreadController::setCurrentRenderFlowThread): Deleted.
+        * rendering/LayoutState.h:
+        (WebCore::LayoutState::currentRenderFlowThread):
+        (WebCore::LayoutState::setCurrentRenderFlowThread):
+        * rendering/RenderBlock.cpp:
+        (WebCore::RenderBlockRareData::RenderBlockRareData):
+        (WebCore::RenderBlock::styleWillChange):
+        (WebCore::RenderBlock::styleDidChange):
+        (WebCore::RenderBlock::collapseAnonymousBoxChild):
+        (WebCore::RenderBlock::cachedFlowThreadContainingBlock):
+        (WebCore::RenderBlock::cachedFlowThreadContainingBlockNeedsUpdate):
+        (WebCore::RenderBlock::setCachedFlowThreadContainingBlockNeedsUpdate):
+        (WebCore::RenderBlock::updateCachedFlowThreadContainingBlock):
+        (WebCore::RenderBlock::locateFlowThreadContainingBlock):
+        * rendering/RenderBlock.h:
+        * rendering/RenderBlockFlow.cpp:
+        (WebCore::RenderBlockFlow::styleWillChange):
+        * rendering/RenderBox.cpp:
+        (WebCore::RenderBox::borderBoxRectInRegion):
+        * rendering/RenderFlowThread.cpp:
+        (WebCore::RenderFlowThread::layout):
+        (WebCore::RenderFlowThread::updateAllLayerToRegionMappings):
+        (WebCore::RenderFlowThread::repaintRectangleInRegions):
+        (WebCore::CurrentRenderFlowThreadMaintainer::CurrentRenderFlowThreadMaintainer): Deleted.
+        (WebCore::CurrentRenderFlowThreadMaintainer::~CurrentRenderFlowThreadMaintainer): Deleted.
+        (WebCore::CurrentRenderFlowThreadDisabler::CurrentRenderFlowThreadDisabler): Deleted.
+        (WebCore::CurrentRenderFlowThreadDisabler::~CurrentRenderFlowThreadDisabler): Deleted.
+        * rendering/RenderFlowThread.h:
+        * rendering/RenderLayer.cpp:
+        (WebCore::RenderLayer::paintLayer):
+        (WebCore::RenderLayer::hitTestLayer):
+        (WebCore::RenderLayer::mapLayerClipRectsToFragmentationLayer):
+        (WebCore::RenderLayer::calculateClipRects):
+        * rendering/RenderObject.cpp:
+        (WebCore::RenderObject::showRegionsInformation):
+        (WebCore::RenderObject::insertedIntoTree):
+        (WebCore::RenderObject::removeFromRenderFlowThread):
+        (WebCore::RenderObject::removeFromRenderFlowThreadIncludingDescendants):
+        (WebCore::RenderObject::invalidateFlowThreadContainingBlockIncludingDescendants):
+        (WebCore::RenderObject::currentRenderNamedFlowFragment):
+        (WebCore::RenderObject::locateFlowThreadContainingBlock):
+        (WebCore::RenderObject::locateFlowThreadContainingBlockNoCache): Deleted.
+        (WebCore::RenderObject::removeFromRenderFlowThreadRecursive): Deleted.
+        * rendering/RenderObject.h:
+        (WebCore::RenderObject::flowThreadContainingBlock):
+        * rendering/RenderRegion.cpp:
+        (WebCore::RenderRegion::computeOverflowFromFlowThread):
+        * rendering/RenderView.cpp:
+        (WebCore::RenderView::pushLayoutStateForCurrentFlowThread):
+        (WebCore::RenderView::popLayoutStateForCurrentFlowThread):
+        * rendering/RenderView.h:
+
 2014-10-15  Chris Dumez  <cdumez@apple.com>
 
         Use is<>() / downcast<>() for list-related render objects

Source/WebCore/rendering/FlowThreadController.cpp

@@ -42,7 +42,6 @@ namespace WebCore {
 
 FlowThreadController::FlowThreadController(RenderView* view)
     : m_view(view)
-    , m_currentRenderFlowThread(0)
     , m_isRenderNamedFlowThreadOrderDirty(false)
     , m_flowThreadsWithAutoLogicalHeightRegions(0)
 {

Source/WebCore/rendering/FlowThreadController.h

@@ -48,9 +48,6 @@ class FlowThreadController {
     explicit FlowThreadController(RenderView*);
     ~FlowThreadController();
 
-    RenderFlowThread* currentRenderFlowThread() const { return m_currentRenderFlowThread; }
-    void setCurrentRenderFlowThread(RenderFlowThread* flowThread) { m_currentRenderFlowThread = flowThread; }
-
     bool isRenderNamedFlowThreadOrderDirty() const { return m_isRenderNamedFlowThreadOrderDirty; }
     void setIsRenderNamedFlowThreadOrderDirty(bool dirty)
     {
@@ -96,7 +93,6 @@ class FlowThreadController {
 
 private:
     RenderView* m_view;
-    RenderFlowThread* m_currentRenderFlowThread;
     bool m_isRenderNamedFlowThreadOrderDirty;
     unsigned m_flowThreadsWithAutoLogicalHeightRegions;
     std::unique_ptr<RenderNamedFlowThreadList> m_renderNamedFlowThreadList;

Source/WebCore/rendering/LayoutState.h

@@ -83,6 +83,10 @@ class LayoutState {
     void setLineGridPaginationOrigin(const LayoutSize& origin) { m_lineGridPaginationOrigin = origin; }
 
     bool needsBlockDirectionLocationSetBeforeLayout() const { return m_lineGrid || (m_isPaginated && m_pageLogicalHeight); }
+
+    RenderFlowThread* currentRenderFlowThread() const { return m_currentRenderFlowThread; }
+    void setCurrentRenderFlowThread(RenderFlowThread* flowThread) { m_currentRenderFlowThread = flowThread; }
+
 private:
     void propagateLineGridInfo(RenderBox*);
     void establishLineGrid(RenderBlockFlow*);
@@ -122,6 +126,8 @@ class LayoutState {
     LayoutSize m_lineGridOffset;
     LayoutSize m_lineGridPaginationOrigin;
 
+    RenderFlowThread* m_currentRenderFlowThread { nullptr };
+
 #ifndef NDEBUG
     RenderObject* m_renderer;
 #endif

Source/WebCore/rendering/RenderBlock.cpp

@@ -68,7 +68,9 @@
 #include "ShadowRoot.h"
 #include "TextBreakIterator.h"
 #include "TransformState.h"
+
 #include <wtf/NeverDestroyed.h>
+#include <wtf/Optional.h>
 #include <wtf/StackStats.h>
 #include <wtf/TemporaryChange.h>
 
@@ -94,7 +96,7 @@ static TrackedDescendantsMap* gPercentHeightDescendantsMap = 0;
 
 static TrackedContainerMap* gPositionedContainerMap = 0;
 static TrackedContainerMap* gPercentHeightContainerMap = 0;
-    
+
 typedef HashMap<RenderBlock*, std::unique_ptr<ListHashSet<RenderInline*>>> ContinuationOutlineTableMap;
 
 struct UpdateScrollInfoAfterLayoutTransaction {
@@ -121,14 +123,17 @@ static std::unique_ptr<DelayedUpdateScrollInfoStack>& updateScrollInfoAfterLayou
 struct RenderBlockRareData {
     WTF_MAKE_NONCOPYABLE(RenderBlockRareData); WTF_MAKE_FAST_ALLOCATED;
 public:
-    RenderBlockRareData() 
+    RenderBlockRareData()
         : m_paginationStrut(0)
         , m_pageLogicalOffset(0)
-    { 
+        , m_flowThreadContainingBlock(Nullopt)
+    {
     }
 
     LayoutUnit m_paginationStrut;
     LayoutUnit m_pageLogicalOffset;
+
+    Optional<RenderFlowThread*> m_flowThreadContainingBlock;
 };
 
 typedef HashMap<const RenderBlock*, std::unique_ptr<RenderBlockRareData>> RenderBlockRareDataMap;
@@ -266,7 +271,7 @@ void RenderBlock::styleWillChange(StyleDifference diff, const RenderStyle& newSt
     const RenderStyle* oldStyle = hasInitializedStyle() ? &style() : nullptr;
 
     setReplaced(newStyle.isDisplayInlineType());
-    
+
     if (oldStyle && parent() && diff == StyleDifferenceLayout && oldStyle->position() != newStyle.position()) {
         if (newStyle.position() == StaticPosition)
             // Clear our positioned objects list. Our absolutely positioned descendants will be
@@ -283,7 +288,7 @@ void RenderBlock::styleWillChange(StyleDifference diff, const RenderStyle& newSt
                 }
                 containingBlock = containingBlock->parent();
             }
-            
+
             if (is<RenderBlock>(*containingBlock))
                 downcast<RenderBlock>(*containingBlock).removePositionedObjects(this, NewContainingBlock);
         }
@@ -308,10 +313,20 @@ static bool borderOrPaddingLogicalWidthChanged(const RenderStyle* oldStyle, cons
 
 void RenderBlock::styleDidChange(StyleDifference diff, const RenderStyle* oldStyle)
 {
-    RenderBox::styleDidChange(diff, oldStyle);
-
     RenderStyle& newStyle = style();
 
+    bool hadTransform = hasTransform();
+    bool flowThreadContainingBlockInvalidated = false;
+    if (oldStyle && oldStyle->position() != newStyle.position()) {
+        invalidateFlowThreadContainingBlockIncludingDescendants();
+        flowThreadContainingBlockInvalidated = true;
+    }
+
+    RenderBox::styleDidChange(diff, oldStyle);
+
+    if (hadTransform != hasTransform() && !flowThreadContainingBlockInvalidated)
+        invalidateFlowThreadContainingBlockIncludingDescendants();
+
     if (!isAnonymousBlock()) {
         // Ensure that all of our continuation blocks pick up the new style.
         for (RenderBlock* currCont = blockElementContinuation(); currCont; currCont = currCont->blockElementContinuation()) {
@@ -324,7 +339,7 @@ void RenderBlock::styleDidChange(StyleDifference diff, const RenderStyle* oldSty
 
     propagateStyleToAnonymousChildren(PropagateToBlockChildrenOnly);
     m_lineHeight = -1;
-    
+
     // It's possible for our border/padding to change, but for the overall logical width of the block to
     // end up being the same. We keep track of this change so in layoutBlock, we can know to set relayoutChildren=true.
     m_hasBorderOrPaddingLogicalWidthChanged = oldStyle && diff == StyleDifferenceLayout && needsLayout() && borderOrPaddingLogicalWidthChanged(oldStyle, &newStyle);
@@ -690,7 +705,6 @@ void RenderBlock::collapseAnonymousBoxChild(RenderBlock& parent, RenderBlock* ch
     RenderObject* nextSibling = child->nextSibling();
 
     RenderFlowThread* childFlowThread = child->flowThreadContainingBlock();
-    CurrentRenderFlowThreadMaintainer flowThreadMaintainer(childFlowThread);
     if (childFlowThread && childFlowThread->isRenderNamedFlowThread())
         toRenderNamedFlowThread(childFlowThread)->removeFlowChildInfo(child);
 
@@ -3254,6 +3268,50 @@ void RenderBlock::updateFirstLetter()
     createFirstLetterRenderer(firstLetterContainer, downcast<RenderText>(firstLetterObj));
 }
 
+RenderFlowThread* RenderBlock::cachedFlowThreadContainingBlock() const
+{
+    RenderBlockRareData* rareData = getRareData(this);
+
+    if (!rareData || !rareData->m_flowThreadContainingBlock)
+        return nullptr;
+
+    return rareData->m_flowThreadContainingBlock.value();
+}
+
+bool RenderBlock::cachedFlowThreadContainingBlockNeedsUpdate() const
+{
+    RenderBlockRareData* rareData = getRareData(this);
+
+    if (!rareData || !rareData->m_flowThreadContainingBlock)
+        return true;
+
+    return false;
+}
+
+void RenderBlock::setCachedFlowThreadContainingBlockNeedsUpdate()
+{
+    RenderBlockRareData& rareData = ensureRareData(this);
+    rareData.m_flowThreadContainingBlock = Nullopt;
+}
+
+RenderFlowThread* RenderBlock::updateCachedFlowThreadContainingBlock(RenderFlowThread* flowThread) const
+{
+    RenderBlockRareData& rareData = ensureRareData(this);
+    rareData.m_flowThreadContainingBlock = flowThread;
+
+    return flowThread;
+}
+
+RenderFlowThread* RenderBlock::locateFlowThreadContainingBlock() const
+{
+    RenderBlockRareData* rareData = getRareData(this);
+    if (!rareData || !rareData->m_flowThreadContainingBlock)
+        return updateCachedFlowThreadContainingBlock(RenderBox::locateFlowThreadContainingBlock());
+
+    ASSERT(rareData->m_flowThreadContainingBlock.value() == RenderBox::locateFlowThreadContainingBlock());
+    return rareData->m_flowThreadContainingBlock.value();
+}
+
 LayoutUnit RenderBlock::paginationStrut() const
 {
     RenderBlockRareData* rareData = getRareData(this);