[regression] animatedType crash when animations end

[Philip Rogers]

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

[Philip Rogers]

Created attachment 141841 [details]
Minimized testcase

[Philip Rogers]

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.

[Philip Rogers]

Created attachment 142494 [details]
Accumulate SVG animations into first contributing element

[Nikolas Zimmermann]

Comment on attachment 142494 [details]
Accumulate SVG animations into first contributing element

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

[Philip Rogers]

Created attachment 142604 [details]
Update per reviewer comments

[Philip Rogers]

(In reply to comment #4)
> (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.

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.

[Nikolas Zimmermann]

Comment on attachment 142604 [details]
Update per reviewer comments

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.

[Philip Rogers]

Created attachment 142914 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 142914 [details]
Patch for landing

Clearing flags on attachment: 142914

Committed r117711: <http://trac.webkit.org/changeset/117711>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.