Bug 95517 – ASSERTion failure when SVG element is removed from document and readded

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Tim Horton]

See attached test case: if you have an <svg> element in an HTML page, then remove it and add it again, we assert:

ASSERTION FAILED: !m_beginTime
/Volumes/SSD/src/WebKit/OpenSource/Source/WebCore/svg/animation/SMILTimeContainer.cpp(91) : void WebCore::SMILTimeContainer::begin()
1   0x103e43962 WebCore::SMILTimeContainer::begin()
2   0x104082648 WebCore::SVGSVGElement::insertedInto(WebCore::ContainerNode*)
3   0x102b04494 WebCore::ChildNodeInsertionNotifier::notifyNodeInsertedIntoDocument(WebCore::Node*)
4   0x102b00c25 WebCore::ChildNodeInsertionNotifier::notify(WebCore::Node*)
5   0x102afd69a WebCore::updateTreeAfterInsertion(WebCore::ContainerNode*, WebCore::Node*, bool)
6   0x102afd0cd WebCore::ContainerNode::appendChild(WTF::PassRefPtr<WebCore::Node>, int&, bool)
7   0x1039be15a WebCore::Node::appendChild(WTF::PassRefPtr<WebCore::Node>, int&, bool)

This is because m_beginTime wasn't cleared when the element was removed, so it's still set from this element's previous call to begin().

I have a patch, however, I wonder what behavior is expected? Should animations continue from where they were, or should they restart from the beginning when the <svg> is added to the document?

<rdar://problem/12175583>

[Tim Horton]

Created attachment 161595 [details]
repro

[Tim Horton]

The SVG Animation spec says that the timeline is relative to "document begin", which is "the exact time at which the svg element's SVGLoad event is triggered", which only happens once, so I guess we can't just reset the time container.

[Dean Jackson]

(In reply to comment #0)

> I have a patch, however, I wonder what behavior is expected? Should animations continue from where they were, or should they restart from the beginning when the <svg> is added to the document?

Seems like this is undefined in the spec. I vote for restarting.

[Tim Horton]

Re-reading the spec, it's pretty clear that they're not supposed to restart (that the TimeContainer starts when SVGLoad happens and therefore cannot be reset), so maybe the assertion is just bogus. Let's see if any tests are depending on this behavior.

[Tim Horton]

(In reply to comment #4)
> Re-reading the spec, it's pretty clear that they're not supposed to restart (that the TimeContainer starts when SVGLoad happens and therefore cannot be reset), so maybe the assertion is just bogus. Let's see if any tests are depending on this behavior.

Firefox and Opera agree with ^^ (and do not restart the animation).

However, our current behavior is actually *more* broken -- the animation doesn't resume when the <svg> is readded.

[Tim Horton]

(In reply to comment #5)
> (In reply to comment #4)
> > Re-reading the spec, it's pretty clear that they're not supposed to restart (that the TimeContainer starts when SVGLoad happens and therefore cannot be reset), so maybe the assertion is just bogus. Let's see if any tests are depending on this behavior.
> 
> Firefox and Opera agree with ^^ (and do not restart the animation).

They also don't pause the animation while the <svg> is out-of-document, so that's kind of odd... (it jumps when it gets readded)

[Tim Horton]

Created attachment 161799 [details]
patch

[Tim Horton]

http://trac.webkit.org/changeset/127474