Block reusing may remove floating object too early

alanbaradlay · alanbaradlay · commit d9617b11409e · 2025-04-02T06:03:13.000-07:00
https://bugs.webkit.org/show_bug.cgi?id=290862 <rdar://147215658> Reviewed by Antti Koivisto. "Reusing block" type mutations (see RenderTreeBuilder::Inline::splitFlow) followed by float removal may lead to an unexpected state where we have a float to remove, but we have already destroyed m_floatingObjects, causing us to incorrectly assume that the float no longer belongs here (markSiblingsWithFloatsForLayout) and, therefore, does not need to be removed from sibling blocks (in case it is intrusive). What happens here is: 1. tree mutation makes an anon block reused (pre block) 2. a float is removed from said anon block's subtree At #1 we call removeFloatingObjects() which simply clears and destroys m_floatingObjects on the anon block. Now at #2, when we try to remove this float from sibling block containers by calling RenderBlockFlow::markSiblingsWithFloatsForLayout, and we consult m_floatingObjects to see if there's any float associated with the block and we early return as we had already cleared this set at #1. This patch ensures that when markSiblingsWithFloatsForLayout is called with a valid float, we always try to clean up sibling content. * LayoutTests/fast/block/float-remove-after-block-collapse-crash-expected.txt: Added. * LayoutTests/fast/block/float-remove-after-block-collapse-crash.html: Added. * Source/WebCore/rendering/RenderBlockFlow.cpp: (WebCore::RenderBlockFlow::markSiblingsWithFloatsForLayout): Change for (siblings) for (set items) to for (set items) for (siblings) so that the 'for (siblings)' logic can be moved to a lambda and used when there's a valid incoming float. Canonical link: https://commits.webkit.org/293094@main
diff --git a/LayoutTests/fast/block/float-remove-after-block-collapse-crash-expected.txt b/LayoutTests/fast/block/float-remove-after-block-collapse-crash-expected.txt
@@ -0,0 +1,3 @@
+  PASS if no crash or assert.
+A
+
diff --git a/LayoutTests/fast/block/float-remove-after-block-collapse-crash.html b/LayoutTests/fast/block/float-remove-after-block-collapse-crash.html
@@ -0,0 +1,44 @@
+<style>
+body, div, span, ol, li {
+  -webkit-column-break-inside: avoid;
+  border-bottom-style: groove;
+  break-after: column;
+}
+
+#x5 {
+  float: left;
+}
+
+:scope { 
+  columns: 40;
+  writing-mode: vertical-lr;
+}
+
+:empty { 
+  float: left;
+  list-style: url(data:image/gif;base64,++==)
+}
+</style>
+<script>
+window.testRunner?.dumpAsText();
+
+function main() {
+  document.body.offsetHeight;
+  x1.scrollAmount = 10;
+  x10.reportValidity();
+  
+  document.body.offsetHeight;
+  x57.setAttribute("reversed","");
+  
+  document.body.offsetHeight;
+  document.styleSheets[0].media.appendMedium("print (foobar)");
+}
+</script>
+<body onload="main()">
+<div><div> </div><span> </span></div>
+<marquee id="x1" scrollamount="0"> </marquee>
+<span id="x5"><span><div></div></span></span>
+<textarea id="x10" required="">
+</textarea>
+PASS if no crash or assert.<div><li> </li>A</div>
+<ol id="x57"> <li></li></ol>
diff --git a/Source/WebCore/rendering/RenderBlockFlow.cpp b/Source/WebCore/rendering/RenderBlockFlow.cpp
@@ -3069,25 +3069,28 @@ void RenderBlockFlow::markAllDescendantsWithFloatsForLayout(RenderBox* floatToRe
 
 void RenderBlockFlow::markSiblingsWithFloatsForLayout(RenderBox* floatToRemove)
 {
-    if (!m_floatingObjects)
-        return;
-
-    const FloatingObjectSet& floatingObjectSet = m_floatingObjects->set();
-    auto end = floatingObjectSet.end();
-
-    for (RenderObject* next = nextSibling(); next; next = next->nextSibling()) {
-        CheckedPtr nextBlock = dynamicDowncast<RenderBlockFlow>(*next);
-        if (!nextBlock || (!floatToRemove && (next->isFloatingOrOutOfFlowPositioned() || nextBlock->avoidsFloats())))
-            continue;
+    ASSERT(!floatToRemove || floatToRemove->isFloating());
 
-        for (auto it = floatingObjectSet.begin(); it != end; ++it) {
-            RenderBox& floatingBox = (*it)->renderer();
-            if (floatToRemove && &floatingBox != floatToRemove)
+    auto markSiblingsWithIntrusiveFloatForLayoutIfApplicable = [&](auto& floatBoxToRemove) {
+        for (auto* nextSibling = this->nextSibling(); nextSibling; nextSibling = nextSibling->nextSibling()) {
+            CheckedPtr nextSiblingBlockFlow = dynamicDowncast<RenderBlockFlow>(*nextSibling);
+            if (!nextSiblingBlockFlow)
                 continue;
-            if (nextBlock->containsFloat(floatingBox))
-                nextBlock->markAllDescendantsWithFloatsForLayout(&floatingBox);
+            if (nextSiblingBlockFlow->containsFloat(floatBoxToRemove))
+                nextSiblingBlockFlow->markAllDescendantsWithFloatsForLayout(&floatBoxToRemove);
         }
+    };
+
+    if (floatToRemove) {
+        markSiblingsWithIntrusiveFloatForLayoutIfApplicable(*floatToRemove);
+        return;
     }
+
+    if (!m_floatingObjects)
+        return;
+
+    for (auto& floatingObject : m_floatingObjects->set())
+        markSiblingsWithIntrusiveFloatForLayoutIfApplicable(floatingObject->renderer());
 }
 
 LayoutPoint RenderBlockFlow::flipFloatForWritingModeForChild(const FloatingObject& child, const LayoutPoint& point) const

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,3 @@`
	`1`	`+ PASS if no crash or assert.`
	`2`	`+A`
	`3`	`+`