Bug 941315 - Update the timing model even when invalid values are set; r=longsonr

When an invalid dur, min, max, or repeatDur is set we were failing to call
UpdateCurrentInterval to update the timing model. This patch makes sure we
update the current interval in error cases too.

Mochitests test the interval is being updated in these cases and, for
completeness, the case of repeatCount as well.

content/smil/nsSMILTimedElement.cpp

@@ -112,8 +112,9 @@ namespace
 //----------------------------------------------------------------------
 // Helper class: AutoIntervalUpdateBatcher
 
-// RAII helper to set the mDeferIntervalUpdates flag on an nsSMILTimedElement
-// and perform the UpdateCurrentInterval when the object is destroyed.
+// Stack-based helper class to set the mDeferIntervalUpdates flag on an
+// nsSMILTimedElement and perform the UpdateCurrentInterval when the object is
+// destroyed.
 //
 // If several of these objects are allocated on the stack, the update will not
 // be performed until the last object for a given nsSMILTimedElement is
@@ -146,6 +147,31 @@ private:
   bool mDidSetFlag;
 };
 
+//----------------------------------------------------------------------
+// Helper class: AutoIntervalUpdater
+
+// Stack-based helper class to call UpdateCurrentInterval when it is destroyed
+// which helps avoid bugs where we forget to call UpdateCurrentInterval in the
+// case of early returns (e.g. due to parse errors).
+//
+// This can be safely used in conjunction with AutoIntervalUpdateBatcher; any
+// calls to UpdateCurrentInterval made by this class will simply be deferred if
+// there is an AutoIntervalUpdateBatcher on the stack.
+class MOZ_STACK_CLASS nsSMILTimedElement::AutoIntervalUpdater
+{
+public:
+  AutoIntervalUpdater(nsSMILTimedElement& aTimedElement)
+    : mTimedElement(aTimedElement) { }
+
+  ~AutoIntervalUpdater()
+  {
+    mTimedElement.UpdateCurrentInterval();
+  }
+
+private:
+  nsSMILTimedElement& mTimedElement;
+};
+
 //----------------------------------------------------------------------
 // Templated helper functions
 
@@ -918,8 +944,10 @@ nsSMILTimedElement::UnsetEndSpec(RemovalTestFunction aRemove)
 nsresult
 nsSMILTimedElement::SetSimpleDuration(const nsAString& aDurSpec)
 {
-  nsSMILTimeValue duration;
+  // Update the current interval before returning
+  AutoIntervalUpdater updater(*this);
 
+  nsSMILTimeValue duration;
   const nsAString& dur = nsSMILParserUtils::TrimWhitespace(aDurSpec);
 
   // SVG-specific: "For SVG's animation elements, if "media" is specified, the
@@ -939,7 +967,6 @@ nsSMILTimedElement::SetSimpleDuration(const nsAString& aDurSpec)
     "Setting unresolved simple duration");
 
   mSimpleDur = duration;
-  UpdateCurrentInterval();
 
   return NS_OK;
 }
@@ -954,8 +981,10 @@ nsSMILTimedElement::UnsetSimpleDuration()
 nsresult
 nsSMILTimedElement::SetMin(const nsAString& aMinSpec)
 {
-  nsSMILTimeValue duration;
+  // Update the current interval before returning
+  AutoIntervalUpdater updater(*this);
 
+  nsSMILTimeValue duration;
   const nsAString& min = nsSMILParserUtils::TrimWhitespace(aMinSpec);
 
   if (min.EqualsLiteral("media")) {
@@ -970,7 +999,6 @@ nsSMILTimedElement::SetMin(const nsAString& aMinSpec)
   NS_ABORT_IF_FALSE(duration.GetMillis() >= 0L, "Invalid duration");
 
   mMin = duration;
-  UpdateCurrentInterval();
 
   return NS_OK;
 }
@@ -985,8 +1013,10 @@ nsSMILTimedElement::UnsetMin()
 nsresult
 nsSMILTimedElement::SetMax(const nsAString& aMaxSpec)
 {
-  nsSMILTimeValue duration;
+  // Update the current interval before returning
+  AutoIntervalUpdater updater(*this);
 
+  nsSMILTimeValue duration;
   const nsAString& max = nsSMILParserUtils::TrimWhitespace(aMaxSpec);
 
   if (max.EqualsLiteral("media") || max.EqualsLiteral("indefinite")) {
@@ -1001,7 +1031,6 @@ nsSMILTimedElement::SetMax(const nsAString& aMaxSpec)
   }
 
   mMax = duration;
-  UpdateCurrentInterval();
 
   return NS_OK;
 }
@@ -1036,15 +1065,16 @@ nsSMILTimedElement::UnsetRestart()
 nsresult
 nsSMILTimedElement::SetRepeatCount(const nsAString& aRepeatCountSpec)
 {
+  // Update the current interval before returning
+  AutoIntervalUpdater updater(*this);
+
   nsSMILRepeatCount newRepeatCount;
 
   if (nsSMILParserUtils::ParseRepeatCount(aRepeatCountSpec, newRepeatCount)) {
     mRepeatCount = newRepeatCount;
-    UpdateCurrentInterval();
     return NS_OK;
   }
   mRepeatCount.Unset();
-  UpdateCurrentInterval();
   return NS_ERROR_FAILURE;
 }
 
@@ -1058,6 +1088,9 @@ nsSMILTimedElement::UnsetRepeatCount()
 nsresult
 nsSMILTimedElement::SetRepeatDur(const nsAString& aRepeatDurSpec)
 {
+  // Update the current interval before returning
+  AutoIntervalUpdater updater(*this);
+
   nsSMILTimeValue duration;
 
   const nsAString& repeatDur =
@@ -1073,7 +1106,6 @@ nsSMILTimedElement::SetRepeatDur(const nsAString& aRepeatDurSpec)
   }
 
   mRepeatDur = duration;
-  UpdateCurrentInterval();
 
   return NS_OK;
 }

content/smil/nsSMILTimedElement.h

@@ -616,6 +616,9 @@ protected:
   bool mDoDeferredUpdate; // Set if an update to the current interval was
                           // requested while mDeferIntervalUpdates was set
 
+  // Stack-based helper class to call UpdateCurrentInterval when it is destroyed
+  class AutoIntervalUpdater;
+
   // Recursion depth checking
   uint8_t              mDeleteCount;
   uint8_t              mUpdateIntervalRecursionDepth;

content/smil/test/mochitest.ini

@@ -32,6 +32,7 @@ support-files =
 [test_smilGetSimpleDuration.xhtml]
 [test_smilGetStartTime.xhtml]
 [test_smilHyperlinking.xhtml]
+[test_smilInvalidValues.html]
 [test_smilKeySplines.xhtml]
 [test_smilKeyTimes.xhtml]
 [test_smilKeyTimesPacedMode.xhtml]

content/smil/test/test_smilInvalidValues.html

@@ -0,0 +1,113 @@
+<!doctype html>
+<html>
+<!--
+https://bugzilla.mozilla.org/show_bug.cgi?id=941315
+-->
+<head>
+  <meta charset="utf-8">
+  <title>Test invalid values cause the model to be updated (bug 941315)</title>
+  <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" />
+</head>
+<body>
+<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=941315">Mozilla Bug 941315</a>
+<p id="display"></p>
+<div id="content" style="display: none">
+<svg width="100%" height="1" onload="this.pauseAnimations()">
+  <rect>
+    <animate id="a" dur="100s"/>
+    <animate id="b" dur="5s" begin="a.end"/>
+  </rect>
+  <circle cx="-100" cy="20" r="15" fill="blue" id="circle"/>
+</svg>
+</div>
+<pre id="test">
+<script class="testbody" type="text/javascript">
+  var a = $('a'),
+      b = $('b');
+
+  // Animation doesn't start until onload
+  SimpleTest.waitForExplicitFinish();
+  window.addEventListener("load", runTests, false);
+
+  // Make testing getStartTime easier
+  SVGAnimationElement.prototype.safeGetStartTime = function() {
+    try {
+      return this.getStartTime();
+    } catch(e) {
+      if (e.name == "InvalidStateError" &&
+          e.code == DOMException.INVALID_STATE_ERR) {
+        return 'none';
+      } else {
+        ok(false, "Unexpected exception: " + e);
+        return null;
+      }
+    }
+  };
+
+  function runTests() {
+    [testSimpleDuration, testMin, testMax, testRepeatDur, testRepeatCount]
+      .forEach(function(test) {
+        ise(b.getStartTime(), 100, "initial state before running " + test.name);
+        test();
+        ise(b.getStartTime(), 100, "final state after running " + test.name);
+      });
+    SimpleTest.finish();
+  }
+
+  function testSimpleDuration() {
+    // Verify a valid value updates as expected
+    a.setAttribute("dur", "50s");
+    ise(b.safeGetStartTime(), 50, "valid simple duration");
+
+    // Check an invalid value also causes the model to be updated
+    a.setAttribute("dur", "abc"); // -> indefinite
+    ise(b.safeGetStartTime(), "none", "invalid simple duration");
+
+    // Restore state
+    a.setAttribute("dur", "100s");
+  }
+
+  function testMin() {
+    a.setAttribute("min", "200s");
+    ise(b.safeGetStartTime(), 200, "valid min duration");
+
+    a.setAttribute("min", "abc"); // -> indefinite
+    ise(b.safeGetStartTime(), 100, "invalid min duration");
+
+    a.removeAttribute("min");
+  }
+
+  function testMax() {
+    a.setAttribute("max", "50s");
+    ise(b.safeGetStartTime(), 50, "valid max duration");
+
+    a.setAttribute("max", "abc"); // -> indefinite
+    ise(b.safeGetStartTime(), 100, "invalid max duration");
+
+    a.removeAttribute("max");
+  }
+
+  function testRepeatDur() {
+    a.setAttribute("repeatDur", "200s");
+    ise(b.safeGetStartTime(), 200, "valid repeatDur duration");
+
+    a.setAttribute("repeatDur", "abc"); // -> indefinite
+    ise(b.safeGetStartTime(), 100, "invalid repeatDur duration");
+
+    a.removeAttribute("repeatDur");
+  }
+
+  function testRepeatCount() {
+    a.setAttribute("repeatCount", "2");
+    ise(b.safeGetStartTime(), 200, "valid repeatCount duration");
+
+    a.setAttribute("repeatCount", "abc"); // -> indefinite
+    ise(b.safeGetStartTime(), 100, "invalid repeatCount duration");
+
+    a.removeAttribute("repeatCount");
+  }
+</script>
+</pre>
+</body>
+</html>