Bug 93528 – ASSERTION FAILED: resultAnimationElement->m_animatedType

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Philip Rogers]

Created attachment 157302 [details]
Minimized testcase

Original report: http://code.google.com/p/chromium/issues/detail?id=141326

In the attached testcase we hit an assertion (and crash in release builds):
ASSERTION FAILED: resultAnimationElement->m_animatedType
../../third_party/WebKit/Source/WebCore/svg/SVGAnimateElement.cpp(119) : virtual void WebCore::SVGAnimateElement::calculateAnimatedValue(float, unsigned int, WebCore::SVGSMILElement*)

[David Vest]

Created attachment 195245 [details]
Patch

[Stephen Chenney]

Comment on attachment 195245 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=195245&action=review

> LayoutTests/svg/animations/animation-dependency-crash.html:21
> +    <circle r="10" cx="50" cy="50">

Minor nit. Set the fill color on this to be green so that it looks to be passing in the event someone opens it in a browser.

[David Vest]

Created attachment 195313 [details]
Patch

[Stephen Chenney]

Comment on attachment 195313 [details]
Patch

FYI, for small nits like this, when the reviewer has marked R+ you are free to make the change and commit without another review - you just have to make the requested change and then manually add the "Reviewed by ..." line in the ChangeLog. If you're not comfortable with the change you can always ask for review, as you did here. If you do not have commit right, please mark the "cq: ?" so the reviewer knows you need someone to commit for you.

[David Vest]

Comment on attachment 195313 [details]
Patch

Thanks for the review and advice. No, I don't have commit rights, so I'll add cq:? to the updated patch and hope for the best :)

[Philip Rogers]

Comment on attachment 195313 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=195313&action=review

Apologies about last-minute kicking this out of the queue.

> LayoutTests/svg/animations/animation-dependency-crash.html:14
> +    }, 300);

This value is too high and will cause our testing infrastructure to run even slower. Can this be reduced (preferably to 0 or 1?)

[David Vest]

(In reply to comment #6)
> (From update of attachment 195313 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=195313&action=review
> 
> Apologies about last-minute kicking this out of the queue.
> 
> > LayoutTests/svg/animations/animation-dependency-crash.html:14
> > +    }, 300);
> 
> This value is too high and will cause our testing infrastructure to run even slower. Can this be reduced (preferably to 0 or 1?)

Any lower and it won't crash chrome on my machine. The Gtk port crashes with a zero value, but I erred on the side of caution. So yes, we might lower the value but the test might lose relevance.

[Stephen Chenney]

(In reply to comment #7)
> (In reply to comment #6)
> > (From update of attachment 195313 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=195313&action=review
> > 
> > Apologies about last-minute kicking this out of the queue.
> > 
> > > LayoutTests/svg/animations/animation-dependency-crash.html:14
> > > +    }, 300);
> > 
> > This value is too high and will cause our testing infrastructure to run even slower. Can this be reduced (preferably to 0 or 1?)
> 
> Any lower and it won't crash chrome on my machine. The Gtk port crashes with a zero value, but I erred on the side of caution. So yes, we might lower the value but the test might lose relevance.

Another possibility is to make it an SVG animation test, which uses JS to jump the timeline ahead. I have no idea at all whether or not that will trigger the crash.

[Philip Rogers]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > (From update of attachment 195313 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=195313&action=review
> > > 
> > > Apologies about last-minute kicking this out of the queue.
> > > 
> > > > LayoutTests/svg/animations/animation-dependency-crash.html:14
> > > > +    }, 300);
> > > 
> > > This value is too high and will cause our testing infrastructure to run even slower. Can this be reduced (preferably to 0 or 1?)
> > 
> > Any lower and it won't crash chrome on my machine. The Gtk port crashes with a zero value, but I erred on the side of caution. So yes, we might lower the value but the test might lose relevance.
> 
> Another possibility is to make it an SVG animation test, which uses JS to jump the timeline ahead. I have no idea at all whether or not that will trigger the crash.

I think crashing on at least one platform is probably enough test coverage for a crasher. I'm in favor of just using 0 since we will still have coverage, albeit not the best coverage. Stephen and David, what do you think?

[David Vest]

(In reply to comment #8)

> Another possibility is to make it an SVG animation test, which uses JS to jump the timeline ahead. I have no idea at all whether or not that will trigger the crash.

FYI, I stumbled onto https://bugs.webkit.org/show_bug.cgi?id=113485 when trying to use <svg>.setCurrentTime().

[David Vest]

Comment on attachment 195313 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=195313&action=review

>>>>> LayoutTests/svg/animations/animation-dependency-crash.html:14
>>>>> +    }, 300);
>>>> 
>>>> This value is too high and will cause our testing infrastructure to run even slower. Can this be reduced (preferably to 0 or 1?)
>>> 
>>> Any lower and it won't crash chrome on my machine. The Gtk port crashes with a zero value, but I erred on the side of caution. So yes, we might lower the value but the test might lose relevance.
>> 
>> Another possibility is to make it an SVG animation test, which uses JS to jump the timeline ahead. I have no idea at all whether or not that will trigger the crash.
> 
> I think crashing on at least one platform is probably enough test coverage for a crasher. I'm in favor of just using 0 since we will still have coverage, albeit not the best coverage. Stephen and David, what do you think?

I'm fine with that. I'll upload a new patch.

[David Vest]

Comment on attachment 195313 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=195313&action=review

>>>>>> LayoutTests/svg/animations/animation-dependency-crash.html:14
>>>>>> +    }, 300);
>>>>> 
>>>>> This value is too high and will cause our testing infrastructure to run even slower. Can this be reduced (preferably to 0 or 1?)
>>>> 
>>>> Any lower and it won't crash chrome on my machine. The Gtk port crashes with a zero value, but I erred on the side of caution. So yes, we might lower the value but the test might lose relevance.
>>> 
>>> Another possibility is to make it an SVG animation test, which uses JS to jump the timeline ahead. I have no idea at all whether or not that will trigger the crash.
>> 
>> I think crashing on at least one platform is probably enough test coverage for a crasher. I'm in favor of just using 0 since we will still have coverage, albeit not the best coverage. Stephen and David, what do you think?
> 
> I'm fine with that. I'll upload a new patch.

Sorry. When preparing the patch, I found out I can't reproduce the crash with a 0 or 1 timeout, not even in the gtk port. (For some reason, my <script> didn't run when I placed it below the <svg>, which actually in practice meant I had an infinite timeout value, not a zero one). I will try to find another way of testing this.

[David Vest]

(In reply to comment #12)
> I will try to find another way of testing this.

I've investigated the possibility of using setCurrentTime (or SVGAnimationTestCase.js which uses setCurrentTime under the hood) and concluded that it is not possible to trigger the crash using setCurrentTime. The reason is that each call to setCurrentTime resets the animation state necessary to trigger the crash.

So, some choices going forward:

1. Commit fix without layout test.
2. Commit fix with layout test and accept a non-zero (>~200) ms delay in the test.
(3. Commit fix with manual test somehow. I'm not sure about the procedure for that, if any.)

[Stephen Chenney]

(In reply to comment #13)
> (In reply to comment #12)
> > I will try to find another way of testing this.
> 
> I've investigated the possibility of using setCurrentTime (or SVGAnimationTestCase.js which uses setCurrentTime under the hood) and concluded that it is not possible to trigger the crash using setCurrentTime. The reason is that each call to setCurrentTime resets the animation state necessary to trigger the crash.
> 
> So, some choices going forward:
> 
> 1. Commit fix without layout test.
> 2. Commit fix with layout test and accept a non-zero (>~200) ms delay in the test.
> (3. Commit fix with manual test somehow. I'm not sure about the procedure for that, if any.)

Commit it with the slow test. We can live with that. Worst case is that we mark it as slow in expectations.

pdr, flip the bit if you agree.

[David Vest]

Created attachment 196395 [details]
Updated test with lower timeout and shorter duration

[Philip Rogers]

Comment on attachment 196395 [details]
Updated test with lower timeout and shorter duration

Thanks for updating the test. R=me!

[WebKit Review Bot]

Comment on attachment 196395 [details]
Updated test with lower timeout and shorter duration

Clearing flags on attachment: 196395

Committed r147581: <http://trac.webkit.org/changeset/147581>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.