Bug 21445 – on windows, we try to remove m_smallCapsFontData from the font cache, but we never added it there

• Component: WebCore Misc.
• Status: RESOLVED INVALID

[Tony Chang]

This causes an assert to be triggered in FontCache::releaseFontData.  Patch coming up...

[Tony Chang]

Created attachment 24158 [details]
[1/1] Fix a bug where we try to release font data from the font cache when it

was never added to the font cache in the first place.
---
 WebCore/platform/graphics/SimpleFontData.cpp |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

[Dave Hyatt]

Comment on attachment 24158 [details]
[1/1] Fix a bug where we try to release font data from the font cache when it

I think a better fix here would be to fix the Windows port to actually cache small caps font data in the non-custom-font case.

[Tony Chang]

Created attachment 24161 [details]
[1/1] Fix a bug where we try to release font data from the font cache when it

was never added to the font cache in the first place.
---
 WebCore/ChangeLog                                  |   12 ++++++++++++
 .../platform/graphics/win/SimpleFontDataWin.cpp    |    8 +++-----
 2 files changed, 15 insertions(+), 5 deletions(-)

[Dave Hyatt]

Comment on attachment 24161 [details]
[1/1] Fix a bug where we try to release font data from the font cache when it

You don't want to cache a custom font.  You only want to patch the non-custom-font  code path.  Technically on Windows the small caps font won't ever be shared, but for cross-platform consistency it's better to have that in the cache even if it's pointless. :)

However I see an additional bug, namely that small caps font data in the custom font path is leaking.  The SimpleFontData destructor is not deleting m_smallCapsFontData in the custom font case.

[Tony Chang]

Created attachment 24163 [details]
[1/1] Fix a bug where we try to release font data from the font cache when it

was never added to the font cache in the first place.
---
 WebCore/ChangeLog                                  |   12 ++++++++++++
 WebCore/platform/graphics/SimpleFontData.cpp       |    3 ++-
 .../platform/graphics/win/SimpleFontDataWin.cpp    |    6 ++----
 3 files changed, 16 insertions(+), 5 deletions(-)

[Eric Seidel (no email)]

Comment on attachment 24163 [details]
[1/1] Fix a bug where we try to release font data from the font cache when it

Sorting the review queue, since it's pretty full.  Hyatt is your man for this patch.  Mitz probably also has some font expertise these days.

[Dave Hyatt]

Comment on attachment 24163 [details]
[1/1] Fix a bug where we try to release font data from the font cache when it

r=me

[Eric Seidel (no email)]

Comment on attachment 24163 [details]
[1/1] Fix a bug where we try to release font data from the font cache when it

This really should have a layout test.  Marking r- due to lack of layout test.  If you upload a patch with a layout test I'm happy to land.  Or, if you can explain how this is untestable I'm happy to land.

(p.s. ideally ChangeLogs should have links to the bugzilla bug in them.  Also in general you should replace "WARNING:" with either the location of the test case or an explanation why it's not possible)

[Tony Chang]

Created attachment 25902 [details]
updated changelog

I tried to create a layout test that causes the font cache to be purged, but I was unable to do it.  Instead, I updated the changelog.

[Darin Adler]

Comment on attachment 25902 [details]
updated changelog

It's weak when the code that allocates m_smallCapsFontData and the code that destroys it are so far apart. This makes it hard to be sure we're using it right. I think that's an argument for moving the FontCache::releaseFontData call from SimpleFontData::~SimpleFontData into the SimpleFontData::platformDestroy functions for the Mac platform. I think if we did just that it would fix this bug.

> +    } else if (m_smallCapsFontData)
> +        delete m_smallCapsFontData;

This patch is definitely wrong for some platforms other than Windows. If isCustomFont() is false, then both SimpleFontData::~SimpleFontData and SimpleFontData::platformDestroy will call delete on m_smallCapsFontData. Double deletes are not good.

By the way, there's no need for an if here, because C++ already does nothing if it's passed 0. Specifying the if just adds an extra branch, and would only be justified in unusual circumstances.

[Tony Chang]

It looks like on mac m_smallCapsFontData is allocated using new if it's a custom font.
http://trac.webkit.org/browser/trunk/WebCore/platform/graphics/mac/SimpleFontDataMac.mm#L294
So it doesn't look like a double free.

I'll make a new patch which doesn't have the extra "if".

[Tony Chang]

Created attachment 26068 [details]
remove extra branch

I think this was fixed in <http://trac.webkit.org/changeset/40729>. You may want to post and updated patch to actually get the small caps font from the cache on windows.

[Tony Chang]

The original bug was fixed, so I'm marking this bug as invalid.