ElementAnimations should hold an ObservableList of AnimationPlayers.

cc/animation/element_animations_unittest.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "cc/animation/element_animations.h"
 
 #include "cc/animation/animation_delegate.h"
 #include "cc/animation/animation_host.h"
 #include "cc/animation/animation_id_provider.h"
 #include "cc/animation/animation_player.h"
@@ skipping 173 matching lines @@
   EXPECT_EQ(element_animations, player2->element_animations());
 
   host_->PushPropertiesTo(host_impl_);
   GetImplTimelineAndPlayerByID();
 
   scoped_refptr<ElementAnimations> element_animations_impl =
       player_impl_->element_animations();
   EXPECT_TRUE(element_animations_impl);
 
   int list_size_before = 0;
-  for (const ElementAnimations::PlayersListNode* node =
-           element_animations_impl->players_list().head();
-       node != element_animations_impl->players_list().end();
-       node = node->next()) {
-    const AnimationPlayer* player_impl = node->value();
-    EXPECT_TRUE(timeline_->GetPlayerById(player_impl->id()));
+  base::ObserverList<AnimationPlayer>::Iterator it(

[loyso (OOO), 2016/08/05 00:18:48]

ElementAnimations::PlayersList type here and everywhere?

[ymalik, 2016/08/05 00:54:04]

Done.

+      &element_animations_impl->players_list());
+  AnimationPlayer* player;
+  while ((player = it.GetNext()) != nullptr) {
+    EXPECT_TRUE(timeline_->GetPlayerById(player->id()));
     ++list_size_before;
   }
   EXPECT_EQ(3, list_size_before);
 
   player2->DetachElement();
   EXPECT_FALSE(player2->element_animations());
   EXPECT_EQ(element_animations, player_->element_animations());
   EXPECT_EQ(element_animations, player1->element_animations());
 
   host_->PushPropertiesTo(host_impl_);
   EXPECT_EQ(element_animations_impl, player_impl_->element_animations());
 
   int list_size_after = 0;
-  for (const ElementAnimations::PlayersListNode* node =
-           element_animations_impl->players_list().head();
-       node != element_animations_impl->players_list().end();
-       node = node->next()) {
-    const AnimationPlayer* player_impl = node->value();
-    EXPECT_TRUE(timeline_->GetPlayerById(player_impl->id()));
+  it = base::ObserverList<AnimationPlayer>::Iterator(
+      &element_animations_impl->players_list());
+  while ((player = it.GetNext()) != nullptr) {
+    EXPECT_TRUE(timeline_->GetPlayerById(player->id()));
     ++list_size_after;
   }
   EXPECT_EQ(2, list_size_after);
 }
 
 TEST_F(ElementAnimationsTest, SyncNewAnimation) {
   auto animations_impl = ElementAnimations::Create();
   animations_impl->set_has_element_in_active_list(true);
 
   auto animations = ElementAnimations::Create();
@@ skipping 64 matching lines @@
       std::move(curve), animation2_id, 0, TargetProperty::SCROLL_OFFSET));
   animations->AddAnimation(std::move(animation));
   animations->PushPropertiesTo(animations_impl.get());
   EXPECT_VECTOR2DF_EQ(provider_initial_value,
                       animations_impl->GetAnimationById(animation2_id)
                           ->curve()
                           ->ToScrollOffsetAnimationCurve()
                           ->GetValue(base::TimeDelta()));
 }
 
+class TestAnimationDelegateThatDestroysPlayer : public TestAnimationDelegate {
+ public:
+  TestAnimationDelegateThatDestroysPlayer() {}
+
+  void NotifyAnimationStarted(base::TimeTicks monotonic_time,
+                              TargetProperty::Type target_property,
+                              int group) override {
+    TestAnimationDelegate::NotifyAnimationStarted(monotonic_time,
+                                                  target_property, group);
+    // Detaching player from the timeline ensures that the timeline doesn't hold
+    // a reference to the player and the player is destroyed.
+    timeline_->DetachPlayer(player_);
+  };
+
+  void setTimelineAndPlayer(scoped_refptr<AnimationTimeline> timeline,
+                            scoped_refptr<AnimationPlayer> player) {
+    timeline_ = timeline;
+    player_ = player;
+  }
+
+ private:
+  scoped_refptr<AnimationTimeline> timeline_;
+  scoped_refptr<AnimationPlayer> player_;
+};
+
+// Test that we don't crash if a player is deleted while ElementAnimations is
+// iterating through the list of players (see crbug.com/631052). This test
+// passes if it doesn't crash.
+TEST_F(ElementAnimationsTest, AddedPlayerIsDestroyed) {
+  CreateTestLayer(true, false);
+  AttachTimelinePlayerLayer();
+  CreateImplTimelineAndPlayer();
+
+  scoped_refptr<ElementAnimations> animations = element_animations();
+  scoped_refptr<ElementAnimations> animations_impl = element_animations_impl();
+
+  TestAnimationDelegateThatDestroysPlayer delegate;
+  {
+    scoped_refptr<AnimationPlayer> player =
+        AnimationPlayer::Create(AnimationIdProvider::NextPlayerId());
+    delegate.setTimelineAndPlayer(timeline_, player);
+
+    timeline_->AttachPlayer(player);
+    player->AttachElement(element_id_);
+    player->set_animation_delegate(&delegate);
+  }
+
+  int animation_id = AddOpacityTransitionToElementAnimations(
+      animations.get(), 1.0, 0.f, 1.f, false);
+
+  animations->PushPropertiesTo(animations_impl.get());
+  animations_impl->ActivateAnimations();
+  EXPECT_TRUE(animations_impl->GetAnimationById(animation_id));
+
+  animations_impl->Animate(kInitialTickTime);
+
+  auto events = host_impl_->CreateEvents();
+  animations_impl->UpdateState(true, events.get());
+  EXPECT_EQ(1u, events->events_.size());
+  EXPECT_EQ(AnimationEvent::STARTED, events->events_[0].type);
+
+  animations->NotifyAnimationStarted(events->events_[0]);

[loyso (OOO), 2016/08/05 00:13:25]

Could you add the comment like "The actual detachment happens here, inside the
callback"

[ymalik, 2016/08/05 00:54:04]

Done.

+  EXPECT_EQ(1, delegate.started());
+}
+
 // If an animation is started on the impl thread before it is ticked on the main
 // thread, we must be sure to respect the synchronized start time.
 TEST_F(ElementAnimationsTest, DoNotClobberStartTimes) {
   CreateTestLayer(true, false);
   AttachTimelinePlayerLayer();
   CreateImplTimelineAndPlayer();
 
   scoped_refptr<ElementAnimations> animations = element_animations();
   scoped_refptr<ElementAnimations> animations_impl = element_animations_impl();
 
@@ skipping 3302 matching lines @@
   EXPECT_FALSE(animations->IsPotentiallyAnimatingProperty(
       TargetProperty::OPACITY, ElementListType::ACTIVE));
   EXPECT_FALSE(animations->IsCurrentlyAnimatingProperty(
       TargetProperty::OPACITY, ElementListType::PENDING));
   EXPECT_FALSE(animations->IsCurrentlyAnimatingProperty(
       TargetProperty::OPACITY, ElementListType::ACTIVE));
 }
 
 }  // namespace
 }  // namespace cc