Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9371007:
Fix a memory leak in the layer animator (Closed)
|
Created:
8 years, 10 months ago by Ian Vollick Modified:
8 years, 10 months ago CC:
chromium-reviews, piman+watch_chromium.org, dhollowa+watch_chromium.org, jonathan.backer, oshima Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionFix 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
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=121271
Patch Set 1 #Patch Set 2 : Remove valgrind suppression #
Total comments: 9
Patch Set 3 : . #Patch Set 4 : Fixed a comment. #
Messages
Total messages: 11 (0 generated)
I'll take a look at tonight. Can you revert suppression as well? - oshima On Wed, Feb 8, 2012 at 6:36 PM, <vollick@chromium.org> wrote: > Reviewers: sky, > > Description: > 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= > TEST=compositor_unittests > > > Please review this at https://chromiumcodereview.**appspot.com/9371007/<https://chromiumcodereview.... > > SVN Base: svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src> > > Affected files: > M ui/gfx/compositor/layer_**animator.cc > M ui/gfx/compositor/layer_**animator_unittest.cc > > > Index: ui/gfx/compositor/layer_**animator.cc > diff --git a/ui/gfx/compositor/layer_**animator.cc > b/ui/gfx/compositor/layer_**animator.cc > index f98904cfd22c99c710f45b573da2cc**cc1e220266..** > cc10f04ff949cafd6b78bdd3915cea**c290ebff5b 100644 > --- a/ui/gfx/compositor/layer_**animator.cc > +++ b/ui/gfx/compositor/layer_**animator.cc > @@ -396,6 +396,8 @@ void LayerAnimator::**ImmediatelySetNewTarget(**LayerAnimationSequence* > sequence) { > const bool abort = false; > RemoveAllAnimationsWithACommon**Property(sequence, abort); > scoped_ptr<**LayerAnimationSequence> removed(RemoveAnimation(** > sequence)); > + if (!removed.get()) > + removed.reset(sequence); > sequence->Progress(sequence->**duration(), delegate()); > } > > Index: ui/gfx/compositor/layer_**animator_unittest.cc > diff --git a/ui/gfx/compositor/layer_**animator_unittest.cc > b/ui/gfx/compositor/layer_**animator_unittest.cc > index 09d1c661c73dc08508086788ebb508**ed68d85877..** > 711403ee908dc5559c3e755c39ae7d**25fd1b0294 100644 > --- a/ui/gfx/compositor/layer_**animator_unittest.cc > +++ b/ui/gfx/compositor/layer_**animator_unittest.cc > @@ -43,6 +43,23 @@ class TestImplicitAnimationObserver : public > ImplicitAnimationObserver { > DISALLOW_COPY_AND_ASSIGN(**TestImplicitAnimationObserver)**; > }; > > +class TestLayerAnimationSequence : public LayerAnimationSequence { > + public: > + TestLayerAnimationSequence(**LayerAnimationElement* element, > + int* num_live_instances) > + : LayerAnimationSequence(**element), > + num_live_instances_(num_live_**instances) { > + *num_live_instances_ += 1; > + } > + > + virtual ~TestLayerAnimationSequence() { > + *num_live_instances_ -= 1; > + } > + > + private: > + int* num_live_instances_; > +}; > + > } // namespace > > // Checks that setting a property on an implicit animator causes an > animation to > @@ -910,4 +927,48 @@ TEST(LayerAnimatorTest, SettingPropertyDuringAnAnimati > **on) { > EXPECT_EQ(0.5, animator->GetTargetOpacity()); > } > > +// Schedule [{o}, {o,b}, {b}] and ensure that {b} doesn't run right away. > That > +// is, ensure that all animations targetting a particular property are > run in > +// order. > +TEST(LayerAnimatorTest, ImmediatelySettingNewTargetDoe**sNotLeak) { > + 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, target_bounds, middle_bounds; > + start_bounds = gfx::Rect(0, 0, 50, 50); > + middle_bounds = gfx::Rect(10, 10, 100, 100); > + target_bounds = gfx::Rect(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 > > >
On 2012/02/09 02:38:43, oshima wrote: > I'll take a look at tonight. Can you revert suppression as well? > Done. (Reverted to the previous version of suppressions.txt). > - oshima > > On Wed, Feb 8, 2012 at 6:36 PM, <mailto:vollick@chromium.org> wrote: > > > Reviewers: sky, > > > > Description: > > 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= > > TEST=compositor_unittests > > > > > > Please review this at > https://chromiumcodereview.**appspot.com/9371007/%3Chttps://chromiumcoderevie...> > > > > SVN Base: > svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src> > > > > Affected files: > > M ui/gfx/compositor/layer_**animator.cc > > M ui/gfx/compositor/layer_**animator_unittest.cc > > > > > > Index: ui/gfx/compositor/layer_**animator.cc > > diff --git a/ui/gfx/compositor/layer_**animator.cc > > b/ui/gfx/compositor/layer_**animator.cc > > index f98904cfd22c99c710f45b573da2cc**cc1e220266..** > > cc10f04ff949cafd6b78bdd3915cea**c290ebff5b 100644 > > --- a/ui/gfx/compositor/layer_**animator.cc > > +++ b/ui/gfx/compositor/layer_**animator.cc > > @@ -396,6 +396,8 @@ void > LayerAnimator::**ImmediatelySetNewTarget(**LayerAnimationSequence* > > sequence) { > > const bool abort = false; > > RemoveAllAnimationsWithACommon**Property(sequence, abort); > > scoped_ptr<**LayerAnimationSequence> removed(RemoveAnimation(** > > sequence)); > > + if (!removed.get()) > > + removed.reset(sequence); > > sequence->Progress(sequence->**duration(), delegate()); > > } > > > > Index: ui/gfx/compositor/layer_**animator_unittest.cc > > diff --git a/ui/gfx/compositor/layer_**animator_unittest.cc > > b/ui/gfx/compositor/layer_**animator_unittest.cc > > index 09d1c661c73dc08508086788ebb508**ed68d85877..** > > 711403ee908dc5559c3e755c39ae7d**25fd1b0294 100644 > > --- a/ui/gfx/compositor/layer_**animator_unittest.cc > > +++ b/ui/gfx/compositor/layer_**animator_unittest.cc > > @@ -43,6 +43,23 @@ class TestImplicitAnimationObserver : public > > ImplicitAnimationObserver { > > DISALLOW_COPY_AND_ASSIGN(**TestImplicitAnimationObserver)**; > > }; > > > > +class TestLayerAnimationSequence : public LayerAnimationSequence { > > + public: > > + TestLayerAnimationSequence(**LayerAnimationElement* element, > > + int* num_live_instances) > > + : LayerAnimationSequence(**element), > > + num_live_instances_(num_live_**instances) { > > + *num_live_instances_ += 1; > > + } > > + > > + virtual ~TestLayerAnimationSequence() { > > + *num_live_instances_ -= 1; > > + } > > + > > + private: > > + int* num_live_instances_; > > +}; > > + > > } // namespace > > > > // Checks that setting a property on an implicit animator causes an > > animation to > > @@ -910,4 +927,48 @@ TEST(LayerAnimatorTest, SettingPropertyDuringAnAnimati > > **on) { > > EXPECT_EQ(0.5, animator->GetTargetOpacity()); > > } > > > > +// Schedule [{o}, {o,b}, {b}] and ensure that {b} doesn't run right away. > > That > > +// is, ensure that all animations targetting a particular property are > > run in > > +// order. > > +TEST(LayerAnimatorTest, ImmediatelySettingNewTargetDoe**sNotLeak) { > > + 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, target_bounds, middle_bounds; > > + start_bounds = gfx::Rect(0, 0, 50, 50); > > + middle_bounds = gfx::Rect(10, 10, 100, 100); > > + target_bounds = gfx::Rect(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 > > > > > >
https://chromiumcodereview.appspot.com/9371007/diff/1003/tools/valgrind/memch... File tools/valgrind/memcheck/suppressions.txt (right): https://chromiumcodereview.appspot.com/9371007/diff/1003/tools/valgrind/memch... tools/valgrind/memcheck/suppressions.txt:4899: fun:_ZN15BalloonViewHost4InitEP10_GtkWidget Is this change intentional? https://chromiumcodereview.appspot.com/9371007/diff/1003/ui/gfx/compositor/la... File ui/gfx/compositor/layer_animator.cc (right): https://chromiumcodereview.appspot.com/9371007/diff/1003/ui/gfx/compositor/la... ui/gfx/compositor/layer_animator.cc:400: removed.reset(sequence); Add a comment here. https://chromiumcodereview.appspot.com/9371007/diff/1003/ui/gfx/compositor/la... File ui/gfx/compositor/layer_animator_unittest.cc (right): https://chromiumcodereview.appspot.com/9371007/diff/1003/ui/gfx/compositor/la... ui/gfx/compositor/layer_animator_unittest.cc:60: int* num_live_instances_; DISALLOW_COPY_AND_ASSIGN. And add a description of what this class does.
http://codereview.chromium.org/9371007/diff/1003/tools/valgrind/memcheck/supp... File tools/valgrind/memcheck/suppressions.txt (right): http://codereview.chromium.org/9371007/diff/1003/tools/valgrind/memcheck/supp... tools/valgrind/memcheck/suppressions.txt:4899: fun:_ZN15BalloonViewHost4InitEP10_GtkWidget On 2012/02/09 03:48:56, sky wrote: > Is this change intentional? Probably not. This part should be reverted. http://codereview.chromium.org/9371007/diff/1003/ui/gfx/compositor/layer_anim... File ui/gfx/compositor/layer_animator_unittest.cc (right): http://codereview.chromium.org/9371007/diff/1003/ui/gfx/compositor/layer_anim... ui/gfx/compositor/layer_animator_unittest.cc:52: *num_live_instances_ += 1; ++ http://codereview.chromium.org/9371007/diff/1003/ui/gfx/compositor/layer_anim... ui/gfx/compositor/layer_animator_unittest.cc:56: *num_live_instances_ -= 1; -- http://codereview.chromium.org/9371007/diff/1003/ui/gfx/compositor/layer_anim... ui/gfx/compositor/layer_animator_unittest.cc:940: gfx::Rect start_bounds, target_bounds, middle_bounds; any good reason not doing start_bounds(0, 0, 50, 50) ... ? http://codereview.chromium.org/9371007/diff/1003/ui/gfx/compositor/layer_anim... ui/gfx/compositor/layer_animator_unittest.cc:973: remove extra line http://codereview.chromium.org/9371007/diff/1003/ui/gfx/compositor/layer_anim... ui/gfx/compositor/layer_animator_unittest.cc:974: } // namespace ui two spaces before //
On 2012/02/09 03:48:55, sky wrote: > https://chromiumcodereview.appspot.com/9371007/diff/1003/tools/valgrind/memch... > File tools/valgrind/memcheck/suppressions.txt (right): > > https://chromiumcodereview.appspot.com/9371007/diff/1003/tools/valgrind/memch... > tools/valgrind/memcheck/suppressions.txt:4899: > fun:_ZN15BalloonViewHost4InitEP10_GtkWidget > Is this change intentional? Fixed. I had simply checked out the previous version of this file, but you're right, this line didn't need to change here. Switched it back. > > https://chromiumcodereview.appspot.com/9371007/diff/1003/ui/gfx/compositor/la... > File ui/gfx/compositor/layer_animator.cc (right): > > https://chromiumcodereview.appspot.com/9371007/diff/1003/ui/gfx/compositor/la... > ui/gfx/compositor/layer_animator.cc:400: removed.reset(sequence); > Add a comment here. Fixed. I've clarified the code, and I've added a comment and a DCHECK. > > https://chromiumcodereview.appspot.com/9371007/diff/1003/ui/gfx/compositor/la... > File ui/gfx/compositor/layer_animator_unittest.cc (right): > > https://chromiumcodereview.appspot.com/9371007/diff/1003/ui/gfx/compositor/la... > ui/gfx/compositor/layer_animator_unittest.cc:60: int* num_live_instances_; > DISALLOW_COPY_AND_ASSIGN. > And add a description of what this class does. Done.
On 2012/02/09 07:46:02, oshima wrote: > http://codereview.chromium.org/9371007/diff/1003/tools/valgrind/memcheck/supp... > File tools/valgrind/memcheck/suppressions.txt (right): > > http://codereview.chromium.org/9371007/diff/1003/tools/valgrind/memcheck/supp... > tools/valgrind/memcheck/suppressions.txt:4899: > fun:_ZN15BalloonViewHost4InitEP10_GtkWidget > On 2012/02/09 03:48:56, sky wrote: > > Is this change intentional? > > Probably not. This part should be reverted. Done. > > http://codereview.chromium.org/9371007/diff/1003/ui/gfx/compositor/layer_anim... > File ui/gfx/compositor/layer_animator_unittest.cc (right): > > http://codereview.chromium.org/9371007/diff/1003/ui/gfx/compositor/layer_anim... > ui/gfx/compositor/layer_animator_unittest.cc:52: *num_live_instances_ += 1; > ++ Done. > > http://codereview.chromium.org/9371007/diff/1003/ui/gfx/compositor/layer_anim... > ui/gfx/compositor/layer_animator_unittest.cc:56: *num_live_instances_ -= 1; > -- Done. > > http://codereview.chromium.org/9371007/diff/1003/ui/gfx/compositor/layer_anim... > ui/gfx/compositor/layer_animator_unittest.cc:940: gfx::Rect start_bounds, > target_bounds, middle_bounds; > any good reason not doing start_bounds(0, 0, 50, 50) ... ? > Nope, fixed. > http://codereview.chromium.org/9371007/diff/1003/ui/gfx/compositor/layer_anim... > ui/gfx/compositor/layer_animator_unittest.cc:973: > remove extra line > Done. > http://codereview.chromium.org/9371007/diff/1003/ui/gfx/compositor/layer_anim... > ui/gfx/compositor/layer_animator_unittest.cc:974: } // namespace ui > two spaces before // Done.
lgtm
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vollick@chromium.org/9371007/7002
Change committed as 121271 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
