[RenderTreeBuilder] REGRESSION(r228238) Detach renderer before destroying its subtree.
authorzalan@apple.com <zalan@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sun, 18 Feb 2018 18:44:34 +0000 (18:44 +0000)
committerzalan@apple.com <zalan@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sun, 18 Feb 2018 18:44:34 +0000 (18:44 +0000)
https://bugs.webkit.org/show_bug.cgi?id=182908
<rdar://problem/37619394>

Reviewed by Antti Koivisto.

Source/WebCore:

Prior to r228238 we first detached the to-be-destroyed renderer and then
started nuking its descendants. r228238 changed the order and now the descendants are
destroyed while they are still attached to the tree. Apparently some of the takeChild()
normalization logic gets triggered now that the renderers still have access to their previous/next
siblings. This is unexpected and it shouldn't matter whether the subtree is still attached.
Let's revert it to the original order for now (see webkit.org/b/182909).

Test: fast/block/crash-when-subtree-is-still-attached.html

* rendering/RenderElement.cpp:
(WebCore::RenderElement::removeAndDestroyChild):

LayoutTests:

* fast/block/crash-when-subtree-is-still-attached-expected.txt: Added.
* fast/block/crash-when-subtree-is-still-attached.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/fast/block/crash-when-subtree-is-still-attached-expected.txt [new file with mode: 0644]
LayoutTests/fast/block/crash-when-subtree-is-still-attached.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/rendering/RenderElement.cpp

index 8fd1d70..1ca15e1 100644 (file)
@@ -1,3 +1,14 @@
+2018-02-18  Zalan Bujtas  <zalan@apple.com>
+
+        [RenderTreeBuilder] REGRESSION(r228238) Detach renderer before destroying its subtree.
+        https://bugs.webkit.org/show_bug.cgi?id=182908
+        <rdar://problem/37619394>
+
+        Reviewed by Antti Koivisto.
+
+        * fast/block/crash-when-subtree-is-still-attached-expected.txt: Added.
+        * fast/block/crash-when-subtree-is-still-attached.html: Added.
+
 2018-02-16  Ryan Haddad  <ryanhaddad@apple.com>
 
         Unreviewed, rolling out r228575.
diff --git a/LayoutTests/fast/block/crash-when-subtree-is-still-attached-expected.txt b/LayoutTests/fast/block/crash-when-subtree-is-still-attached-expected.txt
new file mode 100644 (file)
index 0000000..0a1653b
--- /dev/null
@@ -0,0 +1,3 @@
+PASS if
+no crash.
+
diff --git a/LayoutTests/fast/block/crash-when-subtree-is-still-attached.html b/LayoutTests/fast/block/crash-when-subtree-is-still-attached.html
new file mode 100644 (file)
index 0000000..24ce206
--- /dev/null
@@ -0,0 +1,13 @@
+<!DOCTYPE html>
+<html>
+<body>
+<b>PASS if<div></div> no crash.<div></div>foobar</b><div></div>
+<script>
+if (window.testRunner)
+    testRunner.dumpAsText();
+document.designMode = "on";
+document.execCommand("SelectAll");
+document.execCommand("RemoveFormat", true, "Arial");
+</script>
+</body>
+</html>
\ No newline at end of file
index a55c09f..07a9c02 100644 (file)
@@ -1,3 +1,23 @@
+2018-02-18  Zalan Bujtas  <zalan@apple.com>
+
+        [RenderTreeBuilder] REGRESSION(r228238) Detach renderer before destroying its subtree.
+        https://bugs.webkit.org/show_bug.cgi?id=182908
+        <rdar://problem/37619394>
+
+        Reviewed by Antti Koivisto.
+
+        Prior to r228238 we first detached the to-be-destroyed renderer and then
+        started nuking its descendants. r228238 changed the order and now the descendants are
+        destroyed while they are still attached to the tree. Apparently some of the takeChild()
+        normalization logic gets triggered now that the renderers still have access to their previous/next
+        siblings. This is unexpected and it shouldn't matter whether the subtree is still attached.
+        Let's revert it to the original order for now (see webkit.org/b/182909).
+
+        Test: fast/block/crash-when-subtree-is-still-attached.html
+
+        * rendering/RenderElement.cpp:
+        (WebCore::RenderElement::removeAndDestroyChild):
+
 2018-02-18  Charlie Turner  <cturner@igalia.com>
 
         [GStreamer] Push smaller buffers from HTTP source
index 9382c43..85dd892 100644 (file)
@@ -475,16 +475,20 @@ void RenderElement::didInsertChild(RenderObject& child, RenderObject*)
 
 void RenderElement::removeAndDestroyChild(RenderTreeBuilder& builder, RenderObject& oldChild)
 {
-    if (is<RenderElement>(oldChild)) {
-        auto& child = downcast<RenderElement>(oldChild);
-        while (child.firstChild()) {
-            auto& firstChild = *child.firstChild();
-            if (auto* node = firstChild.node())
-                node->setRenderer(nullptr);
-            child.removeAndDestroyChild(builder, firstChild);
-        }
-    }
     auto toDestroy = builder.takeChild(*this, oldChild);
+    // We need to detach the subtree first so that the descendants don't have
+    // access to previous/next sublings at takeChild().
+    // FIXME: webkit.org/b/182909.
+    if (!is<RenderElement>(toDestroy.get()))
+        return;
+
+    auto& child = downcast<RenderElement>(*toDestroy.get());
+    while (child.firstChild()) {
+        auto& firstChild = *child.firstChild();
+        if (auto* node = firstChild.node())
+            node->setRenderer(nullptr);
+        child.removeAndDestroyChild(builder, firstChild);
+    }
 }
 
 RenderObject* RenderElement::attachRendererInternal(RenderPtr<RenderObject> child, RenderObject* beforeChild)