- Revision
- 142500
- Author
- jchaffr...@webkit.org
- Date
- 2013-02-11 13:11:11 -0800 (Mon, 11 Feb 2013)
Log Message
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):
LayoutTests:
* fast/dynamic/continuation-detach-crash-expected.txt: Added.
* fast/dynamic/continuation-detach-crash.html: Added.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (142499 => 142500)
--- trunk/LayoutTests/ChangeLog 2013-02-11 21:04:50 UTC (rev 142499)
+++ trunk/LayoutTests/ChangeLog 2013-02-11 21:11:11 UTC (rev 142500)
@@ -1,3 +1,13 @@
+2013-02-11 Julien Chaffraix <jchaffr...@webkit.org>
+
+ Regression(r131539): Heap-use-after-free in WebCore::RenderBlock::willBeDestroyed
+ https://bugs.webkit.org/show_bug.cgi?id=107189
+
+ Reviewed by Abhishek Arya.
+
+ * fast/dynamic/continuation-detach-crash-expected.txt: Added.
+ * fast/dynamic/continuation-detach-crash.html: Added.
+
2013-02-11 Tony Chang <t...@chromium.org>
Move setFrameFlatteningEnabled from layoutTestController to window.internals.settings
Added: trunk/LayoutTests/fast/dynamic/continuation-detach-crash-expected.txt (0 => 142500)
--- trunk/LayoutTests/fast/dynamic/continuation-detach-crash-expected.txt (rev 0)
+++ trunk/LayoutTests/fast/dynamic/continuation-detach-crash-expected.txt 2013-02-11 21:11:11 UTC (rev 142500)
@@ -0,0 +1,5 @@
+Bug 107189: Regression(r131539): Heap-use-after-free in WebCore::RenderBlock::willBeDestroyed
+
+This test has PASSED if it didn't CRASH.
+
+
Property changes on: trunk/LayoutTests/fast/dynamic/continuation-detach-crash-expected.txt
___________________________________________________________________
Added: svn:eol-style
Added: trunk/LayoutTests/fast/dynamic/continuation-detach-crash.html (0 => 142500)
--- trunk/LayoutTests/fast/dynamic/continuation-detach-crash.html (rev 0)
+++ trunk/LayoutTests/fast/dynamic/continuation-detach-crash.html 2013-02-11 21:11:11 UTC (rev 142500)
@@ -0,0 +1,37 @@
+<!DOCTYPE html>
+<style>
+ .class1 { -webkit-column-span: all; }
+ .class2 > .class3 { float: right; content: open-quote; }
+ .class3:last-of-type { -webkit-column-width: 1px; }
+ .class3:nth-last-child(2n+10000000000000000) { -webkit-column-span: all;</style>
+<script>
+if (window.testRunner)
+ testRunner.dumpAsText();
+
+function crash() {
+ test1 = document.createElement('header');
+ test2 = document.createElement('footer');
+ test3 = document.createElement('address');
+ test3.setAttribute('class', 'class1');
+ test1.appendChild(test3);
+ test4 = document.createElement('nav');
+ test4.setAttribute('class', 'class3');
+ document.documentElement.appendChild(test4);
+ test5 = document.createElement('dt');
+ test5.setAttribute('class', 'class2');
+ test6 = document.createElement('a');
+ document.documentElement.appendChild(test6);
+ test7 = document.createElement('div');
+ test7.setAttribute('class', 'class3');
+ test5.appendChild(test7);
+ test5.appendChild(test1);
+ test4.appendChild(test5);
+ document.documentElement.offsetTop;
+ test5.setAttribute('class', 'class3');
+ document.documentElement.offsetTop;
+ test2.appendChild(test6);
+}
+window._onload_ = crash;
+</script>
+<p><a href="" 107189</a>: Regression(r131539): Heap-use-after-free in WebCore::RenderBlock::willBeDestroyed</p>
+<p>This test has PASSED if it didn't CRASH.</p>
Property changes on: trunk/LayoutTests/fast/dynamic/continuation-detach-crash.html
___________________________________________________________________
Added: svn:eol-style
Modified: trunk/Source/WebCore/ChangeLog (142499 => 142500)
--- trunk/Source/WebCore/ChangeLog 2013-02-11 21:04:50 UTC (rev 142499)
+++ trunk/Source/WebCore/ChangeLog 2013-02-11 21:11:11 UTC (rev 142500)
@@ -1,3 +1,45 @@
+2013-02-11 Julien Chaffraix <jchaffr...@webkit.org>
+
+ Regression(r131539): Heap-use-after-free in WebCore::RenderBlock::willBeDestroyed
+ https://bugs.webkit.org/show_bug.cgi?id=107189
+
+ Reviewed by Abhishek Arya.
+
+ 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):
+
2013-02-11 Eric Seidel <e...@webkit.org>
Make WebVTTTokenizer stop inheriting from MarkupTokenizerBase
Modified: trunk/Source/WebCore/dom/ContainerNode.cpp (142499 => 142500)
--- trunk/Source/WebCore/dom/ContainerNode.cpp 2013-02-11 21:04:50 UTC (rev 142499)
+++ trunk/Source/WebCore/dom/ContainerNode.cpp 2013-02-11 21:11:11 UTC (rev 142500)
@@ -825,9 +825,9 @@
void ContainerNode::detach()
{
- Node::detach();
detachChildren();
clearChildNeedsStyleRecalc();
+ Node::detach();
}
void ContainerNode::childrenChanged(bool changedByParser, Node*, Node*, int childCountDelta)
Modified: trunk/Source/WebCore/dom/Node.cpp (142499 => 142500)
--- trunk/Source/WebCore/dom/Node.cpp 2013-02-11 21:04:50 UTC (rev 142499)
+++ trunk/Source/WebCore/dom/Node.cpp 2013-02-11 21:11:11 UTC (rev 142500)
@@ -1106,24 +1106,10 @@
detachingNode = this;
#endif
- if (renderer()) {
+ if (renderer())
renderer()->destroyAndCleanupAnonymousWrappers();
-#ifndef NDEBUG
- for (Node* node = this; node; node = NodeTraversal::next(node, 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
- }
+ setRenderer(0);
- ASSERT(!renderer());
-
Document* doc = document();
if (isUserActionElement()) {
if (hovered())
Modified: trunk/Source/WebCore/rendering/RenderObject.cpp (142499 => 142500)
--- trunk/Source/WebCore/rendering/RenderObject.cpp 2013-02-11 21:04:50 UTC (rev 142499)
+++ trunk/Source/WebCore/rendering/RenderObject.cpp 2013-02-11 21:11:11 UTC (rev 142500)
@@ -2391,11 +2391,6 @@
if (AXObjectCache::accessibilityEnabled())
document()->axObjectCache()->remove(this);
- // 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.
@@ -2529,18 +2524,6 @@
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.
Modified: trunk/Source/WebCore/rendering/RenderObjectChildList.cpp (142499 => 142500)
--- trunk/Source/WebCore/rendering/RenderObjectChildList.cpp 2013-02-11 21:04:50 UTC (rev 142499)
+++ trunk/Source/WebCore/rendering/RenderObjectChildList.cpp 2013-02-11 21:11:11 UTC (rev 142500)
@@ -41,10 +41,13 @@
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();
}
}
@@ -58,11 +61,14 @@
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();
}
Modified: trunk/Source/WebCore/rendering/RenderTextFragment.cpp (142499 => 142500)
--- trunk/Source/WebCore/rendering/RenderTextFragment.cpp 2013-02-11 21:04:50 UTC (rev 142499)
+++ trunk/Source/WebCore/rendering/RenderTextFragment.cpp 2013-02-11 21:11:11 UTC (rev 142500)
@@ -85,7 +85,10 @@
ASSERT(!m_contentString);
m_firstLetter->destroy();
m_firstLetter = 0;
- ASSERT(!node() || node()->renderer() == this);
+ if (Node* t = node()) {
+ ASSERT(!t->renderer());
+ t->setRenderer(this);
+ }
}
}
