[oilpan] Move CSSSegmentedFontFace to the managed heap

[oilpan] Move CSSSegmentedFontFace to the managed heap

The lifetime around CSSFontFace is complicated, and the CL in issue 18375005
(which tried to move CSSSegmentedFontFace and CSSFontFace at the same time)
caused a couple of crashes. To separate problems, I'd like to first move
CSSSegmentedFontFace in this CL. I'll move CSSFontFace in the next CL.

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

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

[haraken, 2013-07-09 06:57:55]

This CL solves the issue described in [1] of
https://codereview.chromium.org/18375005/#msg16

[haraken, 2013-07-09 06:58:10]

PTAL

[haraken, 2013-07-09 07:10:53]

This is the first CL I talked about in the sync up. In the second CL, I'll move
CSSFontFace, addressing another lifetime issue.

https://codereview.chromium.org/18882002/diff/1/Source/core/css/CSSFontSelect...
File Source/core/css/CSSFontSelector.cpp (right):

https://codereview.chromium.org/18882002/diff/1/Source/core/css/CSSFontSelect...
Source/core/css/CSSFontSelector.cpp:84: // CSSFontSelector hold
Member<CSSSegmentedFontFace>.

This is the part where I'm clearing the back pointer.

[Vyacheslav Egorov (Chromium), 2013-07-09 11:43:27]

I do not feel that NULLing the m_fontSelector back pointer and then
checking for NULL is the right way to go.

We had discussed this with Ian and when CSSFontSelector is destroyed(*) it
should iterate over all CSSSegmentedFontFace's it owns and ask their
CSSFontFace's to clear their lists of CSSFontFaceSource (field m_sources).

This way we guarantee that no callback will arrive after FontSelector is
destroyed.

We can still NULL m_fontSelector back pointer but instead of checking if it
is NULL or not in routines we should ASSERT that it is not NULL.

(*) When CSSFontSelector is moved to the managed heap we should move this
clearing/unregistering to the moment when StyleResolver is detached from
the document (by clearStyleResolver()).

--
Vyacheslav Egorov


On Tue, Jul 9, 2013 at 9:10 AM, <haraken@chromium.org> wrote:

> This is the first CL I talked about in the sync up. In the second CL, I'll
> move
> CSSFontFace, addressing another lifetime issue.
>
>
> https://codereview.chromium.**org/18882002/diff/1/Source/**
>
core/css/CSSFontSelector.cpp<https://codereview.chromium.org/18882002/diff/1/Source/core/css/CSSFontSelector.cpp>
> File Source/core/css/**CSSFontSelector.cpp (right):
>
> https://codereview.chromium.**org/18882002/diff/1/Source/**
>
core/css/CSSFontSelector.cpp#**newcode84<https://codereview.chromium.org/18882002/diff/1/Source/core/css/CSSFontSelector.cpp#newcode84>
> Source/core/css/**CSSFontSelector.cpp:84: // CSSFontSelector hold
> Member<CSSSegmentedFontFace>.
>
> This is the part where I'm clearing the back pointer.
>
>
https://codereview.chromium.**org/18882002/<https://codereview.chromium.org/1...
>

[zerny-google, 2013-07-09 11:48:21]

On 2013/07/09 11:43:27, Vyacheslav Egorov wrote:
> (*) When CSSFontSelector is moved to the managed heap we should move this
> clearing/unregistering to the moment when StyleResolver is detached from
> the document (by clearStyleResolver()).

I'll postpone moving CSSFontSelector until after Haraken's CL has been landed.

[haraken, 2013-07-09 12:04:33]

> We had discussed this with Ian and when CSSFontSelector is destroyed(*) it
> should iterate over all CSSSegmentedFontFace's it owns and ask their
> CSSFontFace's to clear their lists of CSSFontFaceSource (field m_sources).
> 
> This way we guarantee that no callback will arrive after FontSelector is
> destroyed.

This sounds reasonable to me.

[haraken, 2013-07-09 12:18:23]

Updated the CL. PTAL.

[zerny-google, 2013-07-09 13:09:09]

lgtm

https://codereview.chromium.org/18882002/diff/9001/Source/core/css/CSSFontSel...
File Source/core/css/CSSFontSelector.h (right):

https://codereview.chromium.org/18882002/diff/9001/Source/core/css/CSSFontSel...
Source/core/css/CSSFontSelector.h:89: typedef HashMap<unsigned,
Persistent<CSSSegmentedFontFace> > SegmentedFontFaceHashMap;
I think this should just be a strong manp of members once CSSFontSelector is
moved to the managed heap.

[Vyacheslav Egorov (Google), 2013-07-09 13:09:30]

LGTM!

[haraken, 2013-07-09 13:11:46]

Thanks! I'll check tests again and land it. I'll upload a CL to move CSSFontFace
by the time you wake up tomorrow.

[haraken, 2013-07-09 13:43:41]

Committed patchset #2 manually as r153781 (presubmit successful).