Bug 49035 – SVG <path> inside a <defs> is still marked as needing layout at the end of FrameView::layout

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[James Robinson]

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.

[James Robinson]

Should children of a <defs> exist in the render tree at all?  They should never be rendered according to the SVG spec.

[Dirk Schulze]

(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.

[Nikolas Zimmermann]

(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.

[Nikolas Zimmermann]

(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?

[Dirk Schulze]

(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.

[Rob Buis]

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.

[Dirk Schulze]

(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.

[Dirk Schulze]

(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.

[Nikolas Zimmermann]

Hm, I can't reproduce this anymore. James, do you want to give your patch at bug 49019 another try?

[James Robinson]

(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.

[Nikolas Zimmermann]

(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.

[Nikolas Zimmermann]

(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.

[Stephen Chenney]

Fixed thanks to bug 81006, and change r111601: <http://trac.webkit.org/changeset/111601>