Bug 111939 – Global FontPlatformData cache should use OwnPtr.

• Status: RESOLVED
• Resolution: FIXED
• Component: Text

[Andreas Kling]

Global FontPlatformData cache should use OwnPtr.

[Andreas Kling]

Created attachment 192376 [details]
Patch

[WebKit Review Bot]

Attachment 192376 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/platform/graphics/FontCache.cpp', u'Source/WebCore/platform/graphics/FontCache.h', u'Source/WebCore/platform/graphics/blackberry/FontCacheBlackBerry.cpp', u'Source/WebCore/platform/graphics/chromium/FontCacheAndroid.cpp', u'Source/WebCore/platform/graphics/freetype/FontCacheFreeType.cpp', u'Source/WebCore/platform/graphics/mac/FontCacheMac.mm', u'Source/WebCore/platform/graphics/qt/FontCacheQt.cpp', u'Source/WebCore/platform/graphics/skia/FontCacheSkia.cpp', u'Source/WebCore/platform/graphics/win/FontCacheWin.cpp', u'Source/WebCore/platform/graphics/wince/FontCacheWinCE.cpp', u'Source/WebCore/platform/graphics/wx/FontCacheWx.cpp']" exit_code: 1
Source/WebCore/platform/graphics/FontCache.cpp:209:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/WebCore/platform/graphics/FontCache.cpp:210:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Total errors found: 2 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EFL EWS Bot]

Comment on attachment 192376 [details]
Patch

Attachment 192376 [details] did not pass efl-ews (efl):
Output: http://webkit-commit-queue.appspot.com/results/17118021

[WebKit Review Bot]

Comment on attachment 192376 [details]
Patch

Attachment 192376 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17024086

[WebKit Review Bot]

Comment on attachment 192376 [details]
Patch

Attachment 192376 [details] did not pass cr-linux-debug-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17164006

[Early Warning System Bot]

Comment on attachment 192376 [details]
Patch

Attachment 192376 [details] did not pass qt-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17148013

[Early Warning System Bot]

Comment on attachment 192376 [details]
Patch

Attachment 192376 [details] did not pass qt-wk2-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17142050

[Andreas Kling]

Created attachment 192377 [details]
Patch

Less 0, more nullptr.

[WebKit Review Bot]

Attachment 192377 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/platform/graphics/FontCache.cpp', u'Source/WebCore/platform/graphics/FontCache.h', u'Source/WebCore/platform/graphics/blackberry/FontCacheBlackBerry.cpp', u'Source/WebCore/platform/graphics/chromium/FontCacheAndroid.cpp', u'Source/WebCore/platform/graphics/freetype/FontCacheFreeType.cpp', u'Source/WebCore/platform/graphics/mac/FontCacheMac.mm', u'Source/WebCore/platform/graphics/qt/FontCacheQt.cpp', u'Source/WebCore/platform/graphics/skia/FontCacheSkia.cpp', u'Source/WebCore/platform/graphics/win/FontCacheWin.cpp', u'Source/WebCore/platform/graphics/wince/FontCacheWinCE.cpp', u'Source/WebCore/platform/graphics/wx/FontCacheWx.cpp']" exit_code: 1
Source/WebCore/platform/graphics/FontCache.cpp:209:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/WebCore/platform/graphics/FontCache.cpp:210:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Total errors found: 2 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 192377 [details]
Patch

Attachment 192377 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17023323

[EFL EWS Bot]

Comment on attachment 192377 [details]
Patch

Attachment 192377 [details] did not pass efl-ews (efl):
Output: http://webkit-commit-queue.appspot.com/results/17131076

[Early Warning System Bot]

Comment on attachment 192377 [details]
Patch

Attachment 192377 [details] did not pass qt-wk2-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17164010

[Early Warning System Bot]

Comment on attachment 192377 [details]
Patch

Attachment 192377 [details] did not pass qt-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17017843

[WebKit Review Bot]

Comment on attachment 192377 [details]
Patch

Attachment 192377 [details] did not pass cr-linux-debug-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17131078

[Andreas Kling]

Created attachment 192378 [details]
Patch

Even less 0, even more nullptr.

[WebKit Review Bot]

Attachment 192378 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/platform/graphics/FontCache.cpp', u'Source/WebCore/platform/graphics/FontCache.h', u'Source/WebCore/platform/graphics/blackberry/FontCacheBlackBerry.cpp', u'Source/WebCore/platform/graphics/chromium/FontCacheAndroid.cpp', u'Source/WebCore/platform/graphics/freetype/FontCacheFreeType.cpp', u'Source/WebCore/platform/graphics/mac/FontCacheMac.mm', u'Source/WebCore/platform/graphics/qt/FontCacheQt.cpp', u'Source/WebCore/platform/graphics/skia/FontCacheSkia.cpp', u'Source/WebCore/platform/graphics/win/FontCacheWin.cpp', u'Source/WebCore/platform/graphics/wince/FontCacheWinCE.cpp', u'Source/WebCore/platform/graphics/wx/FontCacheWx.cpp']" exit_code: 1
Source/WebCore/platform/graphics/FontCache.cpp:209:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/WebCore/platform/graphics/FontCache.cpp:210:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Total errors found: 2 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 192378 [details]
Patch

Rejecting attachment 192378 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', '--bot-id=gce-cq-03', 'apply-attachment', '--no-update', '--non-interactive', 192378, '--port=chromium-xvfb']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue

Last 500 characters of output:
 file Source/WebCore/platform/graphics/qt/FontCacheQt.cpp
patching file Source/WebCore/platform/graphics/skia/FontCacheSkia.cpp
patching file Source/WebCore/platform/graphics/win/FontCacheWin.cpp
patching file Source/WebCore/platform/graphics/wince/FontCacheWinCE.cpp
patching file Source/WebCore/platform/graphics/wx/FontCacheWx.cpp

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', '--force', '--reviewer', 'Anders Carlsson']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue

Full output: http://webkit-commit-queue.appspot.com/results/17122572

[Andreas Kling]

Committed r147639: <http://trac.webkit.org/changeset/147639>

[Chris Dumez]

This patch seems to cause assertions in EFL API tests:
ASSERTION FAILED: m_table
/home/buildslave-1/webkit-buildslave/efl-linux-64-debug-wk2/build/Source/WTF/wtf/HashTable.h(210) : void WTF::HashTableConstIterator<Key, Value, Extractor, HashFunctions, Traits, KeyTraits>::checkValidity() const [with Key = WebCore::FontPlatformDataCacheKey, Value = WTF::KeyValuePair<WebCore::FontPlatformDataCacheKey, WTF::OwnPtr<WebCore::FontPlatformData> >, Extractor = WTF::KeyValuePairKeyExtractor<WTF::KeyValuePair<WebCore::FontPlatformDataCacheKey, WTF::OwnPtr<WebCore::FontPlatformData> > >, HashFunctions = WebCore::FontPlatformDataCacheKeyHash, Traits = WTF::HashMapValueTraits<WebCore::FontPlatformDataCacheKeyTraits, WTF::HashTraits<WTF::OwnPtr<WebCore::FontPlatformData> > >, KeyTraits = WebCore::FontPlatformDataCacheKeyTraits]
1   0x2b82fefeb70f WTF::HashTableConstIterator<WebCore::FontPlatformDataCacheKey, WTF::KeyValuePair<WebCore::FontPlatformDataCacheKey, WTF::OwnPtr<WebCore::FontPlatformData> >, WTF::KeyValuePairKeyExtractor<WTF::KeyValuePair<WebCore::FontPlatformDataCacheKey, WTF::OwnPtr<WebCore::FontPlatformData> > >, WebCore::FontPlatformDataCacheKeyHash, WTF::HashMapValueTraits<WebCore::FontPlatformDataCacheKeyTraits, WTF::HashTraits<WTF::OwnPtr<WebCore::FontPlatformData> > >, WebCore::FontPlatformDataCacheKeyTraits>::checkValidity() const
2   0x2b82fefe90d8 WTF::HashTableConstIterator<WebCore::FontPlatformDataCacheKey, WTF::KeyValuePair<WebCore::FontPlatformDataCacheKey, WTF::OwnPtr<WebCore::FontPlatformData> >, WTF::KeyValuePairKeyExtractor<WTF::KeyValuePair<WebCore::FontPlatformDataCacheKey, WTF::OwnPtr<WebCore::FontPlatformData> > >, WebCore::FontPlatformDataCacheKeyHash, WTF::HashMapValueTraits<WebCore::FontPlatformDataCacheKeyTraits, WTF::HashTraits<WTF::OwnPtr<WebCore::FontPlatformData> > >, WebCore::FontPlatformDataCacheKeyTraits>::get() const
3   0x2b82fefe644c WTF::HashTableIterator<WebCore::FontPlatformDataCacheKey, WTF::KeyValuePair<WebCore::FontPlatformDataCacheKey, WTF::OwnPtr<WebCore::FontPlatformData> >, WTF::KeyValuePairKeyExtractor<WTF::KeyValuePair<WebCore::FontPlatformDataCacheKey, WTF::OwnPtr<WebCore::FontPlatformData> > >, WebCore::FontPlatformDataCacheKeyHash, WTF::HashMapValueTraits<WebCore::FontPlatformDataCacheKeyTraits, WTF::HashTraits<WTF::OwnPtr<WebCore::FontPlatformData> > >, WebCore::FontPlatformDataCacheKeyTraits>::get() const
4   0x2b82fefe4928 WTF::HashTableIterator<WebCore::FontPlatformDataCacheKey, WTF::KeyValuePair<WebCore::FontPlatformDataCacheKey, WTF::OwnPtr<WebCore::FontPlatformData> >, WTF::KeyValuePairKeyExtractor<WTF::KeyValuePair<WebCore::FontPlatformDataCacheKey, WTF::OwnPtr<WebCore::FontPlatformData> > >, WebCore::FontPlatformDataCacheKeyHash, WTF::HashMapValueTraits<WebCore::FontPlatformDataCacheKeyTraits, WTF::HashTraits<WTF::OwnPtr<WebCore::FontPlatformData> > >, WebCore::FontPlatformDataCacheKeyTraits>::operator->() const
5   0x2b82fefe2cec WebCore::FontCache::getCachedFontPlatformData(WebCore::FontDescription const&, WTF::AtomicString const&, bool)
6   0x2b82fefe2d68 WebCore::FontCache::getCachedFontData(WebCore::FontDescription const&, WTF::AtomicString const&, bool, WebCore::FontCache::ShouldRetain)
7   0x2b82fe72406c
8   0x2b82fe724494 WebCore::CSSFontSelector::getFontData(WebCore::FontDescription const&, WTF::AtomicString const&)
9   0x2b82fefe39d8 WebCore::FontCache::getFontData(WebCore::Font const&, int&, WebCore::FontSelector*)
10  0x2b82feff00cb WebCore::FontFallbackList::fontDataAt(WebCore::Font const*, unsigned int) const
11  0x2b82febc5384 WebCore::FontFallbackList::primaryFontData(WebCore::Font const*) const
12  0x2b82febc5335 WebCore::FontFallbackList::primarySimpleFontData(WebCore::Font const*)
13  0x2b82febc545a WebCore::Font::primaryFont() const
14  0x2b82febc53ac WebCore::Font::fontMetrics() const
15  0x2b82ff346c86 WebCore::RenderStyle::fontMetrics() const
16  0x2b82ff346f72 WebCore::RenderStyle::computedLineHeight(WebCore::RenderView*) const
17  0x2b82ff156de6 WebCore::RenderBlock::lineHeight(bool, WebCore::LineDirectionMode, WebCore::LinePositionMode) const
18  0x2b82ff18a4e3
19  0x2b82ff19bd86 WebCore::LineWidth::updateAvailableWidth(WebCore::LayoutUnit)
20  0x2b82ff19bba6 WebCore::LineWidth::LineWidth(WebCore::RenderBlock*, bool, WebCore::IndentTextOrNot)
21  0x2b82ff19542a WebCore::RenderBlock::LineBreaker::nextSegmentBreak(WebCore::BidiResolver<WebCore::InlineIterator, WebCore::BidiRun>&, WebCore::LineInfo&, WebCore::RenderBlock::RenderTextInfo&, WebCore::RenderBlock::FloatingObject*, unsigned int, WTF::Vector<WebCore::WordMeasurement, 64ul>&)
22  0x2b82ff194f65 WebCore::RenderBlock::LineBreaker::nextLineBreak(WebCore::BidiResolver<WebCore::InlineIterator, WebCore::BidiRun>&, WebCore::LineInfo&, WebCore::RenderBlock::RenderTextInfo&, WebCore::RenderBlock::FloatingObject*, unsigned int, WTF::Vector<WebCore::WordMeasurement, 64ul>&)
23  0x2b82ff18fb07 WebCore::RenderBlock::layoutRunsAndFloatsInRange(WebCore::LineLayoutState&, WebCore::BidiResolver<WebCore::InlineIterator, WebCore::BidiRun>&, WebCore::InlineIterator const&, WebCore::BidiStatus const&, unsigned int)
24  0x2b82ff18f3fd WebCore::RenderBlock::layoutRunsAndFloats(WebCore::LineLayoutState&, bool)
25  0x2b82ff191dcc WebCore::RenderBlock::layoutInlineChildren(bool, WebCore::LayoutUnit&, WebCore::LayoutUnit&)
26  0x2b82ff13bb95 WebCore::RenderBlock::layoutBlock(bool, WebCore::LayoutUnit)
27  0x2b82ff13b143 WebCore::RenderBlock::layout()
28  0x2b82ff14092e WebCore::RenderBlock::layoutBlockChild(WebCore::RenderBox*, WebCore::RenderBlock::MarginInfo&, WebCore::LayoutUnit&, WebCore::LayoutUnit&)
29  0x2b82ff14052a WebCore::RenderBlock::layoutBlockChildren(bool, WebCore::LayoutUnit&)
30  0x2b82ff13bbb6 WebCore::RenderBlock::layoutBlock(bool, WebCore::LayoutUnit)
31  0x2b82ff13b143 WebCore::RenderBlock::layout()

[Chris Dumez]

Proper gdb backtrace: http://pastebin.com/dVhBAXhy

[Chris Dumez]

Comment on attachment 192378 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=192378&action=review

> Source/WebCore/platform/graphics/FontCache.cpp:213
> +                FontPlatformData* fontPlatformDataForAlternateName = getCachedFontPlatformData(fontDescription, alternateName, true);

This is a recursive call to getCachedFontPlatformData(). Are we sure the iterator stays valid after the call?

[Chris Dumez]

(In reply to comment #21)
> (From update of attachment 192378 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=192378&action=review
> 
> > Source/WebCore/platform/graphics/FontCache.cpp:213
> > +                FontPlatformData* fontPlatformDataForAlternateName = getCachedFontPlatformData(fontDescription, alternateName, true);
> 
> This is a recursive call to getCachedFontPlatformData(). Are we sure the iterator stays valid after the call?

I uploaded a follow-up fix at Bug 113954.