[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


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #83510|review?                     |review+
               Flag|                            |




--- Comment #17 from Darin Adler <darin at apple.com>  2011-02-23 11:37:51 PST ---
(From update of attachment 83510)
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.

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