[oilpan] Move CSSFontFace to the managed heap

[oilpan] Move CSSFontFace to the managed heap

R=vegorov@google.com, zerny@google.com

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

[haraken, 2013-07-09 14:31:37]

Several tests are still crashing. Not yet ready for review.

[haraken, 2013-07-10 05:27:01]

All tests pass. PTAL.

[zerny-google, 2013-07-10 06:34:34]

lgtm

The FIXME's on used-after-free could maybe be replaced with a one-liner just
stating why it is safe (at the exact location).

Regarding the CSSFontFaceSource, either we explicitly call the chain of dispose
once removing the FontSelector from the document, or we need to implement a weak
cache. The former (and current) solution seems reasonable for now.

https://codereview.chromium.org/18856015/diff/3001/Source/core/css/CSSFontFac...
File Source/core/css/CSSFontFace.cpp (right):

https://codereview.chromium.org/18856015/diff/3001/Source/core/css/CSSFontFac...
Source/core/css/CSSFontFace.cpp:105: for (HashSet<Member<CSSSegmentedFontFace>
>::iterator it = m_segmentedFontFaces.begin(); it != end; ++it)
Handle scope.

[haraken, 2013-07-10 06:39:41]

> The FIXME's on used-after-free could maybe be replaced with a one-liner just
> stating why it is safe (at the exact location).
> 
> Regarding the CSSFontFaceSource, either we explicitly call the chain of
dispose
> once removing the FontSelector from the document, or we need to implement a
weak
> cache. The former (and current) solution seems reasonable for now.
> 
>
https://codereview.chromium.org/18856015/diff/3001/Source/core/css/CSSFontFac...
> File Source/core/css/CSSFontFace.cpp (right):
> 
>
https://codereview.chromium.org/18856015/diff/3001/Source/core/css/CSSFontFac...
> Source/core/css/CSSFontFace.cpp:105: for (HashSet<Member<CSSSegmentedFontFace>
> >::iterator it = m_segmentedFontFaces.begin(); it != end; ++it)
> Handle scope.

Fixed. Thanks for reviewing!

I'll wait for comments from slava.

[Vyacheslav Egorov (Google), 2013-07-10 10:59:58]

lgtm

https://codereview.chromium.org/18856015/diff/7001/Source/core/css/CSSFontFac...
File Source/core/css/CSSFontFaceSource.h (right):

https://codereview.chromium.org/18856015/diff/7001/Source/core/css/CSSFontFac...
Source/core/css/CSSFontFaceSource.h:82: CSSFontFace* m_face; // Our owning font
face.
I think you can make it a member right away. There is nothing actually
forbidding to have Members in not heap allocated structures.

https://codereview.chromium.org/18856015/diff/7001/Source/heap/Visitor.h
File Source/heap/Visitor.h (right):

https://codereview.chromium.org/18856015/diff/7001/Source/heap/Visitor.h#newc...
Source/heap/Visitor.h:118: void visit(const Vector<Member<T> >& vector)
I think you don't need this overload anymore. The next one should cover this
case of N = 0.

[haraken, 2013-07-10 11:38:40]

Thanks for reviewing!

https://codereview.chromium.org/18856015/diff/7001/Source/core/css/CSSFontFac...
File Source/core/css/CSSFontFaceSource.h (right):

https://codereview.chromium.org/18856015/diff/7001/Source/core/css/CSSFontFac...
Source/core/css/CSSFontFaceSource.h:82: CSSFontFace* m_face; // Our owning font
face.

My concern is that using Members in not heap allocated structures will confuse a
verification framework where we want to verify that all Members are visited.
Even without the verification framework, I might want to have a general rule
that all Members have to be visited in order to manually verify that Members are
correctly used.

https://codereview.chromium.org/18856015/diff/7001/Source/heap/Visitor.h
File Source/heap/Visitor.h (right):

https://codereview.chromium.org/18856015/diff/7001/Source/heap/Visitor.h#newc...
Source/heap/Visitor.h:118: void visit(const Vector<Member<T> >& vector)
On 2013/07/10 10:59:58, Vyacheslav Egorov (Google) wrote:
> I think you don't need this overload anymore. The next one should cover this
> case of N = 0.

Good point. Thanks!

[haraken, 2013-07-10 11:39:39]

Committed patchset #3 manually as r153886 (presubmit successful).