Unregister AnimationPlayer from ActiveDOMObjects on timeline deletion.

Unregister AnimationPlayer from ActiveDOMObjects on timeline deletion.

BUG=472307
R=dstockwell@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=193510

[loyso (OOO), 2015-04-02 02:47:40]

loyso@chromium.org changed reviewers:
+ dstockwell@chromium.org

[loyso (OOO), 2015-04-02 02:52:07]

https://codereview.chromium.org/1054823002/diff/1/Source/core/animation/Anima...
File Source/core/animation/AnimationPlayer.cpp (left):

https://codereview.chromium.org/1054823002/diff/1/Source/core/animation/Anima...
Source/core/animation/AnimationPlayer.cpp:616: PlayStateUpdateScope
updateScope(*this, TimingUpdateOnDemand);
This implicitly sets CompositorPendingChange = SetCompositorPending.
AnimationPlayer's ActiveDOMObject::stop may be called AFTER it's parent
~AnimationTimeline() destroyed.

[loyso (OOO), 2015-04-08 08:30:32]

PTAL!

[dstockwell, 2015-04-10 01:52:16]

The CQ bit was checked by dstockwell@chromium.org

[dstockwell, 2015-04-10 01:52:16]

lgtm

[commit-bot: I haz the power, 2015-04-10 01:52:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1054823002/1

[commit-bot: I haz the power, 2015-04-10 04:55:27]

Committed patchset #1 (id:1) as
https://src.chromium.org/viewvc/blink?view=rev&revision=193510

[sof, 2015-04-10 09:12:51]

sigbjornf@opera.com changed reviewers:
+ sigbjornf@opera.com

[sof, 2015-04-10 09:12:52]

https://codereview.chromium.org/1054823002/diff/1/Source/core/animation/Anima...
File Source/core/animation/AnimationTimeline.cpp (right):

https://codereview.chromium.org/1054823002/diff/1/Source/core/animation/Anima...
Source/core/animation/AnimationTimeline.cpp:80: for (const auto& player :
m_players)
Why are their two loops over m_players here (and dispose() first)?

Accessing other heap objects (AnimationPlayer and the hash set) is not allowed
from within dtors with Oilpan -- finalization isn't ordered.

[dstockwell, 2015-04-10 11:21:35]

https://codereview.chromium.org/1054823002/diff/1/Source/core/animation/Anima...
File Source/core/animation/AnimationTimeline.cpp (right):

https://codereview.chromium.org/1054823002/diff/1/Source/core/animation/Anima...
Source/core/animation/AnimationTimeline.cpp:81: player->dispose();
Hmm, this should probably just move to player->timelineDestroyed() The player
should die together with it's timeline under Oilpan.

But, thinking about this patch more, we should just make setCompositorPending
early out when timeline is null.

If this is causing problems for Oilpan it's fine to just revert this patch.

[sof, 2015-04-10 12:25:08]

On 2015/04/10 11:21:35, dstockwell wrote:
>
https://codereview.chromium.org/1054823002/diff/1/Source/core/animation/Anima...
> File Source/core/animation/AnimationTimeline.cpp (right):
> 
>
https://codereview.chromium.org/1054823002/diff/1/Source/core/animation/Anima...
> Source/core/animation/AnimationTimeline.cpp:81: player->dispose();
> Hmm, this should probably just move to player->timelineDestroyed() The player
> should die together with it's timeline under Oilpan.
> 
> But, thinking about this patch more, we should just make setCompositorPending
> early out when timeline is null.
> 
> If this is causing problems for Oilpan it's fine to just revert this patch.

Thanks, it will be safe not to call dispose() here with Oilpan as the
ExecutionContext keeps a weak reference to the players in any case.

Will address (oilpan bots are a bit red.)

[loyso (OOO), 2015-04-13 00:05:16]

https://codereview.chromium.org/1054823002/diff/1/Source/core/animation/Anima...
File Source/core/animation/AnimationTimeline.cpp (right):

https://codereview.chromium.org/1054823002/diff/1/Source/core/animation/Anima...
Source/core/animation/AnimationTimeline.cpp:80: for (const auto& player :
m_players)
On 2015/04/10 09:12:52, sof wrote:
> Why are their two loops over m_players here (and dispose() first)?
> 
> Accessing other heap objects (AnimationPlayer and the hash set) is not allowed
> from within dtors with Oilpan -- finalization isn't ordered.

Nice to know from now! Sorry and thanks, sof!