Merge r184885 - Overhanging float sets are not cleaned up properly wh…

…en floating renderer is destroyed. https://bugs.webkit.org/show_bug.cgi?id=145323 rdar://problem/20980628 Reviewed by Dave Hyatt. This patch ensures when an overhanging float renderer is destroyed, all the sibling containers' floating object set(m_floatingObjects) gets properly cleaned up. When an overhanging float is present, we cache the renderer on the parent and on the affected sibling containers too. (RenderBlockFlow::m_floatingObjects) These caches(sets) get cleared and repopulated during ::layout(). In order to have a float renderer removed from a set, a layout needs to be initiated on the container. This is normally done through RenderBlockFlow::markSiblingsWithFloatsForLayout() and RenderBlockFlow::markAllDescendantsWithFloatsForLayout(). However, when the float container's parent's writing direction changes (and we promote the children containers to new formatting contexts), the layout propagation through siblings does not work anymore. The avoidsFloats() check in RenderBlockFlow::markSiblingsWithFloatsForLayout() has very little performance gain, but it prevents us from propagating layout to siblings when certain properties of the parent container changes. Source/WebCore: Test: fast/block/float/crash-when-floating-object-is-removed.xhtml * rendering/RenderBlockFlow.cpp: (WebCore::RenderBlockFlow::markSiblingsWithFloatsForLayout): * rendering/RenderBox.cpp: (WebCore::outermostBlockContainingFloatingObject): (WebCore::RenderBox::removeFloatingOrPositionedChildFromBlockLists): (WebCore::RenderBox::outermostBlockContainingFloatingObject): Deleted. * rendering/RenderBox.h: LayoutTests: * fast/block/float/crash-when-floating-object-is-removed-expected.txt: Added. * fast/block/float/crash-when-floating-object-is-removed.xhtml: Added.
WebKit · Jul 6, 2015 · 7f071a9 · 7f071a9
1 parent d70b147
commit 7f071a9
Show file tree

Hide file tree

Showing 7 changed files with 107 additions and 8 deletions.
diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
@@ -1,3 +1,27 @@
+2015-05-26  Zalan Bujtas  <zalan@apple.com>
+
+        Overhanging float sets are not cleaned up properly when floating renderer is destroyed.
+        https://bugs.webkit.org/show_bug.cgi?id=145323
+        rdar://problem/20980628
+
+        Reviewed by Dave Hyatt.
+
+        This patch ensures when an overhanging float renderer is destroyed,
+        all the sibling containers' floating object set(m_floatingObjects) gets properly cleaned up.
+
+        When an overhanging float is present, we cache the renderer on the parent and on the affected
+        sibling containers too. (RenderBlockFlow::m_floatingObjects) These caches(sets) get cleared and repopulated
+        during ::layout(). In order to have a float renderer removed from a set, a layout needs to be initiated on the container.
+        This is normally done through RenderBlockFlow::markSiblingsWithFloatsForLayout() and RenderBlockFlow::markAllDescendantsWithFloatsForLayout().
+        However, when the float container's parent's writing direction changes (and we promote the children containers to new formatting contexts),
+        the layout propagation through siblings does not work anymore.
+
+        The avoidsFloats() check in RenderBlockFlow::markSiblingsWithFloatsForLayout() has very little performance gain, but it prevents us
+        from propagating layout to siblings when certain properties of the parent container changes.
+
+        * fast/block/float/crash-when-floating-object-is-removed-expected.txt: Added.
+        * fast/block/float/crash-when-floating-object-is-removed.xhtml: Added.
+
 2015-05-26  Said Abou-Hallawa  <sabouhallawa@apple.com>
 
         SVG fragment identifier rendering issue

diff --git a/LayoutTests/fast/block/float/crash-when-floating-object-is-removed-expected.txt b/LayoutTests/fast/block/float/crash-when-floating-object-is-removed-expected.txt
@@ -0,0 +1 @@
+ PASS if no crash or ASSERT in debug.
diff --git a/LayoutTests/fast/block/float/crash-when-floating-object-is-removed.xhtml b/LayoutTests/fast/block/float/crash-when-floating-object-is-removed.xhtml
@@ -0,0 +1,45 @@
+<html xmlns="http://www.w3.org/1999/xhtml">
+<head>
+<title>This tests that sets for overhanging floating objects are cleaned up properly when the floating object is destroyed.</title>
+<script>
+  if (window.testRunner)
+    testRunner.dumpAsText();
+</script>
+<style>
+  html {
+    position: absolute;
+  }
+
+  .float { 
+    float: left; 
+  }
+</style>
+</head>
+<head id="head1" style="-webkit-writing-mode: horizontal-bt;"></head>
+
+<i>
+  <button id="button1">PASS if no crash or ASSERT in debug.</button>
+</i>
+<td id="td1">
+  <body></body>
+</td>
+<i></i>
+
+<script>
+var docElement = document.documentElement;
+function crash() {
+    button1 = document.getElementById("button1");
+    button1.classList.toggle("float");
+
+    head1 = document.getElementById("head1");
+    td1 = document.getElementById("td1");
+    head1.appendChild(td1);
+
+    docElement.offsetTop;
+    head1.style.display = "list-item";
+    docElement.offsetTop;
+    button1.classList.toggle("float");
+}
+document.addEventListener("DOMContentLoaded", crash, false);
+</script>
+</html>
diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
@@ -1,3 +1,34 @@
+2015-05-26  Zalan Bujtas  <zalan@apple.com>
+
+        Overhanging float sets are not cleaned up properly when floating renderer is destroyed.
+        https://bugs.webkit.org/show_bug.cgi?id=145323
+        rdar://problem/20980628
+
+        Reviewed by Dave Hyatt.
+
+        This patch ensures when an overhanging float renderer is destroyed,
+        all the sibling containers' floating object set(m_floatingObjects) gets properly cleaned up.
+
+        When an overhanging float is present, we cache the renderer on the parent and on the affected
+        sibling containers too. (RenderBlockFlow::m_floatingObjects) These caches(sets) get cleared and repopulated
+        during ::layout(). In order to have a float renderer removed from a set, a layout needs to be initiated on the container.
+        This is normally done through RenderBlockFlow::markSiblingsWithFloatsForLayout() and RenderBlockFlow::markAllDescendantsWithFloatsForLayout().
+        However, when the float container's parent's writing direction changes (and we promote the children containers to new formatting contexts),
+        the layout propagation through siblings does not work anymore.
+
+        The avoidsFloats() check in RenderBlockFlow::markSiblingsWithFloatsForLayout() has very little performance gain, but it prevents us
+        from propagating layout to siblings when certain properties of the parent container changes.
+
+        Test: fast/block/float/crash-when-floating-object-is-removed.xhtml
+
+        * rendering/RenderBlockFlow.cpp:
+        (WebCore::RenderBlockFlow::markSiblingsWithFloatsForLayout):
+        * rendering/RenderBox.cpp:
+        (WebCore::outermostBlockContainingFloatingObject):
+        (WebCore::RenderBox::removeFloatingOrPositionedChildFromBlockLists):
+        (WebCore::RenderBox::outermostBlockContainingFloatingObject): Deleted.
+        * rendering/RenderBox.h:
+
 2015-05-26  Said Abou-Hallawa  <sabouhallawa@apple.com>
 
         SVG fragment identifier rendering issue

diff --git a/Source/WebCore/rendering/RenderBlockFlow.cpp b/Source/WebCore/rendering/RenderBlockFlow.cpp
@@ -2746,7 +2746,7 @@ void RenderBlockFlow::markSiblingsWithFloatsForLayout(RenderBox* floatToRemove)
     auto end = floatingObjectSet.end();
 
     for (RenderObject* next = nextSibling(); next; next = next->nextSibling()) {
-        if (!is<RenderBlockFlow>(*next) || next->isFloatingOrOutOfFlowPositioned() || downcast<RenderBlockFlow>(*next).avoidsFloats())
+        if (!is<RenderBlockFlow>(*next) || next->isFloatingOrOutOfFlowPositioned())
             continue;
 
         RenderBlockFlow& nextBlock = downcast<RenderBlockFlow>(*next);

diff --git a/Source/WebCore/rendering/RenderBox.cpp b/Source/WebCore/rendering/RenderBox.cpp
@@ -263,14 +263,14 @@ LayoutRect RenderBox::borderBoxRectInRegion(RenderRegion* region, RenderBoxRegio
     return LayoutRect(0, logicalLeft, width(), logicalWidth);
 }
 
-RenderBlockFlow* RenderBox::outermostBlockContainingFloatingObject()
+static RenderBlockFlow* outermostBlockContainingFloatingObject(RenderBox& box)
 {
-    ASSERT(isFloating());
+    ASSERT(box.isFloating());
     RenderBlockFlow* parentBlock = nullptr;
-    for (auto& ancestor : ancestorsOfType<RenderBlockFlow>(*this)) {
+    for (auto& ancestor : ancestorsOfType<RenderBlockFlow>(box)) {
         if (ancestor.isRenderView())
             break;
-        if (!parentBlock || ancestor.containsFloat(*this))
+        if (!parentBlock || ancestor.containsFloat(box))
             parentBlock = &ancestor;
     }
     return parentBlock;
@@ -284,7 +284,7 @@ void RenderBox::removeFloatingOrPositionedChildFromBlockLists()
         return;
 
     if (isFloating()) {
-        if (RenderBlockFlow* parentBlock = outermostBlockContainingFloatingObject()) {
+        if (RenderBlockFlow* parentBlock = outermostBlockContainingFloatingObject(*this)) {
             parentBlock->markSiblingsWithFloatsForLayout(this);
             parentBlock->markAllDescendantsWithFloatsForLayout(this, false);
         }

diff --git a/Source/WebCore/rendering/RenderBox.h b/Source/WebCore/rendering/RenderBox.h
@@ -516,8 +516,6 @@ class RenderBox : public RenderBoxModelObject {
 
     virtual VisiblePosition positionForPoint(const LayoutPoint&, const RenderRegion*) override;
 
-    RenderBlockFlow* outermostBlockContainingFloatingObject();
-
     void removeFloatingOrPositionedChildFromBlockLists();
 
     RenderLayer* enclosingFloatPaintingLayer() const;