Oilpan: KeyframeEffect::m_target shouldn't become stale

Oilpan: KeyframeEffect::m_target shouldn't become stale

r201633 shipped oilpan for core/animations/, but I removed !ENABLE(OILPAN) code too much.
Some of them shouldn't be removed until we ship Oilpan for the Node hierarchy.

This CL recovers the code to clear KeyFrameEffect::m_target when the target Element gets destructed.

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

[haraken, 2015-09-03 00:33:36]

haraken@chromium.org changed reviewers:
+ oilpan-reviews@chromium.org, sigbjornf@opera.com

[haraken, 2015-09-03 00:33:37]

PTAL

[sof, 2015-09-03 05:21:46]

https://codereview.chromium.org/1322883003/diff/40001/Source/core/animation/E...
File Source/core/animation/ElementAnimations.cpp (right):

https://codereview.chromium.org/1322883003/diff/40001/Source/core/animation/E...
Source/core/animation/ElementAnimations.cpp:46: for (KeyframeEffect* effect :
m_effects)
Aren't you touching a heap object here?

And how are notifyElementDestroyed() notifications handled for weakly held
objects that are destructed in GCs prior to their ElementAnimations-registered
object?

[haraken, 2015-09-03 05:55:34]

On 2015/09/03 05:21:46, sof wrote:
>
https://codereview.chromium.org/1322883003/diff/40001/Source/core/animation/E...
> File Source/core/animation/ElementAnimations.cpp (right):
> 
>
https://codereview.chromium.org/1322883003/diff/40001/Source/core/animation/E...
> Source/core/animation/ElementAnimations.cpp:46: for (KeyframeEffect* effect :
> m_effects)
> Aren't you touching a heap object here?
> 
> And how are notifyElementDestroyed() notifications handled for weakly held
> objects that are destructed in GCs prior to their ElementAnimations-registered
> object?

Nice catch; fixed the issue in PS4. PTAL?

[sof, 2015-09-03 06:08:27]

On 2015/09/03 05:55:34, haraken wrote:
> On 2015/09/03 05:21:46, sof wrote:
> >
>
https://codereview.chromium.org/1322883003/diff/40001/Source/core/animation/E...
> > File Source/core/animation/ElementAnimations.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1322883003/diff/40001/Source/core/animation/E...
> > Source/core/animation/ElementAnimations.cpp:46: for (KeyframeEffect* effect
:
> > m_effects)
> > Aren't you touching a heap object here?
> > 
> > And how are notifyElementDestroyed() notifications handled for weakly held
> > objects that are destructed in GCs prior to their
ElementAnimations-registered
> > object?
> 
> Nice catch; fixed the issue in PS4. PTAL?

That looks good wrt the first question; what about the second?

[haraken, 2015-09-03 06:15:57]

On 2015/09/03 06:08:27, sof wrote:
> On 2015/09/03 05:55:34, haraken wrote:
> > On 2015/09/03 05:21:46, sof wrote:
> > >
> >
>
https://codereview.chromium.org/1322883003/diff/40001/Source/core/animation/E...
> > > File Source/core/animation/ElementAnimations.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1322883003/diff/40001/Source/core/animation/E...
> > > Source/core/animation/ElementAnimations.cpp:46: for (KeyframeEffect*
effect
> :
> > > m_effects)
> > > Aren't you touching a heap object here?
> > > 
> > > And how are notifyElementDestroyed() notifications handled for weakly held
> > > objects that are destructed in GCs prior to their
> ElementAnimations-registered
> > > object?
> > 
> > Nice catch; fixed the issue in PS4. PTAL?
> 
> That looks good wrt the first question; what about the second?

I don't fully understand the second question. KeyfrmaeEffects that have been
already destructed shouldn't be stored in ElementAnimations::m_effects when we
reach ElementAnimations::dispose(), should it?

[sof, 2015-09-03 06:18:24]

On 2015/09/03 06:15:57, haraken wrote:
> On 2015/09/03 06:08:27, sof wrote:
> > On 2015/09/03 05:55:34, haraken wrote:
> > > On 2015/09/03 05:21:46, sof wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/1322883003/diff/40001/Source/core/animation/E...
> > > > File Source/core/animation/ElementAnimations.cpp (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1322883003/diff/40001/Source/core/animation/E...
> > > > Source/core/animation/ElementAnimations.cpp:46: for (KeyframeEffect*
> effect
> > :
> > > > m_effects)
> > > > Aren't you touching a heap object here?
> > > > 
> > > > And how are notifyElementDestroyed() notifications handled for weakly
held
> > > > objects that are destructed in GCs prior to their
> > ElementAnimations-registered
> > > > object?
> > > 
> > > Nice catch; fixed the issue in PS4. PTAL?
> > 
> > That looks good wrt the first question; what about the second?
> 
> I don't fully understand the second question. KeyfrmaeEffects that have been
> already destructed shouldn't be stored in ElementAnimations::m_effects when we
> reach ElementAnimations::dispose(), should it?

They shouldn't, but who gets to call notifyElementDestroyed() for those?

[haraken, 2015-09-03 07:19:39]

On 2015/09/03 06:18:24, sof wrote:
> On 2015/09/03 06:15:57, haraken wrote:
> > On 2015/09/03 06:08:27, sof wrote:
> > > On 2015/09/03 05:55:34, haraken wrote:
> > > > On 2015/09/03 05:21:46, sof wrote:
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1322883003/diff/40001/Source/core/animation/E...
> > > > > File Source/core/animation/ElementAnimations.cpp (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1322883003/diff/40001/Source/core/animation/E...
> > > > > Source/core/animation/ElementAnimations.cpp:46: for (KeyframeEffect*
> > effect
> > > :
> > > > > m_effects)
> > > > > Aren't you touching a heap object here?
> > > > > 
> > > > > And how are notifyElementDestroyed() notifications handled for weakly
> held
> > > > > objects that are destructed in GCs prior to their
> > > ElementAnimations-registered
> > > > > object?
> > > > 
> > > > Nice catch; fixed the issue in PS4. PTAL?
> > > 
> > > That looks good wrt the first question; what about the second?
> > 
> > I don't fully understand the second question. KeyfrmaeEffects that have been
> > already destructed shouldn't be stored in ElementAnimations::m_effects when
we
> > reach ElementAnimations::dispose(), should it?
> 
> They shouldn't, but who gets to call notifyElementDestroyed() for those?

As far as I understand, notifyElementDestroyed() doesn't need to be called for
already dead KeyframeEffects. notifyElementDestroyed() is just to clear raw
pointers in KeyframeEffect so that they don't become stale.

[sof, 2015-09-03 07:30:26]

On 2015/09/03 07:19:39, haraken wrote:
> On 2015/09/03 06:18:24, sof wrote:
> > On 2015/09/03 06:15:57, haraken wrote:
> > > On 2015/09/03 06:08:27, sof wrote:
> > > > On 2015/09/03 05:55:34, haraken wrote:
> > > > > On 2015/09/03 05:21:46, sof wrote:
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1322883003/diff/40001/Source/core/animation/E...
> > > > > > File Source/core/animation/ElementAnimations.cpp (right):
> > > > > > 
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1322883003/diff/40001/Source/core/animation/E...
> > > > > > Source/core/animation/ElementAnimations.cpp:46: for (KeyframeEffect*
> > > effect
> > > > :
> > > > > > m_effects)
> > > > > > Aren't you touching a heap object here?
> > > > > > 
> > > > > > And how are notifyElementDestroyed() notifications handled for
weakly
> > held
> > > > > > objects that are destructed in GCs prior to their
> > > > ElementAnimations-registered
> > > > > > object?
> > > > > 
> > > > > Nice catch; fixed the issue in PS4. PTAL?
> > > > 
> > > > That looks good wrt the first question; what about the second?
> > > 
> > > I don't fully understand the second question. KeyfrmaeEffects that have
been
> > > already destructed shouldn't be stored in ElementAnimations::m_effects
when
> we
> > > reach ElementAnimations::dispose(), should it?
> > 
> > They shouldn't, but who gets to call notifyElementDestroyed() for those?
> 
> As far as I understand, notifyElementDestroyed() doesn't need to be called for
> already dead KeyframeEffects. notifyElementDestroyed() is just to clear raw
> pointers in KeyframeEffect so that they don't become stale.

It seems like that is all it does; possibly add a comment saying that dispose()
is called on effects that live beyond the 'owning' element, so as to clear out
the back reference?

The explicit dispose() will invoke the notification on effects that are possibly
GCed & don't need to be told strictly speaking, but that seems quite acceptable.

[sof, 2015-09-03 07:31:04]

lgtm, update the CL title to something a bit more descriptive?

[haraken, 2015-09-03 08:41:54]

On 2015/09/03 07:31:04, sof wrote:
> update the CL title to something a bit more descriptive?

Added a comment and updated the CL title.

[haraken, 2015-09-03 08:41:57]

The CQ bit was checked by haraken@chromium.org

[haraken, 2015-09-03 08:41:57]

The patchset sent to the CQ was uploaded after l-g-t-m from sigbjornf@opera.com
Link to the patchset: https://codereview.chromium.org/1322883003/#ps100001
(title: " ")

[commit-bot: I haz the power, 2015-09-03 08:42:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1322883003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1322883003/100001

[commit-bot: I haz the power, 2015-09-03 09:51:05]

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