Fix a memory leak in the layer animator

Ensure that the new sequence is destroyed when using the IMMEDIATELY_SET_NEW_TARGET preemption strategy.

BUG=113276
TEST=compositor_unittests


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

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

3 files changed, 67 insertions, 23 deletions

diff --git a/tools/valgrind/memcheck/suppressions.txt b/tools/valgrind/memcheck/suppressions.txt
index 95bf6d8..2d237bb 100644
--- a/tools/valgrind/memcheck/suppressions.txt
+++ b/tools/valgrind/memcheck/suppressions.txt
@@ -5801,27 +5801,6 @@
    fun:_ZN8remoting29ConvertAndScaleYUVToRGB32RectEPKhS1_S1_iiRK7SkTSizeIiERK7SkIRectPhiS5_S8_S8_
    fun:_ZN8remoting14YuvToRgbTester7RunTestE7SkTSizeIiERK7SkIRect
 }
-{
-   bug_113276
-   Memcheck:Leak
-   fun:_Znw*
-   ...
-   fun:_ZN2ui13LayerAnimator9SetBoundsERKN3gfx4RectE
-   fun:_ZN2ui5Layer9SetBoundsERKN3gfx4RectE
-   fun:_ZN4aura6Window17SetBoundsInternalERKN3gfx4RectE
-   fun:_ZN4aura13LayoutManager20SetChildBoundsDirectEPNS_6WindowERKN3gfx4RectE
-   fun:_ZN3ash8internal23StatusAreaLayoutManager14SetChildBoundsEPN4aura6WindowERKN3gfx4RectE
-   fun:_ZN4aura6Window9SetBoundsERKN3gfx4RectE
-   fun:_ZN5views16NativeWidgetAura7SetSizeERKN3gfx4SizeE
-   fun:_ZN5views6Widget7SetSizeERKN3gfx4SizeE
-   fun:_ZN14StatusAreaView20PreferredSizeChangedEv
-   fun:_ZN14StatusAreaView25ChildPreferredSizeChangedEPN5views4ViewE
-   fun:_ZN5views4View20PreferredSizeChangedEv
-   fun:_ZN5views14TextButtonBase14UpdateTextSizeEv
-   fun:_ZN5views14TextButtonBase7SetFontERKN3gfx4FontE
-   fun:_ZN16StatusAreaButton15UpdateTextStyleEv
-   fun:_ZN14StatusAreaView21UpdateButtonTextStyleEv
-}
 
 #-----------------------------------------------------------------------
 # 4. These only occur on our Google workstations
diff --git a/ui/gfx/compositor/layer_animator.cc b/ui/gfx/compositor/layer_animator.cc
index f98904c..a200881 100644
--- a/ui/gfx/compositor/layer_animator.cc
+++ b/ui/gfx/compositor/layer_animator.cc
@@ -393,9 +393,12 @@ void LayerAnimator::RemoveAllAnimationsWithACommonProperty(
 }
 
 void LayerAnimator::ImmediatelySetNewTarget(LayerAnimationSequence* sequence) {
+  // Ensure that sequence is disposed of when this function completes.
+  scoped_ptr<LayerAnimationSequence> to_dispose(sequence);
   const bool abort = false;
   RemoveAllAnimationsWithACommonProperty(sequence, abort);
-  scoped_ptr<LayerAnimationSequence> removed(RemoveAnimation(sequence));
+  LayerAnimationSequence* removed = RemoveAnimation(sequence);
+  DCHECK(removed == NULL || removed == sequence);
   sequence->Progress(sequence->duration(), delegate());
 }
 
diff --git a/ui/gfx/compositor/layer_animator_unittest.cc b/ui/gfx/compositor/layer_animator_unittest.cc
index 09d1c66..214dc12 100644
--- a/ui/gfx/compositor/layer_animator_unittest.cc
+++ b/ui/gfx/compositor/layer_animator_unittest.cc
@@ -43,6 +43,27 @@ class TestImplicitAnimationObserver : public ImplicitAnimationObserver {
   DISALLOW_COPY_AND_ASSIGN(TestImplicitAnimationObserver);
 };
 
+// The test layer animation sequence updates a live instances count when it is
+// created and destroyed.
+class TestLayerAnimationSequence : public LayerAnimationSequence {
+ public:
+  TestLayerAnimationSequence(LayerAnimationElement* element,
+                             int* num_live_instances)
+      : LayerAnimationSequence(element),
+        num_live_instances_(num_live_instances) {
+    (*num_live_instances_)++;
+  }
+
+  virtual ~TestLayerAnimationSequence() {
+    (*num_live_instances_)--;
+  }
+
+ private:
+  int* num_live_instances_;
+
+  DISALLOW_COPY_AND_ASSIGN(TestLayerAnimationSequence);
+};
+
 } // namespace
 
 // Checks that setting a property on an implicit animator causes an animation to
@@ -910,4 +931,45 @@ TEST(LayerAnimatorTest, SettingPropertyDuringAnAnimation) {
   EXPECT_EQ(0.5, animator->GetTargetOpacity());
 }
 
-} // namespace ui
+// Tests that the preemption mode IMMEDIATELY_SET_NEW_TARGET, doesn't cause the
+// second sequence to be leaked.
+TEST(LayerAnimatorTest, ImmediatelySettingNewTargetDoesNotLeak) {
+  scoped_ptr<LayerAnimator> animator(LayerAnimator::CreateDefaultAnimator());
+  animator->set_preemption_strategy(LayerAnimator::IMMEDIATELY_SET_NEW_TARGET);
+  animator->set_disable_timer_for_test(true);
+  TestLayerAnimationDelegate delegate;
+  animator->SetDelegate(&delegate);
+
+  gfx::Rect start_bounds(0, 0, 50, 50);
+  gfx::Rect middle_bounds(10, 10, 100, 100);
+  gfx::Rect target_bounds(5, 5, 5, 5);
+
+  delegate.SetBoundsFromAnimation(start_bounds);
+
+  {
+    // start an implicit bounds animation.
+    ScopedLayerAnimationSettings settings(animator.get());
+    animator->SetBounds(middle_bounds);
+  }
+
+  EXPECT_TRUE(animator->IsAnimatingProperty(LayerAnimationElement::BOUNDS));
+
+  int num_live_instances = 0;
+  base::TimeDelta delta = base::TimeDelta::FromSeconds(1);
+  scoped_ptr<TestLayerAnimationSequence> sequence(
+      new TestLayerAnimationSequence(
+          LayerAnimationElement::CreateBoundsElement(target_bounds, delta),
+          &num_live_instances));
+
+  EXPECT_EQ(1, num_live_instances);
+
+  // This should interrupt the running sequence causing us to immediately set
+  // the target value. The sequence should alse be destructed.
+  animator->StartAnimation(sequence.release());
+
+  EXPECT_FALSE(animator->IsAnimatingProperty(LayerAnimationElement::BOUNDS));
+  EXPECT_EQ(0, num_live_instances);
+  CheckApproximatelyEqual(delegate.GetBoundsForAnimation(), target_bounds);
+}
+
+}  // namespace ui