Heap-use-after-free in WebCore::RenderLayer::addChild
authoralokp@chromium.org <alokp@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 28 Feb 2012 19:15:16 +0000 (19:15 +0000)
committeralokp@chromium.org <alokp@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 28 Feb 2012 19:15:16 +0000 (19:15 +0000)
https://bugs.webkit.org/show_bug.cgi?id=79698

Reviewed by Simon Fraser.

Source/WebCore:

This patch fixes a regression introduced in r108659.
The reflection layer was moved to the parent by mistake. It was then
deleted and the parent was left holding on to a deleted pointer. This
patch restores the location where reflection layer is removed - before
moving the child layers.

Test: fast/reflections/toggle-reflection-crash.html

* rendering/RenderLayer.cpp:
(WebCore::RenderLayer::removeOnlyThisLayer):

LayoutTests:

* fast/reflections/toggle-reflection-crash-expected.txt: Added.
* fast/reflections/toggle-reflection-crash.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/fast/reflections/toggle-reflection-crash-expected.txt [new file with mode: 0644]
LayoutTests/fast/reflections/toggle-reflection-crash.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/rendering/RenderLayer.cpp

index ea68d46..8f450b9 100644 (file)
@@ -1,3 +1,13 @@
+2012-02-28  Alok Priyadarshi  <alokp@chromium.org>
+
+        Heap-use-after-free in WebCore::RenderLayer::addChild
+        https://bugs.webkit.org/show_bug.cgi?id=79698
+
+        Reviewed by Simon Fraser.
+
+        * fast/reflections/toggle-reflection-crash-expected.txt: Added.
+        * fast/reflections/toggle-reflection-crash.html: Added.
+
 2012-02-28  Ken Buchanan  <kenrb@chromium.org>
 
         Crash from list marker having inline and block children
diff --git a/LayoutTests/fast/reflections/toggle-reflection-crash-expected.txt b/LayoutTests/fast/reflections/toggle-reflection-crash-expected.txt
new file mode 100644 (file)
index 0000000..792cd52
--- /dev/null
@@ -0,0 +1,3 @@
+This test should not crash when run with Address Sanitizer.
+
+
diff --git a/LayoutTests/fast/reflections/toggle-reflection-crash.html b/LayoutTests/fast/reflections/toggle-reflection-crash.html
new file mode 100644 (file)
index 0000000..202ddd5
--- /dev/null
@@ -0,0 +1,37 @@
+<!DOCTYPE html>
+
+<html>
+<head>
+<script>
+  var reflectionOn = true;
+  function toggleReflection() {
+    reflectionOn = !reflectionOn;
+    var box = document.getElementById('box');
+    box.style.webkitBoxReflect = reflectionOn ? 'below' : 'none';
+  };
+
+  function finishTest() {
+    toggleReflection();
+    if (window.layoutTestController)
+      layoutTestController.notifyDone();
+  };
+
+  function startTest() {
+    if (window.layoutTestController) {
+      layoutTestController.waitUntilDone();
+      layoutTestController.dumpAsText();
+    }
+    toggleReflection();
+    window.setTimeout(function() { finishTest(); }, 0);
+  };
+
+  window.addEventListener('load', startTest, false);
+
+</script>
+</head>
+
+<body>
+  <p>This test should not crash when run with Address Sanitizer.</p>
+  <div id="box" style="-webkit-box-reflect: below;"</div>
+</body>
+</html>
index 8ff760a..b09fd21 100644 (file)
@@ -1,3 +1,21 @@
+2012-02-28  Alok Priyadarshi  <alokp@chromium.org>
+
+        Heap-use-after-free in WebCore::RenderLayer::addChild
+        https://bugs.webkit.org/show_bug.cgi?id=79698
+
+        Reviewed by Simon Fraser.
+
+        This patch fixes a regression introduced in r108659.
+        The reflection layer was moved to the parent by mistake. It was then
+        deleted and the parent was left holding on to a deleted pointer. This
+        patch restores the location where reflection layer is removed - before
+        moving the child layers.
+
+        Test: fast/reflections/toggle-reflection-crash.html
+
+        * rendering/RenderLayer.cpp:
+        (WebCore::RenderLayer::removeOnlyThisLayer):
+
 2012-02-28  Ken Buchanan  <kenrb@chromium.org>
 
         Crash from list marker having inline and block children
index 7720ce6..0fb0b64 100644 (file)
@@ -1197,6 +1197,11 @@ void RenderLayer::removeOnlyThisLayer()
     bool hasLayerOffset;
     const LayoutPoint offsetFromRootBeforeMove = computeOffsetFromRoot(hasLayerOffset);
 
+    // Remove the child reflection layer before moving other child layers.
+    // The reflection layer should not be moved to the parent.
+    if (reflection())
+        removeChild(reflectionLayer());
+
     // Now walk our kids and reattach them to our parent.
     RenderLayer* current = m_first;
     while (current) {
@@ -1212,10 +1217,7 @@ void RenderLayer::removeOnlyThisLayer()
     }
 
     // Remove us from the parent.
-    if (reflection())
-        removeChild(reflectionLayer());
     m_parent->removeChild(this);
-
     m_renderer->destroyLayer();
 }