Closed Bug 613899 Opened 9 years ago Closed 9 years ago

Crash when using feImage (infinite recursion)

Categories

(Core :: SVG, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: heycam, Assigned: dholbert)

References

Details

(Keywords: crash, regression)

Attachments

(4 files)

The attached document causes a crash.
Attached image Crashing test document
It seems like nsSVGFEImageElement::LoadSVGImage should be checking to see if xlink:href is pointing within the current document.  If it's exactly the current document's URI, then bail.  If it's a reference to an element within the current document, then it's meant to do something <use>-like with it, according to the spec: http://www.w3.org/TR/SVG/filters.html#feImageElement

Even for external element references from feImage, we don't do the style-inheriting-through-referencing-element thing that <use>-like behaviour would require, right?  That's an issue that could be fixed separately, I guess.

If we wanted to ignore the <use>-like thing for now, then we'd at least need to paint the element-within-the-current-document to a surface for nsSVGFEImageElement::Filter to use instead of the decoded frame.
At the moment feImage only works with bitmap images. bug 598204 is for extending that to svg images i.e. something using the img tag. bug 455986 is for more general any fragment support.
OK, thanks for the pointers.  Easiest thing for now then seems to be to make LoadSVGImage not actually load if there is a fragment or if the xlink:href points to the same document.

For the latter, it looks like I want something like EqualExceptRef in nsReferencedElement.cpp, but that's static.  Is there a public API I can use for a check like this, maybe I should move EqualExceptRef to some common place?

Another thing I wonder: why does this cause a stack overflow rather than just take up lots of memory until you kill the tab?  Is it because the LoadSVGImage succeeds immediately (because it's just a reference to the current document) rather than having to go through the event loop to grab it from the network?
Duplicate of this bug: 620810
See also the testcase on the bug I just duped -- attachment 499173 [details].
(currently crashes nightlies w/ infinite recursion, & is a little more reduced than the testcase attached here.)
OS: Mac OS X → All
Hardware: x86 → All
Version: unspecified → Trunk
I identified the same crashing behavior in many David's examples, and I consider them to be the same problem:

http://srufaculty.sru.edu/david.dailey/svg/ripplepattern1.svg 
Crashes in nightly FFX even with SMIL svg.smil.enabled = false
Works in FFX 3.6, althought there is no SMIL animation.
Reason: although there is attempt to create via JS unsupported animateColor, it is not the problem of the crash, but filter line is: feImage xlink:href="#r" result="grad"

http://srufaculty.sru.edu/david.dailey/svg/later/displace2.svg 
Crashes in nightly FFX even with SMIL svg.smil.enabled = false
Works in FFX 3.6, althought there is no SMIL animation.
Reason: seems (again) that filter lines like: feImage xlink:href="#E" result="grad"  are causing the problem

http://srufaculty.sru.edu/david.dailey/svg/flicker.svg 
Crashes in nightly FFX even with SMIL svg.smil.enabled = false
Works in FFX 3.6, althought there is SMIL animation missing.
Reason: seems (again) that filter lines like: feImage xlink:href="#E" result="grad"  are causing the problem
Attached patch fixSplinter Review
(In reply to comment #3)
> It seems like nsSVGFEImageElement::LoadSVGImage should be checking to see if
> xlink:href is pointing within the current document.  If it's exactly the
> current document's URI, then bail.

This attached patch performs this check, and it fixes all instances of this crash that I've seen.

Patch includes Cameron's original test from this bug, as well as mine from comment 7, as crashtests.

> If it's a reference to an element within
> the current document, then it's meant to do something <use>-like with it,

Yup, though that's already filed as bug 455986.
Attachment #502024 - Flags: review?(longsonr)
Comment on attachment 502024 [details] [diff] [review]
fix

>+	<filter id="f" x="0%" y="0%" width="100%" height="100%">

you could omit the x and y here.

>+  <rect filter="url(#f)" x="0" y="0" width="16" height="16"/>

and here.

Feel free not to bother as these are just nits though.
Attachment #502024 - Flags: review?(longsonr) → review+
(In reply to comment #10)
> >+	<filter id="f" x="0%" y="0%" width="100%" height="100%">
> 
> you could omit the x and y here.

The default for x="" and y="" on <filter> is -10%.

http://www.w3.org/TR/SVG/filters.html#FilterRegionXYWidthHeightAttributes

Although it probably doesn't matter for this crashtest.
(In reply to comment #10)
> you could omit the x and y here.
> Feel free not to bother as these are just nits though.

I just included the attached testcase directly, since they're already pretty minimal & confirmed to crash as-is.  So, I think I'll opt for the "feel free not to bother" option. :) Thanks for the review!
Comment on attachment 502024 [details] [diff] [review]
fix

Requesting approval.

Justification: this is a targeted, well-understood crash fix, fixing what would be a new crash in Firefox 4.0 (new vs. Firefox 3.6).
Attachment #502024 - Flags: approval2.0?
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Keywords: crash, regression
Attached file Testcases
Moving/copying there my comments from issue 617623. 

I wouldn't say so, or that animation must be in some relation there...
I modified original file by commenting out the animation only, and tried to
open it 20 times, and it never crashed for me, not a once (spout6ok.svg).
But the original file (spout6ko.svg) crashed for me every time, immediately
after document load (tried another 20 times.)
This test done on Win7 I tried it couple of times on WinXP as well and it
behaves the very same...
Not crashing case with removed feImage and animate element still there
But probably the animation's relation is really weak, as you are saying,
Daniel, because this case (spout6ok2.svg) does not crashes. However, some relation there (statistically, according to repetitions I did) maybe is...
Marek: I confirmed that the patch here fixes your "crashing case" from bug 617623 comment 5. (I just loaded it directly from that bug -- I didn't bother downloading the zip of those testcases that you attached here.)

Like you, I noticed that animation helps to trigger the crash -- my testcase from comment 7 here is pretty much *just* a <set> element and a <feImage> element.

Anyway, this has a patch and just needs approval. Yay!
Landed: http://hg.mozilla.org/mozilla-central/rev/1d5cbf9617ea
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Depends on: 684358
You need to log in before you can comment on or make changes to this bug.