Bug 63911

Summary: REGRESSION (r89774): svg/W3C-SVG-1.1/animate-elem-82-t.svg hits assertion failure.
Product: WebKit Reporter: Young Han Lee <joybro201>
Component: Layout and RenderingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: krit, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
URL: http://dev.w3.org/SVG/profiles/1.1F2/test/svg/animate-elem-82-t.svg
Attachments:
Description Flags
Patch none

Young Han Lee
Reported 2011-07-04 08:22:42 PDT
ASSERTION FAILED: i < size() /Users/dirk/Downloads/webkit-trunk/WebKitBuild/Debug/JavaScriptCore.framework/PrivateHeaders/Vector.h(542) : const T& WTF::Vector<T, inlineCapacity>::at(size_t) const [with T = float, long unsigned int inlineCapacity = 0ul] 1 WTF::Vector<float, 0ul>::at(unsigned long) const 2 WTF::Vector<float, 0ul>::operator[](unsigned long) const 3 WebCore::SVGAnimationElement::currentValuesForValuesAnimation(float, float&, WTF::String&, WTF::String&) const 4 WebCore::SVGAnimationElement::updateAnimation(float, unsigned int, WebCore::SVGSMILElement*) 5 WebCore::SVGSMILElement::progress(WebCore::SMILTime, WebCore::SVGSMILElement*) 6 WebCore::SMILTimeContainer::updateAnimations(WebCore::SMILTime, double, WTF::String const&) 7 WebCore::SMILTimeContainer::timerFired(WebCore::Timer<WebCore::SMILTimeContainer>*) 8 WebCore::Timer<WebCore::SMILTimeContainer>::fired()
Attachments
Patch (4.04 KB, patch)
2011-07-04 11:32 PDT, Young Han Lee 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Young Han Lee
Comment 1 2011-07-04 09:30:42 PDT
svg/W3C-SVG-1.1/animate-elem-33-t.svg also hits assertion.
Young Han Lee
Comment 2 2011-07-04 11:32:04 PDT
Created attachment 99648 [details] Patch
Dirk Schulze
Comment 3 2011-07-04 13:50:50 PDT
Comment on attachment 99648 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=99648&action=review This is a regression, so we need a regression test. Can you add a test to svg/animation please? I also have a question to your fix. r- because of the missing test. > Source/WebCore/svg/SVGAnimationElement.cpp:494 > + if (percent == 1) { > + from = m_values[valuesCount - 1]; > + to = m_values[valuesCount - 1]; > + effectivePercent = 1; > + return; > + } I just wonder. Here we return always effectivePercent = 1 if percent is 1,... > Source/WebCore/svg/SVGAnimationElement.cpp:520 > if (calcMode == CalcModeDiscrete) { > if (!keyTimesCount) > - index = percent == 1 ? valuesCount - 1 : static_cast<unsigned>(percent * valuesCount); > + index = static_cast<unsigned>(percent * valuesCount); > from = m_values[index]; > to = m_values[index]; > effectivePercent = 0; here we return effectivePercent = 0 for discrete animations. Sure that this doesn't cause new problems?
Young Han Lee
Comment 4 2011-07-04 19:01:46 PDT
(In reply to comment #3) > (From update of attachment 99648 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=99648&action=review > > This is a regression, so we need a regression test. Can you add a test to svg/animation please? I also have a question to your fix. r- because of the missing test. I'm not sure what kind of test could be added in this case because this regression has no symptoms, looks like working correctly, apart from assertion failures, and the assertion failures are not even occurred if the testcase has runAnimationTests() like our testcases in svg/animations. > > > Source/WebCore/svg/SVGAnimationElement.cpp:494 > > + if (percent == 1) { > > + from = m_values[valuesCount - 1]; > > + to = m_values[valuesCount - 1]; > > + effectivePercent = 1; > > + return; > > + } > > I just wonder. Here we return always effectivePercent = 1 if percent is 1,... > > > Source/WebCore/svg/SVGAnimationElement.cpp:520 > > if (calcMode == CalcModeDiscrete) { > > if (!keyTimesCount) > > - index = percent == 1 ? valuesCount - 1 : static_cast<unsigned>(percent * valuesCount); > > + index = static_cast<unsigned>(percent * valuesCount); > > from = m_values[index]; > > to = m_values[index]; > > effectivePercent = 0; > > here we return effectivePercent = 0 for discrete animations. Sure that this doesn't cause new problems? Yes, the effectivePercent is meaningful only if the 'from' and 'to' have different values. I checked that this doesn't affect the calculation result.
Young Han Lee
Comment 5 2011-07-06 11:00:55 PDT
On second thoughts, we already have the regression test, svg/W3C-SVG-1.1/animate-elem-82-t.svg, in the LayoutTests! how about that?
Young Han Lee
Comment 6 2011-07-07 10:21:13 PDT
Comment on attachment 99648 [details] Patch Dirk, would you review this again? I believe that this patch doesn't need another test because of the above-mentioned reasons.
WebKit Review Bot
Comment 7 2011-07-14 09:20:14 PDT
Comment on attachment 99648 [details] Patch Clearing flags on attachment: 99648 Committed r91002: <http://trac.webkit.org/changeset/91002>
WebKit Review Bot
Comment 8 2011-07-14 09:20:19 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.