.: Heap-use-after-free in WebCore::ScrollingCoordinator::hasVisibleSlowRepaintViewpor...
authorwangxianzhu@chromium.org <wangxianzhu@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 13 Feb 2013 18:37:30 +0000 (18:37 +0000)
committerwangxianzhu@chromium.org <wangxianzhu@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 13 Feb 2013 18:37:30 +0000 (18:37 +0000)
https://bugs.webkit.org/show_bug.cgi?id=108695

Add a manual test. Unable to write a normal layout test because
1) must waitUntilDone() to reproduce the crash but the redirected URL can't notifyDone();
2) Can't use a frame to contain the test because ScrollingCoordinator handles only the main frame.

Reviewed by Abhishek Arya.

* ManualTests/scrolling-coordinator-viewport-constrained-crash.html: Added.

Source/WebCore: Heap-use-after-free in WebCore::ScrollingCoordinator::hasVisibleSlowRepaintViewportConstrainedObjects
https://bugs.webkit.org/show_bug.cgi?id=108695

See comments of RenderLayerModelObject::willBeDestroyed() below for details.

Reviewed by Abhishek Arya.

Test: ManulTests/scrolling-coordinator-viewport-constrained-crash.html
Unable to write a normal layout test because
1) must waitUntilDone() to reproduce the crash but the redirected URL can't notifyDone();
2) Can't use a frame to contain the test because ScrollingCoordinator handles only the main frame.

* rendering/RenderBoxModelObject.cpp:
(WebCore::RenderBoxModelObject::willBeDestroyed): Moved removeViewportConstrainedObject() call into RenderLayerModelObject::willBeDestroyed() because only RenderLayerModelObjects can be added as viewportConstrainedObjects.
* rendering/RenderLayerModelObject.cpp:
(WebCore::RenderLayerModelObject::willBeDestroyed): Changed this->view() (then view->frameView()) to this->frame() (then frame->view()) because when willBeDestroyed() is called, the document has set its renderView to 0 thus this->view() will return 0, causing removeViewportConstrainedObject() not called and a deleted RenderLayerModelObject in FrameView's viewportConstrainedObjects.

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

ChangeLog
ManualTests/scrolling-coordinator-viewport-constrained-crash.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/rendering/RenderBoxModelObject.cpp
Source/WebCore/rendering/RenderLayerModelObject.cpp

index 1bd33dd..6c32452 100644 (file)
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,16 @@
+2013-02-13  Xianzhu Wang  <wangxianzhu@chromium.org>
+
+        Heap-use-after-free in WebCore::ScrollingCoordinator::hasVisibleSlowRepaintViewportConstrainedObjects.
+        https://bugs.webkit.org/show_bug.cgi?id=108695
+
+        Add a manual test. Unable to write a normal layout test because
+        1) must waitUntilDone() to reproduce the crash but the redirected URL can't notifyDone();
+        2) Can't use a frame to contain the test because ScrollingCoordinator handles only the main frame.
+
+        Reviewed by Abhishek Arya.
+
+        * ManualTests/scrolling-coordinator-viewport-constrained-crash.html: Added.
+
 2013-02-13  Martin Robinson  <mrobinson@igalia.com>
 
         [GTK] Remove support for compiling with GStreamer 0.10
diff --git a/ManualTests/scrolling-coordinator-viewport-constrained-crash.html b/ManualTests/scrolling-coordinator-viewport-constrained-crash.html
new file mode 100644 (file)
index 0000000..8b17717
--- /dev/null
@@ -0,0 +1,20 @@
+<!DOCTYPE html>
+<html>
+<head>
+  <script>
+  function test()
+  {
+    if (window.testRunner) {
+      testRunner.dumpAsText();
+      testRunner.waitUntilDone();
+    }
+    window.location.href = "javascript:'>'";
+  }
+  </script>
+</head>
+<body onload="test()">
+  <!-- Tests https://bugs.webkit.org/show_bug.cgi?id=108695. Should not crash. -->
+  <div style="position: fixed"></div>
+  <div style="position: -webkit-sticky"></div>
+</body>
+</html>
index 437a0d1..20f2c66 100644 (file)
@@ -1,3 +1,22 @@
+2013-02-13  Xianzhu Wang  <wangxianzhu@chromium.org>
+
+        Heap-use-after-free in WebCore::ScrollingCoordinator::hasVisibleSlowRepaintViewportConstrainedObjects
+        https://bugs.webkit.org/show_bug.cgi?id=108695
+
+        See comments of RenderLayerModelObject::willBeDestroyed() below for details.
+
+        Reviewed by Abhishek Arya.
+
+        Test: ManulTests/scrolling-coordinator-viewport-constrained-crash.html
+        Unable to write a normal layout test because
+        1) must waitUntilDone() to reproduce the crash but the redirected URL can't notifyDone();
+        2) Can't use a frame to contain the test because ScrollingCoordinator handles only the main frame.
+
+        * rendering/RenderBoxModelObject.cpp:
+        (WebCore::RenderBoxModelObject::willBeDestroyed): Moved removeViewportConstrainedObject() call into RenderLayerModelObject::willBeDestroyed() because only RenderLayerModelObjects can be added as viewportConstrainedObjects.
+        * rendering/RenderLayerModelObject.cpp:
+        (WebCore::RenderLayerModelObject::willBeDestroyed): Changed this->view() (then view->frameView()) to this->frame() (then frame->view()) because when willBeDestroyed() is called, the document has set its renderView to 0 thus this->view() will return 0, causing removeViewportConstrainedObject() not called and a deleted RenderLayerModelObject in FrameView's viewportConstrainedObjects.
+
 2013-02-13  Florin Malita  <fmalita@chromium.org>
 
         [SVG] OOB access in SVGListProperty::replaceItemValues()
index 00886b3..58ba294 100644 (file)
@@ -331,15 +331,6 @@ void RenderBoxModelObject::willBeDestroyed()
     // A continuation of this RenderObject should be destroyed at subclasses.
     ASSERT(!continuation());
 
-    if (isPositioned()) {
-        if (RenderView* view = this->view()) {
-            if (FrameView* frameView = view->frameView()) {
-                if (style()->hasViewportConstrainedPosition())
-                    frameView->removeViewportConstrainedObject(this);
-            }
-        }
-    }
-
     // If this is a first-letter object with a remaining text fragment then the
     // entry needs to be cleared from the map.
     if (firstLetterRemainingText())
index d4af219..ec7d69b 100644 (file)
@@ -76,6 +76,16 @@ bool RenderLayerModelObject::hasSelfPaintingLayer() const
 
 void RenderLayerModelObject::willBeDestroyed()
 {
+    if (isPositioned()) {
+        // Don't use this->view() because the document's renderView has been set to 0 during destruction.
+        if (Frame* frame = this->frame()) {
+            if (FrameView* frameView = frame->view()) {
+                if (style()->hasViewportConstrainedPosition())
+                    frameView->removeViewportConstrainedObject(this);
+            }
+        }
+    }
+
     // RenderObject::willBeDestroyed calls back to destroyLayer() for layer destruction
     RenderObject::willBeDestroyed();
 }