Overhanging float sets are not cleaned up properly when floating renderer is destroyed.
authorzalan@apple.com <zalan@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 26 May 2015 22:59:40 +0000 (22:59 +0000)
committerzalan@apple.com <zalan@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 26 May 2015 22:59:40 +0000 (22:59 +0000)
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.

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

LayoutTests/ChangeLog
LayoutTests/fast/block/float/crash-when-floating-object-is-removed-expected.txt [new file with mode: 0644]
LayoutTests/fast/block/float/crash-when-floating-object-is-removed.xhtml [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/rendering/RenderBlockFlow.cpp
Source/WebCore/rendering/RenderBox.cpp
Source/WebCore/rendering/RenderBox.h

index 2060aa6..067d8f5 100644 (file)
@@ -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  Beth Dakin  <bdakin@apple.com>
 
         storage/indexeddb/deleteIndex-bug110792.html is flaky
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
new file mode 100644 (file)
index 0000000..3b69c2d
--- /dev/null
@@ -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
new file mode 100644 (file)
index 0000000..5e80268
--- /dev/null
@@ -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>
index f0a23b8..9af8a5a 100644 (file)
@@ -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  Ryuan Choi  <ryuan.choi@navercorp.com>
 
         [EFL][CoordinatedGraphics] Remove CoordinatedTileClient and CoordinatedTileBackend
index e2ce59a..af4ca71 100644 (file)
@@ -2760,7 +2760,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);
index f922d89..eb1a49d 100644 (file)
@@ -264,14 +264,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;
@@ -285,7 +285,7 @@ void RenderBox::removeFloatingOrPositionedChildFromBlockLists()
         return;
 
     if (isFloating()) {
-        if (RenderBlockFlow* parentBlock = outermostBlockContainingFloatingObject()) {
+        if (RenderBlockFlow* parentBlock = outermostBlockContainingFloatingObject(*this)) {
             parentBlock->markSiblingsWithFloatsForLayout(this);
             parentBlock->markAllDescendantsWithFloatsForLayout(this, false);
         }
index 41f4e15..267fd73 100644 (file)
@@ -518,8 +518,6 @@ public:
 
     virtual VisiblePosition positionForPoint(const LayoutPoint&, const RenderRegion*) override;
 
-    RenderBlockFlow* outermostBlockContainingFloatingObject();
-
     void removeFloatingOrPositionedChildFromBlockLists();
     
     RenderLayer* enclosingFloatPaintingLayer() const;