This causes an assert to be triggered in FontCache::releaseFontData. Patch coming up...
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(-)
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.
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(-)
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.
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(-)
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.
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
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)
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.
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.
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".
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.
The original bug was fixed, so I'm marking this bug as invalid.