Bug 650732 - Move interval change notifications into the timed element, r=dholbert

content/smil/nsSMILInstanceTime.cpp

@@ -128,8 +128,13 @@ nsSMILInstanceTime::HandleChangedInterval(
     PRBool aBeginObjectChanged,
     PRBool aEndObjectChanged)
 {
-  NS_ABORT_IF_FALSE(mBaseInterval,
+  // It's possible a sequence of notifications might cause our base interval to
-      "Got call to HandleChangedInterval on an independent instance time.");
+  // be updated and then deleted. Furthermore, the delete might happen whilst
+  // we're still in the queue to be notified of the change. In any case, if we
+  // don't have a base interval, just ignore the change.
+  if (!mBaseInterval)
+    return;
+
   NS_ABORT_IF_FALSE(mCreator, "Base interval is set but creator is not.");
 
   if (mVisited) {

content/smil/nsSMILInterval.cpp

@@ -40,9 +40,7 @@
 nsSMILInterval::nsSMILInterval()
 :
   mBeginFixed(PR_FALSE),
-  mEndFixed(PR_FALSE),
+  mEndFixed(PR_FALSE)
-  mBeginObjectChanged(PR_FALSE),
-  mEndObjectChanged(PR_FALSE)
 {
 }
 
@@ -51,9 +49,7 @@ nsSMILInterval::nsSMILInterval(const nsSMILInterval& aOther)
   mBegin(aOther.mBegin),
   mEnd(aOther.mEnd),
   mBeginFixed(PR_FALSE),
-  mEndFixed(PR_FALSE),
+  mEndFixed(PR_FALSE)
-  mBeginObjectChanged(PR_FALSE),
-  mEndObjectChanged(PR_FALSE)
 {
   NS_ABORT_IF_FALSE(aOther.mDependentTimes.IsEmpty(),
       "Attempting to copy-construct an interval with dependent times, "
@@ -74,18 +70,6 @@ nsSMILInterval::~nsSMILInterval()
       "Unlink was not called");
 }
 
-void
-nsSMILInterval::NotifyChanged(const nsSMILTimeContainer* aContainer)
-{
-  for (PRInt32 i = mDependentTimes.Length() - 1; i >= 0; --i) {
-    mDependentTimes[i]->HandleChangedInterval(aContainer,
-                                              mBeginObjectChanged,
-                                              mEndObjectChanged);
-  }
-  mBeginObjectChanged = PR_FALSE;
-  mEndObjectChanged = PR_FALSE;
-}
-
 void
 nsSMILInterval::Unlink(PRBool aFiltered)
 {
@@ -131,11 +115,7 @@ nsSMILInterval::SetBegin(nsSMILInstanceTime& aBegin)
   NS_ABORT_IF_FALSE(!mBeginFixed,
       "Attempting to set begin time but the begin point is fixed");
 
-  if (mBegin == &aBegin)
-    return;
-
   mBegin = &aBegin;
-  mBeginObjectChanged = PR_TRUE;
 }
 
 void
@@ -144,11 +124,7 @@ nsSMILInterval::SetEnd(nsSMILInstanceTime& aEnd)
   NS_ABORT_IF_FALSE(!mEndFixed,
       "Attempting to set end time but the end point is fixed");
 
-  if (mEnd == &aEnd)
-    return;
-
   mEnd = &aEnd;
-  mEndObjectChanged = PR_TRUE;
 }
 
 void
@@ -193,6 +169,12 @@ nsSMILInterval::RemoveDependentTime(const nsSMILInstanceTime& aTime)
   NS_ABORT_IF_FALSE(found, "Couldn't find instance time to delete.");
 }
 
+void
+nsSMILInterval::GetDependentTimes(InstanceTimeList& aTimes)
+{
+  aTimes = mDependentTimes;
+}
+
 PRBool
 nsSMILInterval::IsDependencyChainLink() const
 {

content/smil/nsSMILInterval.h

@@ -56,7 +56,6 @@ public:
   nsSMILInterval();
   nsSMILInterval(const nsSMILInterval& aOther);
   ~nsSMILInterval();
-  void NotifyChanged(const nsSMILTimeContainer* aContainer);
   void Unlink(PRBool aFiltered = PR_FALSE);
 
   const nsSMILInstanceTime* Begin() const
@@ -86,8 +85,11 @@ public:
   void FixBegin();
   void FixEnd();
 
+  typedef nsTArray<nsRefPtr<nsSMILInstanceTime> > InstanceTimeList;
+
   void AddDependentTime(nsSMILInstanceTime& aTime);
   void RemoveDependentTime(const nsSMILInstanceTime& aTime);
+  void GetDependentTimes(InstanceTimeList& aTimes);
 
   // Cue for assessing if this interval can be filtered
   PRBool IsDependencyChainLink() const;
@@ -96,8 +98,6 @@ private:
   nsRefPtr<nsSMILInstanceTime> mBegin;
   nsRefPtr<nsSMILInstanceTime> mEnd;
 
-  typedef nsTArray<nsRefPtr<nsSMILInstanceTime> > InstanceTimeList;
-
   // nsSMILInstanceTimes to notify when this interval is changed or deleted.
   InstanceTimeList mDependentTimes;
 
@@ -112,21 +112,6 @@ private:
   // OBJECT returned for that end point and its TIME value will not change.
   PRPackedBool mBeginFixed;
   PRPackedBool mEndFixed;
-
-  // When change notifications are passed around the timing model we try to
-  // filter out all changes where there is no observable difference to an
-  // instance time. Changes that may produce an observable difference are:
-  //
-  // * Changes to the time of an interval endpoint
-  // * Changes in the relative times of different time containers
-  // * Changes to the dependency chain (which may affect the animation sandwich)
-  //
-  // The nsSMILTimeValueSpec can detect the first two changes by recalculating
-  // the time but in order to help detect the third change we simply set a flag
-  // whenever the mBegin or mEnd pointers are changed. These flags are reset
-  // when the next change notification is sent.
-  PRPackedBool mBeginObjectChanged;
-  PRPackedBool mEndObjectChanged;
 };
 
 #endif // NS_SMILINTERVAL_H_

content/smil/nsSMILTimedElement.cpp

@@ -551,7 +551,10 @@ nsSMILTimedElement::DoSampleAt(nsSMILTime aContainerTime, PRBool aEndOnly)
 
     case STATE_ACTIVE:
       {
-        ApplyEarlyEnd(sampleTime);
+        // Ending early will change the interval but we don't notify dependents
+        // of the change until we have closed off the current interval (since we
+        // don't want dependencies to un-end our early end).
+        PRBool didApplyEarlyEnd = ApplyEarlyEnd(sampleTime);
 
         if (mCurrentInterval->End()->Time() <= sampleTime) {
           nsSMILInterval newInterval;
@@ -567,15 +570,23 @@ nsSMILTimedElement::DoSampleAt(nsSMILTime aContainerTime, PRBool aEndOnly)
           }
           mCurrentRepeatIteration = 0;
           mOldIntervals.AppendElement(mCurrentInterval.forget());
-          // We must update mOldIntervals before calling SampleFillValue
           SampleFillValue();
           if (mElementState == STATE_WAITING) {
             mCurrentInterval = new nsSMILInterval(newInterval);
+          }
+          // We are now in a consistent state to dispatch notifications
+          if (didApplyEarlyEnd) {
+            NotifyChangedInterval(
+                mOldIntervals[mOldIntervals.Length() - 1], PR_FALSE, PR_TRUE);
+          }
+          if (mElementState == STATE_WAITING) {
             NotifyNewInterval();
           }
           FilterHistory();
           stateChanged = PR_TRUE;
         } else {
+          NS_ABORT_IF_FALSE(!didApplyEarlyEnd,
+              "We got an early end, but didn't end");
           nsSMILTime beginTime = mCurrentInterval->Begin()->Time().GetMillis();
           NS_ASSERTION(aContainerTime >= beginTime,
                        "Sample time should not precede current interval");
@@ -626,7 +637,7 @@ nsSMILTimedElement::HandleContainerTimeChange()
   // the nsSMILTimeValueSpec we'll check if anything has changed and if not, we
   // won't go any further.
   if (mElementState == STATE_WAITING || mElementState == STATE_ACTIVE) {
-    NotifyChangedInterval();
+    NotifyChangedInterval(mCurrentInterval, PR_FALSE, PR_FALSE);
   }
 }
 
@@ -1237,13 +1248,15 @@ nsSMILTimedElement::ClearIntervalProgress()
   mOldIntervals.Clear();
 }
 
-void
+PRBool
 nsSMILTimedElement::ApplyEarlyEnd(const nsSMILTimeValue& aSampleTime)
 {
   // This should only be called within DoSampleAt as a helper function
   NS_ABORT_IF_FALSE(mElementState == STATE_ACTIVE,
       "Unexpected state to try to apply an early end");
 
+  PRBool updated = PR_FALSE;
+
   // Only apply an early end if we're not already ending.
   if (mCurrentInterval->End()->Time() > aSampleTime) {
     nsSMILInstanceTime* earlyEnd = CheckForEarlyEnd(aSampleTime);
@@ -1258,9 +1271,10 @@ nsSMILTimedElement::ApplyEarlyEnd(const nsSMILTimeValue& aSampleTime)
       } else {
         mCurrentInterval->SetEnd(*earlyEnd);
       }
-      NotifyChangedInterval();
+      updated = PR_TRUE;
     }
   }
+  return updated;
 }
 
 namespace
@@ -1815,22 +1829,23 @@ nsSMILTimedElement::UpdateCurrentInterval(PRBool aForceChangeNotice)
 
     } else {
 
-      PRBool changed = PR_FALSE;
+      PRBool beginChanged = PR_FALSE;
+      PRBool endChanged   = PR_FALSE;
 
       if (mElementState != STATE_ACTIVE &&
           !updatedInterval.Begin()->SameTimeAndBase(
             *mCurrentInterval->Begin())) {
         mCurrentInterval->SetBegin(*updatedInterval.Begin());
-        changed = PR_TRUE;
+        beginChanged = PR_TRUE;
       }
 
       if (!updatedInterval.End()->SameTimeAndBase(*mCurrentInterval->End())) {
         mCurrentInterval->SetEnd(*updatedInterval.End());
-        changed = PR_TRUE;
+        endChanged = PR_TRUE;
       }
 
-      if (changed || aForceChangeNotice) {
+      if (beginChanged || endChanged || aForceChangeNotice) {
-        NotifyChangedInterval();
+        NotifyChangedInterval(mCurrentInterval, beginChanged, endChanged);
       }
     }
 
@@ -1844,7 +1859,7 @@ nsSMILTimedElement::UpdateCurrentInterval(PRBool aForceChangeNotice)
       if (!mCurrentInterval->End()->SameTimeAndBase(*mCurrentInterval->Begin()))
       {
         mCurrentInterval->SetEnd(*mCurrentInterval->Begin());
-        NotifyChangedInterval();
+        NotifyChangedInterval(mCurrentInterval, PR_FALSE, PR_TRUE);
       }
       // The transition to the postactive state will take place on the next
       // sample (along with firing end events, clearing intervals etc.)
@@ -2023,18 +2038,27 @@ nsSMILTimedElement::NotifyNewInterval()
 }
 
 void
-nsSMILTimedElement::NotifyChangedInterval()
+nsSMILTimedElement::NotifyChangedInterval(nsSMILInterval* aInterval,
+                                          PRBool aBeginObjectChanged,
+                                          PRBool aEndObjectChanged)
 {
-  NS_ABORT_IF_FALSE(mCurrentInterval,
+  NS_ABORT_IF_FALSE(aInterval, "Null interval for change notification");
-      "Attempting to notify dependents of a changed interval but the interval "
-      "is not set--perhaps we should be deleting the interval instead?");
 
   nsSMILTimeContainer* container = GetTimeContainer();
   if (container) {
     container->SyncPauseTime();
   }
 
-  mCurrentInterval->NotifyChanged(container);
+  // Copy the instance times list since notifying the instance times can result
+  // in a chain reaction whereby our own interval gets deleted along with its
+  // instance times.
+  InstanceTimeList times;
+  aInterval->GetDependentTimes(times);
+
+  for (PRUint32 i = 0; i < times.Length(); ++i) {
+    times[i]->HandleChangedInterval(container, aBeginObjectChanged,
+                                    aEndObjectChanged);
+  }
 }
 
 void

content/smil/nsSMILTimedElement.h

@@ -423,8 +423,10 @@ protected:
    *                    applied at the last possible moment (i.e. if they are at
    *                    or before the current sample time) and only if the
    *                    current interval is not already ending.
+   * @return PR_TRUE if the end time of the current interval was updated,
+   *         PR_FALSE otherwise.
    */
-  void ApplyEarlyEnd(const nsSMILTimeValue& aSampleTime);
+  PRBool ApplyEarlyEnd(const nsSMILTimeValue& aSampleTime);
 
   /**
    * Clears certain state in response to the element restarting.
@@ -504,8 +506,17 @@ protected:
   void              RegisterMilestone();
   PRBool            GetNextMilestone(nsSMILMilestone& aNextMilestone) const;
 
+  // Notification methods. Note that these notifications can result in nested
+  // calls to this same object. Therefore,
+  // (i)  we should not perform notification until this object is in
+  //      a consistent state to receive callbacks, and
+  // (ii) after calling these methods we must assume that the state of the
+  //      element may have changed.
   void              NotifyNewInterval();
-  void              NotifyChangedInterval();
+  void              NotifyChangedInterval(nsSMILInterval* aInterval,
+                                          PRBool aBeginObjectChanged,
+                                          PRBool aEndObjectChanged);
+
   void              FireTimeEventAsync(PRUint32 aMsg, PRInt32 aDetail);
   const nsSMILInstanceTime* GetEffectiveBeginInstance() const;
   const nsSMILInterval* GetPreviousInterval() const;