Bug 1223445 - KeyframeEffectReadOnly objects end up keeping lots of other objects alive too long, r=birtles, a=ritu
authorOlli Pettay <Olli.Pettay@helsinki.fi>
Mon, 16 Nov 2015 19:44:55 +0200
changeset 305599 6b11e7539edc545d4fee3f55f121c2b9f8f9046c
parent 305598 86415471ce2c373a6003f26f322e0f1ce99ea7dd
child 305600 d4caa8fa3d081026498ec1b3acf6906a2109c7a7
push id1001
push userraliiev@mozilla.com
push dateMon, 18 Jan 2016 19:06:03 +0000
treeherdermozilla-release@8b89261f3ac4 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbirtles, ritu
bugs1223445
milestone44.0a2
Bug 1223445 - KeyframeEffectReadOnly objects end up keeping lots of other objects alive too long, r=birtles, a=ritu
dom/animation/Animation.cpp
dom/animation/Animation.h
dom/animation/AnimationTimeline.cpp
dom/animation/AnimationTimeline.h
dom/animation/DocumentTimeline.cpp
dom/base/Element.cpp
--- a/dom/animation/Animation.cpp
+++ b/dom/animation/Animation.cpp
@@ -596,16 +596,20 @@ Animation::DoCancel()
   ResetFinishedPromise();
 
   DispatchPlaybackEvent(NS_LITERAL_STRING("cancel"));
 
   mHoldTime.SetNull();
   mStartTime.SetNull();
 
   UpdateTiming(SeekFlag::NoSeek, SyncNotifyFlag::Async);
+
+  if (mTimeline) {
+    mTimeline->RemoveAnimation(this);
+  }
 }
 
 void
 Animation::UpdateRelevance()
 {
   bool wasRelevant = mIsRelevant;
   mIsRelevant = HasCurrentEffect() || IsInEffect();
 
--- a/dom/animation/Animation.h
+++ b/dom/animation/Animation.h
@@ -5,16 +5,17 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #ifndef mozilla_dom_Animation_h
 #define mozilla_dom_Animation_h
 
 #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
 #include "mozilla/DOMEventTargetHelper.h" // for DOMEventTargetHelper
 #include "mozilla/dom/KeyframeEffect.h" // for KeyframeEffectReadOnly
 #include "mozilla/dom/Promise.h" // for Promise
 #include "nsCSSProperty.h" // for nsCSSProperty
 #include "nsIGlobalObject.h"
@@ -43,16 +44,17 @@ class CommonAnimationManager;
 
 namespace dom {
 
 class CSSAnimation;
 class CSSTransition;
 
 class Animation
   : public DOMEventTargetHelper
+  , public LinkedListElement<Animation>
 {
 protected:
   virtual ~Animation() {}
 
 public:
   explicit Animation(nsIGlobalObject* aGlobal)
     : DOMEventTargetHelper(aGlobal)
     , mPlaybackRate(1.0)
--- a/dom/animation/AnimationTimeline.cpp
+++ b/dom/animation/AnimationTimeline.cpp
@@ -1,22 +1,35 @@
 /* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
 /* vim: set ts=8 sts=2 et sw=2 tw=80: */
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this file,
  * You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #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)
 
 NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(AnimationTimeline)
   NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY
   NS_INTERFACE_MAP_ENTRY(nsISupports)
 NS_INTERFACE_MAP_END
@@ -27,19 +40,20 @@ AnimationTimeline::GetAnimations(Animati
   nsCOMPtr<nsPIDOMWindow> window = do_QueryInterface(mWindow);
   if (mWindow) {
     nsIDocument* doc = window->GetDoc();
     if (doc) {
       doc->FlushPendingNotifications(Flush_Style);
     }
   }
 
-  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
     // on the next tick.
     if (!animation->IsRelevant() || animation->GetTimeline() != this) {
       continue;
     }
 
@@ -64,14 +78,28 @@ AnimationTimeline::GetAnimations(Animati
 
 void
 AnimationTimeline::NotifyAnimationUpdated(Animation& aAnimation)
 {
   if (mAnimations.Contains(&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
 } // namespace mozilla
--- a/dom/animation/AnimationTimeline.h
+++ b/dom/animation/AnimationTimeline.h
@@ -35,17 +35,20 @@ class AnimationTimeline
 public:
   explicit AnimationTimeline(nsIGlobalObject* aWindow)
     : mWindow(aWindow)
   {
     MOZ_ASSERT(mWindow);
   }
 
 protected:
-  virtual ~AnimationTimeline() { }
+  virtual ~AnimationTimeline()
+  {
+    mAnimationOrder.clear();
+  }
 
 public:
   NS_DECL_CYCLE_COLLECTING_ISUPPORTS
   NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(AnimationTimeline)
 
   nsIGlobalObject* GetParentObject() const { return mWindow; }
 
   typedef nsTArray<RefPtr<Animation>> AnimationSequence;
@@ -86,29 +89,29 @@ public:
   /**
    * Inform this timeline that |aAnimation| which is or was observing the
    * timeline, has been updated. This serves as both the means to associate
    * AND disassociate animations with a timeline. The timeline itself will
    * determine if it needs to begin, continue or stop tracking this animation.
    */
   virtual void NotifyAnimationUpdated(Animation& aAnimation);
 
+  void RemoveAnimation(Animation* aAnimation);
+
 protected:
   nsCOMPtr<nsIGlobalObject> mWindow;
 
   // Animations observing this timeline
   //
   // 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
 } // namespace mozilla
 
 #endif // mozilla_dom_AnimationTimeline_h
--- a/dom/animation/DocumentTimeline.cpp
+++ b/dom/animation/DocumentTimeline.cpp
@@ -107,41 +107,45 @@ DocumentTimeline::NotifyAnimationUpdated
 }
 
 void
 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;
     }
 
     needsTicks |= animation->NeedsTicks();
     // Even if |animation| doesn't need future ticks, we should still
     // Tick it this time around since it might just need a one-off tick in
     // 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,
     // nsRefreshDriver will detect it and we won't be called.
     MOZ_ASSERT(GetRefreshDriver(),
                "Refresh driver should still be valid inside WillRefresh");
     GetRefreshDriver()->RemoveRefreshObserver(this, Flush_Style);
     mIsObservingRefreshDriver = false;
@@ -150,17 +154,17 @@ DocumentTimeline::WillRefresh(mozilla::T
 
 void
 DocumentTimeline::NotifyRefreshDriverCreated(nsRefreshDriver* aDriver)
 {
   MOZ_ASSERT(!mIsObservingRefreshDriver,
              "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;
   }
 }
 
 void
 DocumentTimeline::NotifyRefreshDriverDestroying(nsRefreshDriver* aDriver)
 {
--- a/dom/base/Element.cpp
+++ b/dom/base/Element.cpp
@@ -1755,16 +1755,33 @@ Element::UnbindFromTree(bool aDeep, bool
     if (GetParent()) {
       RefPtr<nsINode> p;
       p.swap(mParent);
     } else {
       mParent = nullptr;
     }
     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
   // are in the uncomposed document.
   ResetEditableDescendantCount();
 
   if (aNullParent || !mParent->IsInShadowTree()) {
     UnsetFlags(NODE_IS_IN_SHADOW_TREE);
@@ -1789,29 +1806,16 @@ Element::UnbindFromTree(bool aDeep, bool
     // Detached must be enqueued whenever custom element is removed from
     // the document and this document has a browsing context.
     if (GetCustomElementData() && document->GetDocShell()) {
       // Enqueue a detached callback for the custom element.
       document->EnqueueLifecycleCallback(nsIDocument::eDetached, this);
     }
   }
 
-  // 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;
 
 #ifdef MOZ_XUL
   nsXULElement* xulElem = nsXULElement::FromContent(this);
   if (xulElem) {
     xulElem->SetXULBindingParent(nullptr);