[webkit-reviews] review requested: [Bug 61875] Fonts returned by FontCache::getFontDataForCharacters() are never released : [Attachment 96254] Patch with updates in response to comment #20

[bugzilla-daemon at webkit.org]

Michael Saboff <msaboff at apple.com> has asked  for review:
Bug 61875: Fonts returned by FontCache::getFontDataForCharacters() are never
released
https://bugs.webkit.org/show_bug.cgi?id=61875

Attachment 96254: Patch with updates in response to comment #20
https://bugs.webkit.org/attachment.cgi?id=96254&action=review

------- Additional Comments from Michael Saboff <msaboff at apple.com>
(In reply to comment #20)
> Dan is still your best reviewer here, but I noticed these inline declarations
he mentioned, which you can remove:
> 
> > +	 inline FontCachePurgePreventer() { fontCache()->disablePurging(); }
> > +	 inline ~FontCachePurgePreventer() { fontCache()->enablePurging(); }

This was an oversight and has been fixed.

> > +	 const SimpleFontData* fontData = getCachedFontData(&data);
> > +	 releasePurgeProtectedFont(fontData);
> 
> I'm very unclear on when a call to releasePurgeProtectedFont() is required
or, if not required, desirable. The "retain and immediately release" idiom seen
here is very odd to me. If I don't need to retain the object, why am I calling
a retaining API? If I do need to retain the object, why is releasing it OK?

It was being called only by platform specific
FontCache::getFontDataForCharacters(), the method used to access system
fallback fonts.

To make it less confusing, I removed releasePurgeProtectedFont() and added a
"shouldRetain" parameter to getCachedFontData() with a default value of true. 
This change also eliminated one hash lookup (find()) for the system fallback
font case.

> Perhaps the default cache behavior should be to ASSERT that purging is
prohibited, and return a font without retaining, while the few clients that do
explicitly retain their fonts can call a special function that does not ASSERT
that purging is prohibited, and does a retain instead:
> 
> getCachedFontData(FontPlatformData*); // ASSERTs that purging is prohibited,
does not retain &data. Returned SimpleFontData* is implicitly owned by the
current FontCachePurgePreventer scope. 
> 
> retainCachedFontData(FontPlatformData*); // Retains returned SimpleFontData*,
does not ASSERT that purging is prohibited. Returned SimpleFontData* is
explicitly owned by the caller.
> releaseCachedFontData(SimpleFontData*); // Releases SimpleFontData* obtained
by retainCachedFontData.
> 
> I think this default would be congruent with the code's intention, given your
comment:
> 
> > almost all calls to getCachedFontData() now happen when purging is not
allowed.
> 
> In such a design a "FontCachePurgePreventer" is really a
"CachedFontDataOwner" or "CachedFontDataScope", since it implicitly owns all
cached font data accessed within its scope.

Most font requests to the cache rely on the reference counting.  It is only the
system fallback font accesses from the cache that needs the purge prevention
handling and no change to reference count. It is the case though that these two
cache requests are intermingled and therefore purging is disallowed even if we
are requesting the font for the normal reference counted paths.

Although FontCachePurgePreventer can be seen as an "owner", a better
description is that a font can only be released if its reference count is zero
AND there isn't an active FontCachePurgePreventer.  It is likely the case that
a font needs both types of protection, as a font can be accessed and actively
used via a reference counted path as well as via the system fallback access
path for the same time period.