Bug 113954 – Regression(r147639) Causes assertion hit in HashTable

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Chris Dumez]

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

[Chris Dumez]

Created attachment 196514 [details]
Patch

[Benjamin Poulain]

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.

[Chris Dumez]

Created attachment 196527 [details]
Patch

Ok, now I lookup the key in the hash table again after the recursive call.

[Chris Dumez]

Created attachment 196528 [details]
Patch

[Chris Dumez]

Created attachment 196532 [details]
Patch

Found much simpler.

[Chris Dumez]

(In reply to comment #5)
> Created an attachment (id=196532) [details]
> Patch
> 
> Found much simpler.

No, it is not correct. Sorry.

[Benjamin Poulain]

(In reply to comment #6)
> > Found much simpler.
> 
> No, it is not correct. Sorry.

No, it is not  :)

[Chris Dumez]

Created attachment 196550 [details]
Patch

[Benjamin Poulain]

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.

[Chris Dumez]

Committed r147681: <http://trac.webkit.org/changeset/147681>