Be careful not to use a deleted animation.

As a result of stepping or finishing an animation, another animation may be removed. This CL adds checks to ensure that we do not use an animation that has been removed this way.

BUG=122141
TEST=LayerAnimatorTest.ObserverDeletesAnimations


Review URL: http://codereview.chromium.org/10007032

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@131623 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 128 insertions, 4 deletions

diff --git a/ui/gfx/compositor/layer_animator.cc b/ui/gfx/compositor/layer_animator.cc
index 46c8d82..df70518 100644
--- a/ui/gfx/compositor/layer_animator.cc
+++ b/ui/gfx/compositor/layer_animator.cc
@@ -226,6 +226,23 @@ void LayerAnimator::RemoveObserver(LayerAnimationObserver* observer) {
   }
 }
 
+// LayerAnimator protected -----------------------------------------------------
+
+bool LayerAnimator::ProgressAnimation(LayerAnimationSequence* sequence,
+                                      base::TimeDelta delta) {
+  return sequence->Progress(delta, delegate());
+}
+
+
+bool LayerAnimator::HasAnimation(LayerAnimationSequence* sequence) const {
+  for (AnimationQueue::const_iterator queue_iter = animation_queue_.begin();
+       queue_iter != animation_queue_.end(); ++queue_iter) {
+    if ((*queue_iter).get() == sequence)
+      return true;
+  }
+  return false;
+}
+
 // LayerAnimator private -------------------------------------------------------
 
 void LayerAnimator::Step(base::TimeTicks now) {
@@ -238,14 +255,15 @@ void LayerAnimator::Step(base::TimeTicks now) {
   RunningAnimations running_animations_copy = running_animations_;
   bool needs_redraw = false;
   for (size_t i = 0; i < running_animations_copy.size(); ++i) {
+    if (!HasAnimation(running_animations_copy[i].sequence))
+      continue;
+
     base::TimeDelta delta = now - running_animations_copy[i].start_time;
     if (delta >= running_animations_copy[i].sequence->duration() &&
         !running_animations_copy[i].sequence->is_cyclic()) {
       FinishAnimation(running_animations_copy[i].sequence);
-    } else {
-      if (running_animations_copy[i].sequence->Progress(delta, delegate()))
-        needs_redraw = true;
-    }
+    } else if (ProgressAnimation(running_animations_copy[i].sequence, delta))
+      needs_redraw = true;
   }
 
   if (needs_redraw)
@@ -318,6 +336,9 @@ void LayerAnimator::FinishAnyAnimationWithZeroDuration() {
   // cause new animations to start running.
   RunningAnimations running_animations_copy = running_animations_;
   for (size_t i = 0; i < running_animations_copy.size(); ++i) {
+    if (!HasAnimation(running_animations_copy[i].sequence))
+      continue;
+
     if (running_animations_copy[i].sequence->duration() == base::TimeDelta()) {
       running_animations_copy[i].sequence->Progress(
           running_animations_copy[i].sequence->duration(), delegate());
@@ -334,6 +355,9 @@ void LayerAnimator::ClearAnimations() {
   // clients are badly behaved, we will use a copy of the running animations.
   RunningAnimations running_animations_copy = running_animations_;
   for (size_t i = 0; i < running_animations_copy.size(); ++i) {
+    if (!HasAnimation(running_animations_copy[i].sequence))
+      continue;
+
     scoped_ptr<LayerAnimationSequence> removed(
         RemoveAnimation(running_animations_copy[i].sequence));
     if (removed.get())
@@ -380,6 +404,9 @@ void LayerAnimator::RemoveAllAnimationsWithACommonProperty(
   // operate on a copy.
   RunningAnimations running_animations_copy = running_animations_;
   for (size_t i = 0; i < running_animations_copy.size(); ++i) {
+    if (!HasAnimation(running_animations_copy[i].sequence))
+      continue;
+
     if (running_animations_copy[i].sequence->HasCommonProperty(
             sequence->properties())) {
       scoped_ptr<LayerAnimationSequence> removed(
@@ -400,6 +427,9 @@ void LayerAnimator::RemoveAllAnimationsWithACommonProperty(
     sequences.push_back((*queue_iter).get());
 
   for (size_t i = 0; i < sequences.size(); ++i) {
+    if (!HasAnimation(sequences[i]))
+      continue;
+
     if (sequences[i]->HasCommonProperty(sequence->properties())) {
       scoped_ptr<LayerAnimationSequence> removed(
           RemoveAnimation(sequences[i]));
diff --git a/ui/gfx/compositor/layer_animator.h b/ui/gfx/compositor/layer_animator.h
index b4ab2ab..070534a 100644
--- a/ui/gfx/compositor/layer_animator.h
+++ b/ui/gfx/compositor/layer_animator.h
@@ -149,6 +149,13 @@ class COMPOSITOR_EXPORT LayerAnimator : public AnimationContainerElement {
   LayerAnimationDelegate* delegate() { return delegate_; }
   const LayerAnimationDelegate* delegate() const { return delegate_; }
 
+  // Virtual for testing.
+  virtual bool ProgressAnimation(LayerAnimationSequence* sequence,
+                                 base::TimeDelta delta);
+
+  // Returns true if the sequence is owned by this animator.
+  bool HasAnimation(LayerAnimationSequence* sequence) const;
+
  private:
   friend class ScopedLayerAnimationSettings;
 
diff --git a/ui/gfx/compositor/layer_animator_unittest.cc b/ui/gfx/compositor/layer_animator_unittest.cc
index d2c0baf..2ad34c3 100644
--- a/ui/gfx/compositor/layer_animator_unittest.cc
+++ b/ui/gfx/compositor/layer_animator_unittest.cc
@@ -51,6 +51,46 @@ class TestImplicitAnimationObserver : public ImplicitAnimationObserver {
   DISALLOW_COPY_AND_ASSIGN(TestImplicitAnimationObserver);
 };
 
+// When notified that an animation has ended, stops all other animations.
+class DeletingLayerAnimationObserver : public LayerAnimationObserver {
+ public:
+  DeletingLayerAnimationObserver(LayerAnimator* animator,
+                                 LayerAnimationSequence* sequence)
+    : animator_(animator),
+      sequence_(sequence) {
+  }
+
+  virtual void OnLayerAnimationEnded(LayerAnimationSequence* sequence) {
+    animator_->StopAnimating();
+  }
+
+  virtual void OnLayerAnimationAborted(LayerAnimationSequence* sequence) {}
+
+  virtual void OnLayerAnimationScheduled(LayerAnimationSequence* sequence) {}
+
+ private:
+  LayerAnimator* animator_;
+  LayerAnimationSequence* sequence_;
+
+  DISALLOW_COPY_AND_ASSIGN(DeletingLayerAnimationObserver);
+};
+
+class TestLayerAnimator : public LayerAnimator {
+ public:
+  TestLayerAnimator() : LayerAnimator(base::TimeDelta::FromSeconds(0)) {}
+  virtual ~TestLayerAnimator() {}
+
+ protected:
+  virtual bool ProgressAnimation(LayerAnimationSequence* sequence,
+                                 base::TimeDelta delta) OVERRIDE {
+    EXPECT_TRUE(HasAnimation(sequence));
+    return LayerAnimator::ProgressAnimation(sequence, delta);
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(TestLayerAnimator);
+};
+
 // The test layer animation sequence updates a live instances count when it is
 // created and destroyed.
 class TestLayerAnimationSequence : public LayerAnimationSequence {
@@ -942,6 +982,53 @@ TEST(LayerAnimatorTest, ObserverDetachedBeforeAnimationFinished) {
   EXPECT_TRUE(observer.animations_completed());
 }
 
+// This checks that if an animation is deleted due to a callback, that the
+// animator does not try to use the deleted animation. For example, if we have
+// two running animations, and the first finishes and the resulting callback
+// causes the second to be deleted, we should not attempt to animate the second
+// animation.
+TEST(LayerAnimatorTest, ObserverDeletesAnimations) {
+  LayerAnimator::set_disable_animations_for_test(false);
+  scoped_ptr<LayerAnimator> animator(new TestLayerAnimator());
+  AnimationContainerElement* element = animator.get();
+  animator->set_disable_timer_for_test(true);
+  TestLayerAnimationDelegate delegate;
+  animator->SetDelegate(&delegate);
+
+  double start_opacity(0.0);
+  double target_opacity(1.0);
+
+  gfx::Rect start_bounds(0, 0, 50, 50);
+  gfx::Rect target_bounds(5, 5, 5, 5);
+
+  delegate.SetOpacityFromAnimation(start_opacity);
+  delegate.SetBoundsFromAnimation(start_bounds);
+
+  base::TimeDelta opacity_delta = base::TimeDelta::FromSeconds(1);
+  base::TimeDelta halfway_delta = base::TimeDelta::FromSeconds(2);
+  base::TimeDelta bounds_delta = base::TimeDelta::FromSeconds(3);
+
+  LayerAnimationSequence* to_delete = new LayerAnimationSequence(
+      LayerAnimationElement::CreateBoundsElement(target_bounds, bounds_delta));
+
+  scoped_ptr<DeletingLayerAnimationObserver> observer(
+      new DeletingLayerAnimationObserver(animator.get(), to_delete));
+
+  animator->AddObserver(observer.get());
+
+  animator->StartAnimation(
+      new LayerAnimationSequence(
+          LayerAnimationElement::CreateOpacityElement(
+              target_opacity, opacity_delta)));
+
+  animator->StartAnimation(to_delete);
+
+  base::TimeTicks start_time = animator->last_step_time();
+  element->Step(start_time + halfway_delta);
+
+  animator->RemoveObserver(observer.get());
+}
+
 // Check that setting a property during an animation with a default animator
 // cancels the original animation.
 TEST(LayerAnimatorTest, SettingPropertyDuringAnAnimation) {