Add pre finalizer to Animation to ensure compositor handles get cleaned up

Add pre finalizer to Animation to ensure compositor handles get cleaned up

BUG=631052
Committed: https://crrev.com/7fec064b3a37032605f091ba1ba52846c0e05824
Cr-Commit-Position: refs/heads/master@{#408554}

[alancutter (OOO until 2018), 2016-07-28 10:21:14]

Description was changed from

==========
fix

BUG=
==========

to

==========
Add pre finalizer to Animation to ensure compositor handles get cleaned up

BUG=631052
==========

[alancutter (OOO until 2018), 2016-07-28 10:21:14]

alancutter@chromium.org changed reviewers:
+ haraken@chromium.org

[alancutter (OOO until 2018), 2016-07-28 10:25:45]

I'm not sure what the best way to test this change is as the bug is from
threaded access to Animation from the impl thread during a GC between deleting
Animation and calling ~Animation().

There's no way to pause Oilpan half way between deleting and destruction in a
unit test is there?

[alancutter (OOO until 2018), 2016-07-28 10:30:12]

The CQ bit was checked by alancutter@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-28 10:30:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[haraken, 2016-07-28 10:31:25]

On 2016/07/28 10:25:45, alancutter wrote:
> I'm not sure what the best way to test this change is as the bug is from
> threaded access to Animation from the impl thread during a GC between deleting
> Animation and calling ~Animation().
> 
> There's no way to pause Oilpan half way between deleting and destruction in a
> unit test is there?

Animation::dispose() is already called by AnimationTimeline's pre-finalizer,
isn't it?

[alancutter (OOO until 2018), 2016-07-28 10:37:51]

On 2016/07/28 at 10:31:25, haraken wrote:
> On 2016/07/28 10:25:45, alancutter wrote:
> > I'm not sure what the best way to test this change is as the bug is from
> > threaded access to Animation from the impl thread during a GC between
deleting
> > Animation and calling ~Animation().
> > 
> > There's no way to pause Oilpan half way between deleting and destruction in
a
> > unit test is there?
> 
> Animation::dispose() is already called by AnimationTimeline's pre-finalizer,
isn't it?

An Animation's lifetime can be shorter than AnimationTimeline.

[commit-bot: I haz the power, 2016-07-28 10:40:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-28 10:40:13]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[haraken, 2016-07-28 10:40:46]

On 2016/07/28 10:37:51, alancutter wrote:
> On 2016/07/28 at 10:31:25, haraken wrote:
> > On 2016/07/28 10:25:45, alancutter wrote:
> > > I'm not sure what the best way to test this change is as the bug is from
> > > threaded access to Animation from the impl thread during a GC between
> deleting
> > > Animation and calling ~Animation().
> > > 
> > > There's no way to pause Oilpan half way between deleting and destruction
in
> a
> > > unit test is there?
> > 
> > Animation::dispose() is already called by AnimationTimeline's pre-finalizer,
> isn't it?
> 
> An Animation's lifetime can be shorter than AnimationTimeline.

Would it be possible to explicitly call dispose() when the AnimationTimeline
loses references to the Animation object?

[alancutter (OOO until 2018), 2016-07-28 10:52:22]

On 2016/07/28 at 10:40:46, haraken wrote:
> On 2016/07/28 10:37:51, alancutter wrote:
> > On 2016/07/28 at 10:31:25, haraken wrote:
> > > On 2016/07/28 10:25:45, alancutter wrote:
> > > > I'm not sure what the best way to test this change is as the bug is from
> > > > threaded access to Animation from the impl thread during a GC between
> > deleting
> > > > Animation and calling ~Animation().
> > > > 
> > > > There's no way to pause Oilpan half way between deleting and destruction
in
> > a
> > > > unit test is there?
> > > 
> > > Animation::dispose() is already called by AnimationTimeline's
pre-finalizer,
> > isn't it?
> > 
> > An Animation's lifetime can be shorter than AnimationTimeline.
> 
> Would it be possible to explicitly call dispose() when the AnimationTimeline
loses references to the Animation object?

AnimationTimeline never explicitly removes its references to its Animations. It
keeps its references in a HeapHashSet<WeakMember<Animation>> so it's up to the
GC when they leave.

[haraken, 2016-07-28 10:57:11]

On 2016/07/28 10:52:22, alancutter wrote:
> On 2016/07/28 at 10:40:46, haraken wrote:
> > On 2016/07/28 10:37:51, alancutter wrote:
> > > On 2016/07/28 at 10:31:25, haraken wrote:
> > > > On 2016/07/28 10:25:45, alancutter wrote:
> > > > > I'm not sure what the best way to test this change is as the bug is
from
> > > > > threaded access to Animation from the impl thread during a GC between
> > > deleting
> > > > > Animation and calling ~Animation().
> > > > > 
> > > > > There's no way to pause Oilpan half way between deleting and
destruction
> in
> > > a
> > > > > unit test is there?
> > > > 
> > > > Animation::dispose() is already called by AnimationTimeline's
> pre-finalizer,
> > > isn't it?
> > > 
> > > An Animation's lifetime can be shorter than AnimationTimeline.
> > 
> > Would it be possible to explicitly call dispose() when the AnimationTimeline
> loses references to the Animation object?
> 
> AnimationTimeline never explicitly removes its references to its Animations.
It
> keeps its references in a HeapHashSet<WeakMember<Animation>> so it's up to the
> GC when they leave.

Ah, thanks. Now I understand. Then this CL makes sense, but we should instead
remove the pre-finalizer from AnimationTimeline (since it's no longer needed).

Just to confirm: I'm assuming that there won't be too many Animation objects,
but is it correct?

[alancutter (OOO until 2018), 2016-07-28 11:13:26]

On 2016/07/28 at 10:57:11, haraken wrote:
> Ah, thanks. Now I understand. Then this CL makes sense, but we should instead
remove the pre-finalizer from AnimationTimeline (since it's no longer needed).
> 
> Just to confirm: I'm assuming that there won't be too many Animation objects,
but is it correct?

Most sites will have <100 at a time.
If performance is an issue would it be acceptable to move the call to
registerPreFinalizer(this) down into Animation::createCompositorPlayer()?
The dispose() won't do anything unless createCompositorPlayer() has been called
which is not the case for a large number of animations.

[haraken, 2016-07-28 11:16:26]

On 2016/07/28 11:13:26, alancutter wrote:
> On 2016/07/28 at 10:57:11, haraken wrote:
> > Ah, thanks. Now I understand. Then this CL makes sense, but we should
instead
> remove the pre-finalizer from AnimationTimeline (since it's no longer needed).
> > 
> > Just to confirm: I'm assuming that there won't be too many Animation
objects,
> but is it correct?
> 
> Most sites will have <100 at a time.
> If performance is an issue would it be acceptable to move the call to
> registerPreFinalizer(this) down into Animation::createCompositorPlayer()?
> The dispose() won't do anything unless createCompositorPlayer() has been
called
> which is not the case for a large number of animations.

Yeah, that sounds nicer.

[alancutter (OOO until 2018), 2016-07-28 11:41:32]

On 2016/07/28 at 11:16:26, haraken wrote:
> On 2016/07/28 11:13:26, alancutter wrote:
> > On 2016/07/28 at 10:57:11, haraken wrote:
> > > Ah, thanks. Now I understand. Then this CL makes sense, but we should
instead
> > remove the pre-finalizer from AnimationTimeline (since it's no longer
needed).
> > > 
> > > Just to confirm: I'm assuming that there won't be too many Animation
objects,
> > but is it correct?
> > 
> > Most sites will have <100 at a time.
> > If performance is an issue would it be acceptable to move the call to
> > registerPreFinalizer(this) down into Animation::createCompositorPlayer()?
> > The dispose() won't do anything unless createCompositorPlayer() has been
called
> > which is not the case for a large number of animations.
> 
> Yeah, that sounds nicer.

Moved the call to registerPreFinalizer().

[alancutter (OOO until 2018), 2016-07-28 11:41:43]

The CQ bit was checked by alancutter@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-28 11:42:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-28 11:51:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-28 11:51:19]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[haraken, 2016-07-28 11:51:51]

On 2016/07/28 11:41:32, alancutter wrote:
> On 2016/07/28 at 11:16:26, haraken wrote:
> > On 2016/07/28 11:13:26, alancutter wrote:
> > > On 2016/07/28 at 10:57:11, haraken wrote:
> > > > Ah, thanks. Now I understand. Then this CL makes sense, but we should
> instead
> > > remove the pre-finalizer from AnimationTimeline (since it's no longer
> needed).
> > > > 
> > > > Just to confirm: I'm assuming that there won't be too many Animation
> objects,
> > > but is it correct?
> > > 
> > > Most sites will have <100 at a time.
> > > If performance is an issue would it be acceptable to move the call to
> > > registerPreFinalizer(this) down into Animation::createCompositorPlayer()?
> > > The dispose() won't do anything unless createCompositorPlayer() has been
> called
> > > which is not the case for a large number of animations.
> > 
> > Yeah, that sounds nicer.
> 
> Moved the call to registerPreFinalizer().

Thanks. And can we now remove the pre-finalizer from AnimationTlimeline?

[alancutter (OOO until 2018), 2016-07-28 12:01:04]

On 2016/07/28 at 11:51:51, haraken wrote:
> On 2016/07/28 11:41:32, alancutter wrote:
> > Moved the call to registerPreFinalizer().
> 
> Thanks. And can we now remove the pre-finalizer from AnimationTlimeline?

I believe so. Moved the dispose() calls into ~AnimationTimeline and removed its
pre-finalizer.

[haraken, 2016-07-28 12:04:26]

https://codereview.chromium.org/2188623006/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/animation/AnimationTimeline.cpp (right):

https://codereview.chromium.org/2188623006/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/animation/AnimationTimeline.cpp:92:
animation->dispose();

I was assuming that we can entirely stop calling animation->dispose() because
now Animation objects dispose itself in their pre-finalizer.

If my understanding is wrong, we need to keep AnimationTimeline's pre-finalizer
as is because AnimationTimeline's destructor is not allowed to touch other
on-heap objects (i.e., m_animations).

[alancutter (OOO until 2018), 2016-07-28 12:34:31]

On 2016/07/28 at 12:04:26, haraken wrote:
>
https://codereview.chromium.org/2188623006/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/animation/AnimationTimeline.cpp (right):
> 
>
https://codereview.chromium.org/2188623006/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/animation/AnimationTimeline.cpp:92:
animation->dispose();
> 
> I was assuming that we can entirely stop calling animation->dispose() because
now Animation objects dispose itself in their pre-finalizer.
> 
> If my understanding is wrong, we need to keep AnimationTimeline's
pre-finalizer as is because AnimationTimeline's destructor is not allowed to
touch other on-heap objects (i.e., m_animations).

I was thinking it might still be needed in case Animations outlive
AnimationTimeline but since Animations have a Member<AnimationTimeline> that
won't be the case.
Removed ~AnimationTimeline(), GarbageCollectedFinalized is still needed due to
unique_ptr member.

Do you mind if I make these changes to AnimationTimeline in a follow up patch?
I'd like to keep this patch as small as possible.

[haraken, 2016-07-28 12:35:38]

On 2016/07/28 12:34:31, alancutter wrote:
> On 2016/07/28 at 12:04:26, haraken wrote:
> >
>
https://codereview.chromium.org/2188623006/diff/60001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/animation/AnimationTimeline.cpp (right):
> > 
> >
>
https://codereview.chromium.org/2188623006/diff/60001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/animation/AnimationTimeline.cpp:92:
> animation->dispose();
> > 
> > I was assuming that we can entirely stop calling animation->dispose()
because
> now Animation objects dispose itself in their pre-finalizer.
> > 
> > If my understanding is wrong, we need to keep AnimationTimeline's
> pre-finalizer as is because AnimationTimeline's destructor is not allowed to
> touch other on-heap objects (i.e., m_animations).
> 
> I was thinking it might still be needed in case Animations outlive
> AnimationTimeline but since Animations have a Member<AnimationTimeline> that
> won't be the case.
> Removed ~AnimationTimeline(), GarbageCollectedFinalized is still needed due to
> unique_ptr member.
> 
> Do you mind if I make these changes to AnimationTimeline in a follow up patch?
> I'd like to keep this patch as small as possible.

Sure, PS3 LGTM

[alancutter (OOO until 2018), 2016-07-28 12:41:38]

On 2016/07/28 at 12:35:38, haraken wrote:
> On 2016/07/28 12:34:31, alancutter wrote:
> > On 2016/07/28 at 12:04:26, haraken wrote:
> > >
> >
https://codereview.chromium.org/2188623006/diff/60001/third_party/WebKit/Sour...
> > > File third_party/WebKit/Source/core/animation/AnimationTimeline.cpp
(right):
> > > 
> > >
> >
https://codereview.chromium.org/2188623006/diff/60001/third_party/WebKit/Sour...
> > > third_party/WebKit/Source/core/animation/AnimationTimeline.cpp:92:
> > animation->dispose();
> > > 
> > > I was assuming that we can entirely stop calling animation->dispose()
because
> > now Animation objects dispose itself in their pre-finalizer.
> > > 
> > > If my understanding is wrong, we need to keep AnimationTimeline's
> > pre-finalizer as is because AnimationTimeline's destructor is not allowed to
> > touch other on-heap objects (i.e., m_animations).
> > 
> > I was thinking it might still be needed in case Animations outlive
> > AnimationTimeline but since Animations have a Member<AnimationTimeline> that
> > won't be the case.
> > Removed ~AnimationTimeline(), GarbageCollectedFinalized is still needed due
to
> > unique_ptr member.
> > 
> > Do you mind if I make these changes to AnimationTimeline in a follow up
patch?
> > I'd like to keep this patch as small as possible.
> 
> Sure, PS3 LGTM

Thanks for the after hours review!

[alancutter (OOO until 2018), 2016-07-28 12:41:47]

The CQ bit was checked by alancutter@chromium.org

[alancutter (OOO until 2018), 2016-07-28 12:41:47]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/2188623006/#ps100001
(title: "Remove AnimationTimeline changes")

[commit-bot: I haz the power, 2016-07-28 12:42:01]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-28 12:53:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-28 12:53:09]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[alancutter (OOO until 2018), 2016-07-29 00:51:52]

The CQ bit was checked by alancutter@chromium.org

[alancutter (OOO until 2018), 2016-07-29 00:51:52]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/2188623006/#ps120001
(title: "Add bool check")

[commit-bot: I haz the power, 2016-07-29 00:52:15]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-29 02:03:50]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-07-29 02:05:51]

Description was changed from

==========
Add pre finalizer to Animation to ensure compositor handles get cleaned up

BUG=631052
==========

to

==========
Add pre finalizer to Animation to ensure compositor handles get cleaned up

BUG=631052

Committed: https://crrev.com/7fec064b3a37032605f091ba1ba52846c0e05824
Cr-Commit-Position: refs/heads/master@{#408554}
==========

[commit-bot: I haz the power, 2016-07-29 02:05:52]

Patchset 7 (id:??) landed as
https://crrev.com/7fec064b3a37032605f091ba1ba52846c0e05824
Cr-Commit-Position: refs/heads/master@{#408554}