Bug 554202 - SVG SMIL: Fix crash due to infinite recursion negotiating cyclic dependencies. r=roc

content/smil/crashtests/554202-1.svg

@@ -0,0 +1,31 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<svg xmlns="http://www.w3.org/2000/svg">
+
+<bindings xmlns="http://www.mozilla.org/xbl">
+  <binding id="foo">
+    <content>
+      <animate xmlns="http://www.w3.org/2000/svg" begin="a.begin">
+        <children xmlns="http://www.mozilla.org/xbl"/>
+      </animate>
+    </content>
+  </binding>
+</bindings>
+
+<animate id="a" begin="b.begin; 3s"/>
+<animate id="b" begin="a.begin"/>
+
+<script type="text/javascript">
+
+function boom()
+{
+  document.documentElement.pauseAnimations();
+  document.documentElement.setCurrentTime(0);
+  document.getElementById('a').beginElementAt(1);
+  document.documentElement.style.MozBinding = 'url(#foo)';
+}
+
+window.addEventListener("load", boom, false);
+
+</script>
+
+</svg>

content/smil/crashtests/554202-2.svg

@@ -0,0 +1,19 @@
+<svg xmlns="http://www.w3.org/2000/svg"
+     onload="
+        document.documentElement.pauseAnimations();
+        document.documentElement.setCurrentTime(0);
+        document.getElementById('a').beginElementAt(1);
+        document.documentElement.setCurrentTime(2)">
+  <!--
+    This test case sets up a cycle between simultaneous instance times such that
+    when the instance times are sorted if this cycle is not detected we will
+    crash.
+  -->
+  <rect width="100" height="100" fill="red">
+    <set attributeName="fill" to="blue" begin="a.begin" dur="4s"/>
+    <set attributeName="fill" to="green" id="a"
+      begin="b.begin; 3s" dur="4s"/>
+    <set attributeName="fill" to="red" id="b"
+      begin="a.begin" dur="4s"/>
+  </rect>
+</svg>

content/smil/crashtests/crashtests.list

@@ -10,6 +10,8 @@ load 537157-1.svg
 load 541297-1.svg
 load 547333-1.svg
 load 548899-1.svg
+load 554202-1.svg
+load 554202-2.svg
 load 554141-1.svg
 load 555026-1.svg
 load 556841-1.svg

content/smil/nsSMILInstanceTime.cpp

@@ -163,11 +163,10 @@ nsSMILInstanceTime::HandleDeletedInterval()
 }
 
 PRBool
-nsSMILInstanceTime::IsDependent(const nsSMILInstanceTime& aOther,
+nsSMILInstanceTime::IsDependent(const nsSMILInstanceTime& aOther) const
-                                PRUint32 aRecursionDepth) const
 {
-  NS_ABORT_IF_FALSE(aRecursionDepth < 1000,
+  if (mVisited || mChainEnd)
-      "We seem to have created a cycle between instance times");
+    return PR_FALSE;
 
   const nsSMILInstanceTime* myBaseTime = GetBaseTime();
   if (!myBaseTime)
@@ -176,7 +175,9 @@ nsSMILInstanceTime::IsDependent(const nsSMILInstanceTime& aOther,
   if (myBaseTime == &aOther)
     return PR_TRUE;
 
-  return myBaseTime->IsDependent(aOther, ++aRecursionDepth);
+  // mVisited is mutable
+  AutoBoolSetter setVisited(const_cast<nsSMILInstanceTime*>(this)->mVisited);
+  return myBaseTime->IsDependent(aOther);
 }
 
 void
@@ -185,8 +186,6 @@ nsSMILInstanceTime::SetBaseInterval(nsSMILInterval* aBaseInterval)
   NS_ABORT_IF_FALSE(!mBaseInterval,
       "Attempting to reassociate an instance time with a different interval.");
 
-  // Make sure we don't end up creating a cycle between the dependent time
-  // pointers.
   if (aBaseInterval) {
     NS_ABORT_IF_FALSE(mCreator,
         "Attempting to create a dependent instance time without reference "
@@ -194,10 +193,6 @@ nsSMILInstanceTime::SetBaseInterval(nsSMILInterval* aBaseInterval)
     if (!mCreator)
       return;
 
-    const nsSMILInstanceTime* dependentTime = mCreator->DependsOnBegin() ?
-                                              aBaseInterval->Begin() :
-                                              aBaseInterval->End();
-    dependentTime->BreakPotentialCycle(this);
     aBaseInterval->AddDependentTime(*this);
   }
 
@@ -219,21 +214,3 @@ nsSMILInstanceTime::GetBaseTime() const
   return mCreator->DependsOnBegin() ? mBaseInterval->Begin() :
                                       mBaseInterval->End();
 }
-
-void
-nsSMILInstanceTime::BreakPotentialCycle(
-    const nsSMILInstanceTime* aNewTail) const
-{
-  const nsSMILInstanceTime* myBaseTime = GetBaseTime();
-  if (!myBaseTime)
-    return;
-
-  if (myBaseTime == aNewTail) {
-    // Making aNewTail the new tail of the chain would create a cycle so we
-    // prevent this by unlinking the pointer to aNewTail.
-    mBaseInterval->RemoveDependentTime(*this);
-    return;
-  }
-
-  myBaseTime->BreakPotentialCycle(aNewTail);
-}

content/smil/nsSMILInstanceTime.h

@@ -109,8 +109,7 @@ public:
     mTime = aNewTime;
   }
 
-  PRBool IsDependent(const nsSMILInstanceTime& aOther,
+  PRBool IsDependent(const nsSMILInstanceTime& aOther) const;
-                     PRUint32 aRecursionDepth = 0) const;
 
   PRBool SameTimeAndBase(const nsSMILInstanceTime& aOther) const
   {
@@ -126,7 +125,6 @@ public:
 
 protected:
   void SetBaseInterval(nsSMILInterval* aBaseInterval);
-  void BreakPotentialCycle(const nsSMILInstanceTime* aNewTail) const;
   const nsSMILInstanceTime* GetBaseTime() const;
 
   nsSMILTimeValue mTime;
@@ -155,8 +153,11 @@ protected:
   PRUint32      mSerial; // A serial number used by the containing class to
                          // specify the sort order for instance times with the
                          // same mTime.
-  PRPackedBool  mVisited;
+  PRPackedBool  mVisited; // (mutable) Cycle tracking
-  PRPackedBool  mChainEnd;
+  PRPackedBool  mChainEnd; // Flag to indicate that this instance time is part
+                           // of some cyclic dependency and that in order to
+                           // avoid infinite recursion the cycle should not be
+                           // followed any further than this point.
 
   nsSMILTimeValueSpec* mCreator; // The nsSMILTimeValueSpec object that created
                                  // us. (currently only needed for syncbase

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

@@ -83,6 +83,7 @@
 == sandwich-priority-9.svg green-box-ref.svg
 == sandwich-priority-10.svg green-box-ref.svg
 == sandwich-priority-11.svg green-box-ref.svg
+== sandwich-priority-12.svg green-box-ref.svg
 
 # Cross-time container dependencies
 == cross-container-1.xhtml green-box-ref.xhtml

layout/reftests/svg/smil/syncbase/sandwich-priority-12.svg

@@ -0,0 +1,24 @@
+<svg xmlns="http://www.w3.org/2000/svg"
+     xmlns:xlink="http://www.w3.org/1999/xlink"
+     class="reftest-wait"
+     onload="
+        document.documentElement.pauseAnimations();
+        document.documentElement.setCurrentTime(0);
+        document.getElementById('a').beginElementAt(1);
+        setTimeAndSnapshot(2, false)">
+  <script xlink:href="../smil-util.js" type="text/javascript"/>
+  <!--
+    Test of animation sandwich priority based on syncbase dependencies.
+
+    This case includes a complex cycle that should nevertheless produce
+    consistent results.
+
+    If this fails, it will fail intermittently. The test is not so much
+    concerned with which colour should win (there are other tests for that) but
+    simply with determinancy.
+   -->
+  <rect width="100" height="100" fill="orange">
+    <set attributeName="fill" id="a" to="green" begin="b.begin; 3s"/>
+    <set attributeName="fill" id="b" to="red" begin="a.begin"/>
+  </rect>
+</svg>