[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





--- Comment #15 from Dirk Schulze <krit at webkit.org>  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.

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