Do not update the fragmented flow state while internally mutating the…

… render tree

https://bugs.webkit.org/show_bug.cgi?id=230896

Reviewed by Darin Adler.

RenderTree mutations (like those happening when creating/destroying anonymous blocks)
should not affect the fragment state of any renderer. This means that we should not have
to deal with things like creating/restoring placeholders/spanners while doing that.

There is already a IsInternalMove flag that is being used for that. Expand its usage
to a couple more methods to improve correctness.

* rendering/LegacyRootInlineBox.cpp:
(WebCore::LegacyRootInlineBox::~LegacyRootInlineBox): Do not remove the inline box from
the ContainingFragmentMap if we're deleting the tree. It was causing ASSERTs trying to
retrieve the enclosing fragmented flow in some cases.
* rendering/updating/RenderTreeBuilder.cpp:
(WebCore::RenderTreeBuilder::attachToRenderElementInternal): Use the RenderTreeBuilder's
m_internalTreeBuilding instead of the argument.
(WebCore::RenderTreeBuilder::move): Replace passing the IsInternalMove argument by a
scope where we don't update the fragmented flow state.
(WebCore::RenderTreeBuilder::detachFromRenderElement): Use the RenderTreeBuilder's
m_internalMovesType instead of the argument.
* rendering/updating/RenderTreeBuilder.h:
* rendering/updating/RenderTreeBuilderInline.cpp:
(WebCore::RenderTreeBuilder::Inline::splitInlines): Wrap the method by a scope in which
fragmented flow state is not updated because we consider those operations internal arrangements
of the tree.


Canonical link: https://commits.webkit.org/247275@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@289814 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog

@@ -1,3 +1,34 @@
+2022-02-10  Sergio Villar Senin  <svillar@igalia.com>
+
+        Do not update the fragmented flow state while internally mutating the render tree
+        https://bugs.webkit.org/show_bug.cgi?id=230896
+
+        Reviewed by Darin Adler.
+
+        RenderTree mutations (like those happening when creating/destroying anonymous blocks)
+        should not affect the fragment state of any renderer. This means that we should not have
+        to deal with things like creating/restoring placeholders/spanners while doing that.
+
+        There is already a IsInternalMove flag that is being used for that. Expand its usage
+        to a couple more methods to improve correctness.
+
+        * rendering/LegacyRootInlineBox.cpp:
+        (WebCore::LegacyRootInlineBox::~LegacyRootInlineBox): Do not remove the inline box from
+        the ContainingFragmentMap if we're deleting the tree. It was causing ASSERTs trying to
+        retrieve the enclosing fragmented flow in some cases.
+        * rendering/updating/RenderTreeBuilder.cpp:
+        (WebCore::RenderTreeBuilder::attachToRenderElementInternal): Use the RenderTreeBuilder's
+        m_internalTreeBuilding instead of the argument.
+        (WebCore::RenderTreeBuilder::move): Replace passing the IsInternalMove argument by a
+        scope where we don't update the fragmented flow state.
+        (WebCore::RenderTreeBuilder::detachFromRenderElement): Use the RenderTreeBuilder's
+        m_internalMovesType instead of the argument.
+        * rendering/updating/RenderTreeBuilder.h:
+        * rendering/updating/RenderTreeBuilderInline.cpp:
+        (WebCore::RenderTreeBuilder::Inline::splitInlines): Wrap the method by a scope in which
+        fragmented flow state is not updated because we consider those operations internal arrangements
+        of the tree.
+
 2022-02-15  Ziran Sun  <zsun@igalia.com>
 
         [Forms] the select() method returns should be in line with specs

Source/WebCore/rendering/LegacyRootInlineBox.cpp

@@ -75,7 +75,7 @@ LegacyRootInlineBox::~LegacyRootInlineBox()
 {
     detachEllipsisBox();
 
-    if (blockFlow().enclosingFragmentedFlow())
+    if (!renderer().document().renderTreeBeingDestroyed() && blockFlow().enclosingFragmentedFlow())
         containingFragmentMap(blockFlow()).remove(this);
 }
 

Source/WebCore/rendering/updating/RenderTreeBuilder.cpp

@@ -74,6 +74,7 @@
 #include "RenderTreeBuilderTable.h"
 #include "RenderTreeMutationDisallowedScope.h"
 #include "RenderView.h"
+#include <wtf/SetForScope.h>
 
 #if ENABLE(LAYOUT_FORMATTING_CONTEXT)
 #include "FrameView.h"
@@ -456,12 +457,12 @@ void RenderTreeBuilder::attachToRenderElementInternal(RenderElement& parent, Ren
     // Take the ownership.
     auto* newChild = parent.attachRendererInternal(WTFMove(child), beforeChild);
 
-    newChild->initializeFragmentedFlowStateOnInsertion();
+    if (m_internalMovesType == RenderObject::IsInternalMove::No)
+        newChild->initializeFragmentedFlowStateOnInsertion();
     if (!parent.renderTreeBeingDestroyed()) {
         newChild->insertedIntoTree(isInternalMove);
 
-        auto needsStateReset = isInternalMove == RenderObject::IsInternalMove::No;
-        if (needsStateReset) {
+        if (m_internalMovesType == RenderObject::IsInternalMove::No) {
             if (auto* fragmentedFlow = newChild->enclosingFragmentedFlow(); is<RenderMultiColumnFlow>(fragmentedFlow))
                 multiColumnBuilder().multiColumnDescendantInserted(downcast<RenderMultiColumnFlow>(*fragmentedFlow), *newChild);
 
@@ -496,8 +497,9 @@ void RenderTreeBuilder::move(RenderBoxModelObject& from, RenderBoxModelObject& t
         auto childToMove = detachFromRenderElement(from, child);
         attach(to, WTFMove(childToMove), beforeChild);
     } else {
-        auto childToMove = detachFromRenderElement(from, child, WillBeDestroyed::No, RenderObject::IsInternalMove::Yes);
-        attachToRenderElementInternal(to, WTFMove(childToMove), beforeChild, RenderObject::IsInternalMove::Yes);
+        auto internalMoveScope = SetForScope { m_internalMovesType, RenderObject::IsInternalMove::Yes };
+        auto childToMove = detachFromRenderElement(from, child, WillBeDestroyed::No);
+        attachToRenderElementInternal(to, WTFMove(childToMove), beforeChild);
     }
 
     auto findBFCRootAndDestroyInlineTree = [&] {
@@ -943,7 +945,7 @@ RenderPtr<RenderObject> RenderTreeBuilder::detachFromRenderGrid(RenderGrid& pare
     return takenChild;
 }
 
-RenderPtr<RenderObject> RenderTreeBuilder::detachFromRenderElement(RenderElement& parent, RenderObject& child, WillBeDestroyed willBeDestroyed, RenderObject::IsInternalMove isInternalMove)
+RenderPtr<RenderObject> RenderTreeBuilder::detachFromRenderElement(RenderElement& parent, RenderObject& child, WillBeDestroyed willBeDestroyed)
 {
     RELEASE_ASSERT_WITH_MESSAGE(!parent.view().frameView().layoutContext().layoutState(), "Layout must not mutate render tree");
 
@@ -981,10 +983,11 @@ RenderPtr<RenderObject> RenderTreeBuilder::detachFromRenderElement(RenderElement
     if (!parent.renderTreeBeingDestroyed() && willBeDestroyed == WillBeDestroyed::Yes && child.isSelectionBorder())
         parent.frame().selection().setNeedsSelectionUpdate();
 
-    child.resetFragmentedFlowStateOnRemoval();
+    if (!parent.renderTreeBeingDestroyed() && m_internalMovesType == RenderObject::IsInternalMove::No)
+        child.resetFragmentedFlowStateOnRemoval();
 
     if (!parent.renderTreeBeingDestroyed())
-        child.willBeRemovedFromTree(isInternalMove);
+        child.willBeRemovedFromTree(m_internalMovesType);
 
     // WARNING: There should be no code running between willBeRemovedFromTree() and the actual removal below.
     // This is needed to avoid race conditions where willBeRemovedFromTree() would dirty the tree's structure

Source/WebCore/rendering/updating/RenderTreeBuilder.h

@@ -77,7 +77,7 @@ class RenderTreeBuilder {
     void attachToRenderElementInternal(RenderElement& parent, RenderPtr<RenderObject> child, RenderObject* beforeChild = nullptr, RenderObject::IsInternalMove = RenderObject::IsInternalMove::No);
 
     enum class WillBeDestroyed { No, Yes };
-    RenderPtr<RenderObject> detachFromRenderElement(RenderElement& parent, RenderObject& child, WillBeDestroyed = WillBeDestroyed::Yes, RenderObject::IsInternalMove = RenderObject::IsInternalMove::No) WARN_UNUSED_RETURN;
+    RenderPtr<RenderObject> detachFromRenderElement(RenderElement& parent, RenderObject& child, WillBeDestroyed = WillBeDestroyed::Yes) WARN_UNUSED_RETURN;
     RenderPtr<RenderObject> detachFromRenderGrid(RenderGrid& parent, RenderObject& child) WARN_UNUSED_RETURN;
 
     void move(RenderBoxModelObject& from, RenderBoxModelObject& to, RenderObject& child, RenderObject* beforeChild, NormalizeAfterInsertion);
@@ -158,6 +158,7 @@ class RenderTreeBuilder {
     std::unique_ptr<FullScreen> m_fullScreenBuilder;
 #endif
     bool m_hasBrokenContinuation { false };
+    RenderObject::IsInternalMove m_internalMovesType { RenderObject::IsInternalMove::No };
 };
 
 }

Source/WebCore/rendering/updating/RenderTreeBuilderInline.cpp

@@ -34,6 +34,7 @@
 #include "RenderTable.h"
 #include "RenderTreeBuilderMultiColumn.h"
 #include "RenderTreeBuilderTable.h"
+#include <wtf/SetForScope.h>
 
 namespace WebCore {
 
@@ -266,6 +267,7 @@ void RenderTreeBuilder::Inline::splitFlow(RenderInline& parent, RenderObject* be
 
 void RenderTreeBuilder::Inline::splitInlines(RenderInline& parent, RenderBlock* fromBlock, RenderBlock* toBlock, RenderBlock* middleBlock, RenderObject* beforeChild, RenderBoxModelObject* oldCont)
 {
+    auto internalMoveScope = SetForScope { m_builder.m_internalMovesType, RenderObject::IsInternalMove::Yes };
     // Create a clone of this inline.
     RenderPtr<RenderInline> cloneInline = cloneAsContinuation(parent);
 #if ENABLE(FULLSCREEN_API)