Created attachment 44333 [details] Pattern calls itself We need to make sure, that the content of a pattern/mask doesn't call the pattern/mask itself. This would cause infinity recursion, and webkit crashes. The <use> element already takes care about that.
*** Bug 37751 has been marked as a duplicate of this bug. ***
Created attachment 61484 [details] Initial patch
Comment on attachment 61484 [details] Initial patch WebCore/rendering/RenderSVGResourceContainer.h:38 + , m_id(node->hasID() ? node->getIdAttribute() : nullAtom) Is this testable? Do we need a test for this change? WebCore/rendering/RenderSVGResourceContainer.h:59 + m_id = static_cast<Element*>(node())->getIdAttribute(); Don't we have an asElement() call or similar? Is this static_cast always safe? WebCore/svg/SVGPaint.cpp:114 + bool SVGPaint::referencedResourceEqualsToID(SVGPaint* paint, const String& referenceId) This isn't really english. I think you might mean: isURIWithTarget(target) Why should this be a static method? Seems it should be an instance method. Otherwise looks good.
(In reply to comment #3) > (From update of attachment 61484 [details]) > WebCore/rendering/RenderSVGResourceContainer.h:38 > + , m_id(node->hasID() ? node->getIdAttribute() : nullAtom) > Is this testable? Do we need a test for this change? No, this is not testable in SVG documents. The only difference is that the id would have been lowercased before, if the document would be in "compatibility mode", which is not the case for SVG. > WebCore/rendering/RenderSVGResourceContainer.h:59 > + m_id = static_cast<Element*>(node())->getIdAttribute(); > Don't we have an asElement() call or similar? Is this static_cast always safe? Yes, it's always safe, RenderSVGResourceContains are only constructed for SVGStyledElements. See: constructor "RenderSVGResourceContainer(SVGStyledElement* node)". > > WebCore/svg/SVGPaint.cpp:114 > + bool SVGPaint::referencedResourceEqualsToID(SVGPaint* paint, const String& referenceId) > This isn't really english. I think you might mean: > > isURIWithTarget(target) > > Why should this be a static method? Seems it should be an instance method. Going to fix that, and name it "machesTargetURI".
Created attachment 61489 [details] Updated patch Not yet tested build with the new changes, EWS is faster than my machine.
Created attachment 61493 [details] Updated patch v2 This one actually builds and works, tested locally. Turns out EWS was not faster today :-)
Comment on attachment 61493 [details] Updated patch v2 LGTM. Do we need to rip out any of the old recursion prevention code?
(In reply to comment #7) > (From update of attachment 61493 [details]) > LGTM. Do we need to rip out any of the old recursion prevention code? We didn't have any recursion prevention code for masks/patterns until now. Only markers have special code, that can be modified in a follow-up patch (to integrate within this new logic)
Landed in r63300.
Pattern still have an issue with self-referencing, going to upload a fix soon, found by Dirk.
Created attachment 61617 [details] New patch Forgot that patterns can be chained using xlink:href -> also catch recursion in those cases. Masks are not affected. Problem should really be fixed now.
Comment on attachment 61617 [details] New patch OK.
Landed in r63415.