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..209ab63459f36e2dbf7f4169df808b35597f7071 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()));

[alancutter (OOO until 2018), 2016/08/04 00:45:15]

I wonder if it would be cleaner to replace the FOR_EACH_OBSERVER macro with
template<typename Callback> ObserverList::forEach(Callback). This would save you
from having to inline the macro here and there and also from using macros.
I'm not against the code that's already here but personally I'd swap this out
for a lambda.

[ymalik, 2016/08/04 15:31:47]

Acknowledged.

     ++list_size_after;
   }
   EXPECT_EQ(2, list_size_after);
@@ -300,6 +297,75 @@ TEST_F(ElementAnimationsTest,
                           ->GetValue(base::TimeDelta()));
 }
 
+class TestDestroyedAnimationPlayer : public AnimationPlayer {
+ public:
+  TestDestroyedAnimationPlayer(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:
+  ~TestDestroyedAnimationPlayer() 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, AddedPlayerIsDestroyed) {
+  CreateTestLayer(true, false);
+  AttachTimelinePlayerLayer();
+  CreateImplTimelineAndPlayer();
+
+  scoped_refptr<ElementAnimations> animations = element_animations();
+  scoped_refptr<ElementAnimations> animations_impl = element_animations_impl();
+
+  TestAnimationDelegate delegate;
+  {
+    scoped_refptr<TestDestroyedAnimationPlayer> player1 =
+        new TestDestroyedAnimationPlayer(AnimationIdProvider::NextPlayerId(),
+                                         timeline_);
+    scoped_refptr<TestDestroyedAnimationPlayer> player2 =
+        new TestDestroyedAnimationPlayer(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) {