From: mihnea@adobe.com Date: Fri, 29 Jun 2012 14:31:19 +0000 (+0000) Subject: Crash when flowing a fixed positioned element into a region. X-Git-Tag: 070512121124~332 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=d8a2370f35df04559fd4288588b7f374ff4ed6f1;p=profile%2Fivi%2Fwebkit-efl.git 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. 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 --- diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog index 0d807cd..7069458 100644 --- a/LayoutTests/ChangeLog +++ b/LayoutTests/ChangeLog @@ -1,3 +1,25 @@ +2012-06-29 Mihnea Ovidenie + + 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 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 index 0000000..cab26e5 --- /dev/null +++ b/LayoutTests/fast/regions/absolute-pos-elem-in-named-flow-expected.txt @@ -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 index 0000000..7235b73 --- /dev/null +++ b/LayoutTests/fast/regions/absolute-pos-elem-in-named-flow.html @@ -0,0 +1,19 @@ + + + + + + +

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-region-expected.html b/LayoutTests/fast/regions/absolute-pos-elem-in-region-expected.html new file mode 100644 index 0000000..243385d --- /dev/null +++ b/LayoutTests/fast/regions/absolute-pos-elem-in-region-expected.html @@ -0,0 +1,14 @@ + + + + + + +

Test for WebKit Bug 88133 Collecting a fixed positioned element in a named flow causes WebKit to crash.

+

This test collects an absolutely positioned element inside a named flow and flows it into a region.

+

The test passes if it does not crash or assert and no red rectangle is visible.

+
+ + 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 index 0000000..cd4624a --- /dev/null +++ b/LayoutTests/fast/regions/absolute-pos-elem-in-region.html @@ -0,0 +1,18 @@ + + + + + + +

Test for WebKit Bug 88133 Collecting a fixed positioned element in a named flow causes WebKit to crash.

+

This test collects an absolutely positioned element inside a named flow and flows it into a region.

+

The test passes if it does not crash or assert and no red rectangle is visible.

+
+
+
+ + 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 index 0000000..65fc865 --- /dev/null +++ b/LayoutTests/fast/regions/fixed-pos-elem-in-named-flow-expected.txt @@ -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 index 0000000..fbf1664 --- /dev/null +++ b/LayoutTests/fast/regions/fixed-pos-elem-in-named-flow.html @@ -0,0 +1,19 @@ + + + + + + +

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-flow2-expected.txt b/LayoutTests/fast/regions/fixed-pos-elem-in-named-flow2-expected.txt new file mode 100644 index 0000000..28cf87e --- /dev/null +++ b/LayoutTests/fast/regions/fixed-pos-elem-in-named-flow2-expected.txt @@ -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 index 0000000..406c0a9 --- /dev/null +++ b/LayoutTests/fast/regions/fixed-pos-elem-in-named-flow2.html @@ -0,0 +1,25 @@ + + + + + + +

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-region-expected.html b/LayoutTests/fast/regions/fixed-pos-elem-in-region-expected.html new file mode 100644 index 0000000..b1dbbfa --- /dev/null +++ b/LayoutTests/fast/regions/fixed-pos-elem-in-region-expected.html @@ -0,0 +1,14 @@ + + + + + + +

Test for WebKit Bug 88133 Collecting a fixed positioned element in a named flow causes WebKit to crash.

+

This test collects a fixed position element inside a named flow and flows it into a region.

+

The test passes if it does not crash or assert and there is no red rectangle visible.

+
+ + 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 index 0000000..91c9d3c --- /dev/null +++ b/LayoutTests/fast/regions/fixed-pos-elem-in-region.html @@ -0,0 +1,18 @@ + + + + + + +

Test for WebKit Bug 88133 Collecting a fixed positioned element in a named flow causes WebKit to crash.

+

This test collects a fixed position element inside a named flow and flows it into a region.

+

The test passes if it does not crash or assert and there is no red rectangle visible.

+
+
+
+ + diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog index cdef268..0a6a52c 100644 --- a/Source/WebCore/ChangeLog +++ b/Source/WebCore/ChangeLog @@ -1,3 +1,42 @@ +2012-06-29 Mihnea Ovidenie + + 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 Don't hardcode target dpi of 160 (it should be 96 on desktop) diff --git a/Source/WebCore/rendering/RenderBlock.cpp b/Source/WebCore/rendering/RenderBlock.cpp index fdb1c03..f448301 100755 --- a/Source/WebCore/rendering/RenderBlock.cpp +++ b/Source/WebCore/rendering/RenderBlock.cpp @@ -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 { diff --git a/Source/WebCore/rendering/RenderBlock.h b/Source/WebCore/rendering/RenderBlock.h index 81a6ed7..5d39398 100644 --- a/Source/WebCore/rendering/RenderBlock.h +++ b/Source/WebCore/rendering/RenderBlock.h @@ -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 diff --git a/Source/WebCore/rendering/RenderLayer.cpp b/Source/WebCore/rendering/RenderLayer.cpp index 038f661..7ac8524 100644 --- a/Source/WebCore/rendering/RenderLayer.cpp +++ b/Source/WebCore/rendering/RenderLayer.cpp @@ -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. diff --git a/Source/WebCore/rendering/RenderObject.cpp b/Source/WebCore/rendering/RenderObject.cpp index 1916fa0..803b5a6 100755 --- a/Source/WebCore/rendering/RenderObject.cpp +++ b/Source/WebCore/rendering/RenderObject.cpp @@ -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; diff --git a/Source/WebCore/rendering/RenderObject.h b/Source/WebCore/rendering/RenderObject.h index 3f01e90..bd20229 100644 --- a/Source/WebCore/rendering/RenderObject.h +++ b/Source/WebCore/rendering/RenderObject.h @@ -932,7 +932,11 @@ private: StyleDifference adjustStyleDifference(StyleDifference, unsigned contextSensitiveProperties) const; Color selectionColor(int colorProperty) const; - + +#ifndef NDEBUG + void checkBlockPositionedObjectsNeedLayout(); +#endif + RefPtr 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 } }