Bug 49045

Summary: LayoutTests/svg/animations/animate-path-nested-transforms.html causes assertion in debug mode.
Product: WebKit Reporter: Shane Stephens <shanestephens>
Component: SVGAssignee: Rob Buis <rwlbuis>
Status: RESOLVED FIXED    
Severity: Normal CC: dumi, koivisto, krit, mdelaney7, mrobinson, ossy, pnormand, rwlbuis, simon.fraser, zimmermann
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Attachments:
Description Flags
Patch
none
Patch zimmermann: review+

Description Shane Stephens 2010-11-04 18:26:06 PDT
LayoutTests/svg/animations/animate-path-nested-transforms.html causes assertion in debug mode.
Comment 1 Shane Stephens 2010-11-04 18:27:55 PDT
Created attachment 73024 [details]
Patch
Comment 2 Dumitru Daniliuc 2010-11-04 19:54:40 PDT
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?
Comment 3 Dumitru Daniliuc 2010-11-04 20:04:03 PDT
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?
Comment 4 Shane Stephens 2010-11-04 21:54:08 PDT
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.
Comment 5 Martin Robinson 2010-11-06 20:39:05 PDT
*** Bug 49065 has been marked as a duplicate of this bug. ***
Comment 6 Nikolas Zimmermann 2010-11-10 00:16:47 PST
(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.
Comment 7 Nikolas Zimmermann 2010-11-10 02:10:18 PST
This also affects the new testcase provided in bug 48215.
Comment 8 Dirk Schulze 2011-01-23 14:25:26 PST
I must have missed this bug. It should really get fixed :-P
Comment 9 Dirk Schulze 2011-01-23 14:29:23 PST
*** Bug 48987 has been marked as a duplicate of this bug. ***
Comment 10 Martin Robinson 2011-02-16 16:59:15 PST
Thanks for looking into this. Please remember to unskip svg/animations/animate-path-nested-transforms.html on GTK+ when you fix the issue.
Comment 11 Rob Buis 2011-06-01 08:45:09 PDT
*** Bug 61798 has been marked as a duplicate of this bug. ***
Comment 12 Rob Buis 2011-06-01 14:33:39 PDT
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.
Comment 13 Rob Buis 2011-06-01 15:22:13 PDT
Created attachment 95674 [details]
Patch
Comment 14 Nikolas Zimmermann 2011-06-01 16:21:43 PDT
Comment on attachment 95674 [details]
Patch

Good catch, r=me.
Comment 15 Rob Buis 2011-06-02 07:21:10 PDT
Committed r87907: <http://trac.webkit.org/changeset/87907>