Bug 6000 – <use> does not handle recursion safely

• Component: SVG
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

<use> does not handle recursion correctly

If nothing else, <use> needs to check for recursive loops, otherwise Safari might just blow up on bad 
content.  The FireFox devs have some testcase for these sorts of things which I will attach.

[Eric Seidel (no email)]

Because these could all be Safari crashers, we would not want to ship an SVG implementation without these fixed.  Adding SVGHitList keyword and bumping to P2.

I began a patch at one time to fix <pattern> and <gradient>.  The way that I envision solving this is:

1.  Create a on-the-stack simplified "pattern attributes" or "gradient attributes" structure.
2.  Loop backwards through the xlink:href parent chain until either one of the following:
- All attributes in the structure are filled
- The chain terminates (no more, or you reach a node you've seen before).

I would suggest using a HashSet to keep track of all the nodes that you've seen when walking backwards.  In all three cases one of the required "attributes" to track in this structure is a pointer to whichever pattern/gradient/use element actually had children that you're going to render.  In all three cases, if an element does not itself have children, it uses the children of the first element in its xlink:href inheritance chain when rendering.

[Eric Seidel (no email)]

I'm adding the NeedsReduction keyword as this bug really could use probably 10+ test cases to catch all the recursion cases coverd by these elements.

[Eric Seidel (no email)]

At least gradients and patterns are likely to be addressed in Wildfox's next patch.

[Nikolas Zimmermann]

Fixed by bug 12013 patch for gradients & patterns. <use> still TODO. Landed in r18521.

[Nikolas Zimmermann]

Created attachment 12503 [details]
Patch rewriting use - not ready yet!

Patch for early-birds - not ready yet.

[Nikolas Zimmermann]

Created attachment 12731 [details]
Initial patch

Hopefully the last big <use> rewrite - this time using a proper shadow tree!
Fixes all issues & uglyness we had before.

[Oliver Hunt]

Comment on attachment 12731 [details]
Initial patch

SVGUseElement:
-    Copyright (C) 2004, 2005, 2006, 2007 Nikolas Zimmermann <[e-mail]>
-                  2004, 2005, 2006 Rob Buis <[e-mail]>
+    Copyright (C) 2007 Nikolas Zimmermann <[e-mail]>

You're removing Rob's copyright???

SVGUseElement.cpp: replace NOTE: with FIXME:

Looks fine to me, but i'd be happier if eric did a once over as well

[Nikolas Zimmermann]

(In reply to comment #7)
> (From update of attachment 12731 [details] [edit])
> SVGUseElement:
> -    Copyright (C) 2004, 2005, 2006, 2007 Nikolas Zimmermann
> <[e-mail]>
> -                  2004, 2005, 2006 Rob Buis <[e-mail]>
> +    Copyright (C) 2007 Nikolas Zimmermann <[e-mail]>
> 
> You're removing Rob's copyright???
Whoops, copy'n'paste! Will fix. My copyright year is also wrong...

> SVGUseElement.cpp: replace NOTE: with FIXME:
Okay.
 
> Looks fine to me, but i'd be happier if eric did a once over as well
Thanks for the review! :-)

Niko

[Eric Seidel (no email)]

Comment on attachment 12731 [details]
Initial patch

The patch seems fine.  It does not seem to address the bug however.  Nor does it seem to have tests for the bug.

The bug was about <use> recursion, and how recursive <use>'s could send Safari off spinning forever.  Maybe that's handled here and I just don't see it.

Also, this seems strange:
*to->attributes() = *from->attributes();

[Nikolas Zimmermann]

(In reply to comment #9)
> (From update of attachment 12731 [details] [edit])
> The patch seems fine.  It does not seem to address the bug however.  Nor does
> it seem to have tests for the bug.
> 
> The bug was about <use> recursion, and how recursive <use>'s could send Safari
> off spinning forever.  Maybe that's handled here and I just don't see it.
Hehe funny, I mixed up things, I thought this bug was about "expansion" of <use> elements.
Uploading new patch which contains 4 recursion tests, and actually handles recursion.

Good catch :-)

> Also, this seems strange:
> *to->attributes() = *from->attributes();
This is common WebCore code: (see Element::cloneNode)
...
    // clone attributes
    if (namedAttrMap)
        *clone->attributes() = *namedAttrMap;
...
 
Niko

[Nikolas Zimmermann]

Created attachment 12736 [details]
Updated patch

This time it actually also fixes this bug - handle recursion properly.
As SVG spec doesn't say a word about recursion, I'm doing the only logical thing: just ignore cycles.

[Mark Rowe (bdash)]

Comment on attachment 12736 [details]
Updated patch

Discussed a slightly updated version of this on IRC.  r=me

[Sam Weinig]

Comment on attachment 12736 [details]
Updated patch

Marking r- until the leaks are dealt with.

[Nikolas Zimmermann]

Created attachment 12788 [details]
Final patch

Finally it does not leak anymore! The problematic area was SVGElementInstance, it always ref()'ed the correspondingUseElement so these never got deleted, once expandUseElementsInShadowTree() was executed. No new regressions - <use> feels much more stable today (ie. carto.net/window.svg works _perfectly_ now - no bugs!), no new leaks. All fine :-)

[Mark Rowe (bdash)]

Comment on attachment 12788 [details]
Final patch

r=me.  Glad to see you finally tracked those leaks down!

[Sam Weinig]

Landed in r19254.