536660 - Keep around anonymous nodes for <use>, even when the <use> doesn't have any frames

Status: RESOLVED WORKSFORME

(Core :: SVG, defect)

Description

[Daniel Holbert [:dholbert]]

Currently, the anonymous content for <use> elements gets removed from the document as soon as the <use> element's frames go away.  See in particular nsSVGUseFrame::Destroy(), which calls nsSVGUseElement::DestroyAnonymousContent().

This caused problems with SMIL animation in the testcases posted on Bug 534975, because we expect that the content tree's structure won't change during the course of a sample. In particular, we expect that nodes in the document at the beginning of the sample should still be in the document at the end of the sample.

But the problem was if we apply a 'display: none' style the <use> element during an animation sample, then its frame gets destroyed, and right now that will remove all of its anonymous content from the document.

We've worked around this in that bug, but Roc suggests a better solution in Bug 534975 comment #17:
> have the <use> anonymous nodes owned by the element and present whether the
> <use> element has a frame or not.

Comment 1

[Daniel Holbert [:dholbert]]

(In reply to comment #0)
> But the problem was if we apply a 'display: none' style the <use> element
> during an animation sample, then its frame gets destroyed, and right now that
> will remove all of its anonymous content from the document.

(...which is a problem if some of that removed anonymous content was the target of animations and was queued up for compositing later in the same animation sample.)

Comment 2

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Test case

With the attached test case we end up generating animation functions ad infinitum.

Comment 3

[Brian Birtles (:birtles, Feb 26~ parental leave)]

In bug 562815 (Don't always force-recompose animations of CSS properties) we ran into trouble because of this bug.

I don't properly understand it but it seems to be something like this:

* In the test case (attachment 550939 [details]) the <set> is anonymous content of <use> and we regenerate the anonymous content of the <use> every sample (because it sets display to none)
* In nsSMILCSSProperty, when aPreventCachingOfSandwich is PR_TRUE for display:none (as it is now) we somehow clear the anonymous content because we were always set the property
* But if we allow it to be sometimes PR_FALSE (which is what bug 562815 tries to fix) we don't end up re-setting that property and don't end up clearing the anonymous content which gets generated every sample

That may not be accurate (at all) but I think it's something of that ilk.

If we allow aPreventCachingOfSandwich to be PR_FALSE with display:none /content/smil/crashtests/572938-1.svg gets into an infinite update situation and, on Windows at least, the UI becomes unresponsive (you can still click buttons but nothing happens).

For bug 56281 we worked around this by just forcing aPreventCachingOfSandwich to be PR_TRUE for display:

http://mxr.mozilla.org/mozilla-central/source/content/smil/nsSMILCSSProperty.cpp#172

The downside is that it means we miss out on optimising animation of the display property which is probably one of the most commonly animated properties (particularly with <set>).

In fixing this bug, we should check if the hack to nsSMILCSSProperty mentioned above can be removed.

Comment 4

[Robert Longson [:longsonr]]

<use> elements are now based on shadow trees, nsSVGUseFrame no longer has a destroy method.