307322 - Hang with <svg:use> loop

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jesse Ruderman]

Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/20050906
Firefox/1.6a1

Induced crashes with kill -8: TB9107256G, TB9107305W.

Comment 1

[Jesse Ruderman]

Attachment: reduced testcase

Comment 2

[Jesse Ruderman]

Another induced crash: TB9107420K

Comment 3

[tor]

Attachment: try harder to detect <use> loops

Comment 4

[Boris Zbarsky [:bzbarsky]]

I'm not sure I follow this patch.  Why is the descendant test no longer needed?
 Why does this testcase give us a loop the descendant test doesn't catch?

Comment 5

[tor]

The existing test, which caught things like <g id=bar><use href=bar/></g>,
doesn't catch this new testcase because after we've cloned the <use> content
once we have this structure:

<svg>
  <symbol id=foo>
     <use href=foo/>
  </symbol>

  <use>
    <svg> (substituted for <symbol> in clone)
        <use href=foo/>  (*)
    </svg>
  </use>
</svg>

When we do the second expansion (*), we look up the id and get the original
<symbol>, which isn't in the parent chain of (*) - so the old test would pass
and let us clone and get into a loop.

Comment 6

[Boris Zbarsky [:bzbarsky]]

So what we really want to be testing is that neither we nor the original use we
were cloned off of are kids of the thing we're referencing, right?  Except what
happens if I use the DOM to clone a <use>, change the href on the clone, then
insert it into the DOM?

Comment 7

[tor]

Maybe clearing the mOriginal tracking on a href change would be enough to handle
that situation?

Comment 8

[Boris Zbarsky [:bzbarsky]]

Possibly, yes.

Comment 9

[tor]

Attachment: update to tip, clear clone info on href change

Comment 10

[Boris Zbarsky [:bzbarsky]]

So how does this prevent the loops we _used_ to prevent?  The ones not involving
cloned stuff?  I don't see that it does.

Comment 11

[tor]

It will prevent that, but allows one clone before catching the loop.  I can add
back the old test as additional insurance if you'd like.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Oh, I see.  Yeah, I think we want to separately check two things:

1)  We're not referencing our own ancestor.
2)  If we're cloned off something, we're not referencing an ancestor of our
    clone.

Comment 13

[tor]

Attachment: both checks

Comment 14

[tor]

Checked in on trunk.

Comment 15

[Hixie (not reading bugmail)]

The spec says "a set of references that directly or indirectly reference a
element to create a circular dependency is an error" (SVG 1.1, sections 5.6 and
5.3.1) and  then lists specific behaviour for handling such errors (SVG 1.1,
section F.2). Even if we want to ignore these conformance requirements (what
exactly do we do instead? Just ignore the erroneous attribute?), IMHO we should
still be at least reporting these errors to the JS console.

Comment 16

[Martin Hejral]

(In reply to comment #15)
> Even if we want to ignore these conformance requirements (what
> exactly do we do instead? Just ignore the erroneous attribute?), IMHO we should
> still be at least reporting these errors to the JS console.

I am convinced that the best way is exactly this!!
I.e. *tolerate* non-fatal bugs in SVG code and *report* *warnings* at console!

BTW. This is good example of such problem:
Bad SVG code:
<feColorMatrix type="saturate" in="SourceGraphic" values="30%"/>
See also:
http://issues.apache.org/bugzilla/show_bug.cgi?id=36743

Comment 17

[tor]

Checked in on branch.

Comment 18

[Jesse Ruderman]

Crashtest checked in.