Bug 615872 Part 2 - SVG SMIL: Remove local resamples from timed elements; r=dholbert; a=roc

content/smil/crashtests/615872-1.svg

@@ -0,0 +1,21 @@
+<?xml version="1.0"?>
+
+<svg xmlns="http://www.w3.org/2000/svg"><script>
+<![CDATA[
+
+function boom()
+{
+  var r = document.documentElement;
+  var s = document.createElementNS("http://www.w3.org/2000/svg", "set");
+  s.setAttributeNS(null, "begin", "1s");
+  r.appendChild(s);
+  r.setCurrentTime(2);
+  document.removeChild(r);
+  r.setCurrentTime(0);
+  s.beginElementAt(0);
+}
+
+window.addEventListener("load", boom, false);
+
+]]>
+</script></svg>

content/smil/crashtests/crashtests.list

@@ -33,3 +33,4 @@ load 608549-1.svg
 load 608295-1.html
 load 611927-1.svg
 load 615002-1.svg
+load 615872-1.svg

content/smil/nsSMILTimedElement.cpp

@@ -264,39 +264,6 @@ nsSMILTimedElement::BeginElementAt(double aOffsetSeconds)
   nsSMILTime currentTime = container->GetCurrentTime();
   AddInstanceTimeFromCurrentTime(currentTime, aOffsetSeconds, PR_TRUE);
 
-  // After we've added the instance time we must do a local resample.
-  //
-  // The reason for this can be explained by considering the following sequence
-  // of calls in a script block
-  //
-  //   BeginElementAt(0)
-  //   BeginElementAt(-1)
-  //   GetStartTime() <-- should return the time from the first call to
-  //                      BeginElementAt
-  //
-  // After BeginElementAt(0) is called a new begin instance time is added to the
-  // list. Depending on the restart mode this may generate a new interval,
-  // possiblying ending the current interval early.
-  //
-  // Intuitively this change should take effect before the subsequent call to
-  // BeginElementAt however to get this to take effect we need to drive the
-  // state engine through its sequence active-waiting-active by calling Sample.
-  //
-  // When we get the second call to BeginElementAt the element should be in the
-  // active state and hence the new begin instance time will be ignored because
-  // it is before the beginning of the (new) current interval. SMIL says we do
-  // not change the begin of a current interval once it is active.
-  //
-  // See also:
-  // http://www.w3.org/TR/SMIL3/smil-timing.html#Timing-BeginEnd-Restart
-
-  // If we haven't started yet, then there's no point in trying to force the
-  // sample. A series of calls to BeginElementAt before the document starts
-  // should probably just add a series of instance times.
-  if (mElementState != STATE_STARTUP) {
-    DoSampleAt(currentTime, PR_FALSE); // Regular sample, not end sample
-  }
-
   return NS_OK;
 }
 
@@ -309,9 +276,6 @@ nsSMILTimedElement::EndElementAt(double aOffsetSeconds)
 
   nsSMILTime currentTime = container->GetCurrentTime();
   AddInstanceTimeFromCurrentTime(currentTime, aOffsetSeconds, PR_FALSE);
-  if (mElementState != STATE_STARTUP) {
-    DoSampleAt(currentTime, PR_FALSE); // Regular sample, not end sample
-  }
 
   return NS_OK;
 }

content/smil/test/test_smilKeySplines.xhtml

@@ -249,15 +249,11 @@ function testOkSyntax(anim) {
     testAnim.setAttribute('calcMode', 'spline');
     testAnim.setAttribute('keySplines', okStrs[i]);
     checkSample(4, 20);
+    removeElement(testAnim);
   }
 }
 
 function testBadSyntaxA(anim) {
-  anim.setAttribute('dur','4s');
-  anim.setAttribute('from', '0');
-  anim.setAttribute('to', '20');
-  anim.setAttribute('calcMode', 'spline');
-
   var badStrs = ['', // empty
                  '    ', // whitespace only
                  '0,1.1,0,0', // bad range
@@ -279,6 +275,7 @@ function testBadSyntaxA(anim) {
     testAnim.setAttribute('calcMode', 'spline');
     testAnim.setAttribute('keySplines', badStrs[i]);
     checkSample(4, -100);
+    removeElement(testAnim);
   }
 }
 

content/svg/content/src/nsSVGAnimationElement.cpp

@@ -448,6 +448,9 @@ nsSVGAnimationElement::BeginElementAt(float offset)
 {
   NS_ENSURE_FINITE(offset, NS_ERROR_ILLEGAL_VALUE);
 
+  // Make sure the timegraph is up-to-date
+  FlushAnimations();
+
   // This will fail if we're not attached to a time container (SVG document
   // fragment).
   nsresult rv = mTimedElement.BeginElementAt(offset);
@@ -455,6 +458,9 @@ nsSVGAnimationElement::BeginElementAt(float offset)
     return rv;
 
   AnimationNeedsResample();
+  // Force synchronous sample so that events resulting from this call arrive in
+  // the expected order and we get an up-to-date paint.
+  FlushAnimations();
 
   return NS_OK;
 }
@@ -472,11 +478,16 @@ nsSVGAnimationElement::EndElementAt(float offset)
 {
   NS_ENSURE_FINITE(offset, NS_ERROR_ILLEGAL_VALUE);
 
+  // Make sure the timegraph is up-to-date
+  FlushAnimations();
+
   nsresult rv = mTimedElement.EndElementAt(offset);
   if (NS_FAILED(rv))
     return rv;
 
   AnimationNeedsResample();
+  // Force synchronous sample
+  FlushAnimations();
  
   return NS_OK;
 }

content/svg/content/src/nsSVGSVGElement.cpp

@@ -539,25 +539,20 @@ nsSVGSVGElement::SetCurrentTime(float seconds)
 #ifdef MOZ_SMIL
   if (NS_SMILEnabled()) {
     if (mTimedDocumentRoot) {
+      // Make sure the timegraph is up-to-date
+      FlushAnimations();
       double fMilliseconds = double(seconds) * PR_MSEC_PER_SEC;
       // Round to nearest whole number before converting, to avoid precision
       // errors
       nsSMILTime lMilliseconds = PRInt64(NS_round(fMilliseconds));
       mTimedDocumentRoot->SetCurrentTime(lMilliseconds);
-      // Force a resample now
-      //
-      // It's not sufficient to just request a resample here because calls to
-      // BeginElement etc. expect to operate on an up-to-date timegraph or else
-      // instance times may be incorrectly discarded.
-      //
-      // See the mochitest: test_smilSync.xhtml:testSetCurrentTime()
-      nsIDocument* doc = GetCurrentDoc();
-      if (doc) {
-        nsSMILAnimationController* smilController = doc->GetAnimationController();
-        if (smilController) {
-          smilController->Resample();
-        }
-      }
+      AnimationNeedsResample();
+      // Trigger synchronous sample now, to:
+      //  - Make sure we get an up-to-date paint after this method
+      //  - re-enable event firing (it got disabled during seeking, and it
+      //  doesn't get re-enabled until the first sample after the seek -- so
+      //  let's make that happen now.)
+      FlushAnimations();
     } // else we're not the outermost <svg> or not bound to a tree, so silently
       // fail
     return NS_OK;

layout/reftests/svg/smil/reftest.list

@@ -128,7 +128,7 @@ fails == anim-fillcolor-1.svg      anim-standard-ref.svg # bug 436296
 == anim-gradient-href-01.svg lime.svg
 == anim-image-href-01.svg lime.svg
 == anim-pattern-href-01.svg lime.svg
-asserts(6) == anim-use-href-01.svg lime.svg  # the asserts here are bug 563481
+asserts(9) == anim-use-href-01.svg lime.svg  # the asserts here are bug 563481
 
 # animate where the base value is non-interpolatable but will be replaced anyway
 == anim-fill-overpaintserver-1.svg lime.svg