Bug 86385 - [regression] animatedType crash when animations end
: [regression] animatedType crash when animations end
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: SVG
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Philip Rogers
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-14 11:00 PDT by Philip Rogers
Modified: 2012-05-20 13:57 PDT (History)
2 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Philip Rogers 2012-05-14 11:00:26 PDT
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 Philip Rogers 2012-05-14 19:06:41 PDT
Created attachment 141841 [details]
Minimized testcase
Comment 2 Philip Rogers 2012-05-16 15:50:16 PDT
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 Philip Rogers 2012-05-17 09:59:21 PDT
Created attachment 142494 [details]
Accumulate SVG animations into first contributing element
Comment 4 Nikolas Zimmermann 2012-05-17 11:12:27 PDT
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
Comment 5 Philip Rogers 2012-05-17 18:41:06 PDT
Created attachment 142604 [details]
Update per reviewer comments
Comment 6 Philip Rogers 2012-05-17 18:46:00 PDT
(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.
Comment 7 Nikolas Zimmermann 2012-05-18 22:45:45 PDT
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.
Comment 8 Philip Rogers 2012-05-20 13:06:09 PDT
Created attachment 142914 [details]
Patch for landing
Comment 9 WebKit Review Bot 2012-05-20 13:57:20 PDT
Comment on attachment 142914 [details]
Patch for landing

Clearing flags on attachment: 142914

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