Do not collapse the soon-to-be-parent anon block when we shuffle around the marker...
authorzalan@apple.com <zalan@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 13 Nov 2018 02:38:06 +0000 (02:38 +0000)
committerzalan@apple.com <zalan@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 13 Nov 2018 02:38:06 +0000 (02:38 +0000)
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.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@238119 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/fast/inline/marker-list-item-move-should-not-crash-expected.txt [new file with mode: 0644]
LayoutTests/fast/inline/marker-list-item-move-should-not-crash.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/rendering/updating/RenderTreeBuilder.cpp
Source/WebCore/rendering/updating/RenderTreeBuilder.h
Source/WebCore/rendering/updating/RenderTreeBuilderBlock.cpp
Source/WebCore/rendering/updating/RenderTreeBuilderBlock.h
Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp

index 356a58f..1fa3e72 100644 (file)
@@ -1,3 +1,14 @@
+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>
 
         imported/w3c/web-platform-tests/IndexedDB/keygenerator-explicit.html crashing on iOS device
diff --git a/LayoutTests/fast/inline/marker-list-item-move-should-not-crash-expected.txt b/LayoutTests/fast/inline/marker-list-item-move-should-not-crash-expected.txt
new file mode 100644 (file)
index 0000000..ae2e352
--- /dev/null
@@ -0,0 +1 @@
+Pass if no crash.
diff --git a/LayoutTests/fast/inline/marker-list-item-move-should-not-crash.html b/LayoutTests/fast/inline/marker-list-item-move-should-not-crash.html
new file mode 100644 (file)
index 0000000..25c9a71
--- /dev/null
@@ -0,0 +1,13 @@
+<style>
+:matches(::marker) {
+   all: inherit;
+}
+</style>
+
+<li>Pass if no crash.</li>
+<script>
+if (window.testRunner)
+    testRunner.dumpAsText();
+document.body.offsetHeight;
+document.linkColor = "foobar";
+</script>
index 04a320d..06a24e9 100644 (file)
@@ -1,3 +1,29 @@
+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>
 
         [css-grid] Refactoring to make more explicit the orthogonal items' pre-layout logic
index 9d11d85..1f9faa6 100644 (file)
@@ -317,7 +317,7 @@ void RenderTreeBuilder::attachIgnoringContinuation(RenderElement& parent, Render
     attach(parent, WTFMove(child), beforeChild);
 }
 
-RenderPtr<RenderObject> RenderTreeBuilder::detach(RenderElement& parent, RenderObject& child)
+RenderPtr<RenderObject> RenderTreeBuilder::detach(RenderElement& parent, RenderObject& child, CanCollapseAnonymousBlock canCollapseAnonymousBlock)
 {
     if (is<RenderRubyAsInline>(parent))
         return rubyBuilder().detach(downcast<RenderRubyAsInline>(parent), child);
@@ -350,10 +350,10 @@ RenderPtr<RenderObject> RenderTreeBuilder::detach(RenderElement& parent, RenderO
         return svgBuilder().detach(downcast<RenderSVGRoot>(parent), child);
 
     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);
 }
index 7e93692..5a7b56d 100644 (file)
@@ -44,7 +44,8 @@ public:
     void attach(RenderTreePosition&, RenderPtr<RenderObject>);
     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);
 
index e5ca14e..85bdfe5 100644 (file)
@@ -272,7 +272,7 @@ void RenderTreeBuilder::Block::removeLeftoverAnonymousBlock(RenderBlock& anonymo
     // anonymousBlock is dead here.
 }
 
-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.
     // We can just bail out if our document is getting destroyed.
@@ -329,27 +329,27 @@ RenderPtr<RenderObject> RenderTreeBuilder::Block::detach(RenderBlock& parent, Re
         }
     }
 
-    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);
         }
     }
 
@@ -373,14 +373,14 @@ void RenderTreeBuilder::Block::dropAnonymousBoxChild(RenderBlock& parent, Render
     child.deleteLines();
 }
 
-RenderPtr<RenderObject> RenderTreeBuilder::Block::detach(RenderBlockFlow& parent, RenderObject& child)
+RenderPtr<RenderObject> RenderTreeBuilder::Block::detach(RenderBlockFlow& parent, RenderObject& child, CanCollapseAnonymousBlock canCollapseAnonymousBlock)
 {
     if (!parent.renderTreeBeingDestroyed()) {
         auto* fragmentedFlow = parent.multiColumnFlow();
         if (fragmentedFlow && fragmentedFlow != &child)
             m_builder.multiColumnBuilder().multiColumnRelativeWillBeRemoved(*fragmentedFlow, child);
     }
-    return detach(static_cast<RenderBlock&>(parent), child);
+    return detach(static_cast<RenderBlock&>(parent), child, canCollapseAnonymousBlock);
 }
 
 }
index c9b6f5e..c1fc993 100644 (file)
@@ -37,8 +37,8 @@ public:
     void attach(RenderBlock& parent, RenderPtr<RenderObject> child, RenderObject* beforeChild);
     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);
     void childBecameNonInline(RenderBlock& parent, RenderElement& child);
index 94e3dfe..be436e0 100644 (file)
@@ -115,7 +115,7 @@ void RenderTreeBuilder::List::updateItemMarker(RenderListItem& listItemRenderer)
         return;
 
     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));