Bug 102655 – REGRESSION (r129670): SVG animation examples no longer working

• Component: SVG
• Status: RESOLVED FIXED

[Dominic Mazzoni]

It appears that after r129670 (https://bugs.webkit.org/show_bug.cgi?id=96697), the following two SVG examples no longer animate (tested in Chromium):

http://www.kelvinlawrence.net/svg/animate-lines.html
http://www.kelvinlawrence.net/svg/animate-lines2.html

I only bisected to the range 129568 ... 129708, so my apologies if it was a different revision within that range that broke these examples, but this one seems the most likely to me. The example used to work fine, and it still works fine in Firefox.

[Philip Rogers]

Confirmed. Thank you for taking the time to report this and file a nice repro.

The issue is this works:
  <rect x1="100" y="100" width="100" height="100" id="rect" fill="green" />
  <animate attributeName="x" xlink:href="#rect" dur="5s" from="200" to="400" fill="freeze"/>
but this does not:
  <animate attributeName="x" xlink:href="#rect" dur="5s" from="200" to="400" fill="freeze"/>
  <rect x1="100" y="100" width="100" height="100" id="rect" fill="green" />

If you need a temporary workaround, moving the defs below the animating elements will fix things.


It looks like https://bugs.webkit.org/show_bug.cgi?id=96697 regressed this case; I'm surprised we do not have tests covering this. The fix is to hook animate elements into our resource tracking which, thankfully, we just finished the infrastructure for: http://trac.webkit.org/changeset/132847.

Patch forthcoming.

[Philip Rogers]

Just a quick update, I was offline last week (holiday) but I'm still hacking away at this and hope to have a patch up by EOD.

[Philip Rogers]

Created attachment 176362 [details]
Preview patch - not yet ready for review

[Philip Rogers]

Created attachment 176414 [details]
Preview patch 2 - almost ready for review

[Philip Rogers]

Created attachment 176582 [details]
Remove redundant SVG animation target tracking framework.

[Florin Malita]

Comment on attachment 176582 [details]
Remove redundant SVG animation target tracking framework.

View in context: https://bugs.webkit.org/attachment.cgi?id=176582&action=review

I'm not familiar with animations, but this sounds like a neat idea!

> Source/WebCore/svg/animation/SVGSMILElement.cpp:174
> +        if (hasPendingResources())
> +            return;

This looks problematic: if href is updated while still pending the initial resource, it seems this would prevent us from registering the new id. You should also add a test to cover this case.

[Eric Seidel (no email)]

Comment on attachment 176582 [details]
Remove redundant SVG animation target tracking framework.

Seems reasonable.  But Antti and Niko are going to have much more context in this code than I.

[Nikolas Zimmermann]

Comment on attachment 176582 [details]
Remove redundant SVG animation target tracking framework.

View in context: https://bugs.webkit.org/attachment.cgi?id=176582&action=review

Nice patch, I look forward to when its landed.

> Source/WebCore/svg/SVGAnimateElement.cpp:415
> +    // FIXME: unify with setTargetElement.

I just wanted to propose this. Shouldn't be too hard, I'd include this here.

>> Source/WebCore/svg/animation/SVGSMILElement.cpp:174
>> +            return;
> 
> This looks problematic: if href is updated while still pending the initial resource, it seems this would prevent us from registering the new id. You should also add a test to cover this case.

Good catch. For the generic case (not for animation elements) SVGStyledElement::buildPendingResourcesIfNeeded() takes care of that IIRC (handling removal). Usually it's handled like this:
void SVGStyledElement::svgAttributeChanged(const QualifiedName& attrName)
{
...
    if (isIdAttributeName(attrName)) {
        RenderObject* object = renderer();
        // Notify resources about id changes, this is important as we cache resources by id in SVGDocumentExtensions
        if (object && object->isSVGResourceContainer())
            object->toRenderSVGResourceContainer()->idChanged();
        if (inDocument())
            buildPendingResourcesIfNeeded();
        SVGElementInstance::invalidateAllInstancesOfElement(this);
        return;
    }
}

If an animation element references an element with the id #foobar (during buildPendingResources in your patch) which is not available, it's registered as pending resource.
Now the expectation is that buildPendingResourceIfNeeded() is called via the SVGStyledElement code path I quoted above, as soon as "foobar" appears. "foobar" has the responsibility
of notifying the other resources depending on it, that it's now available. This deregisteres foobar as pending.

If the referenced ID changes from #foobar, to #blub, before #foobar came available, then Florin is right nothing would happen, even if #blub exists.
SVGFEImageElement for instance if the xlink:href changes, just calls buildPendingResources()  (I think just like Philip did), but it fails to deregister the old ID as pending.

I fear there's still a flaw in the current design that we should resolve before converting animations to use it as well. Agreed?

We basically need to check all callers of addPendingResources() within svg/ (rendering/ is not affected, eg. clip-path="#blub" references are handled differently via SVGResources).
It's basically SVGMPathElement, TextPathElement, FEImageElement, .. A grep should help :-)

What do you think?

> Source/WebCore/svg/animation/SVGSMILElement.h:-36
> -enum ResolveTarget { DoNotResolveNewTarget, ResolveNewTarget };

Great this hack is gone.

[Philip Rogers]

Created attachment 177095 [details]
Unify SVG's animation and target tracking systems.

[Philip Rogers]

Created attachment 177096 [details]
Unify SVG's animation and target tracking systems.

[Philip Rogers]

Florin and Niko's catch was a great one: we can leave extra pending resource entries around. I investigated fixing this in this bug but I think it's out of scope. The issue comes down to needing to track {Element*, AttributeName} -> id instead of just Element* -> id. I've created a proposal for fixing this fundamental flaw but I'd like to track it separately in https://bugs.webkit.org/show_bug.cgi?id=103811.

The extra pending resource entries will only occur in fairly non-standard scenarios and don't have security implications. I think this patch is valuable without that fundamental fix. Do you guys agree or should 103811 block this?

(In reply to comment #8)
> (From update of attachment 176582 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=176582&action=review
> 
> Nice patch, I look forward to when its landed.
> 
> > Source/WebCore/svg/SVGAnimateElement.cpp:415
> > +    // FIXME: unify with setTargetElement.
> 
> I just wanted to propose this. Shouldn't be too hard, I'd include this here.

Done.

> 
> >> Source/WebCore/svg/animation/SVGSMILElement.cpp:174
> >> +            return;
> > 
> > This looks problematic: if href is updated while still pending the initial resource, it seems this would prevent us from registering the new id. You should also add a test to cover this case.
> 
> Good catch. For the generic case (not for animation elements) SVGStyledElement::buildPendingResourcesIfNeeded() takes care of that IIRC (handling removal). Usually it's handled like this:
> void SVGStyledElement::svgAttributeChanged(const QualifiedName& attrName)
> {
> ...
>     if (isIdAttributeName(attrName)) {
>         RenderObject* object = renderer();
>         // Notify resources about id changes, this is important as we cache resources by id in SVGDocumentExtensions
>         if (object && object->isSVGResourceContainer())
>             object->toRenderSVGResourceContainer()->idChanged();
>         if (inDocument())
>             buildPendingResourcesIfNeeded();
>         SVGElementInstance::invalidateAllInstancesOfElement(this);
>         return;
>     }
> }
> 
> If an animation element references an element with the id #foobar (during buildPendingResources in your patch) which is not available, it's registered as pending resource.
> Now the expectation is that buildPendingResourceIfNeeded() is called via the SVGStyledElement code path I quoted above, as soon as "foobar" appears. "foobar" has the responsibility
> of notifying the other resources depending on it, that it's now available. This deregisteres foobar as pending.
> 
> If the referenced ID changes from #foobar, to #blub, before #foobar came available, then Florin is right nothing would happen, even if #blub exists.
> SVGFEImageElement for instance if the xlink:href changes, just calls buildPendingResources()  (I think just like Philip did), but it fails to deregister the old ID as pending.
> 
> I fear there's still a flaw in the current design that we should resolve before converting animations to use it as well. Agreed?
> 
> We basically need to check all callers of addPendingResources() within svg/ (rendering/ is not affected, eg. clip-path="#blub" references are handled differently via SVGResources).
> It's basically SVGMPathElement, TextPathElement, FEImageElement, .. A grep should help :-)
> 
> What do you think?
> 
> > Source/WebCore/svg/animation/SVGSMILElement.h:-36
> > -enum ResolveTarget { DoNotResolveNewTarget, ResolveNewTarget };
> 
> Great this hack is gone.

[Philip Rogers]

(In reply to comment #6)
> (From update of attachment 176582 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=176582&action=review
> 
> I'm not familiar with animations, but this sounds like a neat idea!
> 
> > Source/WebCore/svg/animation/SVGSMILElement.cpp:174
> > +        if (hasPendingResources())
> > +            return;
> 
> This looks problematic: if href is updated while still pending the initial resource, it seems this would prevent us from registering the new id. You should also add a test to cover this case.

Fantastic catch. I've fixed the other cases (SVGMPathElement and SVGTextPathElement), fixed the code, and added a test that failed with the previous patch but now passes.

[Philip Rogers]

Ping for reviews

[Dirk Schulze]

Comment on attachment 177096 [details]
Unify SVG's animation and target tracking systems.

View in context: https://bugs.webkit.org/attachment.cgi?id=177096&action=review

Is the diff broken, or the tool? I get:

(see the test.)
addPendingResource
addPendingResource
addPendingResource

At the end.

> Source/WebCore/ChangeLog:25
> +        which fixes a bug (see the test.)

s/(see the test.)/(see the test)./ ;)

[Philip Rogers]

Created attachment 178064 [details]
Fix egregious Changelog error and rebaseline

[Philip Rogers]

(In reply to comment #14)
> (From update of attachment 177096 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=177096&action=review
> 
> Is the diff broken, or the tool? I get:
> 
> (see the test.)
> addPendingResource
> addPendingResource
> addPendingResource
> 
> At the end.

Not sure what's up here but lets see if the updated patch fixes this.

> 
> > Source/WebCore/ChangeLog:25
> > +        which fixes a bug (see the test.)
> 
> s/(see the test.)/(see the test)./ ;)

Fixed! I'm ashamed :(

[Dirk Schulze]

Comment on attachment 178064 [details]
Fix egregious Changelog error and rebaseline

It looks good to me. r=me.

[WebKit Review Bot]

Comment on attachment 178064 [details]
Fix egregious Changelog error and rebaseline

Clearing flags on attachment: 178064

Committed r136906: <http://trac.webkit.org/changeset/136906>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Nikolas Zimmermann]

Again thanks for that great patch, looks much more unified now :-)

[Said Abou-Hallawa]

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