Closed Bug 307322 Opened 19 years ago Closed 19 years ago
Hang with <svg:use> loop
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.
Another induced crash: TB9107420K
Assignee: general → tor
Status: NEW → ASSIGNED
Attachment #195184 - Flags: review?(bzbarsky)
Summary: Highly recursive hang with <svg:symbol> and <svg:use> → Hang with <svg:use> loop
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?
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.
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?
Maybe clearing the mOriginal tracking on a href change would be enough to handle that situation?
So how does this prevent the loops we _used_ to prevent? The ones not involving cloned stuff? I don't see that it does.
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.
Attachment #195184 - Flags: review?(bzbarsky) → review-
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.
Attachment #196683 - Flags: review?(bzbarsky) → review+
Checked in on trunk.
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.
(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
Attachment #196683 - Flags: approval1.8b5? → approval1.8b5+
Checked in on branch.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Crashtest checked in.
You need to log in before you can comment on or make changes to this bug.