Bug 32171 – SVG patterns and masks should not be able to reference themselves

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Dirk Schulze]

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.

[Dirk Schulze]

*** Bug 37751 has been marked as a duplicate of this bug. ***

[Nikolas Zimmermann]

Created attachment 61484 [details]
Initial patch

[Eric Seidel (no email)]

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.

[Nikolas Zimmermann]

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

[Nikolas Zimmermann]

Created attachment 61489 [details]
Updated patch

Not yet tested build with the new changes, EWS is faster than my machine.

[Nikolas Zimmermann]

Created attachment 61493 [details]
Updated patch v2

This one actually builds and works, tested locally. Turns out EWS was not faster today :-)

[Eric Seidel (no email)]

Comment on attachment 61493 [details]
Updated patch v2

LGTM.  Do we need to rip out any of the old recursion prevention code?

[Nikolas Zimmermann]

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

[Nikolas Zimmermann]

Landed in r63300.

[Nikolas Zimmermann]

Pattern still have an issue with self-referencing, going to upload a fix soon, found by Dirk.

[Nikolas Zimmermann]

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.

[Eric Seidel (no email)]

Comment on attachment 61617 [details]
New patch

OK.

[Nikolas Zimmermann]

Landed in r63415.