Bug 62914 – Incorrectly placed SVG gradients can cause crashes when referenced

• Component: SVG
• Status: RESOLVED FIXED

[Vicki Pfau]

Created attachment 97665 [details]
Repro

<rdar://problem/9516596>

Reproducing test case attached

  1 com.apple.WebCore              0x7fff919990d6 WebCore::SVGStopElement::stopColorIncludingOpacity() const + 0x8
   2 com.apple.WebCore              0x7fff91998f0a WebCore::SVGGradientElement::buildStops() + 0x9c
   3 com.apple.WebCore              0x7fff92098718 WebCore::SVGLinearGradientElement::collectGradientAttributes(WebCore::LinearGradientAttributes&) + 0x1b0
   4 com.apple.WebCore              0x7fff91fed34c WebCore::RenderSVGResourceLinearGradient::collectGradientAttributes(WebCore::SVGGradientElement*) + 0x192
   5 com.apple.WebCore              0x7fff919987a8 WebCore::RenderSVGResourceGradient::applyResource(WebCore::RenderObject*, WebCore::RenderStyle*, WebCore::GraphicsContext*&, unsigned short) + 0x86
   6 com.apple.WebCore              0x7fff91ff0fad non-virtual thunk to WebCore::RenderSVGResourceGradient::applyResource(WebCore::RenderObject*, WebCore::RenderStyle*, WebCore::GraphicsContext*&, unsigned short) + 0xd
   7 com.apple.WebCore              0x7fff91ff48c3 WebCore::RenderSVGPath::fillAndStrokePath(WebCore::GraphicsContext*) + 0x6b
   8 com.apple.WebCore              0x7fff91fe8adf WebCore::RenderSVGPath::paint(WebCore::PaintInfo&, int, int) + 0x1c7
   9 com.apple.WebCore              0x7fff91fe4d24 WebCore::RenderSVGContainer::paint(WebCore::PaintInfo&, int, int) + 0x1ac
  10 com.apple.WebCore              0x7fff91fe4d24 WebCore::RenderSVGContainer::paint(WebCore::PaintInfo&, int, int) + 0x1ac
  11 com.apple.WebCore              0x7fff91fa42c4 WebCore::RenderBox::paint(WebCore::PaintInfo&, int, int) + 0x94
  12 com.apple.WebCore              0x7fff91ff0271 WebCore::RenderSVGRoot::paint(WebCore::PaintInfo&, int, int) + 0x295
  13 com.apple.WebCore              0x7fff91caad43 WebCore::InlineBox::paint(WebCore::PaintInfo&, int, int, int, int) + 0x157
  14 com.apple.WebCore              0x7fff91cab789 WebCore::InlineFlowBox::paint(WebCore::PaintInfo&, int, int, int, int) + 0x339
  15 com.apple.WebCore              0x7fff92022443 WebCore::RootInlineBox::paint(WebCore::PaintInfo&, int, int, int, int) + 0x37
  16 com.apple.WebCore              0x7fff91fcbc01 WebCore::RenderLineBoxList::paint(WebCore::RenderBoxModelObject*, WebCore::PaintInfo&, int, int) const + 0x355
  17 com.apple.WebCore              0x7fff91f92621 WebCore::RenderBlock::paintObject(WebCore::PaintInfo&, int, int) + 0x14d
  18 com.apple.WebCore              0x7fff91f929e2 WebCore::RenderBlock::paint(WebCore::PaintInfo&, int, int) + 0x112
  19 com.apple.WebCore              0x7fff91f91d90 WebCore::RenderBlock::paintChildren(WebCore::PaintInfo&, int, int) + 0x1e2
  20 com.apple.WebCore              0x7fff91f92635 WebCore::RenderBlock::paintObject(WebCore::PaintInfo&, int, int) + 0x161
  21 com.apple.WebCore              0x7fff91f929e2 WebCore::RenderBlock::paint(WebCore::PaintInfo&, int, int) + 0x112
  22 com.apple.WebCore              0x7fff91f91d90 WebCore::RenderBlock::paintChildren(WebCore::PaintInfo&, int, int) + 0x1e2
  23 com.apple.WebCore              0x7fff91f92635 WebCore::RenderBlock::paintObject(WebCore::PaintInfo&, int, int) + 0x161
  24 com.apple.WebCore              0x7fff91f929e2 WebCore::RenderBlock::paint(WebCore::PaintInfo&, int, int) + 0x112
  25 com.apple.WebCore              0x7fff91fc4513 WebCore::RenderLayer::paintLayer(WebCore::RenderLayer*, WebCore::GraphicsContext*, WebCore::IntRect const&, unsigned int, WebCore::RenderObject*, WTF::HashMap<WebCore::OverlapTestRequestClient*, WebCore::IntRect, WTF::PtrHash<WebCore::OverlapTestRequestClient*>, WTF::HashTraits<WebCore::OverlapTestRequestClient*>, WTF::HashTraits<WebCore::IntRect> >*, unsigned int) + 0xdb9
  26 com.apple.WebCore              0x7fff91fc47dd WebCore::RenderLayer::paintLayer(WebCore::RenderLayer*, WebCore::GraphicsContext*, WebCore::IntRect const&, unsigned int, WebCore::RenderObject*, WTF::HashMap<WebCore::OverlapTestRequestClient*, WebCore::IntRect, WTF::PtrHash<WebCore::OverlapTestRequestClient*>, WTF::HashTraits<WebCore::OverlapTestRequestClient*>, WTF::HashTraits<WebCore::IntRect> >*, unsigned int) + 0x1083
  27 com.apple.WebCore              0x7fff91770fcd WebCore::RenderLayer::paint(WebCore::GraphicsContext*, WebCore::IntRect const&, unsigned int, WebCore::RenderObject*) + 0x47
  28 com.apple.WebCore              0x7fff91770e90 WebCore::FrameView::paintContents(WebCore::GraphicsContext*, WebCore::IntRect const&) + 0x27a
  29 com.apple.WebKit               0x7fff948a42df -[WebFrame(WebInternal) _drawRect:contentsOnly:] + 0x10f
  30 com.apple.WebKit               0x7fff948a3ee0 -[WebHTMLView drawSingleRect:] + 0x1c0
  31 com.apple.WebKit               0x7fff948a3bd2 -[WebHTMLView drawRect:] + 0x2f2
  32 com.apple.AppKit               0x7fff92c2437b -[NSView _drawRect:clip:] + 0xeae
  33 com.apple.AppKit               0x7fff92c51a30 -[NSView _recursiveDisplayAllDirtyWithLockFocus:visRect:] + 0x62f
  34 com.apple.WebKit               0x7fff948a37c3 -[WebHTMLView(WebPrivate) _recursiveDisplayAllDirtyWithLockFocus:visRect:] + 0x103
  35 com.apple.AppKit               0x7fff92c51e5c -[NSView _recursiveDisplayAllDirtyWithLockFocus:visRect:] + 0xa5b
  36 com.apple.AppKit               0x7fff92c51e5c -[NSView _recursiveDisplayAllDirtyWithLockFocus:visRect:] + 0xa5b
  37 com.apple.AppKit               0x7fff92c51e5c -[NSView _recursiveDisplayAllDirtyWithLockFocus:visRect:] + 0xa5b
  38 com.apple.AppKit               0x7fff92c51e5c -[NSView _recursiveDisplayAllDirtyWithLockFocus:visRect:] + 0xa5b
  39 com.apple.AppKit               0x7fff92c51e5c -[NSView _recursiveDisplayAllDirtyWithLockFocus:visRect:] + 0xa5b
  40 com.apple.AppKit               0x7fff92c51e5c -[NSView _recursiveDisplayAllDirtyWithLockFocus:visRect:] + 0xa5b
  41 com.apple.AppKit               0x7fff92c21959 -[NSView _recursiveDisplayRectIfNeededIgnoringOpacity:isVisibleRect:rectIsVisibleRectForView:topView:] + 0x408
  42 com.apple.AppKit               0x7fff92c22c0e -[NSView _recursiveDisplayRectIfNeededIgnoringOpacity:isVisibleRect:rectIsVisibleRectForView:topView:] + 0x16bd
  43 com.apple.AppKit               0x7fff92c22c0e -[NSView _recursiveDisplayRectIfNeededIgnoringOpacity:isVisibleRect:rectIsVisibleRectForView:topView:] + 0x16bd
  44 com.apple.AppKit               0x7fff92c20e4f -[NSThemeFrame _recursiveDisplayRectIfNeededIgnoringOpacity:isVisibleRect:rectIsVisibleRectForView:topView:] + 0x10e
  45 com.apple.AppKit               0x7fff92c1c267 -[NSView _displayRectIgnoringOpacity:isVisibleRect:rectIsVisibleRectForView:] + 0x1293
  46 com.apple.AppKit               0x7fff92c14d10 -[NSView displayIfNeeded] + 0x5b9
  47 com.apple.Safari.framework     0x7fff94004e12 -[BrowserWindow displayIfNeeded] + 0x2b
  48 com.apple.AppKit               0x7fff92c14522 _handleWindowNeedsDisplayOrLayoutOrUpdateConstraints + 0x288
  49 com.apple.CoreFoundation       0x7fff8d41cfb7 __CFRUNLOOP_IS_CALLING_OUT_TO_AN_OBSERVER_CALLBACK_FUNCTION__ + 0x17
  50 com.apple.CoreFoundation       0x7fff8d41cf16 __CFRunLoopDoObservers + 0x176
  51 com.apple.CoreFoundation       0x7fff8d3f2059 __CFRunLoopRun + 0x339
  52 com.apple.CoreFoundation       0x7fff8d3f19e6 CFRunLoopRunSpecific + 0xe6
  53 com.apple.HIToolbox            0x7fff97aa9857 RunCurrentEventLoopInMode + 0x115
  54 com.apple.HIToolbox            0x7fff97ab0ff1 ReceiveNextEventCommon + 0x163
  55 com.apple.HIToolbox            0x7fff97ab0e7e BlockUntilNextEventMatchingListInMode + 0x3e
  56 com.apple.AppKit               0x7fff92bd93b1 _DPSNextEvent + 0x293
  57 com.apple.AppKit               0x7fff92bd8cb8 -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] + 0x87
  58 com.apple.Safari.framework     0x7fff93fad821 -[BrowserApplication nextEventMatchingMask:untilDate:inMode:dequeue:] + 0xab
  59 com.apple.AppKit               0x7fff92bd565f -[NSApplication run] + 0x1c8
  60 com.apple.AppKit               0x7fff92e52f35 NSApplicationMain + 0x35c
  61 com.apple.Safari.framework     0x7fff9415db87 SafariMain + 0xc5
  62 com.apple.Safari               0x108cf9f24 start + 0x0

[Vicki Pfau]

Created attachment 97823 [details]
Patch

[Dirk Schulze]

Comment on attachment 97823 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=97823&action=review

The fix is not correct, see comments below.

> LayoutTests/svg/custom/invalid-gradient-with-xlink.svg:22
> +<?xml version="1.0" encoding="UTF-8" standalone="no"?>
> +<svg
> +   xmlns="http://www.w3.org/2000/svg"
> +   xmlns:xlink="http://www.w3.org/1999/xlink"
> +   version="1.0"
> +   width="200"
> +   height="200">
> +<script><![CDATA[
> +      if (window.layoutTestController)
> +        layoutTestController.dumpAsText();
> +]]></script>
> +<defs>
> +  <linearGradient id="linearGradient1" xlink:href="#linearGradient0" />
> +</defs>
> +<path d="M 0,0 C 0,1 1,1 1,0 L 0,0 z " style="fill:url(#linearGradient1);"  id="path0" />
> +<path id="path1">
> +  <linearGradient id="linearGradient0">
> +    <stop />
> +  </linearGradient>
> +</path>
> +<text y="20" x="10">PASS: The test did not crash.</text>
> +</svg>

Given the fact that we should fill the rect with a gradient, a dumpAsText test is not enough. We need a pixel test. The style of the SVG does not make it self explaining. Use something more simple like that:

<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">
<defs>
  <linearGradient id="gradient1" xlink:href="#gradient2" />
</defs>
<rect width="200" height="200" fill="url(#gradient1)">
  <linearGradient id="gradient2">
    <stop stop-color="green"/>
  </linearGradient>
</rect>
</svg>

The green indicates that we should draw fill the rect with the gradient. On pass you'd see a green rect.

> Source/WebCore/ChangeLog:8
> +        Added check to ensure gradient stops have the correct properties before attempting to access them.

The patch is not correct. We should draw the gradient, independent where it is located. In a <defs> section or not does not matter.

[Vicki Pfau]

(In reply to comment #2)
> <svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">
> <defs>
>   <linearGradient id="gradient1" xlink:href="#gradient2" />
> </defs>
> <rect width="200" height="200" fill="url(#gradient1)">
>   <linearGradient id="gradient2">
>     <stop stop-color="green"/>
>   </linearGradient>
> </rect>
> </svg>
> 
> The green indicates that we should draw fill the rect with the gradient. On pass you'd see a green rect.
> 
> > Source/WebCore/ChangeLog:8
> > +        Added check to ensure gradient stops have the correct properties before attempting to access them.
> 
> The patch is not correct. We should draw the gradient, independent where it is located. In a <defs> section or not does not matter.

The real problem here is that <linearGradient> is not allowed to be inside of a <rect> or a <path> element (or any other shape element). Thus, the spec does not define the correct behavior. The reason it was crashing was that the color properties are stored in the renderer, but because the parent object of the linear gradient, a path, can't have children (as RenderSVGPath is not a subclass of RenderSVGContainer), it doesn't generate a rendering context for the gradient. However, the styles for the stops are stored in the context. Thus, to have a gradient work wherever it is encountered, it must always have a context, which means that its parent (in this case, a RenderSVGPath) must be able to have children (which it can't). Unless one of these behaviors (colors stored in the renderer, RenderSVGPath can't have children, etc) is changed, having a gradient defined anywhere in the file is not feasible.

[Vicki Pfau]

For what it's worth, Firefox also chokes on this case and spits out something that is not a gradient (just pure black fill) when rendering the invalid SVG.

[Maciej Stachowiak]

(In reply to comment #2)

> > Source/WebCore/ChangeLog:8
> > +        Added check to ensure gradient stops have the correct properties before attempting to access them.
> 
> The patch is not correct. We should draw the gradient, independent where it is located. In a <defs> section or not does not matter.

I think you are right that the code will now misrender. But surely misrendering is better than crashing. Is there any case that would have worked without crashing under the old code that would get worse with this patch?

If not, I think we should land the simple crash fix for now, and file a follow-up bug on the rendering issue. What do you think?

[Maciej Stachowiak]

(In reply to comment #5)
> (In reply to comment #2)
> 
> > > Source/WebCore/ChangeLog:8
> > > +        Added check to ensure gradient stops have the correct properties before attempting to access them.
> > 
> > The patch is not correct. We should draw the gradient, independent where it is located. In a <defs> section or not does not matter.
> 
> I think you are right that the code will now misrender. But surely misrendering is better than crashing. Is there any case that would have worked without crashing under the old code that would get worse with this patch?
> 
> If not, I think we should land the simple crash fix for now, and file a follow-up bug on the rendering issue. What do you think?

Hmm, actually, based on Jeffrey's latest comment, the patch does not break gradients defined in <defs>, it only affects gradients defined in illegal places (such as inside a <rect> or <path>), where per the SVG spec they should *not* render.

[Vicki Pfau]

Created attachment 97896 [details]
Patch

[Dirk Schulze]

(In reply to comment #6)

> Hmm, actually, based on Jeffrey's latest comment, the patch does not break gradients defined in <defs>, it only affects gradients defined in illegal places (such as inside a <rect> or <path>), where per the SVG spec they should *not* render.

I don't see a comment in the spec that adding resources like gradients, to styled elements like rect and path is forbidden. Please post a link if I'm wrong. But I agree that RenderSVGPath can't have child renderers in WebKit (or at least it is not implemented yet). I tested  the example from https://bugs.webkit.org/show_bug.cgi?id=62914#c2 in Batik, Opera and Firefox. Opera and Batik show a green rect, while Firefox fills the rect white. If the rect references the gradient directly, Firefox fills the rect black (the fallback color), the others still green.

[Vicki Pfau]

(In reply to comment #8)
> I don't see a comment in the spec that adding resources like gradients, to styled elements like rect and path is forbidden. Please post a link if I'm wrong. But I agree that RenderSVGPath can't have child renderers in WebKit (or at least it is not implemented yet). I tested  the example from https://bugs.webkit.org/show_bug.cgi?id=62914#c2 in Batik, Opera and Firefox. Opera and Batik show a green rect, while Firefox fills the rect white. If the rect references the gradient directly, Firefox fills the rect black (the fallback color), the others still green.

It's not so much that gradients in rects, etc, is explicitly disallowed, so much as gradients are not in the list of allowed elements within a rect (according to the DTD, etc). As such, it's not explicitly required that we support the gradient as being seen as valid in this context. While I agree that it would be a desirable behavior for it to be seen as valid, I don't see it as a necessary behavior, and this patch is at least a push in the right direction.

[Dirk Schulze]

Comment on attachment 97896 [details]
Patch

Given the fact that firefox does not handle the case as well, we can land the fix. But please update the ChangeLog that we follow Firefox on handling such cases.

Can you please update your test to the style of https://bugs.webkit.org/show_bug.cgi?id=62914#c2 ? There is no need to add unnecessary elements and information. Make test cases as simple as possible. But it should still be a dumpAsText test of course.


r- because of the test.

[Dirk Schulze]

(In reply to comment #9)
> (In reply to comment #8)
> > I don't see a comment in the spec that adding resources like gradients, to styled elements like rect and path is forbidden. Please post a link if I'm wrong. But I agree that RenderSVGPath can't have child renderers in WebKit (or at least it is not implemented yet). I tested  the example from https://bugs.webkit.org/show_bug.cgi?id=62914#c2 in Batik, Opera and Firefox. Opera and Batik show a green rect, while Firefox fills the rect white. If the rect references the gradient directly, Firefox fills the rect black (the fallback color), the others still green.
> 
> It's not so much that gradients in rects, etc, is explicitly disallowed, so much as gradients are not in the list of allowed elements within a rect (according to the DTD, etc). As such, it's not explicitly required that we support the gradient as being seen as valid in this context. While I agree that it would be a desirable behavior for it to be seen as valid, I don't see it as a necessary behavior, and this patch is at least a push in the right direction.

I'm sorry. Did not see this comment. You're right. The DTD disallows linearGradient as child. This means for my example, that we should never find #gradient2 at all. Now it is clear why Firefox fills the rect black (the fallback color) if you fill it with #gradient2. I guess we would still fill it with the gradient, have you checked this?

[Vicki Pfau]

(In reply to comment #11) 
> I'm sorry. Did not see this comment. You're right. The DTD disallows linearGradient as child. This means for my example, that we should never find #gradient2 at all. Now it is clear why Firefox fills the rect black (the fallback color) if you fill it with #gradient2. I guess we would still fill it with the gradient, have you checked this?

It currently renders the entire region as transparent. I can change this behavior, though.

[Vicki Pfau]

Created attachment 98034 [details]
Repro with visible rect

Here is a version of the repro that outputs a rect that, if filled, should be visible.

[Rob Buis]

I looked at this and it seems to me that the problem/crash is due to RenderSVGPath not allowing any render children at all. This is by itself correct, but it means the linear gradient+stop have no renderer associated, causing the crash. The patch fixes the problem, but I think you can look at detecting the problem earlier. How about testing somewhere between applyResource and buildStops that the referenced gradient has no renderer and bail out early? This has of course the added benefit of doing less work before detecting the problem. What do you think?
Cheers,

Rob.

[Nikolas Zimmermann]

(In reply to comment #14)
> I looked at this and it seems to me that the problem/crash is due to RenderSVGPath not allowing any render children at all. This is by itself correct, but it means the linear gradient+stop have no renderer associated, causing the crash. The patch fixes the problem, but I think you can look at detecting the problem earlier. How about testing somewhere between applyResource and buildStops that the referenced gradient has no renderer and bail out early? This has of course the added benefit of doing less work before detecting the problem. What do you think?
> Cheers,

Yes, it has to be done earlier, everything else is a hack/work-around.
The SVGResourcesCycleSolver could be extended to handle checking whether a gradient has a renderer or not, and bail out ("break the cycle") if needed. This seems a much safer place to that, than relying on a null check somewhere.

[Vicki Pfau]

Created attachment 98061 [details]
Patch

[Vicki Pfau]

It looks like the first place that the gradient rendering routine checks anything about the gradient's linked gradient other than if it exists is during the collect loop, which is pretty far along. Anything before that is really just probing to see if the gradient exists, and doesn't continue down the chain to see if it's valid or if it has children, etc. I've hijacked the loop a bit, but this should work.

[Nikolas Zimmermann]

Comment on attachment 98061 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=98061&action=review

Almost perfect! I'd like to see another testcase, then I'll r+ it.

> LayoutTests/svg/custom/invalid-gradient-with-xlink.svg:10
> +        layoutTestController.dumpAsText(true);

Great, dumpAsText + pixel test!

> Source/WebCore/rendering/svg/RenderSVGResourceGradient.cpp:160
> -        collectGradientAttributes(gradientElement);
> +        if (!collectGradientAttributes(gradientElement))
> +            return false;
> +

Ok I made up my mind, this place is just fine as well, no need to dig into SVGResourcesCycleSolver.

I'd like to request another testcase:
<defs>
    <linearGradient id="vallidGradient">
         <stop offset=....
    </linearGradient>
 </defs>

<rect fill="url(#grad1) green">
    <linearGradient id="grad1" xlink:href="#validGradient"/>
</rect>

This should assure that invalidGradient -> validGradient, indirections are also ignored.

[Dirk Schulze]

(In reply to comment #18)
> (From update of attachment 98061 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=98061&action=review
> 
> Almost perfect! I'd like to see another testcase, then I'll r+ it.
> 
> > LayoutTests/svg/custom/invalid-gradient-with-xlink.svg:10
> > +        layoutTestController.dumpAsText(true);
> 
> Great, dumpAsText + pixel test!
> 
> > Source/WebCore/rendering/svg/RenderSVGResourceGradient.cpp:160
> > -        collectGradientAttributes(gradientElement);
> > +        if (!collectGradientAttributes(gradientElement))
> > +            return false;
> > +
> 
> Ok I made up my mind, this place is just fine as well, no need to dig into SVGResourcesCycleSolver.
> 
> I'd like to request another testcase:
> <defs>
>     <linearGradient id="vallidGradient">
>          <stop offset=....
>     </linearGradient>
>  </defs>
> 
> <rect fill="url(#grad1) green">
>     <linearGradient id="grad1" xlink:href="#validGradient"/>
> </rect>
> 
> This should assure that invalidGradient -> validGradient, indirections are also ignored.

I'm not sure, but I think it is still strange that we do not support gradients nested in Shapes, but the following example works (shows a red rect):

<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">
<rect width="100" height="100">
	<g id="test">
		<rect width="100" height="100" fill="red"/>
	</g>
</rect>
<use xlink:href="#test"/>
</svg>

[Nikolas Zimmermann]

(In reply to comment #19)
> (In reply to comment #18)
> > (From update of attachment 98061 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=98061&action=review
> > 
> > Almost perfect! I'd like to see another testcase, then I'll r+ it.
> > 
> > > LayoutTests/svg/custom/invalid-gradient-with-xlink.svg:10
> > > +        layoutTestController.dumpAsText(true);
> > 
> > Great, dumpAsText + pixel test!
> > 
> > > Source/WebCore/rendering/svg/RenderSVGResourceGradient.cpp:160
> > > -        collectGradientAttributes(gradientElement);
> > > +        if (!collectGradientAttributes(gradientElement))
> > > +            return false;
> > > +
> > 
> > Ok I made up my mind, this place is just fine as well, no need to dig into SVGResourcesCycleSolver.
> > 
> > I'd like to request another testcase:
> > <defs>
> >     <linearGradient id="vallidGradient">
> >          <stop offset=....
> >     </linearGradient>
> >  </defs>
> > 
> > <rect fill="url(#grad1) green">
> >     <linearGradient id="grad1" xlink:href="#validGradient"/>
> > </rect>
> > 
> > This should assure that invalidGradient -> validGradient, indirections are also ignored.
> 
> I'm not sure, but I think it is still strange that we do not support gradients nested in Shapes, but the following example works (shows a red rect):
First of all, I want to assure with the testcase I requested, that we don't ignore the fact that "grad1" has no renderer. Before Jeffs patch, this would just work, as the indirection to "grad1" is ignored. It ends up using "validGradient" in trunk.

> 
> <svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">
> <rect width="100" height="100">
>     <g id="test">
>         <rect width="100" height="100" fill="red"/>
>     </g>
> </rect>
> <use xlink:href="#test"/>
> </svg>

You can not compare this in any way. You should know what happens if we <use> anything in the document. It just gets cloned, no matter in any way where it is. Argubly, this is wrong. Though just excluding nodes from <use> cloning, because they have no renderer is wrong. Anyhow, this is offtopic for this bug report, feel free to open another one.

[Dirk Schulze]

(In reply to comment #20)

> You can not compare this in any way. You should know what happens if we <use> anything in the document. It just gets cloned, no matter in any way where it is. Argubly, this is wrong. Though just excluding nodes from <use> cloning, because they have no renderer is wrong. Anyhow, this is offtopic for this bug report, feel free to open another one.

At first, I don't think it is off topic. Like Jeffrey wrote before, this is not allowed according to the DTD of SVG. So the SVG is invalid at this place. Second, why should webdevelopers care about how we handle use. Both are references by xlink. While it works with <use> it doesn't work with resources. Third, our plan for the future was to share renderers per node and don't clone nodes anymore. So it definitely will get relevant again later. Also, do we have tests with other resources? What about pattern, clipper and masker? Should they work? Do they work already?

[Nikolas Zimmermann]

(In reply to comment #21)
> (In reply to comment #20)
> 
> > You can not compare this in any way. You should know what happens if we <use> anything in the document. It just gets cloned, no matter in any way where it is. Argubly, this is wrong. Though just excluding nodes from <use> cloning, because they have no renderer is wrong. Anyhow, this is offtopic for this bug report, feel free to open another one.
> 
> At first, I don't think it is off topic. Like Jeffrey wrote before, this is not allowed according to the DTD of SVG. So the SVG is invalid at this place. Second, why should webdevelopers care about how we handle use. 
As I said before it's off topic: this bug report is about incorrect placed _gradients_.


>Both are references by xlink. While it works with <use> it doesn't work with resources. Third, our plan for the future was to share renderers per node and don't clone nodes anymore. So it definitely will get relevant again later. Also, do we have tests with other resources? What about pattern, clipper and masker? Should they work? Do they work already?

<mask> / <clip-path> don't support xlink:href. <pattern> is affected as well.

To rephrase: that it currently works with <use> is a side-effect of our use implementation, and thus it's _irrelevant_ here.

Please let's focus on Jeffs patch here, and move the <use> discussion somewhere else.
Jeffrey, can you have a look at patterns as well? Dirk has a good point, they're affect in the same way as gradients.

[Dirk Schulze]

(In reply to comment #22)
> (In reply to comment #21)
> > (In reply to comment #20)
> > 
> > > You can not compare this in any way. You should know what happens if we <use> anything in the document. It just gets cloned, no matter in any way where it is. Argubly, this is wrong. Though just excluding nodes from <use> cloning, because they have no renderer is wrong. Anyhow, this is offtopic for this bug report, feel free to open another one.
> > 
> > At first, I don't think it is off topic. Like Jeffrey wrote before, this is not allowed according to the DTD of SVG. So the SVG is invalid at this place. Second, why should webdevelopers care about how we handle use. 
> As I said before it's off topic: this bug report is about incorrect placed _gradients_.
Indeed, incorrect placed _gradients_ that should (according to the DTD) lead to a parsing error. Therefor we should render the SVG up to this point and give a parsing error back. At least if we follow the DTD. Thats what I was referring to.And it of course affects the <use> case as well.

If we take the document as valid, I agree that pattern and gradient are the only candidates that should/could be affected.

[Nikolas Zimmermann]

(In reply to comment #23)
> (In reply to comment #22)
> > (In reply to comment #21)
> > > (In reply to comment #20)
> > > 
> > > > You can not compare this in any way. You should know what happens if we <use> anything in the document. It just gets cloned, no matter in any way where it is. Argubly, this is wrong. Though just excluding nodes from <use> cloning, because they have no renderer is wrong. Anyhow, this is offtopic for this bug report, feel free to open another one.
> > > 
> > > At first, I don't think it is off topic. Like Jeffrey wrote before, this is not allowed according to the DTD of SVG. So the SVG is invalid at this place. Second, why should webdevelopers care about how we handle use. 
> > As I said before it's off topic: this bug report is about incorrect placed _gradients_.
> Indeed, incorrect placed _gradients_ that should (according to the DTD) lead to a parsing error. Therefor we should render the SVG up to this point and give a parsing error back. At least if we follow the DTD. Thats what I was referring to.And it of course affects the <use> case as well.
Okay, if you think about stopping the parsing, then you're right, and it's closely related as it affects all elements and such constructs.
Hm, I never saw any SVG viewer (except Batik) to report DTD violations and stop parsing.
Is that not the case anymore?
Another case: say the document is valid, and I'm adding a <linearGradient> element as <rect> child, that's not allowed to be there according to the DTD. Do we disallow that? From a DOM perspective this insertion is fine. From a render tree perspective, we're already ignoring this and don't build a linearGradient renderer.

We have to think about this. Do you remember the crashes we had because arbitary children were allowed as <text> kids. That lead to wrong assumptions in the rendering code. We fixed this by specifically disallowing renderer creation for all text children but the allowed ones: altGlyph/tspan/tref/textPath/a.

> If we take the document as valid, I agree that pattern and gradient are the only candidates that should/could be affected.
Okay great we reached consensus. I see it in exactly the same: if the doc is valid, pattern/gradients are the only candidates that we have to look into, otherwhise a whole broad range of new possibilities have to be considered regarding strict DTD correctness (if we want that -- I'd say yes, we want this for the _render tree_, but I'm not sure about the DOM tree).

This is gettin interssting :-)

[Vicki Pfau]

(In reply to comment #22)
> Jeffrey, can you have a look at patterns as well? Dirk has a good point, they're affect in the same way as gradients.

Patterns don't crash, but they handle invalid cases somewhat oddly: xlink to a pattern in defs that has an xlink to an invalid pattern doesn't crash, it gives us transparent (but not the fallback color, if there is a fallback color). xlink to an invalid pattern gives us the fallback color or black if there is no fallback color. This sounds like it might be appropriate for a separate bug.

I'll add the requested test case soon, and then resubmit.

[Vicki Pfau]

Created attachment 98278 [details]
Patch

[Nikolas Zimmermann]

Comment on attachment 98278 [details]
Patch

r=me. Jeff, can you create the same testcases for pattern in a follow-up bug?

[WebKit Review Bot]

Comment on attachment 98278 [details]
Patch

Clearing flags on attachment: 98278

Committed r89550: <http://trac.webkit.org/changeset/89550>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.