Stop bounds animation if a view is removed

Bug: 1034078,
Test: Covered by unittest
Change-Id: If9dd8a777b83a02e572c2f1953e70e5ce6d4a439
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1968241
Commit-Queue: Mitsuru Oshima <oshima@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#725970}

ui/views/animation/bounds_animator.cc

@@ -22,8 +22,12 @@ BoundsAnimator::BoundsAnimator(View* parent)
 
 BoundsAnimator::~BoundsAnimator() {
   // Delete all the animations, but don't remove any child views. We assume the
-  // view owns us and is going to be deleted anyway.
-  for (auto& entry : data_)
+  // view owns us and is going to be deleted anyway. However, deleting a
+  // delegate may results in removing a child view, so empty the data_ first so
+  // that it won't call AnimationCanceled().
+  ViewToDataMap data;
+  data.swap(data_);
+  for (auto& entry : data)
     CleanupData(false, &entry.second);
 }
 
@@ -273,4 +277,13 @@ void BoundsAnimator::AnimationContainerProgressed(
 void BoundsAnimator::AnimationContainerEmpty(
     gfx::AnimationContainer* container) {}
 
+void BoundsAnimator::OnChildViewRemoved(views::View* observed_view,
+                                        views::View* removed) {
+  DCHECK_EQ(parent_, observed_view);
+  const auto iter = data_.find(removed);
+  if (iter == data_.end())
+    return;
+  AnimationCanceled(iter->second.animation.get());
+}
+
 }  // namespace views

ui/views/animation/bounds_animator.h

@@ -156,6 +156,8 @@ class VIEWS_EXPORT BoundsAnimator : public AnimationDelegateViews {
   void AnimationContainerProgressed(
       gfx::AnimationContainer* container) override;
   void AnimationContainerEmpty(gfx::AnimationContainer* container) override;
+  void OnChildViewRemoved(views::View* observed_view,
+                          views::View* child) override;
 
   // Parent of all views being animated.
   View* parent_;

ui/views/animation/bounds_animator_unittest.cc

@@ -104,12 +104,12 @@ TEST_F(BoundsAnimatorTest, AnimateViewTo) {
   child()->SetBoundsRect(initial_bounds);
   gfx::Rect target_bounds(10, 10, 20, 20);
   animator()->AnimateViewTo(child(), target_bounds);
-  animator()->SetAnimationDelegate(
-      child(),
-      std::unique_ptr<gfx::AnimationDelegate>(new TestAnimationDelegate()));
+  animator()->SetAnimationDelegate(child(),
+                                   std::make_unique<TestAnimationDelegate>());
 
   // The animator should be animating now.
   EXPECT_TRUE(animator()->IsAnimating());
+  EXPECT_TRUE(animator()->IsAnimating(child()));
 
   // Run the message loop; the delegate exits the loop when the animation is
   // done.
@@ -118,22 +118,39 @@ TEST_F(BoundsAnimatorTest, AnimateViewTo) {
   // Make sure the bounds match of the view that was animated match.
   EXPECT_EQ(target_bounds, child()->bounds());
 
+  // |child| shouldn't be animating anymore.
+  EXPECT_FALSE(animator()->IsAnimating(child()));
+
   // The parent should have been told to repaint as the animation progressed.
   // The resulting rect is the union of the original and target bounds.
   EXPECT_EQ(gfx::UnionRects(target_bounds, initial_bounds),
             parent()->dirty_rect());
 }
 
+// Make sure that removing/deleting a child view while animating stops the
+// view's animation and will not result in a crash.
+TEST_F(BoundsAnimatorTest, DeleteWhileAnimating) {
+  animator()->AnimateViewTo(child(), gfx::Rect(0, 0, 10, 10));
+  animator()->SetAnimationDelegate(child(), std::make_unique<OwnedDelegate>());
+
+  EXPECT_TRUE(animator()->IsAnimating(child()));
+
+  // Make sure that animation is removed upon deletion.
+  delete child();
+  EXPECT_FALSE(animator()->GetAnimationForView(child()));
+  EXPECT_FALSE(animator()->IsAnimating(child()));
+}
+
 // Make sure an AnimationDelegate is deleted when canceled.
 TEST_F(BoundsAnimatorTest, DeleteDelegateOnCancel) {
   animator()->AnimateViewTo(child(), gfx::Rect(0, 0, 10, 10));
-  animator()->SetAnimationDelegate(
-      child(), std::unique_ptr<gfx::AnimationDelegate>(new OwnedDelegate()));
+  animator()->SetAnimationDelegate(child(), std::make_unique<OwnedDelegate>());
 
   animator()->Cancel();
 
   // The animator should no longer be animating.
   EXPECT_FALSE(animator()->IsAnimating());
+  EXPECT_FALSE(animator()->IsAnimating(child()));
 
   // The cancel should both cancel the delegate and delete it.
   EXPECT_TRUE(OwnedDelegate::GetAndClearCanceled());
@@ -144,8 +161,7 @@ TEST_F(BoundsAnimatorTest, DeleteDelegateOnCancel) {
 // scheduled.
 TEST_F(BoundsAnimatorTest, DeleteDelegateOnNewAnimate) {
   animator()->AnimateViewTo(child(), gfx::Rect(0, 0, 10, 10));
-  animator()->SetAnimationDelegate(
-      child(), std::unique_ptr<gfx::AnimationDelegate>(new OwnedDelegate()));
+  animator()->SetAnimationDelegate(child(), std::make_unique<OwnedDelegate>());
 
   animator()->AnimateViewTo(child(), gfx::Rect(0, 0, 10, 10));
 
@@ -156,16 +172,16 @@ TEST_F(BoundsAnimatorTest, DeleteDelegateOnNewAnimate) {
 
 // Makes sure StopAnimating works.
 TEST_F(BoundsAnimatorTest, StopAnimating) {
-  std::unique_ptr<OwnedDelegate> delegate(new OwnedDelegate());
+  std::unique_ptr<OwnedDelegate> delegate(std::make_unique<OwnedDelegate>());
 
   animator()->AnimateViewTo(child(), gfx::Rect(0, 0, 10, 10));
-  animator()->SetAnimationDelegate(
-      child(), std::unique_ptr<gfx::AnimationDelegate>(new OwnedDelegate()));
+  animator()->SetAnimationDelegate(child(), std::make_unique<OwnedDelegate>());
 
   animator()->StopAnimatingView(child());
 
   // Shouldn't be animating now.
   EXPECT_FALSE(animator()->IsAnimating());
+  EXPECT_FALSE(animator()->IsAnimating(child()));
 
   // Stopping should both cancel the delegate and delete it.
   EXPECT_TRUE(OwnedDelegate::GetAndClearDeleted());