Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1515573002:
Safely finalize an AnimationTimeline's still-attached Animations. (Closed)
|
Created:
5 years ago by sof Modified:
5 years ago CC:
chromium-reviews, shans, rjwright, blink-reviews-animation_chromium.org, darktears, blink-reviews, Eric Willigers Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSafely finalize an AnimationTimeline's still-attached Animations.
R=haraken
BUG=568084
Committed: https://crrev.com/6275d5b1652c48f4066f1752a091dea80d26e9ba
Cr-Commit-Position: refs/heads/master@{#364300}
Patch Set 1 #Patch Set 2 : complete the comment #Patch Set 3 : dispose of AnimationTimelines irrespective of ENABLE(OILPAN) #
Total comments: 4
Patch Set 4 : unconditonally register prefinalizer #
Messages
Total messages: 22 (7 generated)
sigbjornf@opera.com changed reviewers: + dstockwell@chromium.org, oilpan-reviews@chromium.org
please take a look, associated bug has some stack traces of what is going wrong currently. There won't be very many of these AnimationTimeline objects, hence excessive prefinalizer buildup shouldn't be an issue.
Description was changed from ========== Oilpan: safely finalize an AnimationTimeline's still-attached Animations. R= BUG=568084 ========== to ========== Safely finalize an AnimationTimeline's still-attached Animations. R= BUG=568084 ==========
haraken@chromium.org changed reviewers: + haraken@chromium.org
https://codereview.chromium.org/1515573002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/animation/Animation.cpp (right): https://codereview.chromium.org/1515573002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/animation/Animation.cpp:114: destroyCompositorPlayer(); Can we remove this now? https://codereview.chromium.org/1515573002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/animation/AnimationTimeline.cpp (right): https://codereview.chromium.org/1515573002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/animation/AnimationTimeline.cpp:101: // impending destruction. Why can't we add a pre-finalizer on Animation? The pre-finalizer should be able to access the AnimationTimeline safely.
https://codereview.chromium.org/1515573002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/animation/Animation.cpp (right): https://codereview.chromium.org/1515573002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/animation/Animation.cpp:114: destroyCompositorPlayer(); On 2015/12/09 14:02:04, haraken wrote: > > Can we remove this now? No, the AnimationTimeline keeps a weak reference to its animations. So Animation objects may be swept independently. afaict. https://codereview.chromium.org/1515573002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/animation/AnimationTimeline.cpp (right): https://codereview.chromium.org/1515573002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/animation/AnimationTimeline.cpp:101: // impending destruction. On 2015/12/09 14:02:04, haraken wrote: > > Why can't we add a pre-finalizer on Animation? The pre-finalizer should be able > to access the AnimationTimeline safely. > There's many more of these.
On 2015/12/09 14:04:33, sof wrote: > https://codereview.chromium.org/1515573002/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/animation/Animation.cpp (right): > > https://codereview.chromium.org/1515573002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/animation/Animation.cpp:114: > destroyCompositorPlayer(); > On 2015/12/09 14:02:04, haraken wrote: > > > > Can we remove this now? > > No, the AnimationTimeline keeps a weak reference to its animations. So Animation > objects may be swept independently. afaict. > > https://codereview.chromium.org/1515573002/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/animation/AnimationTimeline.cpp (right): > > https://codereview.chromium.org/1515573002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/animation/AnimationTimeline.cpp:101: // impending > destruction. > On 2015/12/09 14:02:04, haraken wrote: > > > > Why can't we add a pre-finalizer on Animation? The pre-finalizer should be > able > > to access the AnimationTimeline safely. > > > > There's many more of these. Hmm. A better fix might be: - Add a weak processing to the AnimationTimeline::m_animations. - Call compositorTimeline()->playerDestroyed() in the weak processing and when the AnimationTimeline gets destructed. ?
On 2015/12/09 14:15:33, haraken wrote: > On 2015/12/09 14:04:33, sof wrote: > > > https://codereview.chromium.org/1515573002/diff/40001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/animation/Animation.cpp (right): > > > > > https://codereview.chromium.org/1515573002/diff/40001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/animation/Animation.cpp:114: > > destroyCompositorPlayer(); > > On 2015/12/09 14:02:04, haraken wrote: > > > > > > Can we remove this now? > > > > No, the AnimationTimeline keeps a weak reference to its animations. So > Animation > > objects may be swept independently. afaict. > > > > > https://codereview.chromium.org/1515573002/diff/40001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/animation/AnimationTimeline.cpp (right): > > > > > https://codereview.chromium.org/1515573002/diff/40001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/animation/AnimationTimeline.cpp:101: // > impending > > destruction. > > On 2015/12/09 14:02:04, haraken wrote: > > > > > > Why can't we add a pre-finalizer on Animation? The pre-finalizer should be > > able > > > to access the AnimationTimeline safely. > > > > > > > There's many more of these. > > Hmm. A better fix might be: > > - Add a weak processing to the AnimationTimeline::m_animations. > - Call compositorTimeline()->playerDestroyed() in the weak processing and when > the AnimationTimeline gets destructed. > > ? you won't do any weak processing when the AnimationTimeline object dies.
On 2015/12/09 14:20:12, sof wrote: > On 2015/12/09 14:15:33, haraken wrote: > > On 2015/12/09 14:04:33, sof wrote: > > > > > > https://codereview.chromium.org/1515573002/diff/40001/third_party/WebKit/Sour... > > > File third_party/WebKit/Source/core/animation/Animation.cpp (right): > > > > > > > > > https://codereview.chromium.org/1515573002/diff/40001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/core/animation/Animation.cpp:114: > > > destroyCompositorPlayer(); > > > On 2015/12/09 14:02:04, haraken wrote: > > > > > > > > Can we remove this now? > > > > > > No, the AnimationTimeline keeps a weak reference to its animations. So > > Animation > > > objects may be swept independently. afaict. > > > > > > > > > https://codereview.chromium.org/1515573002/diff/40001/third_party/WebKit/Sour... > > > File third_party/WebKit/Source/core/animation/AnimationTimeline.cpp (right): > > > > > > > > > https://codereview.chromium.org/1515573002/diff/40001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/core/animation/AnimationTimeline.cpp:101: // > > impending > > > destruction. > > > On 2015/12/09 14:02:04, haraken wrote: > > > > > > > > Why can't we add a pre-finalizer on Animation? The pre-finalizer should be > > > able > > > > to access the AnimationTimeline safely. > > > > > > > > > > There's many more of these. > > > > Hmm. A better fix might be: > > > > - Add a weak processing to the AnimationTimeline::m_animations. > > - Call compositorTimeline()->playerDestroyed() in the weak processing and when > > the AnimationTimeline gets destructed. > > > > ? > > you won't do any weak processing when the AnimationTimeline object dies. And calling methods on other heap objects during weak processing is not something we typically do/allow.
On 2015/12/09 14:25:18, sof wrote: > On 2015/12/09 14:20:12, sof wrote: > > On 2015/12/09 14:15:33, haraken wrote: > > > On 2015/12/09 14:04:33, sof wrote: > > > > > > > > > > https://codereview.chromium.org/1515573002/diff/40001/third_party/WebKit/Sour... > > > > File third_party/WebKit/Source/core/animation/Animation.cpp (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1515573002/diff/40001/third_party/WebKit/Sour... > > > > third_party/WebKit/Source/core/animation/Animation.cpp:114: > > > > destroyCompositorPlayer(); > > > > On 2015/12/09 14:02:04, haraken wrote: > > > > > > > > > > Can we remove this now? > > > > > > > > No, the AnimationTimeline keeps a weak reference to its animations. So > > > Animation > > > > objects may be swept independently. afaict. > > > > > > > > > > > > > > https://codereview.chromium.org/1515573002/diff/40001/third_party/WebKit/Sour... > > > > File third_party/WebKit/Source/core/animation/AnimationTimeline.cpp > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1515573002/diff/40001/third_party/WebKit/Sour... > > > > third_party/WebKit/Source/core/animation/AnimationTimeline.cpp:101: // > > > impending > > > > destruction. > > > > On 2015/12/09 14:02:04, haraken wrote: > > > > > > > > > > Why can't we add a pre-finalizer on Animation? The pre-finalizer should > be > > > > able > > > > > to access the AnimationTimeline safely. > > > > > > > > > > > > > There's many more of these. > > > > > > Hmm. A better fix might be: > > > > > > - Add a weak processing to the AnimationTimeline::m_animations. > > > - Call compositorTimeline()->playerDestroyed() in the weak processing and > when > > > the AnimationTimeline gets destructed. > > > > > > ? > > > > you won't do any weak processing when the AnimationTimeline object dies. So I'm suggesting to call playerDestroyed() in both weak processing (i.e., Animation dies but AnimationTimeline doesn't die) and when the AnimationTimeline dies (we need to mark AnimationTimeline eagerlly-finalized). > > And calling methods on other heap objects during weak processing is not > something we typically do/allow. compositorTimeline()->playerDestroyed() won't touch any on-heap object, will it? That said, my proposal would be as nasty as this CL. So LGTM.
On 2015/12/09 15:34:12, haraken wrote: > On 2015/12/09 14:25:18, sof wrote: > > On 2015/12/09 14:20:12, sof wrote: > > > On 2015/12/09 14:15:33, haraken wrote: > > > > On 2015/12/09 14:04:33, sof wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/1515573002/diff/40001/third_party/WebKit/Sour... > > > > > File third_party/WebKit/Source/core/animation/Animation.cpp (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1515573002/diff/40001/third_party/WebKit/Sour... > > > > > third_party/WebKit/Source/core/animation/Animation.cpp:114: > > > > > destroyCompositorPlayer(); > > > > > On 2015/12/09 14:02:04, haraken wrote: > > > > > > > > > > > > Can we remove this now? > > > > > > > > > > No, the AnimationTimeline keeps a weak reference to its animations. So > > > > Animation > > > > > objects may be swept independently. afaict. > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1515573002/diff/40001/third_party/WebKit/Sour... > > > > > File third_party/WebKit/Source/core/animation/AnimationTimeline.cpp > > (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1515573002/diff/40001/third_party/WebKit/Sour... > > > > > third_party/WebKit/Source/core/animation/AnimationTimeline.cpp:101: // > > > > impending > > > > > destruction. > > > > > On 2015/12/09 14:02:04, haraken wrote: > > > > > > > > > > > > Why can't we add a pre-finalizer on Animation? The pre-finalizer > should > > be > > > > > able > > > > > > to access the AnimationTimeline safely. > > > > > > > > > > > > > > > > There's many more of these. > > > > > > > > Hmm. A better fix might be: > > > > > > > > - Add a weak processing to the AnimationTimeline::m_animations. > > > > - Call compositorTimeline()->playerDestroyed() in the weak processing and > > when > > > > the AnimationTimeline gets destructed. > > > > > > > > ? > > > > > > you won't do any weak processing when the AnimationTimeline object dies. > > So I'm suggesting to call playerDestroyed() in both weak processing (i.e., > Animation dies but AnimationTimeline doesn't die) and when the AnimationTimeline > dies (we need to mark AnimationTimeline eagerlly-finalized). > So that we can keep an empty Animation destructor. I think that adds more complexity overall by shifting things around & into work for the marking thread. > > > > And calling methods on other heap objects during weak processing is not > > something we typically do/allow. > > compositorTimeline()->playerDestroyed() won't touch any on-heap object, will it? > I'm not sure if it won't call back into its client argument, the WebCompositorAnimationTimelineImpl::playerDestroyed() override has a number of shutdown steps. > That said, my proposal would be as nasty as this CL. So LGTM. thanks, what's "nasty" about explicitly disposing before executing unordered finalizers? Tedious to have to be explicit & split this out, granted.
Will let dstockwell@ et al have a say if this is acceptable. Has threaded animation been turned on recently or has the test coverage improved? It appears to be a newer issue.
On 2015/12/09 15:51:08, sof wrote: > On 2015/12/09 15:34:12, haraken wrote: > > On 2015/12/09 14:25:18, sof wrote: > > > On 2015/12/09 14:20:12, sof wrote: > > > > On 2015/12/09 14:15:33, haraken wrote: > > > > > On 2015/12/09 14:04:33, sof wrote: > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1515573002/diff/40001/third_party/WebKit/Sour... > > > > > > File third_party/WebKit/Source/core/animation/Animation.cpp (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1515573002/diff/40001/third_party/WebKit/Sour... > > > > > > third_party/WebKit/Source/core/animation/Animation.cpp:114: > > > > > > destroyCompositorPlayer(); > > > > > > On 2015/12/09 14:02:04, haraken wrote: > > > > > > > > > > > > > > Can we remove this now? > > > > > > > > > > > > No, the AnimationTimeline keeps a weak reference to its animations. So > > > > > Animation > > > > > > objects may be swept independently. afaict. > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1515573002/diff/40001/third_party/WebKit/Sour... > > > > > > File third_party/WebKit/Source/core/animation/AnimationTimeline.cpp > > > (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1515573002/diff/40001/third_party/WebKit/Sour... > > > > > > third_party/WebKit/Source/core/animation/AnimationTimeline.cpp:101: // > > > > > impending > > > > > > destruction. > > > > > > On 2015/12/09 14:02:04, haraken wrote: > > > > > > > > > > > > > > Why can't we add a pre-finalizer on Animation? The pre-finalizer > > should > > > be > > > > > > able > > > > > > > to access the AnimationTimeline safely. > > > > > > > > > > > > > > > > > > > There's many more of these. > > > > > > > > > > Hmm. A better fix might be: > > > > > > > > > > - Add a weak processing to the AnimationTimeline::m_animations. > > > > > - Call compositorTimeline()->playerDestroyed() in the weak processing > and > > > when > > > > > the AnimationTimeline gets destructed. > > > > > > > > > > ? > > > > > > > > you won't do any weak processing when the AnimationTimeline object dies. > > > > So I'm suggesting to call playerDestroyed() in both weak processing (i.e., > > Animation dies but AnimationTimeline doesn't die) and when the > AnimationTimeline > > dies (we need to mark AnimationTimeline eagerlly-finalized). > > > > So that we can keep an empty Animation destructor. I think that adds more > complexity overall by shifting things around & into work for the marking thread. > > > > > > > And calling methods on other heap objects during weak processing is not > > > something we typically do/allow. > > > > compositorTimeline()->playerDestroyed() won't touch any on-heap object, will > it? > > > > I'm not sure if it won't call back into its client argument, the > WebCompositorAnimationTimelineImpl::playerDestroyed() override has a number of > shutdown steps. > > > That said, my proposal would be as nasty as this CL. So LGTM. > > thanks, what's "nasty" about explicitly disposing before executing unordered > finalizers? Tedious to have to be explicit & split this out, granted. If the Animation could have a pre-finalizer that calls detachCompositorPlayer(), I'm totally happy. However, we don't want to do that for performance reasons. Consequently we're adding a pre-finalizer to AnimationTimeline, leaving the destroyCompositorPlayer() in Animation's destructor. It is nasty in a sense that we need to reason about why it's structured that way and why it's safe. But I understand my proposal is more nasty than yours. This CL looks like a reasonable solution :)
On 2015/12/10 01:01:44, haraken wrote: > On 2015/12/09 15:51:08, sof wrote: > > On 2015/12/09 15:34:12, haraken wrote: > > > On 2015/12/09 14:25:18, sof wrote: > > > > On 2015/12/09 14:20:12, sof wrote: > > > > > On 2015/12/09 14:15:33, haraken wrote: > > > > > > On 2015/12/09 14:04:33, sof wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1515573002/diff/40001/third_party/WebKit/Sour... > > > > > > > File third_party/WebKit/Source/core/animation/Animation.cpp (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1515573002/diff/40001/third_party/WebKit/Sour... > > > > > > > third_party/WebKit/Source/core/animation/Animation.cpp:114: > > > > > > > destroyCompositorPlayer(); > > > > > > > On 2015/12/09 14:02:04, haraken wrote: > > > > > > > > > > > > > > > > Can we remove this now? > > > > > > > > > > > > > > No, the AnimationTimeline keeps a weak reference to its animations. > So > > > > > > Animation > > > > > > > objects may be swept independently. afaict. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1515573002/diff/40001/third_party/WebKit/Sour... > > > > > > > File third_party/WebKit/Source/core/animation/AnimationTimeline.cpp > > > > (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1515573002/diff/40001/third_party/WebKit/Sour... > > > > > > > third_party/WebKit/Source/core/animation/AnimationTimeline.cpp:101: > // > > > > > > impending > > > > > > > destruction. > > > > > > > On 2015/12/09 14:02:04, haraken wrote: > > > > > > > > > > > > > > > > Why can't we add a pre-finalizer on Animation? The pre-finalizer > > > should > > > > be > > > > > > > able > > > > > > > > to access the AnimationTimeline safely. > > > > > > > > > > > > > > > > > > > > > > There's many more of these. > > > > > > > > > > > > Hmm. A better fix might be: > > > > > > > > > > > > - Add a weak processing to the AnimationTimeline::m_animations. > > > > > > - Call compositorTimeline()->playerDestroyed() in the weak processing > > and > > > > when > > > > > > the AnimationTimeline gets destructed. > > > > > > > > > > > > ? > > > > > > > > > > you won't do any weak processing when the AnimationTimeline object dies. > > > > > > So I'm suggesting to call playerDestroyed() in both weak processing (i.e., > > > Animation dies but AnimationTimeline doesn't die) and when the > > AnimationTimeline > > > dies (we need to mark AnimationTimeline eagerlly-finalized). > > > > > > > So that we can keep an empty Animation destructor. I think that adds more > > complexity overall by shifting things around & into work for the marking > thread. > > > > > > > > > > And calling methods on other heap objects during weak processing is not > > > > something we typically do/allow. > > > > > > compositorTimeline()->playerDestroyed() won't touch any on-heap object, will > > it? > > > > > > > I'm not sure if it won't call back into its client argument, the > > WebCompositorAnimationTimelineImpl::playerDestroyed() override has a number of > > shutdown steps. > > > > > That said, my proposal would be as nasty as this CL. So LGTM. > > > > thanks, what's "nasty" about explicitly disposing before executing unordered > > finalizers? Tedious to have to be explicit & split this out, granted. > > If the Animation could have a pre-finalizer that calls detachCompositorPlayer(), > I'm totally happy. However, we don't want to do that for performance reasons. > Consequently we're adding a pre-finalizer to AnimationTimeline, leaving the > destroyCompositorPlayer() in Animation's destructor. It is nasty in a sense that > we need to reason about why it's structured that way and why it's safe. > > But I understand my proposal is more nasty than yours. This CL looks like a > reasonable solution :) It looks like dstockwell@ is ooo until 15th.
On 2015/12/10 01:01:44, haraken wrote: > ... > > > > > That said, my proposal would be as nasty as this CL. So LGTM. > > > > thanks, what's "nasty" about explicitly disposing before executing unordered > > finalizers? Tedious to have to be explicit & split this out, granted. > > If the Animation could have a pre-finalizer that calls detachCompositorPlayer(), > I'm totally happy. However, we don't want to do that for performance reasons. > Consequently we're adding a pre-finalizer to AnimationTimeline, leaving the > destroyCompositorPlayer() in Animation's destructor. It is nasty in a sense that > we need to reason about why it's structured that way and why it's safe. > Yes, it is not a common set of dependencies between objects here on finalization/unregistration. But figuring out ways to express those reliably with Oilpan will help the next time that pattern shows up, so hashing out the tradeoffs is worthwhile. Let's try going with ps#4.
Description was changed from ========== Safely finalize an AnimationTimeline's still-attached Animations. R= BUG=568084 ========== to ========== Safely finalize an AnimationTimeline's still-attached Animations. R=haraken BUG=568084 ==========
The CQ bit was checked by sigbjornf@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1515573002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1515573002/60001
Message was sent while issue was closed.
Description was changed from ========== Safely finalize an AnimationTimeline's still-attached Animations. R=haraken BUG=568084 ========== to ========== Safely finalize an AnimationTimeline's still-attached Animations. R=haraken BUG=568084 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Safely finalize an AnimationTimeline's still-attached Animations. R=haraken BUG=568084 ========== to ========== Safely finalize an AnimationTimeline's still-attached Animations. R=haraken BUG=568084 Committed: https://crrev.com/6275d5b1652c48f4066f1752a091dea80d26e9ba Cr-Commit-Position: refs/heads/master@{#364300} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/6275d5b1652c48f4066f1752a091dea80d26e9ba Cr-Commit-Position: refs/heads/master@{#364300} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
