Bug 12065 - Removing a SVG animation target during animation crashes WebKit
Summary: Removing a SVG animation target during animation crashes WebKit
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 420+
Hardware: Mac OS X 10.4
: P1 Normal
Assignee: Nobody
URL: http://www.w3.org/Graphics/SVG/Test/2...
Keywords: HasReduction, InRadar
Depends on:
Blocks: 41761 55130
  Show dependency treegraph
 
Reported: 2007-01-01 21:12 PST by Eric Seidel (no email)
Modified: 2011-02-24 08:23 PST (History)
6 users (show)

See Also:


Attachments
test case (will crash Safari) (615 bytes, image/svg+xml)
2007-01-01 21:12 PST, Eric Seidel (no email)
no flags Details
Patch (49.36 KB, patch)
2011-02-20 11:09 PST, Dirk Schulze
no flags Details | Formatted Diff | Diff
Patch (49.03 KB, patch)
2011-02-23 11:27 PST, Dirk Schulze
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2007-01-01 21:12:03 PST
Removing an animation target during animation crashes WebKit

SVGAnimationElement does not hold its target in a RefPtr.  Thus if you remove the target during animation, WebKit will crash.

A simple fix is to remove the m_targetElement cache.  The only problem then becomes un-applying any animations once it's no longer the target of animations.  Perhaps SVGElement should override setId to clear any animVals on the object.  Not sure.  That can be tracked with a separate bug after the crash has been fixed.
Comment 1 Eric Seidel (no email) 2007-01-01 21:12:37 PST
Created attachment 12155 [details]
test case (will crash Safari)
Comment 2 Eric Seidel (no email) 2007-01-27 03:46:13 PST
I believe this test case crashes because of this bug:
http://www.w3.org/Graphics/SVG/Test/20061213/htmlEmbedHarness/full-animate-elem-40-t.html
Comment 3 Maciej Stachowiak 2007-01-29 03:51:00 PST
<rdar://problem/4960656>
Comment 4 Maciej Stachowiak 2007-02-26 11:43:12 PST
Downgrading to P2 since animation is now disabled in the mainline build.
Comment 5 Eric Seidel (no email) 2007-10-18 00:31:24 PDT
Animation has been re-enabled on trunk (as part of SVG_EXPERIMENTAL_FEATURES) thus this should be bumped back to P1 according to the bug guidelines (and mjs).
Comment 6 Eric Seidel (no email) 2007-12-27 01:43:16 PST
Animation is turned off again in trunk, this can be pushed down to a p3.
Comment 7 Alice Liu 2008-02-29 12:16:23 PST
test case no longer crashes.
Comment 8 Alice Liu 2008-02-29 12:18:14 PST
blarg. nevermind. 
Comment 9 Eric Seidel (no email) 2008-03-26 17:07:29 PDT
Animation is turned back on on trunk.  This goes back to P1.
Comment 10 Simon Fraser (smfr) 2008-09-30 17:56:01 PDT
Testcase doesn't seem to crash TOT.
Comment 11 Simon Fraser (smfr) 2009-02-12 21:35:16 PST
Related to bug 12434?
Comment 12 Dirk Schulze 2010-11-12 04:41:50 PST
(In reply to comment #0)
> Removing an animation target during animation crashes WebKit
> 
> SVGAnimationElement does not hold its target in a RefPtr.  Thus if you remove the target during animation, WebKit will crash.
> 
> A simple fix is to remove the m_targetElement cache.  The only problem then becomes un-applying any animations once it's no longer the target of animations.  Perhaps SVGElement should override setId to clear any animVals on the object.  Not sure.  That can be tracked with a separate bug after the crash has been fixed.

We don't save m_targetElement anymore, so this bug is no longer valid. Nevertheless, I plan to re implement m_targetElement with bug 49437, but refcounted. The test didn't crash with this patch either. I'd like to upload the test in the attachment before closing this bug. Upload a a patch here soon.
Comment 13 Dirk Schulze 2011-02-20 11:09:49 PST
Created attachment 83099 [details]
Patch
Comment 14 Nikolas Zimmermann 2011-02-23 03:42:30 PST
Comment on attachment 83099 [details]
Patch

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

Still something todo, but almost there... r- for now:

> Source/WebCore/ChangeLog:12
> +        target gets referenced via 'xlink:href'. At the moment we would call getElementById() multipple

s/multipple/multiple/

> Source/WebCore/ChangeLog:15
> +        reseted, if the target was removed from document or its destructor was called. A HashMap in

s/gets reseted/is reset/

> Source/WebCore/svg/SVGDocumentExtensions.cpp:162
> +    ASSERT(animationElementsForTarget && !animationElementsForTarget->isEmpty());

Better use two seperated ASSERTS.

> Source/WebCore/svg/SVGDocumentExtensions.cpp:166
> +        m_animatedElements.take(targetElement);

This is wrong, you're leaking the HashSet* object, that's returned here.
You have to use delete foobar.take(blub); if you're using values on the heap.

> Source/WebCore/svg/SVGDocumentExtensions.cpp:173
> +    if (!m_animatedElements.contains(targetElement))
> +        return;

Why is this not guaranteed to be true, here, opposed to removeAnimationElementFromTarget?

> Source/WebCore/svg/SVGDocumentExtensions.cpp:179
> +    delete m_animatedElements.take(targetElement);

Here, you did it correct.

> Source/WebCore/svg/SVGElement.cpp:124
> +    document()->accessSVGExtensions()->removeAllAnimationElementsFromTarget(this);
> +    StyledElement::removedFromDocument();

Did you choose this order, on purpose?

> Source/WebCore/svg/SVGHKernElement.cpp:64
> +    SVGElement::removedFromDocument();

The order is intentional? base class call after the stuff above?
Does this fix an unrelated bug as well? What does SVGElement/Element/Node::removedFromDocument() do, what we may have missed for hkern/vkern elements so far? (as they never called the base class method)

> LayoutTests/svg/custom/animate-target-id-changed.svg:15
> +    setTimeout(function() { rect.setAttribute("id", "newId"); }, 250);
> +    setTimeout("layoutTestController.notifyDone()", 1000);

This is not efficient, see below.

> LayoutTests/svg/custom/animate-target-removed-from-document.svg:15
> +    setTimeout(function() { document.getElementsByTagName("svg")[0].removeChild(rect); }, 250);
> +    setTimeout("layoutTestController.notifyDone()", 1000);

This is way too slow! Use at least:
setTimeout(function() {
    document.getElementsByTagName("svg")[0].removeChild(rect);
   setTimeout(function() { layoutTestController.notifyDone(); }, 0);
}, 250);

This should be sufficient.
Hm, I think I have a better idea though:

if (window.layoutTestController) {
    layoutTestController.waitUntilDone();
    setTimeout(function() {
         document.getElementsByTagName("svg")[0].removeChild(document.getElementById('target'));
        setTimeout(function() { layoutTestController.notifyDone(); }, 0);
   }, 0);

That should work as well, but execute much faster.
Comment 15 Dirk Schulze 2011-02-23 08:06:47 PST
(In reply to comment #14)
> This is wrong, you're leaking the HashSet* object, that's returned here.
> You have to use delete foobar.take(blub); if you're using values on the heap.
> 
> > Source/WebCore/svg/SVGDocumentExtensions.cpp:173
> > +    if (!m_animatedElements.contains(targetElement))
> > +        return;
> 
> Why is this not guaranteed to be true, here, opposed to removeAnimationElementFromTarget?
No. The situation is different. We just add elements to the map with at least one animation. But the element itself doesn't have information about that.  Thats why SVGElement calls removeAllAnimationElementsFromTarget independent if it is animated or not.
 
> > Source/WebCore/svg/SVGElement.cpp:124
> > +    document()->accessSVGExtensions()->removeAllAnimationElementsFromTarget(this);
> > +    StyledElement::removedFromDocument();
> 
> Did you choose this order, on purpose?
I followed the example on other elements like SVGStyledElement, no special reason. 

> 
> > Source/WebCore/svg/SVGHKernElement.cpp:64
> > +    SVGElement::removedFromDocument();
> 
> The order is intentional? base class call after the stuff above?
> Does this fix an unrelated bug as well? What does SVGElement/Element/Node::removedFromDocument() do, what we may have missed for hkern/vkern elements so far? (as they never called the base class method)
StyledElement has no remvoedFromDocument, but Element has:
void Element::removedFromDocument()
{
    if (hasID()) {
        if (m_attributeMap) {
            Attribute* idItem = m_attributeMap->getAttributeItem(document()->idAttributeName());
            if (idItem && !idItem->isNull())
                updateId(idItem->value(), nullAtom);
        }
    }

Short description of updateId:
if (document && !id.isEmpty())
   doc->removeElementById(id, this);

So it depends if the element is still in document on calling this function or not and if it has an id or not.

> 
> > LayoutTests/svg/custom/animate-target-id-changed.svg:15
> > +    setTimeout(function() { rect.setAttribute("id", "newId"); }, 250);
> > +    setTimeout("layoutTestController.notifyDone()", 1000);
> 
> This is not efficient, see below.
> 
> > LayoutTests/svg/custom/animate-target-removed-from-document.svg:15
> > +    setTimeout(function() { document.getElementsByTagName("svg")[0].removeChild(rect); }, 250);
> > +    setTimeout("layoutTestController.notifyDone()", 1000);
> 
> This is way too slow! Use at least:
> setTimeout(function() {
>     document.getElementsByTagName("svg")[0].removeChild(rect);
>    setTimeout(function() { layoutTestController.notifyDone(); }, 0);
> }, 250);
> 
> This should be sufficient.
> Hm, I think I have a better idea though:
> 
> if (window.layoutTestController) {
>     layoutTestController.waitUntilDone();
>     setTimeout(function() {
>          document.getElementsByTagName("svg")[0].removeChild(document.getElementById('target'));
>         setTimeout(function() { layoutTestController.notifyDone(); }, 0);
>    }, 0);
> 

Thanks, I'll change it.
> That should work as well, but execute much faster.
Comment 16 Dirk Schulze 2011-02-23 11:27:53 PST
Created attachment 83510 [details]
Patch
Comment 17 Darin Adler 2011-02-23 11:37:51 PST
Comment on attachment 83510 [details]
Patch

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

I’m going to say review+ but I recommend doing a new patch that does fewer hash table lookups.

> Source/WebCore/svg/SVGDocumentExtensions.cpp:152
> +    if (!m_animatedElements.contains(targetElement)) {
> +        HashSet<SVGSMILElement*>* animationElementsForTarget = new HashSet<SVGSMILElement*>;
> +        animationElementsForTarget->add(animationElement);
> +        m_animatedElements.set(targetElement, animationElementsForTarget);
> +        return;
> +    }
> +    HashSet<SVGSMILElement*>* animationElementsForTarget = m_animatedElements.get(targetElement);
> +    if (animationElementsForTarget->contains(animationElement))
> +        return;

This does extra hash table lookups.

It’s not efficient to call contains and then immediately call get afterward. Instead you can just check for 0. But more importantly, you can use the return value for the add call to do the contains, get, and set all in one operation. You simply call add with a 0, and then there are two ways to detect the newly-added case. One is by checking the boolean that add returns to indicate whether a new value was added. Or you can simply look at the value and see the 0 and that will tell you that a set needs to be allocated.

> Source/WebCore/svg/SVGDocumentExtensions.cpp:165
> +    if (animationElementsForTarget->contains(animationElement))
> +        animationElementsForTarget->remove(animationElement);

Extra hash table lookup here. There is no reason to check contains before calling remove, since remove does nothing if the value is already there.

> Source/WebCore/svg/SVGDocumentExtensions.cpp:167
> +    if (animationElementsForTarget->isEmpty())
> +        delete m_animatedElements.take(targetElement);

Since we already have animationElementsForTarget in a local variable, it seems a little strange to call take instead of remove here.

Further, if you used find instead of get above, then you could remove the element here with the version of remove that takes an iterator. That would avoid doing an extra hash table lookup.

> Source/WebCore/svg/SVGDocumentExtensions.cpp:175
> +    HashSet<SVGSMILElement*>* animationElementsForTarget = m_animatedElements.get(targetElement);

Can yo do the take here instead of waiting for the end of the function. Not good to have to do two hash lookups instead of one.

> Source/WebCore/svg/SVGVKernElement.cpp:62
> +    SVGElement::removedFromDocument();

We should probably create a debugging mechanism to ensure people don’t forget to call through from the base class. At the call site of removedFromDocument we could clear a debug-only flag on the node, and set that flag in the Node::removedFromDocument function, then assert the flag is set after the call at the call site.
Comment 18 Dirk Schulze 2011-02-24 01:24:18 PST
Comment on attachment 83510 [details]
Patch

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

>> Source/WebCore/svg/SVGVKernElement.cpp:62
>> +    SVGElement::removedFromDocument();
> 
> We should probably create a debugging mechanism to ensure people don’t forget to call through from the base class. At the call site of removedFromDocument we could clear a debug-only flag on the node, and set that flag in the Node::removedFromDocument function, then assert the flag is set after the call at the call site.

This would not only influence SVG but also WML and HTML. I'd like to open another bug report and check this first. Maybe there are similar problems in the other parts of WebKit.
Comment 19 Dirk Schulze 2011-02-24 08:23:32 PST
Committed r79569: <http://trac.webkit.org/changeset/79569>