Stale float state caused by 'content visibility' may lead to ASSERT in addFloatsToNewParent

alanbaradlay · alanbaradlay · commit cab087edfb44 · 2025-04-02T15:05:30.000-07:00
https://bugs.webkit.org/show_bug.cgi?id=290898 <rdar://143296265> Reviewed by Antti Koivisto. In this patch 1. we let m_floatingObjects go stale on the skipped root (we already do that for the skipped subtree by not running layout) 2. we descend into skipped subtrees while cleaning up floats even when m_floatingObjects is stale/empty Having up-to-date m_floatingObjects on the skipped root, while stale m_floatingObjects on the skipped subtree can lead to issues when (#1) a previously intrusive float (#2) becomes non-intrusive and (#3) eventually gets deleted prevents us from being able to cleanup m_floatingObjects in skipped subtree(s). at #1 m_floatingObjects is populated with the intrusive float (both skipped root and renderers in skipped subtree) and at #2 since we only run layout on the skipped root, m_floatingObjects gets updated by removing this previously intrusive float (skipped subtree becomes stale) and at #3 we don't descend into the skipped subtree to cleanup m_floatingObjects since the skipped root does not have this float anymore (removed at #2). * Source/WebCore/rendering/RenderBlockFlow.cpp: (WebCore::RenderBlockFlow::markSiblingsWithFloatsForLayout): Canonical link: https://commits.webkit.org/293119@main
diff --git a/LayoutTests/fast/block/content-visisbility-and-float-crash-expected.txt b/LayoutTests/fast/block/content-visisbility-and-float-crash-expected.txt
@@ -0,0 +1 @@
+undefined
diff --git a/LayoutTests/fast/block/content-visisbility-and-float-crash.html b/LayoutTests/fast/block/content-visisbility-and-float-crash.html
@@ -0,0 +1,38 @@
+<style>
+:not(marker) {
+  background: url(#svgvar00007) repeat-x scroll top left;
+  zoom: 50%;
+}
+details {
+  font: 1pt monospace;
+}
+</style>
+
+<script>
+window.testRunner?.dumpAsText();
+
+function testRun() {
+  buttonID.outerHTML = "undefined";
+}
+
+function eventhandler3() {
+  detailsID.open = false;
+  
+  document.body.offsetHeight;
+  tableID.deleteRow(1);
+}
+</script>
+
+<body onload=testRun()>
+<button id=buttonID>
+<details id=detailsID ontoggle="eventhandler3()" open="true">
+<summary>
+<table id=tableID align="right">
+<tr></tr>
+<tfoot><th></th></tfoot>
+<th>PASS if no crash or assert</th>
+</table>
+<iframe srcdoc="">content</iframe>
+</summary>
+<span>
+<li><embed><ol>
diff --git a/Source/WebCore/rendering/RenderBlockFlow.cpp b/Source/WebCore/rendering/RenderBlockFlow.cpp
@@ -205,6 +205,9 @@ RenderBlockFlow* RenderBlockFlow::previousSiblingWithOverhangingFloats(bool& par
 
 void RenderBlockFlow::rebuildFloatingObjectSetFromIntrudingFloats()
 {
+    if (isSkippedContentRoot(*this))
+        return;
+
     UncheckedKeyHashSet<CheckedPtr<RenderBox>> oldIntrudingFloatSet;
 
     if (m_floatingObjects) {
@@ -3076,7 +3079,8 @@ void RenderBlockFlow::markSiblingsWithFloatsForLayout(RenderBox* floatToRemove)
             CheckedPtr nextSiblingBlockFlow = dynamicDowncast<RenderBlockFlow>(*nextSibling);
             if (!nextSiblingBlockFlow)
                 continue;
-            if (nextSiblingBlockFlow->containsFloat(floatBoxToRemove))
+            auto shouldCheckSubtree = isSkippedContentRoot(*nextSiblingBlockFlow) || nextSiblingBlockFlow->isSkippedContent() || nextSiblingBlockFlow->containsFloat(floatBoxToRemove);
+            if (shouldCheckSubtree)
                 nextSiblingBlockFlow->markAllDescendantsWithFloatsForLayout(&floatBoxToRemove);
         }
     };
