UI Compositor: Fix cc::AnimationPlayers leak.

It appears that we delete LayerAnimator without calling ResetCompositor on its Layer.

Since cc::AnimationPlayer is an RC object, it stays in cc::AnimationTimeline list forever (it leaks, basically).

BUG=574859

Review URL: https://codereview.chromium.org/1658083002

Cr-Commit-Position: refs/heads/master@{#373132}

4 files changed, 68 insertions, 5 deletions

diff --git a/ui/compositor/layer.cc b/ui/compositor/layer.cc
index 89a1d26..6de9b04 100644
--- a/ui/compositor/layer.cc
+++ b/ui/compositor/layer.cc
@@ -107,9 +107,7 @@ Layer::~Layer() {
   // Destroying the animator may cause observers to use the layer (and
   // indirectly the WebLayer). Destroy the animator first so that the WebLayer
   // is still around.
-  if (animator_.get())
-    animator_->SetDelegate(NULL);
-  animator_ = NULL;
+  SetAnimator(nullptr);
   if (compositor_)
     compositor_->SetRootLayer(NULL);
   if (parent_)
@@ -233,9 +231,21 @@ bool Layer::Contains(const Layer* other) const {
 }
 
 void Layer::SetAnimator(LayerAnimator* animator) {
-  if (animator)
-    animator->SetDelegate(this);
+  Compositor* compositor = GetCompositor();
+
+  if (animator_) {
+    if (compositor)
+      animator_->ResetCompositor(compositor);
+    animator_->SetDelegate(nullptr);
+  }
+
   animator_ = animator;
+
+  if (animator_) {
+    animator_->SetDelegate(this);
+    if (compositor)
+      animator_->SetCompositor(compositor);
+  }
 }
 
 LayerAnimator* Layer::GetAnimator() {
diff --git a/ui/compositor/layer_animator.cc b/ui/compositor/layer_animator.cc
index 08d8e11..3fd5bf8 100644
--- a/ui/compositor/layer_animator.cc
+++ b/ui/compositor/layer_animator.cc
@@ -216,6 +216,10 @@ bool LayerAnimator::HasPendingThreadedAnimationsForTesting() const {
   return animation_player_->has_pending_animations_for_testing();
 }
 
+cc::AnimationPlayer* LayerAnimator::GetAnimationPlayerForTesting() const {
+  return animation_player_.get();
+}
+
 void LayerAnimator::StartAnimation(LayerAnimationSequence* animation) {
   scoped_refptr<LayerAnimator> retain(this);
   OnScheduled(animation);
diff --git a/ui/compositor/layer_animator.h b/ui/compositor/layer_animator.h
index 6fc1878..655c17c 100644
--- a/ui/compositor/layer_animator.h
+++ b/ui/compositor/layer_animator.h
@@ -121,6 +121,7 @@ class COMPOSITOR_EXPORT LayerAnimator
 
   // Whether this animator has animations waiting to get sent to cc::LAC.
   bool HasPendingThreadedAnimationsForTesting() const;
+  cc::AnimationPlayer* GetAnimationPlayerForTesting() const;
 
   // Sets the animation preemption strategy. This determines the behaviour if
   // a property is set during an animation. The default is
diff --git a/ui/compositor/layer_owner_unittest.cc b/ui/compositor/layer_owner_unittest.cc
index 75ec44c..594eba3 100644
--- a/ui/compositor/layer_owner_unittest.cc
+++ b/ui/compositor/layer_owner_unittest.cc
@@ -6,6 +6,7 @@
 
 #include "base/macros.h"
 #include "base/test/null_task_runner.h"
+#include "cc/animation/animation_player.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/compositor/compositor.h"
 #include "ui/compositor/layer.h"
@@ -37,6 +38,11 @@ class TestLayerAnimationObserver : public ImplicitAnimationObserver {
   DISALLOW_COPY_AND_ASSIGN(TestLayerAnimationObserver);
 };
 
+class LayerOwnerForTesting : public LayerOwner {
+ public:
+  void DestroyLayerForTesting() { DestroyLayer(); }
+};
+
 // Test fixture for LayerOwner tests that require a ui::Compositor.
 class LayerOwnerTestWithCompositor : public testing::Test {
  public:
@@ -218,4 +224,46 @@ TEST_F(LayerOwnerTestWithCompositor, RecreateNonRootLayerDuringAnimation) {
   scoped_ptr<Layer> layer_copy = owner.RecreateLayer();
 }
 
+// Tests that if LayerOwner-derived class destroys layer, then
+// LayerAnimator's player becomes detached from compositor timeline.
+TEST_F(LayerOwnerTestWithCompositor, DetachTimelineOnAnimatorDeletion) {
+  scoped_ptr<Layer> root_layer(new Layer);
+  compositor()->SetRootLayer(root_layer.get());
+
+  LayerOwnerForTesting owner;
+  Layer* layer = new Layer;
+  owner.SetLayer(layer);
+  layer->SetOpacity(0.5f);
+  root_layer->Add(layer);
+
+  scoped_refptr<cc::AnimationPlayer> player =
+      layer->GetAnimator()->GetAnimationPlayerForTesting();
+  EXPECT_TRUE(player);
+  EXPECT_TRUE(player->animation_timeline());
+
+  // Destroying layer/animator must detach animator's player from timeline.
+  owner.DestroyLayerForTesting();
+  EXPECT_FALSE(player->animation_timeline());
+}
+
+// Tests that if we run threaded opacity animation on already added layer
+// then LayerAnimator's player becomes attached to timeline.
+TEST_F(LayerOwnerTestWithCompositor,
+       AttachTimelineIfAnimatorCreatedAfterSetCompositor) {
+  scoped_ptr<Layer> root_layer(new Layer);
+  compositor()->SetRootLayer(root_layer.get());
+
+  LayerOwner owner;
+  Layer* layer = new Layer;
+  owner.SetLayer(layer);
+  root_layer->Add(layer);
+
+  layer->SetOpacity(0.5f);
+
+  scoped_refptr<cc::AnimationPlayer> player =
+      layer->GetAnimator()->GetAnimationPlayerForTesting();
+  EXPECT_TRUE(player);
+  EXPECT_TRUE(player->animation_timeline());
+}
+
 }  // namespace ui