Crash when flowing a fixed positioned element into a region.
authormihnea@adobe.com <mihnea@adobe.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 29 Jun 2012 14:31:19 +0000 (14:31 +0000)
committermihnea@adobe.com <mihnea@adobe.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 29 Jun 2012 14:31:19 +0000 (14:31 +0000)
https://bugs.webkit.org/show_bug.cgi?id=88133

Reviewed by Julien Chaffraix and Abhishek Arya.

Source/WebCore:

When a fixed positioned element is collected into a named flow, we have to make sure
that such element has the RenderFlowThread as containing block instead of RenderView,
so that the fixed positioned element is laid out properly.
Making the RenderFlowThread the top most containing block for named flow elements required the
modification of RenderLayer::convertToLayerCoords so that the fixed positioned elements inside the
named flow take the same code path as the absolute positioned elements inside the named flow.
I also added a method, checkBlockPositionedObjectsNeedLayout, in order to verify that a block
that is ending its layout, setNeedsLayout(false), has all the positioned children laid out.
This way, we will hit an assertion if an out-of-flow positioned child inside a RenderFlowThread
is not laid out after the RenderFlowThread is laid out.

Tests: fast/regions/absolute-pos-elem-in-named-flow.html
       fast/regions/absolute-pos-elem-in-region.html
       fast/regions/fixed-pos-elem-in-named-flow.html
       fast/regions/fixed-pos-elem-in-named-flow2.html
       fast/regions/fixed-pos-elem-in-region.html

* rendering/RenderBlock.cpp:
(WebCore::RenderBlock::checkPositionedObjectsNeedLayout):
* rendering/RenderBlock.h:
(RenderBlock):
* rendering/RenderLayer.cpp:
(WebCore::RenderLayer::convertToLayerCoords):
* rendering/RenderObject.cpp:
(WebCore):
(WebCore::RenderObject::checkBlockPositionedObjectsNeedLayout):
(WebCore::RenderObject::containingBlock):
(WebCore::RenderObject::container):
* rendering/RenderObject.h:
(RenderObject):
(WebCore::RenderObject::setNeedsLayout):

LayoutTests:

When a fixed positioned element is collected into a named flow, we have to make sure
that such element has the RenderFlowThread as containing block instead of RenderView,
so that the fixed positioned element is laid out properly.

* fast/regions/absolute-pos-elem-in-named-flow-expected.txt: Added.
* fast/regions/absolute-pos-elem-in-named-flow.html: Added.
* fast/regions/absolute-pos-elem-in-region-expected.html: Added.
* fast/regions/absolute-pos-elem-in-region.html: Added.
* fast/regions/fixed-pos-elem-in-named-flow-expected.txt: Added.
* fast/regions/fixed-pos-elem-in-named-flow.html: Added.
* fast/regions/fixed-pos-elem-in-named-flow2-expected.txt: Added.
* fast/regions/fixed-pos-elem-in-named-flow2.html: Added.
* fast/regions/fixed-pos-elem-in-region-expected.html: Added.
* fast/regions/fixed-pos-elem-in-region.html: Added.

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

17 files changed:
LayoutTests/ChangeLog
LayoutTests/fast/regions/absolute-pos-elem-in-named-flow-expected.txt [new file with mode: 0644]
LayoutTests/fast/regions/absolute-pos-elem-in-named-flow.html [new file with mode: 0644]
LayoutTests/fast/regions/absolute-pos-elem-in-region-expected.html [new file with mode: 0644]
LayoutTests/fast/regions/absolute-pos-elem-in-region.html [new file with mode: 0644]
LayoutTests/fast/regions/fixed-pos-elem-in-named-flow-expected.txt [new file with mode: 0644]
LayoutTests/fast/regions/fixed-pos-elem-in-named-flow.html [new file with mode: 0644]
LayoutTests/fast/regions/fixed-pos-elem-in-named-flow2-expected.txt [new file with mode: 0644]
LayoutTests/fast/regions/fixed-pos-elem-in-named-flow2.html [new file with mode: 0644]
LayoutTests/fast/regions/fixed-pos-elem-in-region-expected.html [new file with mode: 0644]
LayoutTests/fast/regions/fixed-pos-elem-in-region.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/rendering/RenderBlock.cpp
Source/WebCore/rendering/RenderBlock.h
Source/WebCore/rendering/RenderLayer.cpp
Source/WebCore/rendering/RenderObject.cpp
Source/WebCore/rendering/RenderObject.h

index 0d807cd..7069458 100644 (file)
@@ -1,3 +1,25 @@
+2012-06-29  Mihnea Ovidenie  <mihnea@adobe.com>
+
+        Crash when flowing a fixed positioned element into a region.
+        https://bugs.webkit.org/show_bug.cgi?id=88133
+
+        Reviewed by Julien Chaffraix and Abhishek Arya.
+
+        When a fixed positioned element is collected into a named flow, we have to make sure
+        that such element has the RenderFlowThread as containing block instead of RenderView,
+        so that the fixed positioned element is laid out properly.
+
+        * fast/regions/absolute-pos-elem-in-named-flow-expected.txt: Added.
+        * fast/regions/absolute-pos-elem-in-named-flow.html: Added.
+        * fast/regions/absolute-pos-elem-in-region-expected.html: Added.
+        * fast/regions/absolute-pos-elem-in-region.html: Added.
+        * fast/regions/fixed-pos-elem-in-named-flow-expected.txt: Added.
+        * fast/regions/fixed-pos-elem-in-named-flow.html: Added.
+        * fast/regions/fixed-pos-elem-in-named-flow2-expected.txt: Added.
+        * fast/regions/fixed-pos-elem-in-named-flow2.html: Added.
+        * fast/regions/fixed-pos-elem-in-region-expected.html: Added.
+        * fast/regions/fixed-pos-elem-in-region.html: Added.
+
 2012-06-29  Alexander Pavlov  <apavlov@chromium.org>
 
         Web Inspector: [Device Metrics] "Fit window" option inhibits adjusting the page zoom factor
diff --git a/LayoutTests/fast/regions/absolute-pos-elem-in-named-flow-expected.txt b/LayoutTests/fast/regions/absolute-pos-elem-in-named-flow-expected.txt
new file mode 100644 (file)
index 0000000..cab26e5
--- /dev/null
@@ -0,0 +1,7 @@
+Test for WebKit Bug 88133 Collecting a fixed positioned element in a named flow causes WebKit to crash.
+
+This test collects an absolutely positioned object into a named flow without flowing it into a region.
+
+The test passes if it does not crash or assert.
+
+PASS
diff --git a/LayoutTests/fast/regions/absolute-pos-elem-in-named-flow.html b/LayoutTests/fast/regions/absolute-pos-elem-in-named-flow.html
new file mode 100644 (file)
index 0000000..7235b73
--- /dev/null
@@ -0,0 +1,19 @@
+<!doctype html>
+<html>
+    <head>
+        <style>
+            #absolute { position: absolute; -webkit-flow-into: flow; width: 50px; height: 50px; }
+        </style>
+    </head>
+    <body>
+        <p>Test for <a href="https://bugs.webkit.org/show_bug.cgi?id=88133">WebKit Bug 88133</a> Collecting a fixed positioned element in a named flow causes WebKit to crash.</p>
+        <p>This test collects an absolutely positioned object into a named flow without flowing it into a region.</p>
+        <p>The test passes if it does not crash or assert.</p>
+        <div id="absolute"></div>
+        <p>PASS</p>
+        <script>
+            if (window.testRunner)
+                window.testRunner.dumpAsText();
+        </script>
+    </body>
+</html>
diff --git a/LayoutTests/fast/regions/absolute-pos-elem-in-region-expected.html b/LayoutTests/fast/regions/absolute-pos-elem-in-region-expected.html
new file mode 100644 (file)
index 0000000..243385d
--- /dev/null
@@ -0,0 +1,14 @@
+<!doctype html>
+<html>
+    <head>
+        <style>
+            #green { position: absolute; width: 50px; height: 50px; top: 125px; left: 75px; background-color: green; }
+        </style>
+    </head>
+    <body>
+        <p>Test for <a href="https://bugs.webkit.org/show_bug.cgi?id=88133">WebKit Bug 88133</a> Collecting a fixed positioned element in a named flow causes WebKit to crash.</p>
+        <p>This test collects an absolutely positioned element inside a named flow and flows it into a region.</p>
+        <p>The test passes if it does not crash or assert and no red rectangle is visible.</p>
+        <div id="green"></div>
+    </body>
+</html>
diff --git a/LayoutTests/fast/regions/absolute-pos-elem-in-region.html b/LayoutTests/fast/regions/absolute-pos-elem-in-region.html
new file mode 100644 (file)
index 0000000..cd4624a
--- /dev/null
@@ -0,0 +1,18 @@
+<!doctype html>
+<html>
+    <head>
+        <style>
+            #red { position: absolute; width: 50px; height: 50px; top: 125px; left: 75px; background-color: red; }
+            #absolute { -webkit-flow-into: flow; position: absolute; width: 50px; height: 50px; background-color: green; top: 25px; left: 25px; }
+            #region { -webkit-flow-from: flow; width: 100px; height: 100px; position: absolute; top: 100px; left: 50px; }
+        </style>
+    </head>
+    <body>
+        <p>Test for <a href="https://bugs.webkit.org/show_bug.cgi?id=88133">WebKit Bug 88133</a> Collecting a fixed positioned element in a named flow causes WebKit to crash.</p>
+        <p>This test collects an absolutely positioned element inside a named flow and flows it into a region.</p>
+        <p>The test passes if it does not crash or assert and no red rectangle is visible.</p>
+        <div id="red"></div>
+        <div id="absolute"></div>
+        <div id="region"></div>
+    </body>
+</html>
diff --git a/LayoutTests/fast/regions/fixed-pos-elem-in-named-flow-expected.txt b/LayoutTests/fast/regions/fixed-pos-elem-in-named-flow-expected.txt
new file mode 100644 (file)
index 0000000..65fc865
--- /dev/null
@@ -0,0 +1,7 @@
+Test for WebKit Bug 88133 Collecting a fixed positioned element in a named flow causes WebKit to crash.
+
+This test collects a fixed positioned object into a named flow but without flowing it into a region.
+
+The test passes if it does not crash or assert.
+
+PASS
diff --git a/LayoutTests/fast/regions/fixed-pos-elem-in-named-flow.html b/LayoutTests/fast/regions/fixed-pos-elem-in-named-flow.html
new file mode 100644 (file)
index 0000000..fbf1664
--- /dev/null
@@ -0,0 +1,19 @@
+<!doctype html>
+<html>
+    <head>
+        <style>
+            #fixed { position: fixed; -webkit-flow-into: flow; width: 50px; height: 50px; }
+        </style>
+    </head>
+    <body>
+        <p>Test for <a href="https://bugs.webkit.org/show_bug.cgi?id=88133">WebKit Bug 88133</a> Collecting a fixed positioned element in a named flow causes WebKit to crash.</p>
+        <p>This test collects a fixed positioned object into a named flow but without flowing it into a region.</p>
+        <p>The test passes if it does not crash or assert.</p>
+        <div id="fixed"></div>
+        <p>PASS</p>
+        <script>
+            if (window.testRunner)
+                window.testRunner.dumpAsText();
+        </script>
+    </body>
+</html>
diff --git a/LayoutTests/fast/regions/fixed-pos-elem-in-named-flow2-expected.txt b/LayoutTests/fast/regions/fixed-pos-elem-in-named-flow2-expected.txt
new file mode 100644 (file)
index 0000000..28cf87e
--- /dev/null
@@ -0,0 +1,7 @@
+Test for WebKit Bug 88133 Collecting a fixed positioned element in a named flow causes WebKit to crash.
+
+This test collects a fixed positioned object into a named flow but without flowing it into a region. The fixed positioned element is the child of another element that has a transform, so that it requires a layer.
+
+The test passes if it does not crash or assert.
+
+PASS
diff --git a/LayoutTests/fast/regions/fixed-pos-elem-in-named-flow2.html b/LayoutTests/fast/regions/fixed-pos-elem-in-named-flow2.html
new file mode 100644 (file)
index 0000000..406c0a9
--- /dev/null
@@ -0,0 +1,25 @@
+<!doctype html>
+<html>
+    <head>
+        <style>
+            #article { -webkit-flow-into: flow;  }
+            #transform { width: 100px; height: 100px; -webkit-transform: translate3d(50px, 50px, 0px); }
+            #fixed { position: fixed; top: 25px; left: 25px; width: 50px; height: 50px; }
+        </style>
+    </head>
+    <body>
+        <p>Test for <a href="https://bugs.webkit.org/show_bug.cgi?id=88133">WebKit Bug 88133</a> Collecting a fixed positioned element in a named flow causes WebKit to crash.</p>
+        <p>This test collects a fixed positioned object into a named flow but without flowing it into a region. The fixed positioned element is the child of another element that has a transform, so that it requires a layer.</p>
+        <p>The test passes if it does not crash or assert.</p>
+        <div id="article">
+            <div id="transform">
+                <div id="fixed"></div>
+            </div>
+        </div>
+        <p>PASS</p>
+        <script>
+            if (window.testRunner)
+                window.testRunner.dumpAsText();
+        </script>
+    </body>
+</html>
diff --git a/LayoutTests/fast/regions/fixed-pos-elem-in-region-expected.html b/LayoutTests/fast/regions/fixed-pos-elem-in-region-expected.html
new file mode 100644 (file)
index 0000000..b1dbbfa
--- /dev/null
@@ -0,0 +1,14 @@
+<!doctype html>
+<html>
+    <head>
+        <style>
+            #green { position: fixed; width: 50px; height: 50px; top: 125px; left: 75px; background-color: green; }
+        </style>
+    </head>
+    <body>
+        <p>Test for <a href="https://bugs.webkit.org/show_bug.cgi?id=88133">WebKit Bug 88133</a> Collecting a fixed positioned element in a named flow causes WebKit to crash.</p>
+        <p>This test collects a fixed position element inside a named flow and flows it into a region.</p>
+        <p>The test passes if it does not crash or assert and there is no red rectangle visible.</p>
+        <div id="green"></div>
+    </body>
+</html>
diff --git a/LayoutTests/fast/regions/fixed-pos-elem-in-region.html b/LayoutTests/fast/regions/fixed-pos-elem-in-region.html
new file mode 100644 (file)
index 0000000..91c9d3c
--- /dev/null
@@ -0,0 +1,18 @@
+<!doctype html>
+<html>
+    <head>
+        <style>
+            #red { position: fixed; width: 50px; height: 50px; top: 125px; left: 75px; background-color: red; }
+            #fixed { -webkit-flow-into: flow; position: fixed; width: 50px; height: 50px; background-color: green; top: 25px; left: 25px; }
+            #region { -webkit-flow-from: flow; width: 100px; height: 100px; position: absolute; top: 100px; left: 50px; }
+        </style>
+    </head>
+    <body>
+        <p>Test for <a href="https://bugs.webkit.org/show_bug.cgi?id=88133">WebKit Bug 88133</a> Collecting a fixed positioned element in a named flow causes WebKit to crash.</p>
+        <p>This test collects a fixed position element inside a named flow and flows it into a region.</p>
+        <p>The test passes if it does not crash or assert and there is no red rectangle visible.</p>
+        <div id="red"></div>
+        <div id="fixed"></div>
+        <div id="region"></div>
+    </body>
+</html>
index cdef268..0a6a52c 100644 (file)
@@ -1,3 +1,42 @@
+2012-06-29  Mihnea Ovidenie  <mihnea@adobe.com>
+
+        Crash when flowing a fixed positioned element into a region.
+        https://bugs.webkit.org/show_bug.cgi?id=88133
+
+        Reviewed by Julien Chaffraix and Abhishek Arya.
+
+        When a fixed positioned element is collected into a named flow, we have to make sure
+        that such element has the RenderFlowThread as containing block instead of RenderView,
+        so that the fixed positioned element is laid out properly.
+        Making the RenderFlowThread the top most containing block for named flow elements required the
+        modification of RenderLayer::convertToLayerCoords so that the fixed positioned elements inside the
+        named flow take the same code path as the absolute positioned elements inside the named flow.
+        I also added a method, checkBlockPositionedObjectsNeedLayout, in order to verify that a block
+        that is ending its layout, setNeedsLayout(false), has all the positioned children laid out.
+        This way, we will hit an assertion if an out-of-flow positioned child inside a RenderFlowThread
+        is not laid out after the RenderFlowThread is laid out.
+
+        Tests: fast/regions/absolute-pos-elem-in-named-flow.html
+               fast/regions/absolute-pos-elem-in-region.html
+               fast/regions/fixed-pos-elem-in-named-flow.html
+               fast/regions/fixed-pos-elem-in-named-flow2.html
+               fast/regions/fixed-pos-elem-in-region.html
+
+        * rendering/RenderBlock.cpp:
+        (WebCore::RenderBlock::checkPositionedObjectsNeedLayout):
+        * rendering/RenderBlock.h:
+        (RenderBlock):
+        * rendering/RenderLayer.cpp:
+        (WebCore::RenderLayer::convertToLayerCoords):
+        * rendering/RenderObject.cpp:
+        (WebCore):
+        (WebCore::RenderObject::checkBlockPositionedObjectsNeedLayout):
+        (WebCore::RenderObject::containingBlock):
+        (WebCore::RenderObject::container):
+        * rendering/RenderObject.h:
+        (RenderObject):
+        (WebCore::RenderObject::setNeedsLayout):
+
 2012-06-29  Konrad Piascik  <kpiascik@rim.com>
 
         Don't hardcode target dpi of 160 (it should be 96 on desktop)
index fdb1c03..f448301 100755 (executable)
@@ -7275,6 +7275,16 @@ RenderBlock* RenderBlock::createAnonymousColumnSpanWithParentRenderer(const Rend
 }
 
 #ifndef NDEBUG
+void RenderBlock::checkPositionedObjectsNeedLayout()
+{
+    if (PositionedObjectsListHashSet* positionedObjects = this->positionedObjects()) {
+        PositionedObjectsListHashSet::const_iterator end = positionedObjects->end();
+        for (PositionedObjectsListHashSet::const_iterator it = positionedObjects->begin(); it != end; ++it) {
+            RenderBox* currBox = *it;
+            ASSERT(!currBox->needsLayout());
+        }
+    }
+}
 
 void RenderBlock::showLineTreeAndMark(const InlineBox* markedBox1, const char* markedLabel1, const InlineBox* markedBox2, const char* markedLabel2, const RenderObject* obj) const
 {
index 81a6ed7..5d39398 100644 (file)
@@ -384,6 +384,7 @@ public:
     bool runInIsPlacedIntoSiblingBlock(RenderObject* runIn);
 
 #ifndef NDEBUG
+    void checkPositionedObjectsNeedLayout();
     void showLineTreeAndMark(const InlineBox* = 0, const char* = 0, const InlineBox* = 0, const char* = 0, const RenderObject* = 0) const;
 #endif
 
index 038f661..7ac8524 100644 (file)
@@ -1419,15 +1419,24 @@ void RenderLayer::convertToLayerCoords(const RenderLayer* ancestorLayer, LayoutP
         return;
 
     EPosition position = renderer()->style()->position();
-    if (position == FixedPosition && (!ancestorLayer || ancestorLayer == renderer()->view()->layer())) {
+
+    // FIXME: Positioning of out-of-flow(fixed, absolute) elements collected in a RenderFlowThread
+    // may need to be revisited in a future patch.
+    // If the fixed renderer is inside a RenderFlowThread, we should not compute location using localToAbsolute,
+    // since localToAbsolute maps the coordinates from named flow to regions coordinates and regions can be
+    // positioned in a completely different place in the viewport (RenderView).
+    if (position == FixedPosition && !renderer()->inRenderFlowThread() && (!ancestorLayer || ancestorLayer == renderer()->view()->layer())) {
         // If the fixed layer's container is the root, just add in the offset of the view. We can obtain this by calling
         // localToAbsolute() on the RenderView.
         FloatPoint absPos = renderer()->localToAbsolute(FloatPoint(), true);
         location += flooredLayoutSize(absPos);
         return;
     }
-    if (position == FixedPosition) {
+
+    // For the fixed positioned elements inside a render flow thread, we should also skip the code path below
+    // Otherwise, for the case of ancestorLayer == rootLayer and fixed positioned element child of a transformed
+    // element in render flow thread, we will hit the fixed positioned container before hitting the ancestor layer.
+    if (position == FixedPosition && !renderer()->inRenderFlowThread()) {
         // For a fixed layers, we need to walk up to the root to see if there's a fixed position container
         // (e.g. a transformed layer). It's an error to call convertToLayerCoords() across a layer with a transform,
         // so we should always find the ancestor at or before we find the fixed position container.
@@ -1464,6 +1473,9 @@ void RenderLayer::convertToLayerCoords(const RenderLayer* ancestorLayer, LayoutP
         parentLayer = parent();
         bool foundAncestorFirst = false;
         while (parentLayer) {
+            // RenderFlowThread is a positioned container, child of RenderView, positioned at (0,0).
+            // This implies that, for out-of-flow positioned elements inside a RenderFlowThread,
+            // we are bailing out before reaching root layer.
             if (isPositionedContainer(parentLayer))
                 break;
 
@@ -1475,6 +1487,11 @@ void RenderLayer::convertToLayerCoords(const RenderLayer* ancestorLayer, LayoutP
             parentLayer = parentLayer->parent();
         }
 
+        // We should not reach RenderView layer past the RenderFlowThread layer for any
+        // children of the RenderFlowThread.
+        if (renderer()->inRenderFlowThread() && !renderer()->isRenderFlowThread())
+            ASSERT(parentLayer != renderer()->view()->layer());
+
         if (foundAncestorFirst) {
             // Found ancestorLayer before the abs. positioned container, so compute offset of both relative
             // to enclosingPositionedAncestor and subtract.
index 1916fa0..803b5a6 100755 (executable)
@@ -676,6 +676,16 @@ void RenderObject::markContainingBlocksForLayout(bool scheduleRelayout, RenderOb
         last->scheduleRelayout();
 }
 
+#ifndef NDEBUG
+void RenderObject::checkBlockPositionedObjectsNeedLayout()
+{
+    ASSERT(!needsLayout());
+
+    if (isRenderBlock())
+        toRenderBlock(this)->checkPositionedObjectsNeedLayout();
+}
+#endif
+
 void RenderObject::setPreferredLogicalWidthsDirty(bool shouldBeDirty, MarkingBehavior markParents)
 {
     bool alreadyDirty = preferredLogicalWidthsDirty();
@@ -728,6 +738,10 @@ RenderBlock* RenderObject::containingBlock() const
                 break;
             if (o->hasTransform() && o->isRenderBlock())
                 break;
+            // The render flow thread is the top most containing block
+            // for the fixed positioned elements.
+            if (o->isRenderFlowThread())
+                break;
 #if ENABLE(SVG)
             // foreignObject is the containing block for its contents.
             if (o->isSVGForeignObject())
@@ -2218,6 +2232,11 @@ RenderObject* RenderObject::container(const RenderBoxModelObject* repaintContain
             if (o->isSVGForeignObject())
                 break;
 #endif
+            // The render flow thread is the top most containing block
+            // for the fixed positioned elements.
+            if (o->isRenderFlowThread())
+                break;
+
             if (repaintContainerSkipped && o == repaintContainer)
                 *repaintContainerSkipped = true;
 
index 3f01e90..bd20229 100644 (file)
@@ -932,7 +932,11 @@ private:
     StyleDifference adjustStyleDifference(StyleDifference, unsigned contextSensitiveProperties) const;
 
     Color selectionColor(int colorProperty) const;
-    
+
+#ifndef NDEBUG
+    void checkBlockPositionedObjectsNeedLayout();
+#endif
+
     RefPtr<RenderStyle> m_style;
 
     Node* m_node;
@@ -1104,6 +1108,9 @@ inline void RenderObject::setNeedsLayout(bool needsLayout, MarkingBehavior markP
         setNormalChildNeedsLayout(false);
         setNeedsPositionedMovementLayout(false);
         setAncestorLineBoxDirty(false);
+#ifndef NDEBUG
+        checkBlockPositionedObjectsNeedLayout();
+#endif
     }
 }