Bug 158297 - Sporadic crash in HashTableAddResult following CSSValuePool::createFontFamilyValue
Summary: Sporadic crash in HashTableAddResult following CSSValuePool::createFontFamily...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: Safari Technology Preview
Hardware: iPhone / iPad iOS 9.3
: P2 Major
Assignee: Myles C. Maxfield
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-06-02 01:43 PDT by erezg
Modified: 2016-06-16 10:10 PDT (History)
7 users (show)

See Also:


Attachments
Stack Trace of crash (80.83 KB, text/plain)
2016-06-02 01:43 PDT, erezg
no flags Details
Reproduction (483 bytes, text/html)
2016-06-16 01:54 PDT, Myles C. Maxfield
no flags Details
Patch (4.94 KB, patch)
2016-06-16 02:29 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description erezg 2016-06-02 01:43:16 PDT
Created attachment 280318 [details]
Stack Trace of crash

This also happens in Safari Technology Preview on Mac OSX, and WKWebView in iOS versions since iOS 9.0.
Happens when integrating Bing Maps in a website.

Stack Trace is attached.
Could be some race condition as this is sporadic.
Thanks!
Comment 1 Benjamin Poulain 2016-06-02 19:08:44 PDT
Do you have a test page by any chance?
Any way I can attempt to reproduce?
Comment 2 Chris Dumez 2016-06-02 19:25:06 PDT
Likely means we are calling CSSValuePool::createFontFamilyValue() with a null String?
Comment 3 Chris Dumez 2016-06-02 19:45:58 PDT
Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.WebCore             	0x7fff9553497a WTF::HashTableAddResult<WTF::HashTableIterator<std::__1::pair<WTF::String, bool>, WTF::KeyValuePair<std::__1::pair<WTF::String, bool>, WTF::RefPtr<WebCore::CSSPrimitiveValue> >, WTF::KeyValuePairKeyExtractor<WTF::KeyValuePair<std::__1::pair<WTF::String, bool>, WTF::RefPtr<WebCore::CSSPrimitiveValue> > >, WTF::PairHash<WTF::String, bool>, WTF::HashMap<std::__1::pair<WTF::String, bool>, WTF::RefPtr<WebCore::CSSPrimitiveValue>, WTF::PairHash<WTF::String, bool>, WTF::HashTraits<std::__1::pair<WTF::String, bool> >, WTF::HashTraits<WTF::RefPtr<WebCore::CSSPrimitiveValue> > >::KeyValuePairTraits, WTF::HashTraits<std::__1::pair<WTF::String, bool> > > > WTF::HashMap<std::__1::pair<WTF::String, bool>, WTF::RefPtr<WebCore::CSSPrimitiveValue>, WTF::PairHash<WTF::String, bool>, WTF::HashTraits<std::__1::pair<WTF::String, bool> >, WTF::HashTraits<WTF::RefPtr<WebCore::CSSPrimitiveValue> > >::add<std::nullptr_t>(std::__1::pair<WTF::String, bool>&&, std::nullptr_t&&) + 90 (/BuildRoot/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.11.Internal.sdk/usr/local/include/wtf/text/StringImpl.h:544)
1   com.apple.WebCore             	0x7fff955344a2 WebCore::CSSValuePool::createFontFamilyValue(WTF::String const&, WebCore::FromSystemFontID) + 258 (/BuildRoot/Library/Caches/com.apple.xbs/Sources/WebCore/WebCore-7601.6.17/css/CSSValuePool.cpp:132)
2   com.apple.WebCore             	0x7fff952c563b WebCore::fontFamilyFromStyle(WebCore::RenderStyle*) + 267 (/BuildRoot/Library/Caches/com.apple.xbs/Sources/WebCore/WebCore-7601.6.17/css/CSSComputedStyleDeclaration.cpp:1594)
3   com.apple.WebCore             	0x7fff951cf7bb WebCore::ComputedStyleExtractor::propertyValue(WebCore::CSSPropertyID, WebCore::EUpdateLayout) const + 8411 (/BuildRoot/Library/Caches/com.apple.xbs/Sources/WebCore/WebCore-7601.6.17/css/CSSComputedStyleDeclaration.cpp:2676)
4   com.apple.WebCore             	0x7fff951cd69e WebCore::CSSComputedStyleDeclaration::getPropertyCSSValueInternal(WebCore::CSSPropertyID) + 62 (/BuildRoot/Library/Caches/com.apple.xbs/Sources/WebCore/WebCore-7601.6.17/css/CSSComputedStyleDeclaration.cpp:2147)
5   com.apple.WebCore             	0x7fff951c57b4 WebCore::JSCSSStyleDeclaration::getOwnPropertySlotDelegate(JSC::ExecState*, JSC::PropertyName, JSC::PropertySlot&) + 68 (/BuildRoot/Library/Caches/com.apple.xbs/Sources/WebCore/WebCore-7601.6.17/bindings/js/JSCSSStyleDeclarationCustom.cpp:303)
6   com.apple.WebCore             	0x7fff9589eb5c WebCore::JSCSSStyleDeclaration::getOwnPropertySlot(JSC::JSObject*, JSC::ExecState*, JSC::PropertyName, JSC::PropertySlot&) + 460 (/Library/Caches/com.apple.xbs/Binaries/WebCore/WebCore-7601.6.17~1/Symbols/BuiltProducts/DerivedSources/WebCore/JSCSSStyleDeclaration.cpp:207)
7   com.apple.JavaScriptCore      	0x7fff8be84193 llint_slow_path_get_by_id + 1091 (/Library/Caches/com.apple.xbs/Sources/JavaScriptCore/JavaScriptCore-7601.6.13/runtime/JSObject.h:1154)
8   com.apple.JavaScriptCore      	0x00007fff8c2185f0 llint_entry + 10503
Comment 4 Chris Dumez 2016-06-02 19:51:52 PDT
I thought Myles had fixed this in <http://trac.webkit.org/changeset/186971> already. The crash trace was exactly the same. Since it still reproduces in technology preview, I guess the fix did not suffice or the bug is slightly different.
Comment 5 erezg 2016-06-02 21:47:19 PDT
Unfortunately I don't have a test page, as this is an internal page within the PowerBI.com website with tons of JavaScript that fetches data from a backend. But I'll see if I can create a static, smaller page which reproduces.

I managed to get this crash while debugging the MiniBrowser with XCode built on debug. I believe you are right, and we try to call createFontFamilyValue() with a null string. The JSC::PropertyName was "fontFamily".

Anything else that I can check with the XCode debugger to help?
Comment 6 erezg 2016-06-02 22:41:43 PDT
Also, I suspect this typescript function is causing trouble (might be another case):

        export function getSvgMeasurementProperties(svgElement: SVGTextElement): TextProperties {
            debug.assertValue(svgElement, 'svgElement');

            let style = window.getComputedStyle(svgElement, null);
            return {
                text: svgElement.textContent,
                fontFamily: style.fontFamily,
                fontSize: style.fontSize,
                fontWeight: style.fontWeight,
                fontStyle: style.fontStyle,
                fontVariant: style.fontVariant,
                whiteSpace: style.whiteSpace
            };
        }
Comment 7 Myles C. Maxfield 2016-06-14 18:01:35 PDT
The family name in the FontCascadeDescription is never getting set. Therefore, it is staying with its default construction, which is nullptr.

This means that none of FontCascadeDescription::setOneFamily(), FontCascadeDescription::setFamilies(Vector), nor FontCascadeDescription::setFamilies(RefCountedArray) is being run on the relevant object.
Comment 8 Myles C. Maxfield 2016-06-14 19:18:22 PDT
In order for ComputedStyleExtractor::propertyValue() to progress beyond its (!style) null check, the element must have a style. This should have been done earlier in the function by calling updateStyleIfNeededForNode().
Comment 9 Myles C. Maxfield 2016-06-16 00:17:26 PDT
This is because of the following code in TreeResolver::styleForElement():

    if (!m_document.haveStylesheetsLoaded() && !element.renderer()) {
        m_document.setHasNodesWithPlaceholderStyle();
        return RenderStyle::clonePtr(*placeholderStyle);
    }

This placeholder style does not have its font family set.
Comment 10 Myles C. Maxfield 2016-06-16 00:22:12 PDT
(In reply to comment #9)
> This is because of the following code in TreeResolver::styleForElement():
> 
>     if (!m_document.haveStylesheetsLoaded() && !element.renderer()) {
>         m_document.setHasNodesWithPlaceholderStyle();
>         return RenderStyle::clonePtr(*placeholderStyle);
>     }
> 
> This placeholder style does not have its font family set.

This can be resolved by the following code in ensurePlaceholderStyle()

    FontCascadeDescription fontDescription;
    fontDescription.setOneFamily(standardFamily);
    fontDescription.setKeywordSizeFromIdentifier(CSSValueMedium);
    float size = Style::fontSizeForKeyword(CSSValueMedium, false, document);
    fontDescription.setSpecifiedSize(size);
    fontDescription.setComputedSize(size);
    placeholderStyle->setFontDescription(fontDescription);
Comment 11 Myles C. Maxfield 2016-06-16 01:54:34 PDT
Created attachment 281449 [details]
Reproduction
Comment 12 Myles C. Maxfield 2016-06-16 02:06:15 PDT
It looks like the reproduction isn't working because we wait for CSS to finish downloading before running scripts. However, my script needs to run while CSS is downloading.
Comment 13 Myles C. Maxfield 2016-06-16 02:29:49 PDT
Created attachment 281450 [details]
Patch
Comment 14 WebKit Commit Bot 2016-06-16 10:10:38 PDT
Comment on attachment 281450 [details]
Patch

Clearing flags on attachment: 281450

Committed r202127: <http://trac.webkit.org/changeset/202127>
Comment 15 WebKit Commit Bot 2016-06-16 10:10:44 PDT
All reviewed patches have been landed.  Closing bug.