https://bugs.webkit.org/show_bug.cgi?id=57736
authorhyatt@apple.com <hyatt@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 7 Apr 2011 22:33:32 +0000 (22:33 +0000)
committerhyatt@apple.com <hyatt@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 7 Apr 2011 22:33:32 +0000 (22:33 +0000)
Reviewed by Dan Bernstein.

Crash on openstreetmap.org while using the map. Fix a bad interaction between the positioned movement layout
optimization and the simplified layout optimization that could lead to blocks remaining marked as dirty when
layout finished. This would eventually lead to an inability to properly determine the correct layout root and
would cause a deleted root to be used later on.

Added fast/block/positioning/complex-positioned-movement.html.

Source/WebCore:

* page/FrameView.cpp:
(WebCore::FrameView::scheduleRelayoutOfSubtree):
Add asserts to catch cases in the future where a layout root is set that has a dirty containing block.

* rendering/RenderBlock.cpp:
(WebCore::RenderBlock::simplifiedLayout):
Change simplified layout so that the positioned movement optimization no longer clears the other layout
flags. This will ensure that we still lay out our descendants if they need it.

(WebCore::RenderBlock::layoutPositionedObjects):
Changed to clear our layout flags now if the positioned movement is successful, since tryLayoutDoingPositionedMovementOnly
no longer does the clear.

* rendering/RenderBox.h:
(WebCore::RenderBox::tryLayoutDoingPositionedMovementOnly):
tryLayoutDoingPositionedMovementOnly now returns a boolean indicating success or failure.  On success it no longer
does setNeedsLayout(false), but instead will let the caller take care of it. This way the caller can still look at
the other flags in case other kinds of layout are still needed.

* rendering/RenderObject.h:
(WebCore::RenderObject::setNeedsPositionedMovementLayout):
(WebCore::RenderObject::setNeedsSimplifiedNormalFlowLayout):
Changed these methods to only set their respective flags and not to try to be clever about avoiding propagation.

LayoutTests:

* fast/block/positioning/complex-positioned-movement.html: Added.
* platform/mac/fast/block/positioning/complex-positioned-movement-expected.checksum: Added.
* platform/mac/fast/block/positioning/complex-positioned-movement-expected.png: Added.
* platform/mac/fast/block/positioning/complex-positioned-movement-expected.txt: Added.

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

LayoutTests/ChangeLog
LayoutTests/fast/block/positioning/complex-positioned-movement.html [new file with mode: 0644]
LayoutTests/platform/mac/fast/block/positioning/complex-positioned-movement-expected.checksum [new file with mode: 0644]
LayoutTests/platform/mac/fast/block/positioning/complex-positioned-movement-expected.png [new file with mode: 0644]
LayoutTests/platform/mac/fast/block/positioning/complex-positioned-movement-expected.txt [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/page/FrameView.cpp
Source/WebCore/rendering/RenderBlock.cpp
Source/WebCore/rendering/RenderBox.h
Source/WebCore/rendering/RenderObject.h

index aa6c208..a4f4b90 100644 (file)
@@ -1,3 +1,21 @@
+2011-04-07  David Hyatt  <hyatt@apple.com>
+
+        Reviewed by Dan Bernstein.
+
+        https://bugs.webkit.org/show_bug.cgi?id=57736
+        
+        Crash on openstreetmap.org while using the map. Fix a bad interaction between the positioned movement layout
+        optimization and the simplified layout optimization that could lead to blocks remaining marked as dirty when
+        layout finished. This would eventually lead to an inability to properly determine the correct layout root and
+        would cause a deleted root to be used later on.
+
+        Added fast/block/positioning/complex-positioned-movement.html.
+
+        * fast/block/positioning/complex-positioned-movement.html: Added.
+        * platform/mac/fast/block/positioning/complex-positioned-movement-expected.checksum: Added.
+        * platform/mac/fast/block/positioning/complex-positioned-movement-expected.png: Added.
+        * platform/mac/fast/block/positioning/complex-positioned-movement-expected.txt: Added.
+
 2011-04-07  Dan Bernstein  <mitz@apple.com>
 
         Reviewed by Simon Fraser.
diff --git a/LayoutTests/fast/block/positioning/complex-positioned-movement.html b/LayoutTests/fast/block/positioning/complex-positioned-movement.html
new file mode 100644 (file)
index 0000000..e3c491c
--- /dev/null
@@ -0,0 +1,51 @@
+<html>
+<head>
+<title>Complex positioned movement test</title>
+<style>
+.block {
+    position: absolute;
+    left:0;
+    top:0;
+    width: 100px;
+    height: 100px;
+    background-color: green;
+}
+</style>
+</head>
+<div style="position:relative">
+<div class="block" id="outer">
+<div class="block" id="inner">
+</div>
+</div>
+</div>
+<!--PASS if you don't crash<br>
+and green block is shifted.
+-->
+<script>
+// Update layout.
+document.body.offsetLeft;
+
+// First do something to dirty the outer block indirectly.  Adding a child will result
+// in the following flag states:
+// inner - posChildNeedsLayout, normalChildNeedsLayout
+// outer - simplifiedNormalFlowLayout
+document.getElementById('inner').innerHTML = "<div class='block' id='problem' style='overflow:hidden'></div>";
+
+// Next, move the outer block. This will set the needsPositionedMovementLayout flag along with the
+// other two flags that got set.
+document.getElementById('outer').style.left = '300px';
+
+// Now update layout. If we incorrectly try to do only a positioned movement layout, then the
+// inner block is now left with its two flags set.
+document.body.offsetLeft;
+
+// Now let's do something to make inner's child the layout root.
+document.getElementById('problem').innerHTML = "Some text.";
+
+// Now that the problem child has become the layout root, let's destroy it and watch things go horribly wrong.
+document.getElementById('problem').style.display = 'none';
+
+// Final layout to trigger the crash
+document.body.offsetLeft;
+</script>
+
diff --git a/LayoutTests/platform/mac/fast/block/positioning/complex-positioned-movement-expected.checksum b/LayoutTests/platform/mac/fast/block/positioning/complex-positioned-movement-expected.checksum
new file mode 100644 (file)
index 0000000..169d248
--- /dev/null
@@ -0,0 +1 @@
+28cf6f529a112923fb02694ac11e3c1a
\ No newline at end of file
diff --git a/LayoutTests/platform/mac/fast/block/positioning/complex-positioned-movement-expected.png b/LayoutTests/platform/mac/fast/block/positioning/complex-positioned-movement-expected.png
new file mode 100644 (file)
index 0000000..24d33d5
Binary files /dev/null and b/LayoutTests/platform/mac/fast/block/positioning/complex-positioned-movement-expected.png differ
diff --git a/LayoutTests/platform/mac/fast/block/positioning/complex-positioned-movement-expected.txt b/LayoutTests/platform/mac/fast/block/positioning/complex-positioned-movement-expected.txt
new file mode 100644 (file)
index 0000000..ad74281
--- /dev/null
@@ -0,0 +1,9 @@
+layer at (0,0) size 800x600
+  RenderView at (0,0) size 800x600
+layer at (0,0) size 800x600
+  RenderBlock {HTML} at (0,0) size 800x600
+    RenderBody {BODY} at (8,8) size 784x584
+layer at (308,8) size 100x100
+  RenderBlock (positioned) {DIV} at (300,0) size 100x100 [bgcolor=#008000]
+layer at (308,8) size 100x100
+  RenderBlock (positioned) {DIV} at (0,0) size 100x100
index 85b2989..b5dd285 100644 (file)
@@ -1,3 +1,40 @@
+2011-04-07  David Hyatt  <hyatt@apple.com>
+
+        Reviewed by Dan Bernstein.
+
+        https://bugs.webkit.org/show_bug.cgi?id=57736
+        
+        Crash on openstreetmap.org while using the map. Fix a bad interaction between the positioned movement layout
+        optimization and the simplified layout optimization that could lead to blocks remaining marked as dirty when
+        layout finished. This would eventually lead to an inability to properly determine the correct layout root and
+        would cause a deleted root to be used later on.
+
+        Added fast/block/positioning/complex-positioned-movement.html.
+
+        * page/FrameView.cpp:
+        (WebCore::FrameView::scheduleRelayoutOfSubtree):
+        Add asserts to catch cases in the future where a layout root is set that has a dirty containing block.
+    
+        * rendering/RenderBlock.cpp:
+        (WebCore::RenderBlock::simplifiedLayout):
+        Change simplified layout so that the positioned movement optimization no longer clears the other layout
+        flags. This will ensure that we still lay out our descendants if they need it.
+        
+        (WebCore::RenderBlock::layoutPositionedObjects):
+        Changed to clear our layout flags now if the positioned movement is successful, since tryLayoutDoingPositionedMovementOnly
+        no longer does the clear.
+    
+        * rendering/RenderBox.h:
+        (WebCore::RenderBox::tryLayoutDoingPositionedMovementOnly):
+        tryLayoutDoingPositionedMovementOnly now returns a boolean indicating success or failure.  On success it no longer
+        does setNeedsLayout(false), but instead will let the caller take care of it. This way the caller can still look at
+        the other flags in case other kinds of layout are still needed.
+    
+        * rendering/RenderObject.h:
+        (WebCore::RenderObject::setNeedsPositionedMovementLayout):
+        (WebCore::RenderObject::setNeedsSimplifiedNormalFlowLayout):
+        Changed these methods to only set their respective flags and not to try to be clever about avoiding propagation.
+
 2011-04-07  Anna Cavender  <annacc@chromium.org>
 
         Reviewed by Eric Carlson.
index de03d22..be623e7 100644 (file)
@@ -1672,10 +1672,12 @@ void FrameView::scheduleRelayoutOfSubtree(RenderObject* relayoutRoot)
             if (isObjectAncestorContainerOf(m_layoutRoot, relayoutRoot)) {
                 // Keep the current root
                 relayoutRoot->markContainingBlocksForLayout(false, m_layoutRoot);
+                ASSERT(!m_layoutRoot->container() || !m_layoutRoot->container()->needsLayout());
             } else if (m_layoutRoot && isObjectAncestorContainerOf(relayoutRoot, m_layoutRoot)) {
                 // Re-root at relayoutRoot
                 m_layoutRoot->markContainingBlocksForLayout(false, relayoutRoot);
                 m_layoutRoot = relayoutRoot;
+                ASSERT(!m_layoutRoot->container() || !m_layoutRoot->container()->needsLayout());
             } else {
                 // Just do a full relayout
                 if (m_layoutRoot)
@@ -1687,6 +1689,7 @@ void FrameView::scheduleRelayoutOfSubtree(RenderObject* relayoutRoot)
     } else if (m_layoutSchedulingEnabled) {
         int delay = m_frame->document()->minimumLayoutDelay();
         m_layoutRoot = relayoutRoot;
+        ASSERT(!m_layoutRoot->container() || !m_layoutRoot->container()->needsLayout());
         m_delayedLayout = delay != 0;
         m_layoutTimer.startOneShot(delay * 0.001);
     }
index bc95fae..b756b1f 100644 (file)
@@ -2114,11 +2114,8 @@ bool RenderBlock::simplifiedLayout()
 
     LayoutStateMaintainer statePusher(view(), this, IntSize(x(), y()), hasColumns() || hasTransform() || hasReflection() || style()->isFlippedBlocksWritingMode());
     
-    if (needsPositionedMovementLayout()) {
-        tryLayoutDoingPositionedMovementOnly();
-        if (needsLayout())
-            return false;
-    }
+    if (needsPositionedMovementLayout() && !tryLayoutDoingPositionedMovementOnly())
+        return false;
 
     // Lay out positioned descendants or objects that just need to recompute overflow.
     if (needsSimplifiedNormalFlowLayout())
@@ -2174,8 +2171,8 @@ void RenderBlock::layoutPositionedObjects(bool relayoutChildren)
 
         // We don't have to do a full layout.  We just have to update our position. Try that first. If we have shrink-to-fit width
         // and we hit the available width constraint, the layoutIfNeeded() will catch it and do a full layout.
-        if (r->needsPositionedMovementLayoutOnly())
-            r->tryLayoutDoingPositionedMovementOnly();
+        if (r->needsPositionedMovementLayoutOnly() && r->tryLayoutDoingPositionedMovementOnly())
+            r->setNeedsLayout(false);
         r->layoutIfNeeded();
     }
     
index 6fffacf..6b6b437 100644 (file)
@@ -350,15 +350,15 @@ public:
 
     // Called when a positioned object moves but doesn't necessarily change size.  A simplified layout is attempted
     // that just updates the object's position. If the size does change, the object remains dirty.
-    void tryLayoutDoingPositionedMovementOnly()
+    bool tryLayoutDoingPositionedMovementOnly()
     {
         int oldWidth = width();
         computeLogicalWidth();
         // If we shrink to fit our width may have changed, so we still need full layout.
         if (oldWidth != width())
-            return;
+            return false;
         computeLogicalHeight();
-        setNeedsLayout(false);
+        return true;
     }
 
     IntRect maskClipRect();
index 5a8d0e7..f2cc409 100644 (file)
@@ -959,8 +959,9 @@ inline void RenderObject::setChildNeedsLayout(bool b, bool markParents)
 
 inline void RenderObject::setNeedsPositionedMovementLayout()
 {
-    bool alreadyNeededLayout = needsLayout();
+    bool alreadyNeededLayout = m_needsPositionedMovementLayout;
     m_needsPositionedMovementLayout = true;
+    ASSERT(!isSetNeedsLayoutForbidden());
     if (!alreadyNeededLayout) {
         markContainingBlocksForLayout();
         if (hasLayer())
@@ -970,8 +971,9 @@ inline void RenderObject::setNeedsPositionedMovementLayout()
 
 inline void RenderObject::setNeedsSimplifiedNormalFlowLayout()
 {
-    bool alreadyNeededLayout = needsLayout();
+    bool alreadyNeededLayout = m_needsSimplifiedNormalFlowLayout;
     m_needsSimplifiedNormalFlowLayout = true;
+    ASSERT(!isSetNeedsLayoutForbidden());
     if (!alreadyNeededLayout) {
         markContainingBlocksForLayout();
         if (hasLayer())