Created attachment 143617 [details] Crashes when laying out the <defs> subtree We are suppressing the creation of layers inside SVGHiddenContainers (in RenderObject::layerCreationAllowedForSubtree), but RenderBlock is riddled with assumptions that positioned elements always have an associated layer (and child->layer() is de-referenced without a NULL check). For example, an SVG foreignObject in the <defs> section will cause a crash if its content requires layers: #0 0x00007ffff108f0be in WebCore::RenderLayer::setStaticInlinePosition (this=0x0, position=...) at ../../third_party/WebKit/Source/WebCore/rendering/RenderLayer.h:530 #1 0x00007ffff1084f85 in WebCore::RenderBlock::setStaticInlinePositionForChild (this=0x7fffe083a6f8, child= 0x7fffe0a9b8d8, blockOffset=..., inlinePosition=...) at ../../third_party/WebKit/Source/WebCore/rendering/RenderBlock.cpp:6972 #2 0x00007ffff10b1379 in WebCore::setStaticPositions (block=0x7fffe083a6f8, child=0x7fffe0a9b8d8) at ../../third_party/WebKit/Source/WebCore/rendering/RenderBlockLineLayout.cpp:881 #3 0x00007ffff10b65ad in WebCore::RenderBlock::LineBreaker::skipLeadingWhitespace (this=0x7fffffffa2b0, resolver=..., lineInfo=..., lastFloatFromPreviousLine=0x0, width=...) at ../../third_party/WebKit/Source/WebCore/rendering/RenderBlockLineLayout.cpp:1911 #4 0x00007ffff10b7573 in WebCore::RenderBlock::LineBreaker::nextLineBreak (this=0x7fffffffa2b0, resolver=..., lineInfo=..., lineBreakIteratorInfo=..., lastFloatFromPreviousLine=0x0, consecutiveHyphenatedLines=0) at ../../third_party/WebKit/Source/WebCore/rendering/RenderBlockLineLayout.cpp:2124 #5 0x00007ffff10b23d6 in WebCore::RenderBlock::layoutRunsAndFloatsInRange (this=0x7fffe083a6f8, layoutState=..., resolver=..., cleanLineStart=..., cleanLineBidiStatus=..., consecutiveHyphenatedLines=0) at ../../third_party/WebKit/Source/WebCore/rendering/RenderBlockLineLayout.cpp:1256 #6 0x00007ffff10b2035 in WebCore::RenderBlock::layoutRunsAndFloats (this=0x7fffe083a6f8, layoutState=..., hasInlineChild=false) at ../../third_party/WebKit/Source/WebCore/rendering/RenderBlockLineLayout.cpp:1221 #7 0x00007ffff10b41ea in WebCore::RenderBlock::layoutInlineChildren (this=0x7fffe083a6f8, relayoutChildren=true, repaintLogicalTop=..., repaintLogicalBottom=...) at ../../third_party/WebKit/Source/WebCore/rendering/RenderBlockLineLayout.cpp:1517 #8 0x00007ffff1062667 in WebCore::RenderBlock::layoutBlock (this=0x7fffe083a6f8, relayoutChildren=true, pageLogicalHeight=...) at ../../third_party/WebKit/Source/WebCore/rendering/RenderBlock.cpp:1478 ---Type <return> to continue, or q <return> to quit--- #9 0x00007ffff1061b9c in WebCore::RenderBlock::layout (this=0x7fffe083a6f8) at ../../third_party/WebKit/Source/WebCore/rendering/RenderBlock.cpp:1339 #10 0x00007ffff1560ea5 in WebCore::RenderSVGForeignObject::layout (this=0x7fffe083a6f8) at ../../third_party/WebKit/Source/WebCore/rendering/svg/RenderSVGForeignObject.cpp:153 #11 0x00007ffff159e4a8 in WebCore::SVGRenderSupport::layoutChildren (start=0x7fffe0a9b7f8, selfNeedsLayout=true) at ../../third_party/WebKit/Source/WebCore/rendering/svg/SVGRenderSupport.cpp:212 #12 0x00007ffff155de34 in WebCore::RenderSVGContainer::layout (this=0x7fffe0a9b7f8) at ../../third_party/WebKit/Source/WebCore/rendering/svg/RenderSVGContainer.cpp:70 #13 0x00007ffff159e4a8 in WebCore::SVGRenderSupport::layoutChildren (start=0x7fffe081a8d8, selfNeedsLayout=true) at ../../third_party/WebKit/Source/WebCore/rendering/svg/SVGRenderSupport.cpp:212 #14 0x00007ffff1561aa7 in WebCore::RenderSVGHiddenContainer::layout (this=0x7fffe081a8d8) at ../../third_party/WebKit/Source/WebCore/rendering/svg/RenderSVGHiddenContainer.cpp:38 #15 0x00007ffff159e4a8 in WebCore::SVGRenderSupport::layoutChildren (start=0x7fffe0985618, selfNeedsLayout=true) at ../../third_party/WebKit/Source/WebCore/rendering/svg/SVGRenderSupport.cpp:212 #16 0x00007ffff15868c4 in WebCore::RenderSVGRoot::layout (this=0x7fffe0985618) at ../../third_party/WebKit/Source/WebCore/rendering/svg/RenderSVGRoot.cpp:232 #17 0x00007ffff10672c7 in WebCore::RenderBlock::layoutBlockChild (this=0x7fffe43a0498, child=0x7fffe0985618, marginInfo=..., previousFloatLogicalBottom=..., maxFloatLogicalBottom=...) at ../../third_party/WebKit/Source/WebCore/rendering/RenderBlock.cpp:2330 #18 0x00007ffff1066dbf in WebCore::RenderBlock::layoutBlockChildren (this=0x7fffe43a0498, relayoutChildren=true, maxFloatLogicalBottom=...) at ../../third_party/WebKit/Source/WebCore/rendering/RenderBlock.cpp:2266 #19 0x00007ffff1062688 in WebCore::RenderBlock::layoutBlock (this=0x7fffe43a0498, relayoutChildren=true, ---Type <return> to continue, or q <return> to quit--- pageLogicalHeight=...) at ../../third_party/WebKit/Source/WebCore/rendering/RenderBlock.cpp:1480 #20 0x00007ffff1061b9c in WebCore::RenderBlock::layout (this=0x7fffe43a0498) at ../../third_party/WebKit/Source/WebCore/rendering/RenderBlock.cpp:1339 #21 0x00007ffff1213802 in WebCore::RenderView::layout (this=0x7fffe43a0498) at ../../third_party/WebKit/Source/WebCore/rendering/RenderView.cpp:141 #22 0x00007ffff1f6295f in WebCore::FrameView::layout (this=0x7fffe4412380, allowSubtree=true) at ../../third_party/WebKit/Source/WebCore/page/FrameView.cpp:1100
After chatting with schenney & pdr, it seems the core issue here is that we're doing a layout on the <defs> subtree in the first place. According to the spec we shouldn't even build that portion of the render tree: > Elements that are descendants of a ‘defs’ are not rendered directly; they are prevented from becoming part of the rendering tree just as if the ‘defs’ element were a ‘g’ element and the ‘display’ property were set to none. What is the reason for creating the <defs> rendering tree? We are guessing it's needed for filters, etc. Maybe we can exclude the elements that do not need it?
(In reply to comment #1) > After chatting with schenney & pdr, it seems the core issue here is that we're doing a layout on the <defs> subtree in the first place. According to the spec we shouldn't even build that portion of the render tree: > > > Elements that are descendants of a ‘defs’ are not rendered directly; they are prevented from becoming part of the rendering tree just as if the ‘defs’ element were a ‘g’ element and the ‘display’ property were set to none. > > What is the reason for creating the <defs> rendering tree? We are guessing it's needed for filters, etc. Maybe we can exclude the elements that do not need it? Resources currently always need a renderer (linearGradient/clipPath/etc..) - but that's subject to change in future - killing RenderSVGGradientStop is a first step in that direction. There are numerous of other examples why we currently need renderers for any element carrying CSS properties. One hard example is: CSS Transitions on any element inside a <defs> section, which can carry CSS properties, like: <defs> <path id="somePathThatWillBeReferenced"/> </defs>. A transition on that <path> currently needs a renderer() otherwise it has no RenderStyle that could be animated, and no one which actually updates it (usually done via RenderOBject::setAnimatableStyle). I'm currently prototyping CSS transitions on renderer-less Nodes.
*** Bug 100466 has been marked as a duplicate of this bug. ***
Created attachment 171005 [details] Patch
Comment on attachment 171005 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=171005&action=review I'm not sure this is the right approach, even as a temporary fix, because it's inefficient. Can you explain how the HTML render tree is able to accomplish this without crawling back up the tree? > Source/WebCore/svg/SVGForeignObjectElement.cpp:155 > + Element* ancestor = parentElement(); Should this be parentOrHostElement() instead of parentElement in case there's a shadow tree in our path? > Source/WebCore/svg/SVGForeignObjectElement.cpp:160 > + ancestor = ancestor->parentElement(); Same here.
(In reply to comment #5) > I'm not sure this is the right approach, even as a temporary fix, because it's inefficient. Can you explain how the HTML render tree is able to accomplish this without crawling back up the tree? The HTML render tree doesn't have this problem AFAICT (suppressing some but not all renderers for a subtree). See https://bugs.webkit.org/show_bug.cgi?id=41386 for the original motivation. Note that the same approach is used in RenderObject::layerCreationAllowedForSubtree(), and we're only crawling the tree on attach - so I don't expect the impact to be significant. > > Source/WebCore/svg/SVGForeignObjectElement.cpp:155 > > + Element* ancestor = parentElement(); > > Should this be parentOrHostElement() instead of parentElement in case there's a shadow tree in our path? Hmm, I'd say we can stop on shadow tree roots, but not sure. Is it possible to get shadow trees under <defs>?
(In reply to comment #6) > (In reply to comment #5) > > I'm not sure this is the right approach, even as a temporary fix, because it's inefficient. Can you explain how the HTML render tree is able to accomplish this without crawling back up the tree? > > The HTML render tree doesn't have this problem AFAICT (suppressing some but not all renderers for a subtree). See https://bugs.webkit.org/show_bug.cgi?id=41386 for the original motivation. > > Note that the same approach is used in RenderObject::layerCreationAllowedForSubtree(), and we're only crawling the tree on attach - so I don't expect the impact to be significant. I think this is reasonable after all, albeit tricky. Might be useful to transcribe our IRC discussion about renderers and layers in the changelog for reviewers. > > > > Source/WebCore/svg/SVGForeignObjectElement.cpp:155 > > > + Element* ancestor = parentElement(); > > > > Should this be parentOrHostElement() instead of parentElement in case there's a shadow tree in our path? > > Hmm, I'd say we can stop on shadow tree roots, but not sure. Is it possible to get shadow trees under <defs>? I scanned the spec and didn't find this called out explicitly. According to Dirk on IRC, it should be allowed but may or may not be supported in WebKit. Probably need to test this.
Krit: ping?
Comment on attachment 171005 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=171005&action=review <> > Source/WebCore/ChangeLog:10 > + Foreign objects may introduce content which requires layers, but layer creation is > + suppressed for the <defs> subtree and this yields an inconsistent render tree state. > + This patch prevents foreignObject renderer instantiation when used in <defs>. Does this influence: <defs> <foreignObject id="fo"/> </defs> <use xlink:href="#id"/> ? I guess not, since we copy the DOM and the parent should be SVGElementInstance IIRC. Can you check please? >> Source/WebCore/svg/SVGForeignObjectElement.cpp:155 >> + Element* ancestor = parentElement(); > > Should this be parentOrHostElement() instead of parentElement in case there's a shadow tree in our path? Do you have an example? I would like to know what happens in this case with your patch: <defs> <use xlink:href="#fo"/> </defs> <foreignObject id="fo"/> Does this work? Maybe this is the example you are mean?
(In reply to comment #9) > Does this influence: > > <defs> > <foreignObject id="fo"/> > </defs> > <use xlink:href="#id"/> > ? > I guess not, since we copy the DOM and the parent should be SVGElementInstance IIRC. Can you check please? Hmm, that's what I thought too, but upon checking it appears we don't support fO within <use> expansions at all (SVGUseElement::isDisallowedElement() excludes it). I seem to remember a conversation with Niko about it, and I believe the conclusion was that this is correct according to the spec: "Any ‘svg’, ‘symbol’, ‘g’, graphics element or other ‘use’ is potentially a template object that can be re-used (i.e., "instanced") in the SVG document via a ‘use’ element.)" ForeignObject is none of the above, but it might be nested within a group for example. So I'm not convinced that the current behavior is correct, but that's a separate issue from the bug at hand. > >> Source/WebCore/svg/SVGForeignObjectElement.cpp:155 > >> + Element* ancestor = parentElement(); > > > > Should this be parentOrHostElement() instead of parentElement in case there's a shadow tree in our path? > > Do you have an example? > > I would like to know what happens in this case with your patch: > > <defs> > <use xlink:href="#fo"/> > </defs> > <foreignObject id="fo"/> > > Does this work? Yes, because FOs are ignored in <use> expansion :) But if we change that, this might become a problem...
Comment on attachment 171005 [details] Patch Add a comment that SVG 1.1 disallows referencing fo's by use, but that this function would need to take care of it if this changes in a future spec. Then the code should be fine and you can use parentElement(). r=me with comments.
Created attachment 171847 [details] Patch for landing
Comment on attachment 171847 [details] Patch for landing This fix might be too narrow: I noticed that the tests for https://bugs.webkit.org/show_bug.cgi?id=100466 are still crashing.
We need to also check for RenderSVGHiddenContainer descendants, not just <defs> specifically.
Created attachment 171855 [details] Patch
Comment on attachment 171855 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=171855&action=review > Source/WebCore/ChangeLog:11 > + > + Foreign objects may introduce content which requires layers, but layer creation is > + suppressed within RenderSVGHiddenContainer subtrees and this yields an inconsistent render > + tree state. This patch prevents foreignObject renderer instantiation under > + RenderSVGHiddenContainers. This seems more drastically and I am not sure how to deal with it. Just an example: <pattern> <use xlink:href="g"/> </pattern> <g style="persepective: 400px;" id="g"> <rect style="transform: rotateX(45deg)"/> </g> <g> creates a RenderLayer to perform the 3D transform. Referencing elements is something that we explicitly want to support, but seems to be excluded in your patch, no? PS: I know that this doesn't work today, but should once we support 3D transforms. And I would like to worry about it right now instead of later. Since this patch seem to causes the worry in the future :)
(In reply to comment #16) > This seems more drastically and I am not sure how to deal with it. Just an example: > > <pattern> > <use xlink:href="g"/> > </pattern> > > <g style="persepective: 400px;" id="g"> > <rect style="transform: rotateX(45deg)"/> > </g> > > <g> creates a RenderLayer to perform the 3D transform. Referencing elements is something that we explicitly want to support, but seems to be excluded in your patch, no? The patch only deals with foreignObject specifically (it is the only SVG element that may introduce RenderLayers AFAIK), so it would not interfere with your example. But you're right to worry about it: if other SVG elements start requiring layers, chances are we'll hit the same crash. At that point we'll have to deal with the more general problem: either override requiresLayer() to always return false when in a RenderSVGHiddenContainer subtree, or patch all the code to be resilient to missing layers. Or maybe land Niko's big SVG rework :)
Comment on attachment 171855 [details] Patch I was confused. I am sorry. The patch looks correct for me.
Comment on attachment 171855 [details] Patch Clearing flags on attachment: 171855 Committed r133521: <http://trac.webkit.org/changeset/133521>
All reviewed patches have been landed. Closing bug.