[Webkit-unassigned] [Bug 12065] Removing a SVG animation target during animation crashes WebKit

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=12065


Nikolas Zimmermann <zimmermann at kde.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #83099|review?                     |review-
               Flag|                            |




--- Comment #14 from Nikolas Zimmermann <zimmermann at kde.org>  2011-02-23 03:42:30 PST ---
(From update of attachment 83099)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.