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 colle
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) { |