Bug 111939

Summary: Global FontPlatformData cache should use OwnPtr.
Product: WebKit Reporter: Andreas Kling <kling>
Component: TextAssignee: Andreas Kling <kling>
Status: RESOLVED FIXED    
Severity: Normal CC: cc-bugs, cdumez, dglazkov, jamesr, junov, kling, mifenton, noam, rego+ews, rwlbuis, senorblanco, tonikitoo, webkit-ews, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 113954    
Bug Blocks:    
Attachments:
Description Flags
Patch
eflews.bot: commit-queue-
Patch
webkit.review.bot: commit-queue-
Patch andersca: review+, webkit.review.bot: commit-queue-

Description Andreas Kling 2013-03-10 09:32:50 PDT
Global FontPlatformData cache should use OwnPtr.
Comment 1 Andreas Kling 2013-03-10 09:34:45 PDT
Created attachment 192376 [details]
Patch
Comment 2 WebKit Review Bot 2013-03-10 09:38:09 PDT
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.
Comment 3 EFL EWS Bot 2013-03-10 09:39:16 PDT
Comment on attachment 192376 [details]
Patch

Attachment 192376 [details] did not pass efl-ews (efl):
Output: http://webkit-commit-queue.appspot.com/results/17118021
Comment 4 WebKit Review Bot 2013-03-10 09:39:21 PDT
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
Comment 5 WebKit Review Bot 2013-03-10 09:39:46 PDT
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
Comment 6 Early Warning System Bot 2013-03-10 09:39:49 PDT
Comment on attachment 192376 [details]
Patch

Attachment 192376 [details] did not pass qt-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17148013
Comment 7 Early Warning System Bot 2013-03-10 09:40:36 PDT
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
Comment 8 Andreas Kling 2013-03-10 09:43:51 PDT
Created attachment 192377 [details]
Patch

Less 0, more nullptr.
Comment 9 WebKit Review Bot 2013-03-10 09:45:20 PDT
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.
Comment 10 WebKit Review Bot 2013-03-10 09:47:51 PDT
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
Comment 11 EFL EWS Bot 2013-03-10 09:48:47 PDT
Comment on attachment 192377 [details]
Patch

Attachment 192377 [details] did not pass efl-ews (efl):
Output: http://webkit-commit-queue.appspot.com/results/17131076
Comment 12 Early Warning System Bot 2013-03-10 09:49:29 PDT
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
Comment 13 Early Warning System Bot 2013-03-10 09:49:54 PDT
Comment on attachment 192377 [details]
Patch

Attachment 192377 [details] did not pass qt-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17017843
Comment 14 WebKit Review Bot 2013-03-10 09:52:42 PDT
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
Comment 15 Andreas Kling 2013-03-10 09:57:53 PDT
Created attachment 192378 [details]
Patch

Even less 0, even more nullptr.
Comment 16 WebKit Review Bot 2013-03-10 10:01:05 PDT
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.
Comment 17 WebKit Review Bot 2013-03-15 11:24:34 PDT
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
Comment 18 Andreas Kling 2013-04-04 09:35:49 PDT
Committed r147639: <http://trac.webkit.org/changeset/147639>
Comment 19 Chris Dumez 2013-04-04 12:23:36 PDT
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()
Comment 20 Chris Dumez 2013-04-04 12:40:54 PDT
Proper gdb backtrace: http://pastebin.com/dVhBAXhy
Comment 21 Chris Dumez 2013-04-04 13:15:02 PDT
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?
Comment 22 Chris Dumez 2013-04-04 13:33:53 PDT
(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.