Regression(r131539): Heap-use-after-free in WebCore::RenderBlock::willBeDestroyed

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

Reviewed by Abhishek Arya.

Source/WebCore:

Test: fast/dynamic/continuation-detach-crash.html

This patch reverts r131539 and the following changes (r132591 and r139664).
This means we redo detaching from the bottom-up which solves the regression.
It fixes the attached test case as we re-attach child nodes before detaching
the parent. It seems wrong to do but this avoid a stale continuation.

* dom/ContainerNode.cpp:
(WebCore::ContainerNode::detach): Detach the children first, then ourself.
* dom/Node.cpp:
(WebCore::Node::detach): Clear the renderer instead of ASSERT'ing.
* rendering/RenderObject.cpp:
(WebCore::RenderObject::willBeDestroyed): Removed the code to clear the associated node's renderer.
(WebCore::RenderObject::destroyAndCleanupAnonymousWrappers):
* rendering/RenderObjectChildList.cpp:
(WebCore::RenderObjectChildList::removeChildNode):
Moved the repainting logic back into removeChildNode from destroyAndCleanupAnonymousWrappers.
(WebCore::RenderObjectChildList::destroyLeftoverChildren): Re-added the code to clear the associated node's
renderer.
* rendering/RenderTextFragment.cpp:
(WebCore::RenderTextFragment::setText): Re-added the code to set the associated node's renderer.

* dom/ContainerNode.cpp:
(WebCore::ContainerNode::detach):
* dom/Node.cpp:
(WebCore::Node::detach):
* rendering/RenderObject.cpp:
(WebCore::RenderObject::willBeDestroyed):
(WebCore::RenderObject::destroyAndCleanupAnonymousWrappers):
* rendering/RenderObjectChildList.cpp:
(WebCore::RenderObjectChildList::destroyLeftoverChildren):
(WebCore::RenderObjectChildList::removeChildNode):
* rendering/RenderTextFragment.cpp:
(WebCore::RenderTextFragment::setText):

Change-Id: I5c4df1881f041ecd80180cb1638cd811d0972189
git-svn-id: http://svn.webkit.org/repository/webkit/trunk@142500 268f45cc-cd09-0410-ab3c-d52691b4dbfc
Reviewed-by: Jocelyn Turcotte <jocelyn.turcotte@digia.com>

5 files changed, 15 insertions, 36 deletions

diff --git a/Source/WebCore/dom/ContainerNode.cpp b/Source/WebCore/dom/ContainerNode.cpp
index f3c6abd38..a8ae7c893 100644
--- a/Source/WebCore/dom/ContainerNode.cpp
+++ b/Source/WebCore/dom/ContainerNode.cpp
@@ -777,9 +777,9 @@ void ContainerNode::attach()
 
 void ContainerNode::detach()
 {
-    Node::detach();
     detachChildren();
     clearChildNeedsStyleRecalc();
+    Node::detach();
 }
 
 void ContainerNode::childrenChanged(bool changedByParser, Node*, Node*, int childCountDelta)
diff --git a/Source/WebCore/dom/Node.cpp b/Source/WebCore/dom/Node.cpp
index 1d9dfc44d..39d295f52 100644
--- a/Source/WebCore/dom/Node.cpp
+++ b/Source/WebCore/dom/Node.cpp
@@ -1227,23 +1227,10 @@ void Node::detach()
     detachingNode = this;
 #endif
 
-    if (renderer()) {
+    if (renderer())
         renderer()->destroyAndCleanupAnonymousWrappers();
-#ifndef NDEBUG
-        for (Node* node = this; node; node = node->traverseNextNode(this)) {
-            RenderObject* renderer = node->renderer();
-            // RenderFlowThread and the top layer remove elements from the regular tree
-            // hierarchy. They will be cleaned up when we call detach on them.
-#if ENABLE(DIALOG_ELEMENT)
-            ASSERT(!renderer || renderer->inRenderFlowThread() || (renderer->enclosingLayer()->isInTopLayerSubtree()));
-#else
-            ASSERT(!renderer || renderer->inRenderFlowThread());
-#endif
-        }
-#endif
-    }
 
-    ASSERT(!renderer());
+    setRenderer(0);
 
     Document* doc = document();
     if (hovered())
diff --git a/Source/WebCore/rendering/RenderObject.cpp b/Source/WebCore/rendering/RenderObject.cpp
index b3f325e2a..17bdafb54 100644
--- a/Source/WebCore/rendering/RenderObject.cpp
+++ b/Source/WebCore/rendering/RenderObject.cpp
@@ -2361,11 +2361,6 @@ void RenderObject::willBeDestroyed()
 
     remove();
 
-    // Continuation and first-letter can generate several renderers associated with a single node.
-    // We only want to clear the node's renderer if we are the associated renderer.
-    if (node() && node()->renderer() == this)
-        node()->setRenderer(0);
-
 #ifndef NDEBUG
     if (!documentBeingDestroyed() && view() && view()->hasRenderNamedFlowThreads()) {
         // After remove, the object and the associated information should not be in any flow thread.
@@ -2499,18 +2494,6 @@ void RenderObject::destroyAndCleanupAnonymousWrappers()
             break;
     }
 
-    // We repaint, so that the area exposed when this object disappears gets repainted properly.
-    // FIXME: A RenderObject with RenderLayer should probably repaint through it as getting the
-    // repaint rects is O(1) through a RenderLayer (assuming it's up-to-date).
-    if (destroyRoot->everHadLayout()) {
-        if (destroyRoot->isBody())
-            destroyRoot->view()->repaint();
-        else {
-            destroyRoot->repaint();
-            destroyRoot->repaintOverhangingFloats(true);
-        }
-    }
-
     destroyRoot->destroy();
 
     // WARNING: |this| is deleted here.
diff --git a/Source/WebCore/rendering/RenderObjectChildList.cpp b/Source/WebCore/rendering/RenderObjectChildList.cpp
index 202cc36b3..e2a95a207 100644
--- a/Source/WebCore/rendering/RenderObjectChildList.cpp
+++ b/Source/WebCore/rendering/RenderObjectChildList.cpp
@@ -48,10 +48,13 @@ void RenderObjectChildList::destroyLeftoverChildren()
         if (firstChild()->isListMarker() || (firstChild()->style()->styleType() == FIRST_LETTER && !firstChild()->isText()))
             firstChild()->remove();  // List markers are owned by their enclosing list and so don't get destroyed by this container. Similarly, first letters are destroyed by their remaining text fragment.
         else if (firstChild()->isRunIn() && firstChild()->node()) {
+            firstChild()->node()->setRenderer(0);
             firstChild()->node()->setNeedsStyleRecalc();
             firstChild()->destroy();
         } else {
             // Destroy any anonymous children remaining in the render tree, as well as implicit (shadow) DOM elements like those used in the engine-based text fields.
+            if (firstChild()->node())
+                firstChild()->node()->setRenderer(0);
             firstChild()->destroy();
         }
     }
@@ -65,11 +68,14 @@ RenderObject* RenderObjectChildList::removeChildNode(RenderObject* owner, Render
         toRenderBox(oldChild)->removeFloatingOrPositionedChildFromBlockLists();
 
     // So that we'll get the appropriate dirty bit set (either that a normal flow child got yanked or
-    // that a positioned child got yanked).
+    // that a positioned child got yanked). We also repaint, so that the area exposed when the child
+    // disappears gets repainted properly.
     if (!owner->documentBeingDestroyed() && notifyRenderer && oldChild->everHadLayout()) {
         oldChild->setNeedsLayoutAndPrefWidthsRecalc();
         // We only repaint |oldChild| if we have a RenderLayer as its visual overflow may not be tracked by its parent.
-        if (oldChild->hasLayer())
+        if (oldChild->isBody())
+            owner->view()->repaint();
+        else
             oldChild->repaint();
     }
 
diff --git a/Source/WebCore/rendering/RenderTextFragment.cpp b/Source/WebCore/rendering/RenderTextFragment.cpp
index 5d6e165d6..9f497d344 100644
--- a/Source/WebCore/rendering/RenderTextFragment.cpp
+++ b/Source/WebCore/rendering/RenderTextFragment.cpp
@@ -85,7 +85,10 @@ void RenderTextFragment::setText(PassRefPtr<StringImpl> text, bool force)
         ASSERT(!m_contentString);
         m_firstLetter->destroy();
         m_firstLetter = 0;
-        ASSERT(!node() || node()->renderer() == this);
+        if (Node* t = node()) {
+            ASSERT(!t->renderer());
+            t->setRenderer(this);
+        }
     }
 }