REGRESSION (r89774): svg/W3C-SVG-1.1/animate-elem-82-t.svg hits assertion failure.

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Young Han Lee]

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()

[Young Han Lee]

svg/W3C-SVG-1.1/animate-elem-33-t.svg also hits assertion.

[Young Han Lee]

Created attachment 99648 [details]
Patch

[Dirk Schulze]

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]

(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]

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 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 on attachment 99648 [details]
Patch

Clearing flags on attachment: 99648

Committed r91002: <http://trac.webkit.org/changeset/91002>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.