Bug 554202 - SVG SMIL: Fix crash due to infinite recursion negotiating cyclic dependencies. r=roc
authorBrian Birtles <birtles@gmail.com>
Fri, 28 May 2010 21:43:17 +0900
changeset 42895 9c1def95d41c2ca5f73b4ced62593f8a165e9a44
parent 42894 ae76b15954794dc589a772b8f347503278dd8be6
child 42896 381d95a8f5bac023166ebbe20a3b9c3f98b7f748
push idunknown
push userunknown
push dateunknown
reviewersroc
bugs554202
milestone1.9.3a5pre
Bug 554202 - SVG SMIL: Fix crash due to infinite recursion negotiating cyclic dependencies. r=roc
content/smil/crashtests/554202-1.svg
content/smil/crashtests/554202-2.svg
content/smil/crashtests/crashtests.list
content/smil/nsSMILInstanceTime.cpp
content/smil/nsSMILInstanceTime.h
layout/reftests/svg/smil/syncbase/reftest.list
layout/reftests/svg/smil/syncbase/sandwich-priority-12.svg
new file mode 100644
--- /dev/null
+++ b/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>
new file mode 100644
--- /dev/null
+++ b/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>
--- a/content/smil/crashtests/crashtests.list
+++ b/content/smil/crashtests/crashtests.list
@@ -5,11 +5,13 @@ load 525099-1.svg
 load 526536-1.svg
 load 526875-1.svg
 load 526875-2.svg
 load 529387-1.xhtml
 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
--- a/content/smil/nsSMILInstanceTime.cpp
+++ b/content/smil/nsSMILInstanceTime.cpp
@@ -158,51 +158,46 @@ nsSMILInstanceTime::HandleDeletedInterva
   mBaseInterval = nsnull;
 
   nsRefPtr<nsSMILInstanceTime> deathGrip(this);
   mCreator->HandleDeletedInstanceTime(*this);
   mCreator = nsnull;
 }
 
 PRBool
-nsSMILInstanceTime::IsDependent(const nsSMILInstanceTime& aOther,
-                                PRUint32 aRecursionDepth) const
+nsSMILInstanceTime::IsDependent(const nsSMILInstanceTime& aOther) const
 {
-  NS_ABORT_IF_FALSE(aRecursionDepth < 1000,
-      "We seem to have created a cycle between instance times");
+  if (mVisited || mChainEnd)
+    return PR_FALSE;
 
   const nsSMILInstanceTime* myBaseTime = GetBaseTime();
   if (!myBaseTime)
     return PR_FALSE;
 
   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
 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 "
         "to the creating nsSMILTimeValueSpec object.");
     if (!mCreator)
       return;
 
-    const nsSMILInstanceTime* dependentTime = mCreator->DependsOnBegin() ?
-                                              aBaseInterval->Begin() :
-                                              aBaseInterval->End();
-    dependentTime->BreakPotentialCycle(this);
     aBaseInterval->AddDependentTime(*this);
   }
 
   mBaseInterval = aBaseInterval;
 }
 
 const nsSMILInstanceTime*
 nsSMILInstanceTime::GetBaseTime() const
@@ -214,26 +209,8 @@ nsSMILInstanceTime::GetBaseTime() const
   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::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);
-}
--- a/content/smil/nsSMILInstanceTime.h
+++ b/content/smil/nsSMILInstanceTime.h
@@ -104,34 +104,32 @@ public:
 
   void DependentUpdate(const nsSMILTimeValue& aNewTime)
   {
     NS_ABORT_IF_FALSE(MayUpdate(),
         "Updating an instance time that is not expected to be updated");
     mTime = aNewTime;
   }
 
-  PRBool IsDependent(const nsSMILInstanceTime& aOther,
-                     PRUint32 aRecursionDepth = 0) const;
+  PRBool IsDependent(const nsSMILInstanceTime& aOther) const;
 
   PRBool SameTimeAndBase(const nsSMILInstanceTime& aOther) const
   {
     return mTime == aOther.mTime && GetBaseTime() == aOther.GetBaseTime();
   }
 
   // Get and set a serial number which may be used by a containing class to
   // control the sort order of otherwise similar instance times.
   PRUint32 Serial() const { return mSerial; }
   void SetSerial(PRUint32 aIndex) { mSerial = aIndex; }
 
   NS_INLINE_DECL_REFCOUNTING(nsSMILInstanceTime)
 
 protected:
   void SetBaseInterval(nsSMILInterval* aBaseInterval);
-  void BreakPotentialCycle(const nsSMILInstanceTime* aNewTail) const;
   const nsSMILInstanceTime* GetBaseTime() const;
 
   nsSMILTimeValue mTime;
 
   // Internal flags used to represent the behaviour of different instance times
   enum {
     // Indicates if this instance time should be removed when the owning timed
     // element is reset. True for events and DOM calls.
@@ -150,18 +148,21 @@ protected:
     // that attribute (and hence an nsSMILTimeValueSpec), but not those from the
     // DOM.
     kFromDOM = 4
   };
   PRUint8       mFlags; // Combination of kClearOnReset, kMayUpdate, etc.
   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  mChainEnd;
+  PRPackedBool  mVisited; // (mutable) Cycle tracking
+  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
                                  // instance times.)
   nsSMILInterval* mBaseInterval; // Interval from which this time is derived
                                  // (only used for syncbase instance times)
 };
 
--- a/layout/reftests/svg/smil/syncbase/reftest.list
+++ b/layout/reftests/svg/smil/syncbase/reftest.list
@@ -78,13 +78,14 @@
 == sandwich-priority-4.svg green-box-ref.svg
 == sandwich-priority-5.svg green-box-ref.svg
 == sandwich-priority-6.svg green-box-ref.svg
 == sandwich-priority-7.svg green-box-ref.svg
 == sandwich-priority-8.svg green-box-ref.svg
 == 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
 == cross-container-2.xhtml green-box-ref.xhtml
 == cross-container-3.xhtml green-box-ref.xhtml
new file mode 100644
--- /dev/null
+++ b/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>