Changeset 238119 in webkit

Timestamp:

Nov 12, 2018, 6:38:06 PM (8 years ago)

Author:

Alan Bujtas

Message:

Do not collapse the soon-to-be-parent anon block when we shuffle around the marker item renderer.
​https://bugs.webkit.org/show_bug.cgi?id=191554
<rdar://problem/45825265>

Reviewed by Antti Koivisto.

Source/WebCore:

While moving the marker item renderer to its correct subtree, we accidentally remove the soon-to-be parent anonymous block.
Moving a renderer is a 2 step process:

1. Detach the renderer from its current parent
2. Attach it to its new parent.

During step #1, we check if there is a chance to collapse anonymous blocks. In this case the soon-to-be-parent is a sibling anonymous block which, after detaching the marker sibling
is not needed anymore (except we use it as the new parent).

Test: fast/inline/marker-list-item-move-should-not-crash.html

• rendering/updating/RenderTreeBuilder.cpp:

(WebCore::RenderTreeBuilder::detach):

• rendering/updating/RenderTreeBuilder.h:
• rendering/updating/RenderTreeBuilderBlock.cpp:

(WebCore::RenderTreeBuilder::Block::detach):

• rendering/updating/RenderTreeBuilderBlock.h:
• rendering/updating/RenderTreeBuilderList.cpp:

(WebCore::RenderTreeBuilder::List::updateItemMarker):

LayoutTests:

• fast/inline/marker-list-item-move-should-not-crash-expected.txt: Added.
• fast/inline/marker-list-item-move-should-not-crash.html: Added.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/fast/inline/marker-list-item-move-should-not-crash-expected.txt (added)
• LayoutTests/fast/inline/marker-list-item-move-should-not-crash.html (added)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/rendering/updating/RenderTreeBuilder.cpp (modified) (2 diffs)
• Source/WebCore/rendering/updating/RenderTreeBuilder.h (modified) (1 diff)
• Source/WebCore/rendering/updating/RenderTreeBuilderBlock.cpp (modified) (4 diffs)
• Source/WebCore/rendering/updating/RenderTreeBuilderBlock.h (modified) (1 diff)
• Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp (modified) (1 diff)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2018-11-12  Zalan Bujtas  <zalan@apple.com>
+
+        Do not collapse the soon-to-be-parent anon block when we shuffle around the marker item renderer.
+        https://bugs.webkit.org/show_bug.cgi?id=191554
+        <rdar://problem/45825265>
+
+        Reviewed by Antti Koivisto.
+
+        * fast/inline/marker-list-item-move-should-not-crash-expected.txt: Added.
+        * fast/inline/marker-list-item-move-should-not-crash.html: Added.
+
 2018-11-12  Sihui Liu  <sihui_liu@apple.com>
 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2018-11-12  Zalan Bujtas  <zalan@apple.com>
+
+        Do not collapse the soon-to-be-parent anon block when we shuffle around the marker item renderer.
+        https://bugs.webkit.org/show_bug.cgi?id=191554
+        <rdar://problem/45825265>
+
+        Reviewed by Antti Koivisto.
+
+        While moving the marker item renderer to its correct subtree, we accidentally remove the soon-to-be parent anonymous block.
+        Moving a renderer is a 2 step process:
+        1. Detach the renderer from its current parent
+        2. Attach it to its new parent.
+        During step #1, we check if there is a chance to collapse anonymous blocks. In this case the soon-to-be-parent is a sibling anonymous block which, after detaching the marker sibling
+        is not needed anymore (except we use it as the new parent).
+
+        Test: fast/inline/marker-list-item-move-should-not-crash.html
+
+        * rendering/updating/RenderTreeBuilder.cpp:
+        (WebCore::RenderTreeBuilder::detach):
+        * rendering/updating/RenderTreeBuilder.h:
+        * rendering/updating/RenderTreeBuilderBlock.cpp:
+        (WebCore::RenderTreeBuilder::Block::detach):
+        * rendering/updating/RenderTreeBuilderBlock.h:
+        * rendering/updating/RenderTreeBuilderList.cpp:
+        (WebCore::RenderTreeBuilder::List::updateItemMarker):
+
 2018-11-12  Javier Fernandez  <jfernandez@igalia.com>
 

trunk/Source/WebCore/rendering/updating/RenderTreeBuilder.cpp

@@ -318 +318 @@
 }
 
-RenderPtr<RenderObject> RenderTreeBuilder::detach(RenderElement& parent, RenderObject& child)
+RenderPtr<RenderObject> RenderTreeBuilder::detach(RenderElement& parent, RenderObject& child, CanCollapseAnonymousBlock canCollapseAnonymousBlock)
 {
     if (is<RenderRubyAsInline>(parent))
@@ -351 +351 @@
 
     if (is<RenderBlockFlow>(parent))
-        return blockBuilder().detach(downcast<RenderBlockFlow>(parent), child);
+        return blockBuilder().detach(downcast<RenderBlockFlow>(parent), child, canCollapseAnonymousBlock);
 
     if (is<RenderBlock>(parent))
-        return blockBuilder().detach(downcast<RenderBlock>(parent), child);
+        return blockBuilder().detach(downcast<RenderBlock>(parent), child, canCollapseAnonymousBlock);
 
     return detachFromRenderElement(parent, child);

trunk/Source/WebCore/rendering/updating/RenderTreeBuilder.h

@@ -45 +45 @@
     void attach(RenderElement& parent, RenderPtr<RenderObject>, RenderObject* beforeChild = nullptr);
 
-    RenderPtr<RenderObject> detach(RenderElement&, RenderObject&) WARN_UNUSED_RETURN;
+    enum class CanCollapseAnonymousBlock { No, Yes };
+    RenderPtr<RenderObject> detach(RenderElement&, RenderObject&, CanCollapseAnonymousBlock = CanCollapseAnonymousBlock::Yes) WARN_UNUSED_RETURN;
 
     void destroy(RenderObject& renderer);

trunk/Source/WebCore/rendering/updating/RenderTreeBuilderBlock.cpp

@@ -273 +273 @@
 }
 
-RenderPtr<RenderObject> RenderTreeBuilder::Block::detach(RenderBlock& parent, RenderObject& oldChild)
+RenderPtr<RenderObject> RenderTreeBuilder::Block::detach(RenderBlock& parent, RenderObject& oldChild, CanCollapseAnonymousBlock canCollapseAnonymousBlock)
 {
     // No need to waste time in merging or removing empty anonymous blocks.
@@ -330 +330 @@
     }
 
-    RenderObject* child = prev ? prev.get() : next.get();
-    if (canMergeAnonymousBlocks && child && !child->previousSibling() && !child->nextSibling() && parent.canDropAnonymousBlockChild()) {
-        // The removal has knocked us down to containing only a single anonymous
-        // box. We can pull the content right back up into our box.
-        dropAnonymousBoxChild(parent, downcast<RenderBlock>(*child));
-    } else if (((prev && prev->isAnonymousBlock()) || (next && next->isAnonymousBlock())) && parent.canDropAnonymousBlockChild()) {
-        // It's possible that the removal has knocked us down to a single anonymous
-        // block with floating siblings.
-        RenderBlock& anonBlock = downcast<RenderBlock>((prev && prev->isAnonymousBlock()) ? *prev : *next);
-        if (canDropAnonymousBlock(anonBlock)) {
-            bool dropAnonymousBlock = true;
-            for (auto& sibling : childrenOfType<RenderObject>(parent)) {
-                if (&sibling == &anonBlock)
-                    continue;
-                if (!sibling.isFloating()) {
-                    dropAnonymousBlock = false;
-                    break;
+    if (canCollapseAnonymousBlock == CanCollapseAnonymousBlock::Yes && parent.canDropAnonymousBlockChild()) {
+        RenderObject* child = prev ? prev.get() : next.get();
+        if (canMergeAnonymousBlocks && child && !child->previousSibling() && !child->nextSibling()) {
+            // The removal has knocked us down to containing only a single anonymous box. We can pull the content right back up into our box.
+            dropAnonymousBoxChild(parent, downcast<RenderBlock>(*child));
+        } else if ((prev && prev->isAnonymousBlock()) || (next && next->isAnonymousBlock())) {
+            // It's possible that the removal has knocked us down to a single anonymous block with floating siblings.
+            RenderBlock& anonBlock = downcast<RenderBlock>((prev && prev->isAnonymousBlock()) ? *prev : *next);
+            if (canDropAnonymousBlock(anonBlock)) {
+                bool dropAnonymousBlock = true;
+                for (auto& sibling : childrenOfType<RenderObject>(parent)) {
+                    if (&sibling == &anonBlock)
+                        continue;
+                    if (!sibling.isFloating()) {
+                        dropAnonymousBlock = false;
+                        break;
+                    }
                 }
+                if (dropAnonymousBlock)
+                    dropAnonymousBoxChild(parent, anonBlock);
             }
-            if (dropAnonymousBlock)
-                dropAnonymousBoxChild(parent, anonBlock);
         }
     }
@@ -374 +374 @@
 }
 
-RenderPtr<RenderObject> RenderTreeBuilder::Block::detach(RenderBlockFlow& parent, RenderObject& child)
+RenderPtr<RenderObject> RenderTreeBuilder::Block::detach(RenderBlockFlow& parent, RenderObject& child, CanCollapseAnonymousBlock canCollapseAnonymousBlock)
 {
     if (!parent.renderTreeBeingDestroyed()) {
@@ -381 +381 @@
             m_builder.multiColumnBuilder().multiColumnRelativeWillBeRemoved(*fragmentedFlow, child);
     }
-    return detach(static_cast<RenderBlock&>(parent), child);
-}
-
-}
+    return detach(static_cast<RenderBlock&>(parent), child, canCollapseAnonymousBlock);
+}
+
+}

trunk/Source/WebCore/rendering/updating/RenderTreeBuilderBlock.h

@@ -38 +38 @@
     void attachIgnoringContinuation(RenderBlock& parent, RenderPtr<RenderObject> child, RenderObject* beforeChild);
 
-    RenderPtr<RenderObject> detach(RenderBlock& parent, RenderObject& oldChild) WARN_UNUSED_RETURN;
-    RenderPtr<RenderObject> detach(RenderBlockFlow& parent, RenderObject& child) WARN_UNUSED_RETURN;
+    RenderPtr<RenderObject> detach(RenderBlock& parent, RenderObject& oldChild, CanCollapseAnonymousBlock = CanCollapseAnonymousBlock::Yes) WARN_UNUSED_RETURN;
+    RenderPtr<RenderObject> detach(RenderBlockFlow& parent, RenderObject& child, CanCollapseAnonymousBlock = CanCollapseAnonymousBlock::Yes) WARN_UNUSED_RETURN;
 
     void dropAnonymousBoxChild(RenderBlock& parent, RenderBlock& child);

trunk/Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp

@@ -116 +116 @@
 
     if (currentParent)
-        m_builder.attach(*newParent, m_builder.detach(*currentParent, *markerRenderer), firstNonMarkerChild(*newParent));
+        m_builder.attach(*newParent, m_builder.detach(*currentParent, *markerRenderer, RenderTreeBuilder::CanCollapseAnonymousBlock::No), firstNonMarkerChild(*newParent));
     else
         m_builder.attach(*newParent, WTFMove(newMarkerRenderer), firstNonMarkerChild(*newParent));