Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
5 years, 8 months ago by loyso (OOO) Modified:
5 years, 8 months ago Reviewers:
dstockwell, sof CC:
dstockwell, darktears, blink-reviews, blink-reviews-animation_chromium.org, Eric Willigers, Mike Lawther (Google), oilpan-reviews, rjwright, shans, Steve Block, Timothy Loh Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionUnregister AnimationPlayer from ActiveDOMObjects on timeline deletion.
BUG=472307
R=dstockwell@chromium.org
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=193510
Patch Set 1 #
Total comments: 4
Created: 5 years, 8 months ago
Messages
Total messages: 14 (3 generated)
loyso@chromium.org changed reviewers: + dstockwell@chromium.org
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.
PTAL!
The CQ bit was checked by dstockwell@chromium.org
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1054823002/1
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://src.chromium.org/viewvc/blink?view=rev&revision=193510
Message was sent while issue was closed.
sigbjornf@opera.com changed reviewers: + sigbjornf@opera.com
Message was sent while issue was closed.
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.
Message was sent while issue was closed.
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.
Message was sent while issue was closed.
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.)
Message was sent while issue was closed.
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! |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
