Bug 49045 – LayoutTests/svg/animations/animate-path-nested-transforms.html causes assertion in debug mode.

• Component: SVG
• Status: RESOLVED FIXED

[Shane Stephens]

LayoutTests/svg/animations/animate-path-nested-transforms.html causes assertion in debug mode.

[Shane Stephens]

Created attachment 73024 [details]
Patch

[Dumitru Daniliuc]

Comment on attachment 73024 [details]
Patch

i don't think we should be changing the test (it's ok for it to fail for now, i added it to test_expectations). instead, we should try to find somebody familiar with this code and make them take a look at this issue and fix it. i'll try to find a good owner for this bug tomorrow, unless you want to do it.

can you please describe the problem we're observing, and ideally, how it can be reproduced?

[Dumitru Daniliuc]

From http://code.google.com/p/chromium/issues/detail?id=61978: "OK, looks like there's a bug somewhere in the animation test framework that makes it nasty to ask for the location of stuff at 0.01 seconds if executing in debug mode."

Antti, Simon, is this something you're familiar with?

[Shane Stephens]

More details: to use the animation framework you make a call like:

function executeTest() {
    const expectedValues = [
        ["animation", 0, "rect", startSample],
        ["animation", 0.1, "rect", startAnimateSample],
        ["animation", 1.00, "rect", endSample]
    ];
    
    runAnimationTest(expectedValues);
}

I believe this uses a set of extra hooks into the SMILAnimation code (WebCore::SVGDocumentExtensions::sampleAnimationAtTime?) to exactly control the animation state.

if the time value (here 0, 0.1 and 1.00) is 0.01, then in debug mode there's an assert in SVGSMILElement::progress here:

    if (elapsed < m_intervalBegin) {
        ASSERT(m_activeState != Active);
        if (m_activeState == Frozen && resultElement)
            updateAnimation(m_lastPercent, m_lastRepeat, resultElement);
        m_nextProgressTime = m_intervalBegin;
        return;
    }

I didn't get a chance to characterize further than this.  Hope this helps.

[Martin Robinson]

*** Bug 49065 has been marked as a duplicate of this bug. ***

[Nikolas Zimmermann]

(In reply to comment #4)
> More details: to use the animation framework you make a call like:
> 
> function executeTest() {
>     const expectedValues = [
>         ["animation", 0, "rect", startSample],
>         ["animation", 0.1, "rect", startAnimateSample],
>         ["animation", 1.00, "rect", endSample]
>     ];
> 
>     runAnimationTest(expectedValues);
> }
> 
> I believe this uses a set of extra hooks into the SMILAnimation code (WebCore::SVGDocumentExtensions::sampleAnimationAtTime?) to exactly control the animation state.
Correct.

> 
> if the time value (here 0, 0.1 and 1.00) is 0.01, then in debug mode there's an assert in SVGSMILElement::progress here:
> 
>     if (elapsed < m_intervalBegin) {
>         ASSERT(m_activeState != Active);
>         if (m_activeState == Frozen && resultElement)
>             updateAnimation(m_lastPercent, m_lastRepeat, resultElement);
>         m_nextProgressTime = m_intervalBegin;
>         return;
>     }
> 
> I didn't get a chance to characterize further than this.  Hope this helps.

Thanks, I'll have a look at this as soon as I'll find some time.

[Nikolas Zimmermann]

This also affects the new testcase provided in bug 48215.

[Dirk Schulze]

I must have missed this bug. It should really get fixed :-P

[Dirk Schulze]

*** Bug 48987 has been marked as a duplicate of this bug. ***

[Martin Robinson]

Thanks for looking into this. Please remember to unskip svg/animations/animate-path-nested-transforms.html on GTK+ when you fix the issue.

[Rob Buis]

*** Bug 61798 has been marked as a duplicate of this bug. ***

[Rob Buis]

I think I know what goes wrong. First, the id value in the expected values is wrong for this test. That causes a change in this code section:

(SMILTimeContainer::updateAnimations):

            SVGElement* targetElement = animation->targetElement();
            // FIXME: This should probably be using getIdAttribute instead of idForStyleResolution.
            if (!targetElement || !targetElement->hasID() || targetElement->idForStyleResolution() != m_nextSamplingTarget)
                continue;

            samplingDiff = animation->intervalBegin();

Because these tests are only started on click, and intervalBegin() in that case is small but not 0, it is important for the later code calling progress to take this into account. Again, this is caused by a wrong id value in the expected results, as m_nextSamplingTarget will mismatch.
For now I propose to just fix the tests. The only alternative I can think of is to give a warning or even assert when the id is not found at all?
Cheers,

Rob.

[Rob Buis]

Created attachment 95674 [details]
Patch

[Nikolas Zimmermann]

Comment on attachment 95674 [details]
Patch

Good catch, r=me.

[Rob Buis]

Committed r87907: <http://trac.webkit.org/changeset/87907>