Bug 113954

Summary: Regression(r147639) Causes assertion hit in HashTable
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: WebCore Misc.Assignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, benjamin, kling, laszlo.gombos
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 111939    
Attachments:
Description Flags
Patch
benjamin: review-, benjamin: commit-queue-
Patch
none
Patch
none
Patch
none
Patch benjamin: review-

Description Chris Dumez 2013-04-04 13:24:55 PDT 
After <http://trac.webkit.org/changeset/147639>, we see the following crashes on the EFL bots:

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()

gdb backtrace:
http://pastebin.com/dVhBAXhy
Comment 1 Chris Dumez 2013-04-04 13:33:01 PDT 
Created attachment 196514 [details]
Patch
Comment 2 Benjamin Poulain 2013-04-04 13:42:12 PDT 
Comment on attachment 196514 [details]
Patch

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

This is not making recursive use safer:

> Source/WebCore/platform/graphics/FontCache.cpp:207
> +    OwnPtr<FontPlatformData>& value = result.iterator->value;

This reference will be okay in the majority of cases.
But...if the table grows or shrink, you are no pointing to the right object.
Comment 3 Chris Dumez 2013-04-04 14:18:52 PDT 
Created attachment 196527 [details]
Patch

Ok, now I lookup the key in the hash table again after the recursive call.
Comment 4 Chris Dumez 2013-04-04 14:21:37 PDT 
Created attachment 196528 [details]
Patch
Comment 5 Chris Dumez 2013-04-04 14:50:21 PDT 
Created attachment 196532 [details]
Patch

Found much simpler.
Comment 6 Chris Dumez 2013-04-04 14:53:41 PDT 
(In reply to comment #5)
> Created an attachment (id=196532) [details]
> Patch
> 
> Found much simpler.

No, it is not correct. Sorry.
Comment 7 Benjamin Poulain 2013-04-04 14:58:50 PDT 
(In reply to comment #6)
> > Found much simpler.
> 
> No, it is not correct. Sorry.

No, it is not  :)
Comment 8 Chris Dumez 2013-04-04 16:14:52 PDT 
Created attachment 196550 [details]
Patch
Comment 9 Benjamin Poulain 2013-04-04 16:36:27 PDT 
Comment on attachment 196550 [details]
Patch

This is correct. But you are slowing down the common case.

Now, you always need 2 table lookup on insertion.

If fontPlatformData() returns something, you should not need a second table lookup, you should just put the value in the bucket you got with the first table lookup.

It is okay (and the correct thing to do) to have the table lookup again if you use alternateName. But if you don't, you should not have to pay the price for it.
Comment 10 Chris Dumez 2013-04-04 16:54:53 PDT 
Committed r147681: <http://trac.webkit.org/changeset/147681>