Bug 86385 - [regression] animatedType crash when animations end
: [regression] animatedType crash when animations end
Status: RESOLVED FIXED
: WebKit
SVG
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2012-05-14 11:00 PST by
Modified: 2012-05-20 13:57 PST (History)


Attachments
Minimized testcase (334 bytes, image/svg+xml)
2012-05-14 19:06 PST, Philip Rogers
no flags Details
Accumulate SVG animations into first contributing element (5.75 KB, patch)
2012-05-17 09:59 PST, Philip Rogers
zimmermann: review-
Review Patch | Details | Formatted Diff | Diff
Update per reviewer comments (29.69 KB, patch)
2012-05-17 18:41 PST, Philip Rogers
zimmermann: review+
Review Patch | Details | Formatted Diff | Diff
Patch for landing (29.62 KB, patch)
2012-05-20 13:06 PST, Philip Rogers
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-05-14 11:00:26 PST
We can hit an assertion in animated types:
ASSERTION FAILED: resultAnimationElement->m_animatedType
../../third_party/WebKit/Source/WebCore/svg/SVGAnimateElement.cpp(119) : virtual void WebCore::SVGAnimateElement::calculateAnimatedValue(float, unsigned int, WebCore::SVGSMILElement*)

Original bug:
http://code.google.com/p/chromium/issues/detail?id=127794

This may have been regressed by http://trac.webkit.org/changeset/116458
------- Comment #1 From 2012-05-14 19:06:41 PST -------
Created an attachment (id=141841) [details]
Minimized testcase
------- Comment #2 From 2012-05-16 15:50:16 PST -------
Just a quick update, no patch yet. When animating with multiple animate elements, we accumulate the result in the first animate element. This bug is due to us clearing the animated type of the first animate element (when it ends), and then trying to accumulate the other animate results into this cleared animated type.
------- Comment #3 From 2012-05-17 09:59:21 PST -------
Created an attachment (id=142494) [details]
Accumulate SVG animations into first contributing element
------- Comment #4 From 2012-05-17 11:12:27 PST -------
(From update of attachment 142494 [details])
Code changes look good, but this should get tested more. Ideally your crash test turns into a reftest, then we can also verify the animation works as expected.
Ideally you'll also add a test using the JS sampling framework (runAnimationTest), to sample eg. the 'cx' value at 0.099s, 0.1s, 0.1001s to make sure the transition between those animations works as expected. There should be plenty of examples on how to create a test for this (just use the new style tests which load a .svg file from svg/animations/resources/foo.svg, embed it into svg/animations/foo.html, and test it via svg/animation/script-tests/foo.svg
------- Comment #5 From 2012-05-17 18:41:06 PST -------
Created an attachment (id=142604) [details]
Update per reviewer comments
------- Comment #6 From 2012-05-17 18:46:00 PST -------
(In reply to comment #4)
> (From update of attachment 142494 [details] [details])
> Code changes look good, but this should get tested more. Ideally your crash test turns into a reftest, then we can also verify the animation works as expected.

Done.

> Ideally you'll also add a test using the JS sampling framework (runAnimationTest), to sample eg. the 'cx' value at 0.099s, 0.1s, 0.1001s to make sure the transition between those animations works as expected. There should be plenty of examples on how to create a test for this (just use the new style tests which load a .svg file from svg/animations/resources/foo.svg, embed it into svg/animations/foo.html, and test it via svg/animation/script-tests/foo.svg

I added the multiple-animations-ending.html test which covers a bunch of end-animation cases. I hand-verified each value.
------- Comment #7 From 2012-05-18 22:45:45 PST -------
(From update of attachment 142604 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=142604&action=review

Excellent work Philip! r=me. Not setting cq+, as you might want to fixup these comments before landing:

> LayoutTests/svg/animations/svg-two-animate-elements-crash-expected.svg:1
> +<svg xmlns="http://www.w3.org/2000/svg" onload="load()">

The onload="" should be removed.

> LayoutTests/svg/animations/script-tests/multiple-animations-ending.js:337
> +        ["an1", 0.0,   sample1],
> +        ["an1", 0.499,   sample2],

We usually align those sample lines.
------- Comment #8 From 2012-05-20 13:06:09 PST -------
Created an attachment (id=142914) [details]
Patch for landing
------- Comment #9 From 2012-05-20 13:57:20 PST -------
(From update of attachment 142914 [details])
Clearing flags on attachment: 142914

Committed r117711: <http://trac.webkit.org/changeset/117711>
------- Comment #10 From 2012-05-20 13:57:25 PST -------
All reviewed patches have been landed.  Closing bug.