Changeset 286866 in webkit

Timestamp:

Dec 10, 2021 11:37:38 AM (3 years ago)

Author:

commit-queue@webkit.org

Message:

nullptr deref in ComputeFloatOffsetForLineLayoutAdapter<FloatingObject::FloatLeft>::updateOffsetIfNeeded
​https://bugs.webkit.org/show_bug.cgi?id=234018

Patch by Gabriel Nava Marino <​gnavamarino@apple.com> on 2021-12-10
Reviewed by Alan Bujtas.

Source/WebCore:

Test: fast/rendering/floating-object-renderer-crash.html

When destroying a given renderer, we first remove floats and out-of-flow positioned objects
from their containing block before detaching the renderer from the tree. We do this by obtaining
the renderer’s outermost block containing a floating object and recursively marking all siblings
and descendants for layout.

The criteria for continuing down the list of children require the current block to contain floats
or be able to shrink to avoid floats. However, we can have a scenario where the current child block
doesn’t have a float, but one of its descendants does. In this case, although we should continue to
that descendant and remove the float, we do not.

The proposal in this patch will instead check whether the child block contains a float, or any of its descendants do.
If so we should continue traversing towards that descendant.

• rendering/RenderBlockFlow.cpp:

(WebCore::RenderBlockFlow::subtreeContainsFloat const):
(WebCore::RenderBlockFlow::subtreeContainsFloats const):
(WebCore::RenderBlockFlow::markAllDescendantsWithFloatsForLayout):

• rendering/RenderBlockFlow.h:

LayoutTests:

• fast/rendering/floating-object-renderer-crash-expected.txt: Added.
• fast/rendering/floating-object-renderer-crash.html: Added.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/fast/rendering/floating-object-renderer-crash-expected.txt (added)
• LayoutTests/fast/rendering/floating-object-renderer-crash.html (added)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/rendering/RenderBlockFlow.cpp (modified) (2 diffs)
• Source/WebCore/rendering/RenderBlockFlow.h (modified) (1 diff)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2021-12-10  Gabriel Nava Marino  <gnavamarino@apple.com>
+
+        nullptr deref in ComputeFloatOffsetForLineLayoutAdapter<FloatingObject::FloatLeft>::updateOffsetIfNeeded
+        https://bugs.webkit.org/show_bug.cgi?id=234018
+
+        Reviewed by Alan Bujtas.
+
+        * fast/rendering/floating-object-renderer-crash-expected.txt: Added.
+        * fast/rendering/floating-object-renderer-crash.html: Added.
+
 2021-12-10  Commit Queue  <commit-queue@webkit.org>
 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2021-12-10  Gabriel Nava Marino  <gnavamarino@apple.com>
+
+        nullptr deref in ComputeFloatOffsetForLineLayoutAdapter<FloatingObject::FloatLeft>::updateOffsetIfNeeded
+        https://bugs.webkit.org/show_bug.cgi?id=234018
+
+        Reviewed by Alan Bujtas.
+
+        Test: fast/rendering/floating-object-renderer-crash.html
+
+        When destroying a given renderer, we first remove floats and out-of-flow positioned objects
+        from their containing block before detaching the renderer from the tree. We do this by obtaining
+        the renderer’s outermost block containing a floating object and recursively marking all siblings
+        and descendants for layout.
+
+        The criteria for continuing down the list of children require the current block to contain floats
+        or be able to shrink to avoid floats. However, we can have a scenario where the current child block
+        doesn’t have a float, but one of its descendants does. In this case, although we should continue to
+        that descendant and remove the float, we do not.
+
+        The proposal in this patch will instead check whether the child block contains a float, or any of its descendants do.
+        If so we should continue traversing towards that descendant.
+
+        * rendering/RenderBlockFlow.cpp:
+        (WebCore::RenderBlockFlow::subtreeContainsFloat const):
+        (WebCore::RenderBlockFlow::subtreeContainsFloats const):
+        (WebCore::RenderBlockFlow::markAllDescendantsWithFloatsForLayout):
+        * rendering/RenderBlockFlow.h:
+
 2021-12-10  Said Abou-Hallawa  <said@apple.com>
 

trunk/Source/WebCore/rendering/RenderBlockFlow.cpp

@@ -2110 +2110 @@
 }
 
+bool RenderBlockFlow::subtreeContainsFloat(RenderBox& renderer) const
+{
+    bool contains = m_floatingObjects && m_floatingObjects->set().contains<FloatingObjectHashTranslator>(renderer);
+    for (auto& block : childrenOfType<RenderBlock>(*this)) {
+        if (!is<RenderBlockFlow>(block))
+            continue;
+        auto& blockFlow = downcast<RenderBlockFlow>(block);
+        contains |= blockFlow.subtreeContainsFloat(renderer);
+    }
+    return contains;
+}
+
+bool RenderBlockFlow::subtreeContainsFloats() const
+{
+    bool contains = m_floatingObjects && !m_floatingObjects->set().isEmpty();
+    for (auto& block : childrenOfType<RenderBlock>(*this)) {
+        if (!is<RenderBlockFlow>(block))
+            continue;
+        auto& blockFlow = downcast<RenderBlockFlow>(block);
+        contains |= blockFlow.subtreeContainsFloats();
+    }
+    return contains;
+}
+
 void RenderBlockFlow::styleDidChange(StyleDifference diff, const RenderStyle* oldStyle)
 {
@@ -2879 +2903 @@
         }
         auto& blockFlow = downcast<RenderBlockFlow>(block);
-        if ((floatToRemove ? blockFlow.containsFloat(*floatToRemove) : blockFlow.containsFloats()) || blockFlow.shrinkToAvoidFloats())
+        if ((floatToRemove ? blockFlow.subtreeContainsFloat(*floatToRemove) : blockFlow.subtreeContainsFloats()) || blockFlow.shrinkToAvoidFloats())
             blockFlow.markAllDescendantsWithFloatsForLayout(floatToRemove, inLayout);
     }

trunk/Source/WebCore/rendering/RenderBlockFlow.h

@@ -279 +279 @@
     bool containsFloats() const override { return m_floatingObjects && !m_floatingObjects->set().isEmpty(); }
     bool containsFloat(RenderBox&) const;
+    bool subtreeContainsFloats() const;
+    bool subtreeContainsFloat(RenderBox&) const;
 
     void deleteLines() override;