Bug 63911 - REGRESSION (r89774): svg/W3C-SVG-1.1/animate-elem-82-t.svg hits assertion failure.
Summary: REGRESSION (r89774): svg/W3C-SVG-1.1/animate-elem-82-t.svg hits assertion fai...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL: http://dev.w3.org/SVG/profiles/1.1F2/...
Keywords:
Depends on:
Blocks:
 
Reported: 2011-07-04 08:22 PDT by Young Han Lee
Modified: 2011-07-14 09:20 PDT (History)
2 users (show)

See Also:


Attachments
Patch (4.04 KB, patch)
2011-07-04 11:32 PDT, Young Han Lee
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Young Han Lee 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()
Comment 1 Young Han Lee 2011-07-04 09:30:42 PDT
svg/W3C-SVG-1.1/animate-elem-33-t.svg also hits assertion.
Comment 2 Young Han Lee 2011-07-04 11:32:04 PDT
Created attachment 99648 [details]
Patch
Comment 3 Dirk Schulze 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?
Comment 4 Young Han Lee 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.
Comment 5 Young Han Lee 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?
Comment 6 Young Han Lee 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.
Comment 7 WebKit Review Bot 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>
Comment 8 WebKit Review Bot 2011-07-14 09:20:19 PDT
All reviewed patches have been landed.  Closing bug.