ElementAnimations should hold an ObservableList of AnimationPlayers.

cc/animation/element_animations_unittest.cc

Index: cc/animation/element_animations_unittest.cc
diff --git a/cc/animation/element_animations_unittest.cc b/cc/animation/element_animations_unittest.cc
index e5582b0b193b28be3a4cec8de899bffa9c4c544c..ab696e8b033a7fb4fd3321032d8b37055f6cc56f 100644
--- a/cc/animation/element_animations_unittest.cc
+++ b/cc/animation/element_animations_unittest.cc
@@ -191,12 +191,11 @@ TEST_F(ElementAnimationsTest, AddRemovePlayers) {
   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(
+      &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);
@@ -210,12 +209,10 @@ TEST_F(ElementAnimationsTest, AddRemovePlayers) {
   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);
@@ -300,6 +297,75 @@ TEST_F(ElementAnimationsTest,
                           ->GetValue(base::TimeDelta()));
 }
 
+class TestGarbageCollectedAnimationPlayer : public AnimationPlayer {

[jbroman, 2016/07/28 01:01:15]

drive-by: Please don't use the term "garbage collected" for ref-counted objects;
we generally use it to refer to a mark-and-sweep or similar collector, like
Oilpan or V8. "Destroyed", perhaps?

[ymalik, 2016/07/28 02:45:33]

Thanks! Done.

+ public:
+  TestGarbageCollectedAnimationPlayer(int id,
+                                      scoped_refptr<AnimationTimeline> timeline)
+      : AnimationPlayer(id), timeline_(timeline) {}
+
+  void OnAnimationStarted(base::TimeTicks monotonic_time,
+                          TargetProperty::Type target_property,
+                          int group) override {
+    // Detaching this from the timeline ensures that the timeline doesn't hold
+    // a reference to this and we get garbage collected.
+    timeline_->DetachPlayer(this);
+  };
+
+ private:
+  ~TestGarbageCollectedAnimationPlayer() override {}
+
+  scoped_refptr<AnimationTimeline> timeline_;
+};
+
+// 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, AddedPlayerIsGarbageCollected) {
+  CreateTestLayer(true, false);
+  AttachTimelinePlayerLayer();
+  CreateImplTimelineAndPlayer();
+
+  scoped_refptr<ElementAnimations> animations = element_animations();
+  scoped_refptr<ElementAnimations> animations_impl = element_animations_impl();
+
+  TestAnimationDelegate delegate;
+  {
+    scoped_refptr<TestGarbageCollectedAnimationPlayer> player1 =
+        new TestGarbageCollectedAnimationPlayer(
+            AnimationIdProvider::NextPlayerId(), timeline_);
+    scoped_refptr<TestGarbageCollectedAnimationPlayer> player2 =
+        new TestGarbageCollectedAnimationPlayer(
+            AnimationIdProvider::NextPlayerId(), timeline_);
+
+    timeline_->AttachPlayer(player1);
+    timeline_->AttachPlayer(player2);
+
+    player1->AttachElement(element_id_);
+    player2->AttachElement(element_id_);
+
+    player_->set_animation_delegate(&delegate);
+    player1->set_animation_delegate(&delegate);
+    player2->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]);
+  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) {