Bug 663288 - Don't allow instance times to be self-dependent, r=dholbert

content/smil/nsSMILInstanceTime.cpp

@@ -228,6 +228,22 @@ nsSMILInstanceTime::IsDependentOn(const nsSMILInstanceTime& aOther) const
   return myBaseTime->IsDependentOn(aOther);
 }
 
+const nsSMILInstanceTime*
+nsSMILInstanceTime::GetBaseTime() const
+{
+  if (!mBaseInterval) {
+    return nsnull;
+  }
+
+  NS_ABORT_IF_FALSE(mCreator, "Base interval is set but there is no creator.");
+  if (!mCreator) {
+    return nsnull;
+  }
+
+  return mCreator->DependsOnBegin() ? mBaseInterval->Begin() :
+                                      mBaseInterval->End();
+}
+
 void
 nsSMILInstanceTime::SetBaseInterval(nsSMILInterval* aBaseInterval)
 {
@@ -246,19 +262,3 @@ nsSMILInstanceTime::SetBaseInterval(nsSMILInterval* aBaseInterval)
 
   mBaseInterval = aBaseInterval;
 }
-
-const nsSMILInstanceTime*
-nsSMILInstanceTime::GetBaseTime() const
-{
-  if (!mBaseInterval) {
-    return nsnull;
-  }
-
-  NS_ABORT_IF_FALSE(mCreator, "Base interval is set but there is no creator.");
-  if (!mCreator) {
-    return nsnull;
-  }
-
-  return mCreator->DependsOnBegin() ? mBaseInterval->Begin() :
-                                      mBaseInterval->End();
-}

content/smil/nsSMILInstanceTime.h

@@ -117,6 +117,7 @@ public:
   PRBool IsDependent() const { return !!mBaseInterval; }
   PRBool IsDependentOn(const nsSMILInstanceTime& aOther) const;
   const nsSMILInterval* GetBaseInterval() const { return mBaseInterval; }
+  const nsSMILInstanceTime* GetBaseTime() const;
 
   PRBool SameTimeAndBase(const nsSMILInstanceTime& aOther) const
   {
@@ -132,7 +133,6 @@ public:
 
 protected:
   void SetBaseInterval(nsSMILInterval* aBaseInterval);
-  const nsSMILInstanceTime* GetBaseTime() const;
 
   nsSMILTimeValue mTime;
 

content/smil/nsSMILInterval.cpp

@@ -114,6 +114,11 @@ nsSMILInterval::SetBegin(nsSMILInstanceTime& aBegin)
       "Attempting to set unresolved begin time on interval");
   NS_ABORT_IF_FALSE(!mBeginFixed,
       "Attempting to set begin time but the begin point is fixed");
+  // Check that we're not making an instance time dependent on itself. Such an
+  // arrangement does not make intuitive sense and should be detected when
+  // creating or updating intervals.
+  NS_ABORT_IF_FALSE(!mBegin || aBegin.GetBaseTime() != mBegin,
+      "Attempting to make self-dependent instance time");
 
   mBegin = &aBegin;
 }
@@ -123,6 +128,10 @@ nsSMILInterval::SetEnd(nsSMILInstanceTime& aEnd)
 {
   NS_ABORT_IF_FALSE(!mEndFixed,
       "Attempting to set end time but the end point is fixed");
+  // As with SetBegin, check we're not making an instance time dependent on
+  // itself.
+  NS_ABORT_IF_FALSE(!mEnd || aEnd.GetBaseTime() != mEnd,
+      "Attempting to make self-dependent instance time");
 
   mEnd = &aEnd;
 }

content/smil/nsSMILTimedElement.cpp

@@ -511,7 +511,7 @@ nsSMILTimedElement::DoSampleAt(nsSMILTime aContainerTime, PRBool aEndOnly)
     case STATE_STARTUP:
       {
         nsSMILInterval firstInterval;
-        mElementState = GetNextInterval(nsnull, nsnull, firstInterval)
+        mElementState = GetNextInterval(nsnull, nsnull, nsnull, firstInterval)
          ? STATE_WAITING
          : STATE_POSTACTIVE;
         stateChanged = PR_TRUE;
@@ -558,7 +558,8 @@ nsSMILTimedElement::DoSampleAt(nsSMILTime aContainerTime, PRBool aEndOnly)
 
         if (mCurrentInterval->End()->Time() <= sampleTime) {
           nsSMILInterval newInterval;
-          mElementState = GetNextInterval(mCurrentInterval, nsnull, newInterval)
+          mElementState =
+            GetNextInterval(mCurrentInterval, nsnull, nsnull, newInterval)
             ? STATE_WAITING
             : STATE_POSTACTIVE;
           if (mClient) {
@@ -1497,6 +1498,7 @@ nsSMILTimedElement::FilterInstanceTimes(InstanceTimeList& aList)
 //
 PRBool
 nsSMILTimedElement::GetNextInterval(const nsSMILInterval* aPrevInterval,
+                                    const nsSMILInterval* aReplacedInterval,
                                     const nsSMILInstanceTime* aFixedBeginTime,
                                     nsSMILInterval& aResult) const
 {
@@ -1539,10 +1541,19 @@ nsSMILTimedElement::GetNextInterval(const nsSMILInterval* aPrevInterval,
       tempBegin = new nsSMILInstanceTime(nsSMILTimeValue(0));
     } else {
       PRInt32 beginPos = 0;
-      tempBegin = GetNextGreaterOrEqual(mBeginInstances, beginAfter, beginPos);
+      // If we're updating the current interval then skip any begin time that is
-      if (!tempBegin || !tempBegin->Time().IsResolved()) {
+      // dependent on the current interval's begin time. e.g.
-        return PR_FALSE;
+      //   <animate id="a" begin="b.begin; a.begin+2s"...
-      }
+      // If b's interval disappears whilst 'a' is in the waiting state the begin
+      // time at "a.begin+2s" should be skipped since 'a' never begun.
+      do {
+        tempBegin =
+          GetNextGreaterOrEqual(mBeginInstances, beginAfter, beginPos);
+        if (!tempBegin || !tempBegin->Time().IsResolved()) {
+          return PR_FALSE;
+        }
+      } while (aReplacedInterval &&
+               tempBegin->GetBaseTime() == aReplacedInterval->Begin());
     }
     NS_ABORT_IF_FALSE(tempBegin && tempBegin->Time().IsResolved() &&
         tempBegin->Time() >= beginAfter,
@@ -1551,7 +1562,14 @@ nsSMILTimedElement::GetNextInterval(const nsSMILInterval* aPrevInterval,
     // Calculate end time
     {
       PRInt32 endPos = 0;
-      tempEnd = GetNextGreaterOrEqual(mEndInstances, tempBegin->Time(), endPos);
+      // As above with begin times, avoid creating self-referential loops
+      // between instance times by checking that the newly found end instance
+      // time is not already dependent on the end of the current interval.
+      do {
+        tempEnd =
+          GetNextGreaterOrEqual(mEndInstances, tempBegin->Time(), endPos);
+      } while (tempEnd && aReplacedInterval &&
+               tempEnd->GetBaseTime() == aReplacedInterval->End());
 
       // If the last interval ended at the same point and was zero-duration and
       // this one is too, look for another end to use instead
@@ -1817,7 +1835,8 @@ nsSMILTimedElement::UpdateCurrentInterval(PRBool aForceChangeNotice)
                                       ? mCurrentInterval->Begin()
                                       : nsnull;
   nsSMILInterval updatedInterval;
-  if (GetNextInterval(GetPreviousInterval(), beginTime, updatedInterval)) {
+  if (GetNextInterval(GetPreviousInterval(), mCurrentInterval,
+                      beginTime, updatedInterval)) {
 
     if (mElementState == STATE_POSTACTIVE) {
 

content/smil/nsSMILTimedElement.h

@@ -472,6 +472,9 @@ protected:
    * @param aPrevInterval   The previous interval used. If supplied, the first
    *                        interval that begins after aPrevInterval will be
    *                        returned. May be nsnull.
+   * @param aReplacedInterval The interval that is being updated (if any). This
+   *                        used to ensure we don't return interval endpoints
+   *                        that are dependent on themselves. May be nsnull.
    * @param aFixedBeginTime The time to use for the start of the interval. This
    *                        is used when only the endpoint of the interval
    *                        should be updated such as when the animation is in
@@ -482,6 +485,7 @@ protected:
    * @return  PR_TRUE if a suitable interval was found, PR_FALSE otherwise.
    */
   PRBool            GetNextInterval(const nsSMILInterval* aPrevInterval,
+                                    const nsSMILInterval* aReplacedInterval,
                                     const nsSMILInstanceTime* aFixedBeginTime,
                                     nsSMILInterval& aResult) const;
   nsSMILInstanceTime* GetNextGreater(const InstanceTimeList& aList,

layout/reftests/svg/smil/syncbase/cycle-self-ref-4.svg

@@ -0,0 +1,32 @@
+<?xml version="1.0" encoding="UTF-8" standalone="no" ?>
+<svg xmlns="http://www.w3.org/2000/svg"
+     xmlns:xlink="http://www.w3.org/1999/xlink"
+     class="reftest-wait"
+     onload="byeByeB()">
+  <script xlink:href="../smil-util.js" type="text/javascript"/>
+  <script type="text/javascript">
+  function byeByeB()
+  {
+    document.documentElement.pauseAnimations();
+    document.documentElement.setCurrentTime(0);
+    // Drop b
+    var b = document.getElementById('b');
+    b.parentNode.removeChild(b);
+    b = null;
+    // Snapshot at time t=4s. This is because there are two possible error cases
+    // we want to detect:
+    // i) b disappears and we just keep the existing time for a.begin+1s of t=2s
+    // ii) b disappears and we update the time for a.begin+1s to t=3s
+    setTimeAndSnapshot(4, false);
+  }
+  </script>
+  <!-- We have an arrangement where a is dependent on b and itself. If b's
+       interval disappears while a is still in the waiting state then the begin
+       time "a.begin+1s" should disappear too since a never begun. -->
+  <rect width="100" height="100" fill="green">
+    <set id="a" attributeName="fill" attributeType="CSS" to="red"
+      begin="b.begin; a.begin+1s"/>
+    <set id="b" attributeName="y" attributeType="XML" to="0"
+      begin="1s"/>
+  </rect>
+</svg>

layout/reftests/svg/smil/syncbase/cycle-self-ref-5.svg

@@ -0,0 +1,56 @@
+<?xml version="1.0" encoding="UTF-8" standalone="no" ?>
+<svg xmlns="http://www.w3.org/2000/svg"
+     xmlns:xlink="http://www.w3.org/1999/xlink"
+     class="reftest-wait"
+     onload="byeByeB()">
+  <script xlink:href="../smil-util.js" type="text/javascript"/>
+  <script type="text/javascript">
+  function byeByeB()
+  {
+    document.documentElement.pauseAnimations();
+    document.documentElement.setCurrentTime(2.5);
+    // Drop b
+    var b = document.getElementById('b');
+    b.parentNode.removeChild(b);
+    b = null;
+    setTimeAndSnapshot(8, false);
+  }
+  </script>
+  <!-- Similar to cycle-self-ref-4.svg but with end times.
+
+       We have an arrangement where a's end time is dependent on b and on
+       itself.
+
+       Initially:
+       - a's end time will be resolved as "b.end", i.e. 3s.
+       - Accordingly, the instance time for "a.end+2s" will be 5s (3s+2s).
+       - i.e. a's list of end instance times will be: [3, 5, 9].
+
+       If b's interval disappears (because we delete b):
+       - The end time "b.end" will become unresolved.
+       - i.e. a's list of end instance times will be: [5, 9, unresolved].
+
+       However, when updating a's end time we should not use the "5s" instance
+       time since it is based on a's end time which is what we are updating.
+
+       Expected behaviour:
+       - The instance time of "5s" will be skipped and the time of "9s" will be
+         used instead.
+       - At t=8s the animation will still be playing and the rectangle will be
+         green.
+
+       Failure behaviour:
+       - The next end instance time in the list will be used, giving a an end
+         time of 5s.
+       - The time "a.end+2s" will then be accordingly updated to 7s since a's
+         end time is now 5s. (Any subsequent attempts to update the time will be
+         ignored according to SMIL's cycle detection rules.)
+       - At t=8s the animation will have stopped and the rectangle will be red.
+   -->
+  <rect width="100" height="100" fill="red">
+    <set id="a" attributeName="fill" attributeType="CSS" to="green"
+      begin="2s" end="b.end; a.end+2s; 9s"/>
+    <set id="b" attributeName="y" attributeType="XML" to="0"
+      begin="1s" end="3s"/>
+  </rect>
+</svg>

layout/reftests/svg/smil/syncbase/reftest.list

@@ -64,6 +64,8 @@
 == cycle-self-ref-1.svg green-box-ref.svg
 == cycle-self-ref-2.svg green-box-ref.svg
 == cycle-self-ref-3.svg green-box-ref.svg
+== cycle-self-ref-4.svg green-box-ref.svg
+== cycle-self-ref-5.svg green-box-ref.svg
 == cycle-invalid-1.svg green-box-ref.svg
 == cycle-invalid-2.svg green-box-ref.svg
 == cycle-invalid-3.svg green-box-ref.svg