Fix Refcount in FontDataCache for objects from FontFallbackIterator

Fix Refcount in FontDataCache for objects from FontFallbackIterator

FontFallbackIterator did previously not maintain the refcount in
FontDataCache correctly. All calls to FontDataCache::get() should be
balanced with a call to FontCache::releaseFontData().
FontFallbackIterator does not ensure this, so we'll end up never moving
fonts to m_inactiveFontData in FontDataCache, and thus practically never
releasing them on PurgeIfNeeded.

The approach for fixing this is to add a destructor to
FontDataForRangeSet so that those types of SimpleFontDatas that need to
be released in the FontDataCache actually are released.

FontDataForRangeSet is the data structure that is returned from
FontFallbackIterator to HarfBuzzShaper. So when the shaper is done using
the font, the refcount can be reduced.

In order to ensure the correct lifecyle for when the chache-releasing
destructor is called, and avoid additional copies, FontDataForRangeSet
is also turned into a RefPtr in this CL. This helps to simplify the code
on the HarfBuzzShaper side.

Unfortunately I did not see a way to create an automated test for
this. Local testing on Linux and Mac shows with logging added to
FontDataCache and the FontDataCache target size values reduced, then
visiting a URL which cycles a set of URLs in the same renderer, this
change reintroduces effective purges in FontDataCache, while there are
no effective purges in FontDataCache without this change.

Simple path maintains the refcounting correctly and does the
releaseFontData calls in FontFallbackLists destructor. So this change is
likely to affect memory consumption of the complex font code path
positively for long running renderer processes.

BUG=620283
R=eae,tzik

Committed: https://crrev.com/4eb64d8ed0fb5f4f9c90c8c6d9b24c79fd7f3a35
Cr-Commit-Position: refs/heads/master@{#401269}

[drott, 2016-06-15 11:49:46]

drott@chromium.org changed reviewers:
+ haraken@chromium.org, yutak@chromium.org

[drott, 2016-06-15 11:50:11]

PTAL.

haraken@chromium.org, yutak@chromium.org, please review CSSSegmentedFontFace,
thanks.

[drott, 2016-06-15 11:50:21]

The CQ bit was checked by drott@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-15 11:50:29]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2066253002/1

[commit-bot: I haz the power, 2016-06-15 12:29:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-15 12:29:26]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[eae, 2016-06-21 09:17:37]

Exciting!

[eae, 2016-06-21 09:19:13]

LGTM

https://codereview.chromium.org/2066253002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/fonts/FontFallbackList.cpp (right):

https://codereview.chromium.org/2066253002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/fonts/FontFallbackList.cpp:137: const
SimpleFontData* rangeFontData = segmented->faceAt(i)->fontData();
Do we need an ASSERT here?
ASSERT(segmented->faceAt(i));

[haraken, 2016-06-21 09:21:47]

Sorry, I missed this one. LGTM!

[drott, 2016-06-21 11:37:48]

Thanks for the reviews, there still seems to be some unbalanced calls. It's
really hard to track that down. But this CL seems to slightly over-decrement the
refcount, that's why the assertion in FontDataCache on a >0 refcount when
releasing is now hit in a few places. I'll try to figure it out, even though
that causes gray hair. Afterwards I plan to add a CL for connecting FontCache
invalidation to the Android low memory signal, and that will hopefully gives us
some nice headroom in font memory.

[haraken, 2016-06-21 13:32:06]

On 2016/06/21 11:37:48, drott wrote:
> Thanks for the reviews, there still seems to be some unbalanced calls. It's
> really hard to track that down. But this CL seems to slightly over-decrement
the
> refcount, that's why the assertion in FontDataCache on a >0 refcount when
> releasing is now hit in a few places. I'll try to figure it out, even though
> that causes gray hair. Afterwards I plan to add a CL for connecting FontCache
> invalidation to the Android low memory signal, and that will hopefully gives
us
> some nice headroom in font memory.

Yeah, that sounds like a good plan.

[drott, 2016-06-22 11:55:06]

Fallback and fontDataAt() calls do not need release calls

[drott, 2016-06-22 11:55:18]

The CQ bit was checked by drott@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-22 11:55:34]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2066253002/20001

[commit-bot: I haz the power, 2016-06-22 11:57:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-22 11:57:10]

Dry run: Try jobs failed on following builders:
  ios-device on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  ios-simulator on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)

[drott, 2016-06-22 12:11:15]

The CQ bit was checked by drott@chromium.org to run a CQ dry run

[drott, 2016-06-22 12:11:20]

Rebased

[commit-bot: I haz the power, 2016-06-22 12:11:27]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2066253002/40001

[commit-bot: I haz the power, 2016-06-22 13:25:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-22 13:25:35]

Dry run: This issue passed the CQ dry run.

[eae, 2016-06-22 13:28:28]

LGTM

Changes look good, let's give this a try!
Thank you.

[drott, 2016-06-22 13:30:54]

The CQ bit was checked by drott@chromium.org

[drott, 2016-06-22 13:30:54]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/2066253002/#ps40001
(title: "Rebased")

[commit-bot: I haz the power, 2016-06-22 13:31:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2066253002/40001

[commit-bot: I haz the power, 2016-06-22 13:35:35]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-06-22 13:37:05]

Description was changed from

==========
Fix Refcount in FontDataCache for objects from FontFallbackIterator

FontFallbackIterator did previously not maintain the refcount in
FontDataCache correctly. All calls to FontDataCache::get() should be
balanced with a call to FontCache::releaseFontData().
FontFallbackIterator does not ensure this, so we'll end up never moving
fonts to m_inactiveFontData in FontDataCache, and thus practically never
releasing them on PurgeIfNeeded.

The approach for fixing this is to add a destructor to
FontDataForRangeSet so that those types of SimpleFontDatas that need to
be released in the FontDataCache actually are released.

FontDataForRangeSet is the data structure that is returned from
FontFallbackIterator to HarfBuzzShaper. So when the shaper is done using
the font, the refcount can be reduced.

In order to ensure the correct lifecyle for when the chache-releasing
destructor is called, and avoid additional copies, FontDataForRangeSet
is also turned into a RefPtr in this CL. This helps to simplify the code
on the HarfBuzzShaper side.

Unfortunately I did not see a way to create an automated test for
this. Local testing on Linux and Mac shows with logging added to
FontDataCache and the FontDataCache target size values reduced, then
visiting a URL which cycles a set of URLs in the same renderer, this
change reintroduces effective purges in FontDataCache, while there are
no effective purges in FontDataCache without this change.

Simple path maintains the refcounting correctly and does the
releaseFontData calls in FontFallbackLists destructor. So this change is
likely to affect memory consumption of the complex font code path
positively for long running renderer processes.

BUG=620283
R=eae,tzik
==========

to

==========
Fix Refcount in FontDataCache for objects from FontFallbackIterator

FontFallbackIterator did previously not maintain the refcount in
FontDataCache correctly. All calls to FontDataCache::get() should be
balanced with a call to FontCache::releaseFontData().
FontFallbackIterator does not ensure this, so we'll end up never moving
fonts to m_inactiveFontData in FontDataCache, and thus practically never
releasing them on PurgeIfNeeded.

The approach for fixing this is to add a destructor to
FontDataForRangeSet so that those types of SimpleFontDatas that need to
be released in the FontDataCache actually are released.

FontDataForRangeSet is the data structure that is returned from
FontFallbackIterator to HarfBuzzShaper. So when the shaper is done using
the font, the refcount can be reduced.

In order to ensure the correct lifecyle for when the chache-releasing
destructor is called, and avoid additional copies, FontDataForRangeSet
is also turned into a RefPtr in this CL. This helps to simplify the code
on the HarfBuzzShaper side.

Unfortunately I did not see a way to create an automated test for
this. Local testing on Linux and Mac shows with logging added to
FontDataCache and the FontDataCache target size values reduced, then
visiting a URL which cycles a set of URLs in the same renderer, this
change reintroduces effective purges in FontDataCache, while there are
no effective purges in FontDataCache without this change.

Simple path maintains the refcounting correctly and does the
releaseFontData calls in FontFallbackLists destructor. So this change is
likely to affect memory consumption of the complex font code path
positively for long running renderer processes.

BUG=620283
R=eae,tzik

Committed: https://crrev.com/4eb64d8ed0fb5f4f9c90c8c6d9b24c79fd7f3a35
Cr-Commit-Position: refs/heads/master@{#401269}
==========

[commit-bot: I haz the power, 2016-06-22 13:37:06]

Patchset 3 (id:??) landed as
https://crrev.com/4eb64d8ed0fb5f4f9c90c8c6d9b24c79fd7f3a35
Cr-Commit-Position: refs/heads/master@{#401269}