Created attachment 12155 [details] test case (will crash Safari)

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

<rdar://problem/4960656>

Downgrading to P2 since animation is now disabled in the mainline build.

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).

Animation is turned off again in trunk, this can be pushed down to a p3.

test case no longer crashes.

blarg. nevermind.

Animation is turned back on on trunk. This goes back to P1.

Testcase doesn't seem to crash TOT.

Related to bug 12434?

(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.

Created attachment 83099 [details] Patch

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.

(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.

Created attachment 83510 [details] Patch

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 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.

Committed r79569: <http://trac.webkit.org/changeset/79569>