Bug 849593 - Skip samples of active SMIL timed elements when the sample time precedes the current interval; r?dholbert
In some circumstances it is possible to sample a timed element in the active
state with a time that precedes is current interval.
One possible sequence of steps leading to this situation is as follows:
1. A timed element (e.g. <set>, <animate>) with a non-zero begin time is the
child of <svg> element A (its "time container") but has yet to be sampled.
2. In order to resolve its initial interval, the timed element registers a
startup milestone with its time container at time 0.
3. However, before the sample is performed where the timed element's initial
current interval is resolved, <svg> element A is detached from the document
tree.
4. The timed element is then attached to a different <svg> element B that has
a current time greater than the begin time of the timed element and less than
that of <svg> element A.
5. Since the timed element is still in its startup state it registers its
startup milestone again, this time with its new time container, i.e. <svg>
element B.
6. A tick occurs or the document has its style flushed such that a sample is
performed.
This includes running the milestone sample which causes the timed element to
resolve its initial current interval. Furthermore the subsequent regular
sample of the timed element causes it to transition into its active state
because the current time of <svg> element B is greater than the begin time of
the timed element.
7. <svg> element A is re-attached to the document.
8. When we go to run the next sample, we iterate through all time containers
associated with the document's animation controller which includes both <svg>
element A, and <svg> element B.
9. <svg> element A renders up its 0 milestone from step (2) since it has yet to
run it. It converts this to parent time, i.e. the time space of the animation
controller, which will be zero or less depending on the current time of <svg>
element A when it was re-attached.
10. Since the milestone from <svg> element A will be the earliest milestone
time, it will be used as the next milestone sample time.
11. The timed element is then sampled using this time, but first it is converted
to a time in the time space of the timed element's time container, which is
now <svg> element B.
As a result of this conversion, the sample time may end up being *before*
the beginning of the timed element's current interval. Since timed elements
never expect the time to go backwards an assertion fails when it detects
that it is active, but is being sampled before its current interval.
For this particular case, ignoring the "early" sample seems to be the most
appropriate action.
More generally, however, we can anticipate other cases similar to this where
milestones are registered that cause the sample time to temporarily go
backwards. A quick audit of nsSMILTimedElement::DoSampleAt suggests that, with
the code changes from this patch, that is probably ok.
As an alternative we could, perhaps, try to drop and re-create all milestones
when time containers are re-attached to the document tree but that would add
more complexity and would not necessarily cover other similar cases of this
situation.
I have verified that the crashtest included in this changeset fails without the
code changes also in this changeset.
MozReview-Commit-ID: KKGYRayNkpo
new file mode 100644
--- /dev/null
+++ b/dom/smil/crashtests/849593-1.xhtml
@@ -0,0 +1,34 @@
+<html xmlns="http://www.w3.org/1999/xhtml">
+<head class="reftest-wait">
+<meta charset="utf-8"/>
+<script>
+<![CDATA[
+
+function boom()
+{
+ var a = document.getElementById('a');
+ var b = document.getElementById('b');
+ var c = document.getElementById('c');
+ var d = document.getElementById('d');
+
+ b.setCurrentTime(1);
+
+ a.appendChild(c);
+ document.body.removeChild(a);
+ b.appendChild(c);
+ document.documentElement.offsetHeight;
+ d.appendChild(a);
+
+ document.documentElement.removeAttribute('class');
+}
+
+]]>
+</script>
+</head>
+
+<body onload="setTimeout(boom, 0);">
+<svg xmlns="http://www.w3.org/2000/svg" id="a"/>
+<svg xmlns="http://www.w3.org/2000/svg" id="b"/>
+<set xmlns="http://www.w3.org/2000/svg" begin="1s" id="c"><div xmlns="http://www.w3.org/1999/xhtml" id="d"></div></set>
+</body>
+</html>
--- a/dom/smil/crashtests/crashtests.list
+++ b/dom/smil/crashtests/crashtests.list
@@ -46,12 +46,13 @@ load 678847-1.svg
load 678938-1.svg
load 690994-1.svg
load 691337-1.svg
load 691337-2.svg
load 697640-1.svg
load 699325-1.svg
load 709907-1.svg
load 720103-1.svg
+load 849593-1.xhtml
load 1010681-1.svg
load 1322849-1.svg
load 1375596-1.svg
load 1402547-1.html
--- a/dom/smil/nsSMILTimedElement.cpp
+++ b/dom/smil/nsSMILTimedElement.cpp
@@ -697,22 +697,20 @@ nsSMILTimedElement::DoSampleAt(nsSMILTim
NotifyChangedInterval(
mOldIntervals[mOldIntervals.Length() - 1], false, true);
}
if (mElementState == STATE_WAITING) {
NotifyNewInterval();
}
FilterHistory();
stateChanged = true;
- } else {
+ } else if (mCurrentInterval->Begin()->Time() <= sampleTime) {
MOZ_ASSERT(!didApplyEarlyEnd,
"We got an early end, but didn't end");
nsSMILTime beginTime = mCurrentInterval->Begin()->Time().GetMillis();
- NS_ASSERTION(aContainerTime >= beginTime,
- "Sample time should not precede current interval");
nsSMILTime activeTime = aContainerTime - beginTime;
// The 'min' attribute can cause the active interval to be longer than
// the 'repeating interval'.
// In that extended period we apply the fill mode.
if (GetRepeatDuration() <= nsSMILTimeValue(activeTime)) {
if (mClient && mClient->IsActive()) {
mClient->Inactivate(mFillMode == FILL_FREEZE);
@@ -732,16 +730,21 @@ nsSMILTimedElement::DoSampleAt(nsSMILTim
mCurrentRepeatIteration != prevRepeatIteration &&
mCurrentRepeatIteration &&
mSeekState == SEEK_NOT_SEEKING) {
FireTimeEventAsync(eSMILRepeatEvent,
static_cast<int32_t>(mCurrentRepeatIteration));
}
}
}
+ // Otherwise |sampleTime| is *before* the current interval. That
+ // normally doesn't happen but can happen if we get a stray milestone
+ // sample (e.g. if we registered a milestone with a time container that
+ // later got re-attached as a child of a more advanced time container).
+ // In that case we should just ignore the sample.
}
break;
case STATE_POSTACTIVE:
break;
}
// Generally we continue driving the state machine so long as we have changed