[webkit-reviews] review denied: [Bug 12065] Removing a SVG animation target during animation crashes WebKit : [Attachment 83099] Patch

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has denied Dirk Schulze
<krit at webkit.org>'s request for review:
Bug 12065: Removing a SVG animation target during animation crashes WebKit
https://bugs.webkit.org/show_bug.cgi?id=12065

Attachment 83099: Patch
https://bugs.webkit.org/attachment.cgi?id=83099&action=review

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
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('ta
rget'));
	setTimeout(function() { layoutTestController.notifyDone(); }, 0);
   }, 0);

That should work as well, but execute much faster.