Oilpan: Fix leaks through RenderStyle objects.

Oilpan: Fix leaks through RenderStyle objects.

RenderStyle objects are not on the heap and they keep the CSSFontSelector
and its associated ResourceFetcher alive. We turned raw pointers to the
document in CSSFontSelector and ResourceFetcher into traced Members. That
will leak until we can also trace through the RenderStyle structure.

See crbug.com/383860 for details.

This fixed the leaks by using WeakMembers to the document in these two
places. We really should trace through all of this but that is going
to be a larger change and we should fix this leak now.

R=haraken@chromium.org, oilpan-reviews@chromium.org, sigbjornf@opera.com
BUG=383860
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=176034

[sof, 2014-06-12 14:02:06]

lgtm.

This also puts a dent in
http://code.google.com/p/chromium/issues/detail?id=382880 ?

[Mads Ager (chromium), 2014-06-12 14:05:37]

Yes, I just double checked that that leak is also gone with this patch. It is
great that we have the leak detector to find these issues when we attempt to
make this pointer strong again. :)

[zerny-chromium, 2014-06-12 14:07:13]

lgtm2

[haraken, 2014-06-12 14:09:38]

LGTM

https://codereview.chromium.org/335633002/diff/1/Source/core/css/CSSFontSelec...
File Source/core/css/CSSFontSelector.h (right):

https://codereview.chromium.org/335633002/diff/1/Source/core/css/CSSFontSelec...
Source/core/css/CSSFontSelector.h:89: // FIXME: Oilpan: Ideally this should just
be a trace Member but that will

a trace Member => a traced Member

https://codereview.chromium.org/335633002/diff/1/Source/core/css/CSSFontSelec...
Source/core/css/CSSFontSelector.h:90: // currently leak because RenderStyle and
its data is not on the heap.

is not => are not

https://codereview.chromium.org/335633002/diff/1/Source/core/fetch/ResourceFe...
File Source/core/fetch/ResourceFetcher.h (right):

https://codereview.chromium.org/335633002/diff/1/Source/core/fetch/ResourceFe...
Source/core/fetch/ResourceFetcher.h:208: // FIXME: Oilpan: Ideally this should
just be a trace Member but that will

a traced Member

https://codereview.chromium.org/335633002/diff/1/Source/core/fetch/ResourceFe...
Source/core/fetch/ResourceFetcher.h:209: // currently leak because RenderStyle
and its data is not on the heap.

are not

[sof, 2014-06-12 14:12:02]

On 2014/06/12 14:05:37, Mads Ager (chromium) wrote:
> Yes, I just double checked that that leak is also gone with this patch. It is
> great that we have the leak detector to find these issues when we attempt to
> make this pointer strong again. :)

That's good news indeed.

[Mads Ager (chromium), 2014-06-12 14:13:02]

Thanks!

https://codereview.chromium.org/335633002/diff/1/Source/core/css/CSSFontSelec...
File Source/core/css/CSSFontSelector.h (right):

https://codereview.chromium.org/335633002/diff/1/Source/core/css/CSSFontSelec...
Source/core/css/CSSFontSelector.h:89: // FIXME: Oilpan: Ideally this should just
be a trace Member but that will
On 2014/06/12 14:09:38, haraken wrote:
> 
> a trace Member => a traced Member

Done.

https://codereview.chromium.org/335633002/diff/1/Source/core/css/CSSFontSelec...
Source/core/css/CSSFontSelector.h:90: // currently leak because RenderStyle and
its data is not on the heap.
On 2014/06/12 14:09:38, haraken wrote:
> 
> is not => are not

Done.

https://codereview.chromium.org/335633002/diff/1/Source/core/fetch/ResourceFe...
File Source/core/fetch/ResourceFetcher.h (right):

https://codereview.chromium.org/335633002/diff/1/Source/core/fetch/ResourceFe...
Source/core/fetch/ResourceFetcher.h:208: // FIXME: Oilpan: Ideally this should
just be a trace Member but that will
On 2014/06/12 14:09:38, haraken wrote:
> 
> a traced Member

Done.

https://codereview.chromium.org/335633002/diff/1/Source/core/fetch/ResourceFe...
Source/core/fetch/ResourceFetcher.h:209: // currently leak because RenderStyle
and its data is not on the heap.
On 2014/06/12 14:09:38, haraken wrote:
> 
> are not

Done.

[haraken, 2014-06-12 14:14:06]

Just help me understand: Is it safe to change RawPtrWillBeMember to
RawPtrWillBeWeakMember (to avoid leak) in general? I think the answer is no, and
we need to make sure why the weak member is OK in each particular case.

In this particular case, the weak members are OK because RenderStyle objects
ensures that the Document is alive while the CSSFontSelector or the
ResourceFetcher is alive. Once we move the RenderStyle objects to the heap, we
_must_ move the weak members back to strong members, right?

[Mads Ager (chromium), 2014-06-12 14:25:58]

The CQ bit was checked by ager@chromium.org

[commit-bot: I haz the power, 2014-06-12 14:27:15]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ager@chromium.org/335633002/20001

[Mads Ager (chromium), 2014-06-12 14:31:48]

On 2014/06/12 14:14:06, haraken wrote:
> Just help me understand: Is it safe to change RawPtrWillBeMember to
> RawPtrWillBeWeakMember (to avoid leak) in general? I think the answer is no,
and
> we need to make sure why the weak member is OK in each particular case.
> 
> In this particular case, the weak members are OK because RenderStyle objects
> ensures that the Document is alive while the CSSFontSelector or the
> ResourceFetcher is alive. Once we move the RenderStyle objects to the heap, we
> _must_ move the weak members back to strong members, right?

Yes, that's right. It is not safe in general. In these particular cases these
pointers were raw pointers that were cleared when the Document died already so
the code is prepared for m_document becoming cleared here. This particular case
would probably be safe (but not desirable) even when we move RenderStyle to the
heap. I'm not entirely sure though and we do need to revisit when moving
RenderStyle.

[commit-bot: I haz the power, 2014-06-12 16:02:56]

Change committed as 176034