The assertions in https://bugs.webkit.org/show_bug.cgi?id=49019 trigger on several SVG tests. Here's a reduction of one of the issues that triggers on svg/custom/recursive-mask.svg: <svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink"> <defs> <rect id="rect" x="0" y="100" width="50" height="50" fill="white" mask="url(#mask4)"/> <mask id="mask4"> <use xlink:href="#rect"/> </mask> </defs> </svg> At the end of FrameView::layout() the render tree looks like this: RenderView 0x3dbc2d8 #document 0x3e67420 RenderSVGRoot 0x3eba5a8 svg 0x3eb85c0 RenderSVGHiddenContainer 0x3ebc6b8 defs 0x3ebc460 * RenderSVGPath 0x3ebdde8 rect 0x3ebca40 RenderSVGResourceMasker 0x3ec0168 mask 0x3ebfa40 RenderSVGContainer 0x3daf418 use 0x3ebf260 RenderSVGContainer 0x3db05b8 g 0x3daf4e0 RenderSVGPath 0x3db0738 rect 0x3dafd90 and the RenderSVGPath associated with the <rect> is still marked as m_needsLayout.
Should children of a <defs> exist in the render tree at all? They should never be rendered according to the SVG spec.
(In reply to comment #1) > Should children of a <defs> exist in the render tree at all? They should never be rendered according to the SVG spec. They are accessible with xlink:href by <use>, <symbol> and <feImage> and need to have a renderer.
(In reply to comment #2) > (In reply to comment #1) > > Should children of a <defs> exist in the render tree at all? They should never be rendered according to the SVG spec. > > They are accessible with xlink:href by <use>, <symbol> and <feImage> and need to have a renderer. This is unrelated. To answer the question. Any object within <defs> has a renderer, they're all placed in a RenderSVGHiddenContainer. This is needed, as for instance gradients and gradient stops are typically placed in a <defs> object: <defs> <linearGradient> <stop stop-color="red"> ... stop-color is a CSS property, and thus needs to be modifiable through CSS. That requires a renderer for the <stop> otherwhise we can't listen to "styleDidChange" events. I could give more examples why this is needed. Note, that a RenderSVGHiddenContainer::paint() is a no-op, the <defs> subtree will never be painted.
(In reply to comment #0) > The assertions in https://bugs.webkit.org/show_bug.cgi?id=49019 trigger on several SVG tests. Here's a reduction of one of the issues that triggers on svg/custom/recursive-mask.svg: > > <svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink"> > <defs> > <rect id="rect" x="0" y="100" width="50" height="50" fill="white" mask="url(#mask4)"/> > <mask id="mask4"> > <use xlink:href="#rect"/> > </mask> > </defs> > </svg> > > At the end of FrameView::layout() the render tree looks like this: > > > RenderView 0x3dbc2d8 #document 0x3e67420 > RenderSVGRoot 0x3eba5a8 svg 0x3eb85c0 > RenderSVGHiddenContainer 0x3ebc6b8 defs 0x3ebc460 > * RenderSVGPath 0x3ebdde8 rect 0x3ebca40 > RenderSVGResourceMasker 0x3ec0168 mask 0x3ebfa40 > RenderSVGContainer 0x3daf418 use 0x3ebf260 > RenderSVGContainer 0x3db05b8 g 0x3daf4e0 > RenderSVGPath 0x3db0738 rect 0x3dafd90 > > > and the RenderSVGPath associated with the <rect> is still marked as m_needsLayout. The one marked with the star * ? Or the one cloned <rect> within the <use> subtree? I guess that's RenderSVGHiddenContainer::layout's fault. More precisely the SVGRenderSupport::layoutChildren optimizations, we could end up with an inconsistent tree there... Can you investigate a bit further, where it fails to go in the subtree?
(In reply to comment #3) > (In reply to comment #2) > > (In reply to comment #1) > > > Should children of a <defs> exist in the render tree at all? They should never be rendered according to the SVG spec. > > > > They are accessible with xlink:href by <use>, <symbol> and <feImage> and need to have a renderer. > This is unrelated. I was talking about RenderSVGPath.
The testcase can be reduced some more: <svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink"> <defs> <rect id="rect" x="0" y="100" width="50" height="50" fill="white" mask="url(#mask4)"/> <mask id="mask4"> </mask> </defs> </svg> First look points at RenderSVGResourceContainer::layout(), where removeAllClientsFromCache can be called. For the above case this means the already layouted rect (done in SVGSupport::layoutChildren) gets invalidated, so setNeedsLayout(true) is called on it and the assert from bug 49019 is hit. The problem seems to be only triggered when the resource is after the referencing element. Cheers, Rob.
(In reply to comment #6) > The testcase can be reduced some more: > > > <svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink"> > <defs> > <rect id="rect" x="0" y="100" width="50" height="50" fill="white" mask="url(#mask4)"/> > <mask id="mask4"> > </mask> > </defs> > </svg> > > First look points at RenderSVGResourceContainer::layout(), where removeAllClientsFromCache can be called. For the above case this means the already layouted rect (done in SVGSupport::layoutChildren) gets > invalidated, so setNeedsLayout(true) is called on it and the assert from bug 49019 is hit. The problem seems to be only triggered when the resource is after the referencing element. > Cheers, > > Rob. If I reference the rect after the <defs> section by a <use> element, the test works, but if it is before this section, it asserts as well. So it might be a problem that the parsing of the document did not finished and the resources are unknown. Note, this only happens for mask and filter, not for clipper, gradient or pattern.
(In reply to comment #7) > If I reference the rect after the <defs> section by a <use> element, the test works, but if it is before this section, it asserts as well. So it might be a problem that the parsing of the document did not finished and the resources are unknown. Note, this only happens for mask and filter, not for clipper, gradient or pattern. Never mind last comment, wrong bug.
Hm, I can't reproduce this anymore. James, do you want to give your patch at bug 49019 another try?
(In reply to comment #9) > Hm, I can't reproduce this anymore. James, do you want to give your patch at bug 49019 another try? There are 3 other open bugs blocking that one, so I'm not super optimistic. Might give it a try on monday.
(In reply to comment #10) > (In reply to comment #9) > > Hm, I can't reproduce this anymore. James, do you want to give your patch at bug 49019 another try? > > There are 3 other open bugs blocking that one, so I'm not super optimistic. Might give it a try on monday. Heh just realized, that I couldn't see any assertion w/o applying your patch. I'll do that soon, and test svg/ with it.
(In reply to comment #11) > Heh just realized, that I couldn't see any assertion w/o applying your patch. I'll do that soon, and test svg/ with it. Okay, the original testcase from James, and the one modified by Rob don't trigger layout invariant assertions anymore, but there are two other svg tests firing now: svg/custom/layout-loop.svg (evil, will be hard to track down) svg/custom/use-crash-pending-resource.svg (also not a simple one) I'll see what I can do.
Fixed thanks to bug 81006, and change r111601: <http://trac.webkit.org/changeset/111601>