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
Created attachment 97823 [details] Patch
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.
(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.
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.
(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?
(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.
Created attachment 97896 [details] Patch
(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.
(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.
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.
(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?
(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.
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.
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.
(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.
Created attachment 98061 [details] Patch
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.
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.
(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>
(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.
(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?
(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.
(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.
(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 :-)
(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.
Created attachment 98278 [details] Patch
Comment on attachment 98278 [details] Patch r=me. Jeff, can you create the same testcases for pattern in a follow-up bug?
Comment on attachment 98278 [details] Patch Clearing flags on attachment: 98278 Committed r89550: <http://trac.webkit.org/changeset/89550>
All reviewed patches have been landed. Closing bug.