Oilpan: Fix SVGElement leaks introduced in 201698

Oilpan: Fix SVGElement leaks introduced in 201698

The Persistent handle introduced to SVGInterpolation in 201698 created the following cycle.

SVGInterporation =(Persistent)=>
SVGAnimatedProperty =(Member)=>
SVGElement =(Member)=>
ElementRareData =(Member)=>
ElementAnimations =(part of object)=>
CSSAnimations =(part of object)=>
CSSAnimationUpdate =(Member)=>
NewTransition =(Member)=>
InertEffect =(Member)=>
EffectModel =(RefPtr)=>
InterpolationEffect =(RefPtr)=>
InterpolationRecord =(RefPtr)=>
SVGInterpolation

This CL breaks the cycle by making SVGAnimatedProperty::m_contextElement a raw pointer.

BUG=

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

[haraken, 2015-09-04 01:06:59]

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

[haraken, 2015-09-04 01:09:54]

PTAL

Actually I was assuming that the Member reference from ElementAnimations to
CSSAnimationUpdate is temporary (i.e., the CSSAnimationUpdate is cleared at each
animation frame).

Either way, it looks risky to have a Member reference from a leaf node such as
SVGAnimatedProperty back to the Node hierarchy. I won't be surprised if it
causes document leaks somehow.

[sof, 2015-09-04 07:16:50]

lgtm

https://codereview.chromium.org/1325433005/diff/1/Source/core/svg/properties/...
File Source/core/svg/properties/SVGAnimatedProperty.h (right):

https://codereview.chromium.org/1325433005/diff/1/Source/core/svg/properties/...
Source/core/svg/properties/SVGAnimatedProperty.h:120: SVGElement*
m_contextElement;
Add a GC_PLUGIN_IGNORE() (and a tracking issue, if you want to) ?

[haraken, 2015-09-04 07:18:03]

https://codereview.chromium.org/1325433005/diff/1/Source/core/svg/properties/...
File Source/core/svg/properties/SVGAnimatedProperty.h (right):

https://codereview.chromium.org/1325433005/diff/1/Source/core/svg/properties/...
Source/core/svg/properties/SVGAnimatedProperty.h:120: SVGElement*
m_contextElement;
On 2015/09/04 07:16:49, sof wrote:
> Add a GC_PLUGIN_IGNORE() (and a tracking issue, if you want to) ?

Will do in a follow-up (to fix the leak in oilpan build asap).

[haraken, 2015-09-04 07:18:07]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2015-09-04 07:18:17]

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

[commit-bot: I haz the power, 2015-09-04 07:22:29]

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

[sof, 2015-09-04 10:56:02]

On 2015/09/04 07:18:03, haraken wrote:
>
https://codereview.chromium.org/1325433005/diff/1/Source/core/svg/properties/...
> File Source/core/svg/properties/SVGAnimatedProperty.h (right):
> 
>
https://codereview.chromium.org/1325433005/diff/1/Source/core/svg/properties/...
> Source/core/svg/properties/SVGAnimatedProperty.h:120: SVGElement*
> m_contextElement;
> On 2015/09/04 07:16:49, sof wrote:
> > Add a GC_PLUGIN_IGNORE() (and a tracking issue, if you want to) ?
> 
> Will do in a follow-up (to fix the leak in oilpan build asap).

makes good sense. it seems like this only partially addressed the leaks --

 
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Oilpan%...

[haraken, 2015-09-04 12:19:37]

On 2015/09/04 10:56:02, sof wrote:
> On 2015/09/04 07:18:03, haraken wrote:
> >
>
https://codereview.chromium.org/1325433005/diff/1/Source/core/svg/properties/...
> > File Source/core/svg/properties/SVGAnimatedProperty.h (right):
> > 
> >
>
https://codereview.chromium.org/1325433005/diff/1/Source/core/svg/properties/...
> > Source/core/svg/properties/SVGAnimatedProperty.h:120: SVGElement*
> > m_contextElement;
> > On 2015/09/04 07:16:49, sof wrote:
> > > Add a GC_PLUGIN_IGNORE() (and a tracking issue, if you want to) ?
> > 
> > Will do in a follow-up (to fix the leak in oilpan build asap).
> 
> makes good sense. it seems like this only partially addressed the leaks --
> 
>  
>
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Oilpan%...

looking.

[haraken, 2015-09-04 12:35:52]

On 2015/09/04 12:19:37, haraken wrote:
> On 2015/09/04 10:56:02, sof wrote:
> > On 2015/09/04 07:18:03, haraken wrote:
> > >
> >
>
https://codereview.chromium.org/1325433005/diff/1/Source/core/svg/properties/...
> > > File Source/core/svg/properties/SVGAnimatedProperty.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1325433005/diff/1/Source/core/svg/properties/...
> > > Source/core/svg/properties/SVGAnimatedProperty.h:120: SVGElement*
> > > m_contextElement;
> > > On 2015/09/04 07:16:49, sof wrote:
> > > > Add a GC_PLUGIN_IGNORE() (and a tracking issue, if you want to) ?
> > > 
> > > Will do in a follow-up (to fix the leak in oilpan build asap).
> > 
> > makes good sense. it seems like this only partially addressed the leaks --
> > 
> >  
> >
>
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Oilpan%...
> 
> looking.

Changing the Persistent for SVGInterpolatoin::m_attribute to WeakPersistent
resolves the leak. Which means that SVGAnimatedPropertyBase is still holding
some strong reference to the Node hierarchy or something.

[haraken, 2015-09-04 12:38:12]

On 2015/09/04 12:35:52, haraken wrote:
> On 2015/09/04 12:19:37, haraken wrote:
> > On 2015/09/04 10:56:02, sof wrote:
> > > On 2015/09/04 07:18:03, haraken wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/1325433005/diff/1/Source/core/svg/properties/...
> > > > File Source/core/svg/properties/SVGAnimatedProperty.h (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1325433005/diff/1/Source/core/svg/properties/...
> > > > Source/core/svg/properties/SVGAnimatedProperty.h:120: SVGElement*
> > > > m_contextElement;
> > > > On 2015/09/04 07:16:49, sof wrote:
> > > > > Add a GC_PLUGIN_IGNORE() (and a tracking issue, if you want to) ?
> > > > 
> > > > Will do in a follow-up (to fix the leak in oilpan build asap).
> > > 
> > > makes good sense. it seems like this only partially addressed the leaks --
> > > 
> > >  
> > >
> >
>
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Oilpan%...
> > 
> > looking.
> 
> Changing the Persistent for SVGInterpolatoin::m_attribute to WeakPersistent
> resolves the leak. Which means that SVGAnimatedPropertyBase is still holding
> some strong reference to the Node hierarchy or something.

ah, it's SVGPropertyTearOffBase::m_contextElement. Will handle it.