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(
+      &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 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) {
   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