Bug 849593 - Skip samples of active SMIL timed elements when the sample time precedes the current interval; r=dholbert
authorBrian Birtles <birtles@gmail.com>
Tue, 24 Oct 2017 13:06:04 +0900
changeset 388051 680d798e440b9492e0409c01a40d85f6022fe09a
parent 388050 d22b8c27e4ccfcad474213c6d9789a29aa52ca4b
child 388052 3170d21cbd25d32466502d12119f6d9824c84bbf
push id32739
push useracraciun@mozilla.com
push dateWed, 25 Oct 2017 09:29:21 +0000
treeherdermozilla-central@252a8528c5ab [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdholbert
bugs849593
milestone58.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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
dom/smil/crashtests/849593-1.xhtml
dom/smil/crashtests/crashtests.list
dom/smil/nsSMILTimedElement.cpp
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