Bug 1223445 - KeyframeEffectReadOnly objects end up keeping lots of other objects alive too long, r=birtles

--HG--
extra : rebase_source : df6828ce26e22e53b632f25952fd1741a693c36a

dom/animation/Animation.cpp

@@ -601,6 +601,10 @@ Animation::DoCancel()
   mStartTime.SetNull();
 
   UpdateTiming(SeekFlag::NoSeek, SyncNotifyFlag::Async);
+
+  if (mTimeline) {
+    mTimeline->RemoveAnimation(this);
+  }
 }
 
 void

dom/animation/Animation.h

@@ -10,6 +10,7 @@
 #include "nsWrapperCache.h"
 #include "nsCycleCollectionParticipant.h"
 #include "mozilla/Attributes.h"
+#include "mozilla/LinkedList.h"
 #include "mozilla/TimeStamp.h" // for TimeStamp, TimeDuration
 #include "mozilla/dom/AnimationBinding.h" // for AnimationPlayState
 #include "mozilla/dom/AnimationTimeline.h" // for AnimationTimeline
@@ -48,6 +49,7 @@ class CSSTransition;
 
 class Animation
   : public DOMEventTargetHelper
+  , public LinkedListElement<Animation>
 {
 protected:
   virtual ~Animation() {}

dom/animation/AnimationTimeline.cpp

@@ -6,12 +6,25 @@
 
 #include "AnimationTimeline.h"
 #include "mozilla/AnimationComparator.h"
+#include "mozilla/dom/Animation.h"
 
 namespace mozilla {
 namespace dom {
 
-NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE(AnimationTimeline, mWindow,
-                                      mAnimationOrder)
+NS_IMPL_CYCLE_COLLECTION_CLASS(AnimationTimeline)
+
+NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(AnimationTimeline)
+  tmp->mAnimationOrder.clear();
+  NS_IMPL_CYCLE_COLLECTION_UNLINK(mWindow, mAnimations)
+  NS_IMPL_CYCLE_COLLECTION_UNLINK_PRESERVED_WRAPPER
+NS_IMPL_CYCLE_COLLECTION_UNLINK_END
+
+NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(AnimationTimeline)
+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mWindow, mAnimations)
+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_SCRIPT_OBJECTS
+NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END
+
+NS_IMPL_CYCLE_COLLECTION_TRACE_WRAPPERCACHE(AnimationTimeline)
 
 NS_IMPL_CYCLE_COLLECTING_ADDREF(AnimationTimeline)
 NS_IMPL_CYCLE_COLLECTING_RELEASE(AnimationTimeline)
@@ -32,9 +45,10 @@ AnimationTimeline::GetAnimations(AnimationSequence& aAnimations)
     }
   }
 
-  aAnimations.SetCapacity(mAnimationOrder.Length());
+  aAnimations.SetCapacity(mAnimations.Count());
 
-  for (Animation* animation : mAnimationOrder) {
+  for (Animation* animation = mAnimationOrder.getFirst(); animation;
+       animation = animation->getNext()) {
 
     // Skip animations which are no longer relevant or which have been
     // associated with another timeline. These animations will be removed
@@ -69,8 +83,22 @@ AnimationTimeline::NotifyAnimationUpdated(Animation& aAnimation)
     return;
   }
 
+  if (aAnimation.GetTimeline() && aAnimation.GetTimeline() != this) {
+    aAnimation.GetTimeline()->RemoveAnimation(&aAnimation);
+  }
+
   mAnimations.PutEntry(&aAnimation);
-  mAnimationOrder.AppendElement(&aAnimation);
+  mAnimationOrder.insertBack(&aAnimation);
+}
+
+void
+AnimationTimeline::RemoveAnimation(Animation* aAnimation)
+{
+  MOZ_ASSERT(!aAnimation->GetTimeline() || aAnimation->GetTimeline() == this);
+  if (aAnimation->isInList()) {
+    aAnimation->remove();
+  }
+  mAnimations.RemoveEntry(aAnimation);
 }
 
 } // namespace dom

dom/animation/AnimationTimeline.h

@@ -40,7 +40,10 @@ public:
   }
 
 protected:
-  virtual ~AnimationTimeline() { }
+  virtual ~AnimationTimeline()
+  {
+    mAnimationOrder.clear();
+  }
 
 public:
   NS_DECL_CYCLE_COLLECTING_ISUPPORTS
@@ -91,6 +94,8 @@ public:
    */
   virtual void NotifyAnimationUpdated(Animation& aAnimation);
 
+  void RemoveAnimation(Animation* aAnimation);
+
 protected:
   nsCOMPtr<nsIGlobalObject> mWindow;
 
@@ -99,13 +104,11 @@ protected:
   // We store them in (a) a hashset for quick lookup, and (b) an array
   // to maintain a fixed sampling order.
   //
-  // The array keeps a strong reference to each animation in order
-  // to save some addref/release traffic and because we never dereference
-  // the pointers in the hashset.
-  typedef nsTHashtable<nsPtrHashKey<dom::Animation>> AnimationSet;
-  typedef nsTArray<RefPtr<dom::Animation>>         AnimationArray;
-  AnimationSet   mAnimations;
-  AnimationArray mAnimationOrder;
+  // The hashset keeps a strong reference to each animation since
+  // dealing with addref/release with LinkedList is difficult.
+  typedef nsTHashtable<nsRefPtrHashKey<dom::Animation>> AnimationSet;
+  AnimationSet mAnimations;
+  LinkedList<dom::Animation> mAnimationOrder;
 };
 
 } // namespace dom

dom/animation/DocumentTimeline.cpp

@@ -112,14 +112,18 @@ DocumentTimeline::WillRefresh(mozilla::TimeStamp aTime)
   MOZ_ASSERT(mIsObservingRefreshDriver);
 
   bool needsTicks = false;
-  AnimationArray animationsToKeep(mAnimationOrder.Length());
+  nsTArray<Animation*> animationsToRemove(mAnimations.Count());
 
   nsAutoAnimationMutationBatch mb(mDocument);
 
-  for (Animation* animation : mAnimationOrder) {
+  for (Animation* animation = mAnimationOrder.getFirst(); animation;
+       animation = animation->getNext()) {
     // Skip any animations that are longer need associated with this timeline.
     if (animation->GetTimeline() != this) {
-      mAnimations.RemoveEntry(animation);
+      // If animation has some other timeline, it better not be also in the
+      // animation list of this timeline object!
+      MOZ_ASSERT(!animation->GetTimeline());
+      animationsToRemove.AppendElement(animation);
       continue;
     }
 
@@ -129,14 +133,14 @@ DocumentTimeline::WillRefresh(mozilla::TimeStamp aTime)
     // order to dispatch events.
     animation->Tick();
 
-    if (animation->IsRelevant() || animation->NeedsTicks()) {
-      animationsToKeep.AppendElement(animation);
-    } else {
-      mAnimations.RemoveEntry(animation);
+    if (!animation->IsRelevant() && !animation->NeedsTicks()) {
+      animationsToRemove.AppendElement(animation);
     }
   }
 
-  mAnimationOrder.SwapElements(animationsToKeep);
+  for (Animation* animation : animationsToRemove) {
+    RemoveAnimation(animation);
+  }
 
   if (!needsTicks) {
     // If another refresh driver observer destroys the nsPresContext,
@@ -155,7 +159,7 @@ DocumentTimeline::NotifyRefreshDriverCreated(nsRefreshDriver* aDriver)
              "Timeline should not be observing the refresh driver before"
              " it is created");
 
-  if (!mAnimationOrder.IsEmpty()) {
+  if (!mAnimationOrder.isEmpty()) {
     aDriver->AddRefreshObserver(this, Flush_Style);
     mIsObservingRefreshDriver = true;
   }

dom/base/Element.cpp

@@ -1760,6 +1760,23 @@ Element::UnbindFromTree(bool aDeep, bool aNullParent)
     }
     SetParentIsContent(false);
   }
+
+  // Ensure that CSS transitions don't continue on an element at a
+  // different place in the tree (even if reinserted before next
+  // animation refresh).
+  // We need to delete the properties while we're still in document
+  // (if we were in document).
+  // FIXME (Bug 522599): Need a test for this.
+  //XXXsmaug this looks slow.
+  if (HasFlag(NODE_HAS_PROPERTIES)) {
+    DeleteProperty(nsGkAtoms::transitionsOfBeforeProperty);
+    DeleteProperty(nsGkAtoms::transitionsOfAfterProperty);
+    DeleteProperty(nsGkAtoms::transitionsProperty);
+    DeleteProperty(nsGkAtoms::animationsOfBeforeProperty);
+    DeleteProperty(nsGkAtoms::animationsOfAfterProperty);
+    DeleteProperty(nsGkAtoms::animationsProperty);
+  }
+
   ClearInDocument();
 
   // Editable descendant count only counts descendants that
@@ -1794,19 +1811,6 @@ Element::UnbindFromTree(bool aDeep, bool aNullParent)
     }
   }
 
-  // Ensure that CSS transitions don't continue on an element at a
-  // different place in the tree (even if reinserted before next
-  // animation refresh).
-  // FIXME (Bug 522599): Need a test for this.
-  if (HasFlag(NODE_HAS_PROPERTIES)) {
-    DeleteProperty(nsGkAtoms::transitionsOfBeforeProperty);
-    DeleteProperty(nsGkAtoms::transitionsOfAfterProperty);
-    DeleteProperty(nsGkAtoms::transitionsProperty);
-    DeleteProperty(nsGkAtoms::animationsOfBeforeProperty);
-    DeleteProperty(nsGkAtoms::animationsOfAfterProperty);
-    DeleteProperty(nsGkAtoms::animationsProperty);
-  }
-
   // Unset this since that's what the old code effectively did.
   UnsetFlags(NODE_FORCE_XBL_BINDINGS);
   bool clearBindingParent = true;