Fix TypefaceCache race.

[bungeman, 2012-02-15 19:21:31]

This fixes a potential race condition in SkTypefaceCache since the static
methods which return SkTypeface pointers are returning them without ref()ing
them. Internally they are protected by a mutex, but externally they are not.

Note that this does not fix any implementations of SkFontHost::ValidFontID,
since http://codereview.appspot.com/5654055/ will remove it.

The alternative approach of allowing access to the mutex was investigated. The
increased complexity (and the potential for future developers to forget to
obtain the mutex) did not appear to be worth the cost, since all operations
which needed to be fast must ref the SkTypeface anyway.

[reed1, 2012-02-15 20:52:45]

http://codereview.appspot.com/5656066/diff/1/src/core/SkTypefaceCache.h
File src/core/SkTypefaceCache.h (left):

http://codereview.appspot.com/5656066/diff/1/src/core/SkTypefaceCache.h#oldco...
src/core/SkTypefaceCache.h:46: static SkTypeface* FindByID(SkFontID fontID);
Rename to FindAndRefByID, or RefByID

http://codereview.appspot.com/5656066/diff/1/src/core/SkTypefaceCache.h#oldco...
src/core/SkTypefaceCache.h:53: static SkTypeface* FindByProc(FindProc proc,
void* ctx);
Rename to FindAndRefByProc, or RefByProc

http://codereview.appspot.com/5656066/diff/1/src/ports/SkFontHost_freetype_ma...
File src/ports/SkFontHost_freetype_mac.cpp (right):

http://codereview.appspot.com/5656066/diff/1/src/ports/SkFontHost_freetype_ma...
src/ports/SkFontHost_freetype_mac.cpp:76: tf->unref();
We need to ref the stream object, if we're worred about a race, plus we'd need
to ref it *before* we unref the tf...

http://codereview.appspot.com/5656066/diff/1/src/ports/SkFontHost_mac_coretex...
File src/ports/SkFontHost_mac_coretext.cpp (right):

http://codereview.appspot.com/5656066/diff/1/src/ports/SkFontHost_mac_coretex...
src/ports/SkFontHost_mac_coretext.cpp:452: face->unref();
Seems like we need to ref() the CTFontRef object, if we were concerned about
losing the associated typeface object in a race...

[bungeman, 2012-02-15 21:57:53]

Reverted and documented reasoning and behavior of FindByID.

http://codereview.appspot.com/5656066/diff/1/src/core/SkTypefaceCache.h
File src/core/SkTypefaceCache.h (left):

http://codereview.appspot.com/5656066/diff/1/src/core/SkTypefaceCache.h#oldco...
src/core/SkTypefaceCache.h:46: static SkTypeface* FindByID(SkFontID fontID);
On 2012/02/15 20:52:45, reed1 wrote:
> Rename to FindAndRefByID, or RefByID

Now changed to do what it used to do.

http://codereview.appspot.com/5656066/diff/1/src/core/SkTypefaceCache.h#oldco...
src/core/SkTypefaceCache.h:53: static SkTypeface* FindByProc(FindProc proc,
void* ctx);
On 2012/02/15 20:52:45, reed1 wrote:
> Rename to FindAndRefByProc, or RefByProc

Changed to FindByProcAndRef.

http://codereview.appspot.com/5656066/diff/1/src/ports/SkFontHost_freetype_ma...
File src/ports/SkFontHost_freetype_mac.cpp (right):

http://codereview.appspot.com/5656066/diff/1/src/ports/SkFontHost_freetype_ma...
src/ports/SkFontHost_freetype_mac.cpp:76: tf->unref();
On 2012/02/15 20:52:45, reed1 wrote:
> We need to ref the stream object, if we're worred about a race, plus we'd need
> to ref it *before* we unref the tf...

Ooops, this was wrong. Could be fixed, but reverted behavior of FindByID.

http://codereview.appspot.com/5656066/diff/1/src/ports/SkFontHost_mac_coretex...
File src/ports/SkFontHost_mac_coretext.cpp (right):

http://codereview.appspot.com/5656066/diff/1/src/ports/SkFontHost_mac_coretex...
src/ports/SkFontHost_mac_coretext.cpp:452: face->unref();
On 2012/02/15 20:52:45, reed1 wrote:
> Seems like we need to ref() the CTFontRef object, if we were concerned about
> losing the associated typeface object in a race...

This would require a ref count wrapper; the current code doesn't have one
because it assumes the stack has a reference to the typeface. Reverted behavior
of FindByID.

[reed1, 2012-02-15 22:01:00]

I see that you reverted the relevant change, but btw -- CTFontRef is already
reference countable, so it would be easy if we needed it.

On 2012/02/15 21:57:53, bungeman wrote:
> Reverted and documented reasoning and behavior of FindByID.
> 
> http://codereview.appspot.com/5656066/diff/1/src/core/SkTypefaceCache.h
> File src/core/SkTypefaceCache.h (left):
> 
>
http://codereview.appspot.com/5656066/diff/1/src/core/SkTypefaceCache.h#oldco...
> src/core/SkTypefaceCache.h:46: static SkTypeface* FindByID(SkFontID fontID);
> On 2012/02/15 20:52:45, reed1 wrote:
> > Rename to FindAndRefByID, or RefByID
> 
> Now changed to do what it used to do.
> 
>
http://codereview.appspot.com/5656066/diff/1/src/core/SkTypefaceCache.h#oldco...
> src/core/SkTypefaceCache.h:53: static SkTypeface* FindByProc(FindProc proc,
> void* ctx);
> On 2012/02/15 20:52:45, reed1 wrote:
> > Rename to FindAndRefByProc, or RefByProc
> 
> Changed to FindByProcAndRef.
> 
>
http://codereview.appspot.com/5656066/diff/1/src/ports/SkFontHost_freetype_ma...
> File src/ports/SkFontHost_freetype_mac.cpp (right):
> 
>
http://codereview.appspot.com/5656066/diff/1/src/ports/SkFontHost_freetype_ma...
> src/ports/SkFontHost_freetype_mac.cpp:76: tf->unref();
> On 2012/02/15 20:52:45, reed1 wrote:
> > We need to ref the stream object, if we're worred about a race, plus we'd
need
> > to ref it *before* we unref the tf...
> 
> Ooops, this was wrong. Could be fixed, but reverted behavior of FindByID.
> 
>
http://codereview.appspot.com/5656066/diff/1/src/ports/SkFontHost_mac_coretex...
> File src/ports/SkFontHost_mac_coretext.cpp (right):
> 
>
http://codereview.appspot.com/5656066/diff/1/src/ports/SkFontHost_mac_coretex...
> src/ports/SkFontHost_mac_coretext.cpp:452: face->unref();
> On 2012/02/15 20:52:45, reed1 wrote:
> > Seems like we need to ref() the CTFontRef object, if we were concerned about
> > losing the associated typeface object in a race...
> 
> This would require a ref count wrapper; the current code doesn't have one
> because it assumes the stack has a reference to the typeface. Reverted
behavior
> of FindByID.

[reed1, 2012-02-15 22:02:13]

lgtm

[bungeman, 2012-02-16 12:42:05]

Committed revision 3205.