Replacing raw pointer to LayerAnimator with scoped_refptr in ScopedLayerAnimationSettings.

The raw pointer can cause issues: if the ScopedLayerAnimationSettings object survives the
respective LayerAnimator object, a seg fault occurs when ScopedLayerAnimationSettings is deleted.
This is particularly problematic when the animation in question has a 0 duration (which can be
the case for unit tests). When animation's duration is 0, the Animator gets deleted right away,
by the same setter that starts the animation.

BUG=

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

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

4 files changed, 67 insertions, 17 deletions

diff --git a/content/browser/web_contents/aura/overscroll_navigation_overlay.cc b/content/browser/web_contents/aura/overscroll_navigation_overlay.cc
index dc7f3f6..78e3b97 100644
--- a/content/browser/web_contents/aura/overscroll_navigation_overlay.cc
+++ b/content/browser/web_contents/aura/overscroll_navigation_overlay.cc
@@ -77,17 +77,12 @@ class OverlayDismissAnimator
   // the object deletes itself along with the layer.
   void Animate() {
     DCHECK(layer_.get());
-    // Hold on to LayerAnimator to ensure it is not deleted before animation
-    // settings. Without this LayerAnimator would be deleted from SetOpacity if
-    // the animation duration is 0.
-    scoped_refptr<ui::LayerAnimator> animator(layer_->GetAnimator());
-    {
-      // This makes SetOpacity() animate with default duration (which could be
-      // zero, e.g. when running tests).
-      ui::ScopedLayerAnimationSettings settings(animator);
-      animator->AddObserver(this);
-      layer_->SetOpacity(0);
-    }
+    ui::LayerAnimator* animator = layer_->GetAnimator();
+    // This makes SetOpacity() animate with default duration (which could be
+    // zero, e.g. when running tests).
+    ui::ScopedLayerAnimationSettings settings(animator);
+    animator->AddObserver(this);
+    layer_->SetOpacity(0);
   }
 
   // Overridden from ui::LayerAnimationObserver
diff --git a/ui/compositor/layer_animator_unittest.cc b/ui/compositor/layer_animator_unittest.cc
index ed73965..03800fd 100644
--- a/ui/compositor/layer_animator_unittest.cc
+++ b/ui/compositor/layer_animator_unittest.cc
@@ -120,12 +120,41 @@ class DeletingLayerAnimationObserver : public LayerAnimationObserver {
   DISALLOW_COPY_AND_ASSIGN(DeletingLayerAnimationObserver);
 };
 
+class LayerAnimatorDestructionObserver {
+ public:
+  LayerAnimatorDestructionObserver() : animator_deleted_(false) {}
+  virtual ~LayerAnimatorDestructionObserver() {}
+
+  void NotifyAnimatorDeleted() {
+    animator_deleted_ = true;
+  }
+
+  bool IsAnimatorDeleted() {
+    return animator_deleted_;
+  }
+
+ private:
+  bool animator_deleted_;
+
+  DISALLOW_COPY_AND_ASSIGN(LayerAnimatorDestructionObserver);
+};
+
 class TestLayerAnimator : public LayerAnimator {
  public:
-  TestLayerAnimator() : LayerAnimator(base::TimeDelta::FromSeconds(0)) {}
+  TestLayerAnimator() : LayerAnimator(base::TimeDelta::FromSeconds(0)),
+      destruction_observer_(NULL) {}
+
+  void SetDestructionObserver(
+      LayerAnimatorDestructionObserver* observer) {
+    destruction_observer_ = observer;
+  }
 
  protected:
-  virtual ~TestLayerAnimator() {}
+  virtual ~TestLayerAnimator() {
+    if (destruction_observer_) {
+      destruction_observer_->NotifyAnimatorDeleted();
+    }
+  }
 
   virtual void ProgressAnimation(LayerAnimationSequence* sequence,
                                  base::TimeTicks now) OVERRIDE {
@@ -134,6 +163,8 @@ class TestLayerAnimator : public LayerAnimator {
   }
 
  private:
+  LayerAnimatorDestructionObserver* destruction_observer_;
+
   DISALLOW_COPY_AND_ASSIGN(TestLayerAnimator);
 };
 
@@ -1642,6 +1673,30 @@ TEST(LayerAnimatorTest, InterruptedImplicitAnimationObservers) {
   EXPECT_FLOAT_EQ(1.0f, delegate.GetBrightnessForAnimation());
 }
 
+// Tests that LayerAnimator is not deleted after the animation completes as long
+// as there is a live ScopedLayerAnimationSettings object wrapped around it.
+TEST(LayerAnimatorTest, AnimatorKeptAliveBySettings) {
+  // Note we are using a raw pointer unlike in other tests.
+  TestLayerAnimator* animator = new TestLayerAnimator();
+  LayerAnimatorDestructionObserver destruction_observer;
+  animator->SetDestructionObserver(&destruction_observer);
+  AnimationContainerElement* element = animator;
+  animator->set_disable_timer_for_test(true);
+  TestLayerAnimationDelegate delegate;
+  animator->SetDelegate(&delegate);
+  {
+    // ScopedLayerAnimationSettings should keep the Animator alive as long as
+    // it is alive, even beyond the end of the animation.
+    ScopedLayerAnimationSettings settings(animator);
+    base::TimeTicks now = gfx::FrameTime::Now();
+    animator->SetBrightness(0.5);
+    element->Step(now + base::TimeDelta::FromSeconds(1));
+    EXPECT_FALSE(destruction_observer.IsAnimatorDeleted());
+  }
+  // ScopedLayerAnimationSettings was destroyed, so Animator should be deleted.
+  EXPECT_TRUE(destruction_observer.IsAnimatorDeleted());
+}
+
 // Tests that an observer added to a scoped settings object is not notified
 // when the animator is destroyed unless explicitly requested.
 TEST(LayerAnimatorTest, ImplicitObserversAtAnimatorDestruction) {
diff --git a/ui/compositor/scoped_layer_animation_settings.cc b/ui/compositor/scoped_layer_animation_settings.cc
index 9ab0673..487f96a 100644
--- a/ui/compositor/scoped_layer_animation_settings.cc
+++ b/ui/compositor/scoped_layer_animation_settings.cc
@@ -77,9 +77,9 @@ class InvertingObserver : public ImplicitAnimationObserver {
 };
 
 
-// ScoperLayerAnimationSettings ------------------------------------------------
+// ScopedLayerAnimationSettings ------------------------------------------------
 ScopedLayerAnimationSettings::ScopedLayerAnimationSettings(
-    LayerAnimator* animator)
+    scoped_refptr<LayerAnimator> animator)
     : animator_(animator),
       old_is_transition_duration_locked_(
           animator->is_transition_duration_locked_),
diff --git a/ui/compositor/scoped_layer_animation_settings.h b/ui/compositor/scoped_layer_animation_settings.h
index e8a1b46..e201024 100644
--- a/ui/compositor/scoped_layer_animation_settings.h
+++ b/ui/compositor/scoped_layer_animation_settings.h
@@ -26,7 +26,7 @@ class InvertingObserver;
 // (200ms).
 class COMPOSITOR_EXPORT ScopedLayerAnimationSettings {
  public:
-  explicit ScopedLayerAnimationSettings(LayerAnimator* animator);
+  explicit ScopedLayerAnimationSettings(scoped_refptr<LayerAnimator> animator);
   virtual ~ScopedLayerAnimationSettings();
 
   void AddObserver(ImplicitAnimationObserver* observer);
@@ -55,7 +55,7 @@ class COMPOSITOR_EXPORT ScopedLayerAnimationSettings {
   void AddInverselyAnimatedLayer(Layer* inverse_layer);
 
  private:
-  LayerAnimator* animator_;
+  scoped_refptr<LayerAnimator> animator_;
   bool old_is_transition_duration_locked_;
   base::TimeDelta old_transition_duration_;
   gfx::Tween::Type old_tween_type_;