Closed Bug 483584 Opened 16 years ago Closed 15 years ago

SVG SMIL: Reproducible Hang on full-animate-elem-30-t.html SVG animation test

Categories

(Core :: SVG, defect)

x86
All
defect
Not set
critical

Tracking

()

RESOLVED FIXED

People

(Reporter: lars.sonchocky-helldorf, Unassigned)

References

()

Details

(Keywords: hang)

Attachments

(5 files)

User-Agent: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en; rv:1.9.0.8pre) Gecko/2009030300 Camino/2.0b3pre (like Firefox/3.0.8pre) Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090315 Minefield/3.2a1pre hangs after the animation is finished, had to force quit Minefield Reproducible: Always Steps to Reproduce: 1. Go to http://www.w3.org/Graphics/SVG/Test/20061213/htmlObjectHarness/full-animate-elem-30-t.html 2. let the animation finish 3. hang Actual Results: Browser hangs Expected Results: Browser should not hang See attachments
Component: General → SVG
Product: Firefox → Core
QA Contact: general → general
Version: unspecified → Trunk
Attached file stack trace of the first hang (deleted) —
Attached file stack trace of the second hang (deleted) —
btw: This is a bug report on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090315 Minefield/3.2a1pre NOT Camino (I just used camino to report the bug because I have user/pass in my keychain which FireFox can't access)
Forgot to mention: I have set svg.smil.enabled manually to true
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090316 Minefield/3.2a1pre On windows no hang. Persistent 100% CPU but I can still click links, toolbar buttons etc.
I think this should be fixed before enabling SMIL
Blocks: 473705
Attached image svg file from URL (deleted) —
Here's the SVG file from the URL (with the outer 100% height/width replaced with fixed values, for more consistent behavior/testability). I can reproduce the high CPU usage (but not the hang) on Linux. Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20090315 Minefield/3.2a1pre
(In reply to comment #0) > User-Agent: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en; > rv:1.9.0.8pre) Gecko/2009030300 Camino/2.0b3pre (like Firefox/3.0.8pre) > Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; > rv:1.9.2a1pre) Gecko/20090315 Minefield/3.2a1pre > > hangs after the animation is finished, had to force quit Minefield This is not really correct. The animation never does finish.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Mac OS X → All
Summary: Reproducible Hang on full-animate-elem-30-t.html SVG animation test → SVG SMIL: Reproducible Hang on full-animate-elem-30-t.html SVG animation test
Depends on: 486365
Keywords: hang
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090403 Minefield/3.6a1pre sorry guys, I seem to be missing something tried this intel and ppc animation never starts, and neither does hang. am I using the wrong nightlies? or what? the description does not mention installing a patch... instructions would be appreciated. cheers
Did you enable SMIL in about:config? It's disabled by default currently.
svg.smil.enabled
I've investigated this a bit more. I talked to roc a bit on IRC yesterday, and he helped me clarify what's going wrong here. Basically, our technique for notifying that "the transform's animated value has changed" is a bit broken, and in particular, it interacts very badly with <use> elements. The testcase has something like this: <circle id="myCircle"><animateTransform/></circle> <use xlink:href="myCircle"/> So what happens is: - The "<use>" element makes a clone of myCircle's subtree (including a clone of the <animateTransform> element) - Both of the <anmimateTransform> elements (the original & the clone) indepenedently animate their parent <circle> elements, in parallel. (Everything is fine so far...) - HOWEVER -- in each sample, each time we update the animated value for myCircle, we call "DidModify" on its 'transform' attribute, which freaks out the <use> element and makes it think it needs to rebuild its subtree-clone. (because it's registered as a listener for attribute changes on its source subtree) So, with each sample, we trigger an (asynchronous) re-cloning for the <use> element's contents, along with frame reconstruction etc. That's the cause of the hang here -- tons of needless subtree re-cloning. This problem is exacerbated (and turned from a slowdown into a "true hang") by my SMIL + CSS patch (bug 474049), which queries the computed style during each sample (to get a base value). This ComputedStyle query makes us flush restyle events, and thereby forces the queued re-clone to happen **during the sample**. This causes all sorts of problems, including the creation and registration of a new <animateTransform> element **during a sample**. This problem is specific to <animateTransform>, because we use the "DidModify" function to notify of animation updates to the transform attribute. One solution to this would be to replace the "DidModify" call with a new method called something like "DidAnimateTransform". This would be similar to the existing "DidAnimateLength" method, which we already use instead of DidModify for length animations.
Blocks: 468996
Attached patch hacky workaround patch (deleted) — Splinter Review
Just as a demonstration -- this patch fixes the hang by simply commenting out the notification. (It's not an actual fix, because it prevents us from updating our rendering of some transformed content. You can force an update by zooming in and out, though -- this makes us redraw with the updated animated transform value.)
It would be great to have this issue fixed. It is breaking down my animation (complex case http://svg.kvalitne.cz/zx/zx-spectrum-loader.svg , simplified case http://svg.kvalitne.cz/mix/zxload.svg) but it is still the same: element => animateTransform => use and I've plenty of ideas how to use it in other demos, but I can't, 'cause it really saddens me to produce something not runnable in FFX :-(
Attached image reduced testcase (deleted) —
Here's a nice reduced testcase for this issue (from the duplicate bug 538233).
(In reply to comment #13) > Created an attachment (id=387480) [details] > hacky workaround patch > > Just as a demonstration -- this patch fixes the hang by simply commenting out > the notification. (It's not an actual fix, because it prevents us from > updating our rendering of some transformed content. You can force an update by > zooming in and out, though -- this makes us redraw with the updated animated > transform value.) Actually you can get even closer to the correct end rendering by first doing a page reload and then zooming in and out.
This bug's URL & testcases stopped hanging (and in fact become fairly smooth) in between these two nightlies: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.3a2pre) Gecko/20100220 Minefield/3.7a2pre http://hg.mozilla.org/mozilla-central/rev/777fac9d915c Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.3a2pre) Gecko/20100221 Minefield/3.7a2pre http://hg.mozilla.org/mozilla-central/rev/ab5aec9ca05b http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=777fac9d915c&tochange=ab5aec9ca05b --> Appears to have been fixed by Bug 533291. Yay!
Status: NEW → RESOLVED
Closed: 15 years ago
Depends on: 533291
Resolution: --- → FIXED
So: 1) We need to get tests for this in, right? 2) I think this works sort of by accident. The code described in comment 12 changes attributes during frame construction; that's fundamentally unsafe. Bug 533291 made it at least not hang in an infinite loop processing a reframe over and over, but I see no reason that code couldn't trigger DOM mutation listeners or something... during frame construction. Do we need a separate bug on that? See bug 533291 comment 24.
Flags: in-testsuite?
(In reply to comment #21) > 2) I think this works sort of by accident. The code described in comment 12 > changes attributes during frame construction; that's fundamentally unsafe. I posted a followup on Bug 547333 that removes the FlushAnimations call in nsSVGElement::GetAnimatedLengthValues, to prevent this. > 1) We need to get tests for this in, right? Yup. I included both of this bug's attached testcases with that patch. I've verified that (a) I get the hang again, if I disabling Bug 533291's patch, and (b) the hang goes away if I apply the new followup described above. Note to self: File a bug on the still-existing ###!!! ASSERTION: Resample dirty flag set during sample!: '!mResampleNeeded', file ../../../mozilla/content/smil/nsSMILAnimationController.cpp, line 387 assertion-spam from full-animate-elem-30-t.html.
Just checked in followup patch (with this bug's tests), in bug 547333 comment 19: http://hg.mozilla.org/mozilla-central/rev/367cdcfa611b
Flags: in-testsuite? → in-testsuite+
I filed bug 548899 on the assertion mentioned in comment 22.
Had to back out the followup in bug 547333 due to sporadic reftest failures, as detailed on that bug. Re-setting "in-testsuite?" since the backed-out push included this bug's tests.
Flags: in-testsuite+ → in-testsuite?
Can you cc me on bug 547333 please?
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: