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*)
Created attachment 195245 [details] Patch
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.
Created attachment 195313 [details] Patch
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.
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 :)
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?)
(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.
(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.
(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?
(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().
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.
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.
(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.)
(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.
Created attachment 196395 [details] Updated test with lower timeout and shorter duration
Comment on attachment 196395 [details] Updated test with lower timeout and shorter duration Thanks for updating the test. R=me!
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>
All reviewed patches have been landed. Closing bug.