Bug 190748 – [Win] Assertion fails while destructing local static AtomicString of FontCache::lastResortFallbackFont

• Component: New Bugs
• Status: RESOLVED FIXED

[Fujii Hironori]

[WinCairo] Assertion failure "The string being removed is atomic in the string table of an other thread!"

WinCairo, WK1, Debug build, trunk@237282

1. Start MiniBrowser
2. Go to https://webkit.org/
3. Close Window
4. Assertion fails

Callstack:

> WTF.dll!WTFCrash() Line 255	C++
> WTF.dll!WTF::AtomicStringImpl::remove(WTF::AtomicStringImpl * string) Line 489	C++
> WTF.dll!WTF::StringImpl::~StringImpl() Line 119	C++
> [External Code]	
> WTF.dll!WTF::StringImpl::destroy(WTF::StringImpl * stringImpl) Line 151	C++
> WebKit2.dll!WTF::StringImpl::deref() Line 1058	C++
> WebKit2.dll!WTF::derefIfNotNull<WTF::StringImpl>(WTF::StringImpl * ptr) Line 45	C++
> WebKit2.dll!WTF::RefPtr<WTF::StringImpl,WTF::DumbPtrTraits<WTF::StringImpl> >::~RefPtr<WTF::StringImpl,WTF::DumbPtrTraits<WTF::StringImpl> >() Line 69	C++
> WebKit2.dll!WTF::String::~String() Line 377	C++
> [External Code]	
> WebKit2.dll!WebKit::callExit(IPC::Connection * __formal) Line 163	C++
> WebKit2.dll!IPC::Connection::connectionDidClose() Line 824	C++
> WebKit2.dll!IPC::Connection::readEventHandler() Line 159	C++
> WebKit2.dll!IPC::Connection::invokeReadEventHandler::__l2::<lambda>() Line 238	C++
> WebKit2.dll!WTF::Function<void __cdecl(void)>::CallableWrapper<void <lambda>(void) >::call() Line 101	C++
> WTF.dll!WTF::Function<void __cdecl(void)>::operator()() Line 57	C++
> WTF.dll!WTF::WorkQueue::performWorkOnRegisteredWorkThread() Line 62	C++
> WTF.dll!WTF::WorkQueue::workThreadCallback(void * context) Line 44	C++
> [External Code]	


The following assertion failed.

> ASSERT_WITH_MESSAGE(iterator != atomicStringTable.end(), "The string being removed is atomic in the string table of an other thread!");

But, atomicStringTable was null.
I guess the real problem is String's dtor was called in _exit.

[Fujii Hironori]

(In reply to Fujii Hironori from comment #0)
> WinCairo, WK1, Debug build, trunk@237282

Oops. I tested with WK2.

[Fujii Hironori]

The string was "Lucida Sans Unicode".
It is  https://github.com/WebKit/webkit/blob/53d0b766f0692da943a7b01af78940a41581b702/Source/WebCore/platform/graphics/win/FontCacheWin.cpp#L358

[Fujii Hironori]

Created attachment 352775 [details]
WIP patch

[Fujii Hironori]

Created attachment 352780 [details]
Patch

[Don Olmstead]

Comment on attachment 352780 [details]
Patch

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

> Source/WebCore/platform/graphics/win/FontCacheWin.cpp:354
> -    static AtomicString fallbackFonts[] = {
> -        AtomicString("Times New Roman", AtomicString::ConstructFromLiteral),
> -        AtomicString("Microsoft Sans Serif", AtomicString::ConstructFromLiteral),
> -        AtomicString("Tahoma", AtomicString::ConstructFromLiteral),
> -        AtomicString("Lucida Sans Unicode", AtomicString::ConstructFromLiteral),
> -        AtomicString("Arial", AtomicString::ConstructFromLiteral)
> +    const auto fallbackFontNames = {

Does NeverDestroyed not work in this context?

[Fujii Hironori]

Comment on attachment 352780 [details]
Patch

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

Thank you very much for reviewing my patch.

>> Source/WebCore/platform/graphics/win/FontCacheWin.cpp:354
>> +    const auto fallbackFontNames = {
> 
> Does NeverDestroyed not work in this context?

I don't use NeverDestroyed because this code path is executed just only once. I think NeverDestroyed should be use only for instances which are used all the time.

[Fujii Hironori]

Review, please.

[Myles C. Maxfield]

Comment on attachment 352780 [details]
Patch

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

>>> Source/WebCore/platform/graphics/win/FontCacheWin.cpp:354
>>> +    const auto fallbackFontNames = {
>> 
>> Does NeverDestroyed not work in this context?
> 
> I don't use NeverDestroyed because this code path is executed just only once. I think NeverDestroyed should be use only for instances which are used all the time.

Using string literals still makes them live forever, so I'm not sure why one is preferable over the other. We should be using NeverDestroyed instead.

[Fujii Hironori]

Created attachment 353019 [details]
Patch

You are right. Applied the review feedback.

[Fujii Hironori]

Comment on attachment 353019 [details]
Patch

Clearing flags on attachment: 353019

Committed r237375: <https://trac.webkit.org/changeset/237375>

[Fujii Hironori]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/45510349>