Bug 144378 – LazyNeverDestroyed is not thread safe in debug builds, causing assertions

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Alexey Proskuryakov]

Thread 28 Crashed:: WebCore: Worker
0   com.apple.JavaScriptCore      	0x00000001128509da WTFCrash + 42 (Assertions.cpp:321)

ASSERT(m_isConstructed);

1   com.apple.JavaScriptCore      	0x0000000112862ed2 WTF::LazyNeverDestroyed<std::__1::mutex>::asPtr() + 66 (NeverDestroyed.h:102)
2   com.apple.JavaScriptCore      	0x0000000112862e85 WTF::LazyNeverDestroyed<std::__1::mutex>::operator std::__1::mutex&() + 21 (NeverDestroyed.h:94)
3   com.apple.JavaScriptCore      	0x0000000112862161 WTF::cachedCollatorMutex() + 577 (CollatorICU.cpp:62)
4   com.apple.JavaScriptCore      	0x0000000112861c8b WTF::Collator::Collator(char const*, bool) + 43 (CollatorICU.cpp:120)
5   com.apple.JavaScriptCore      	0x0000000112861c4c WTF::Collator::Collator(char const*, bool) + 44 (CollatorICU.cpp:146)
6   com.apple.JavaScriptCore      	0x0000000112788a4b JSC::stringProtoFuncLocaleCompare(JSC::ExecState*) + 155 (StringPrototype.cpp:1429)

LazyNeverDestroyed has a non-trivial constructor in debug builds, which is obviously wrong to have for the pattern we normally use it with.

static std::mutex& cachedCollatorMutex()
{
    static std::once_flag onceFlag;

    static LazyNeverDestroyed<std::mutex> mutex;
    std::call_once(onceFlag, []{
        mutex.construct();
    });

    return mutex;
}

[Zan Dobersek]

Removing the brace initializer for the m_isConstructed member will make it trivial again.

[Alexey Proskuryakov]

Yes, this class is only used for static variables. I don't know of a way to assert that though.

[Alexey Proskuryakov]

Created attachment 251979 [details]
proposed fix

[WebKit Commit Bot]

Comment on attachment 251979 [details]
proposed fix

Clearing flags on attachment: 251979

Committed r183608: <http://trac.webkit.org/changeset/183608>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Nikolas Zimmermann]

10 years later this has hit me:

```
Yes, this class is only used for static variables. I don't know of a way to assert that though.
```

I'm building WebKit with gcc-14, and it complains:

```
unified-sources/UnifiedSource-2f84417a-14.cpp
In file included from /host/home/nzimmermann/Software/GitRepositories/WebKit/WebKitBuild/WPE/Release/WTF/Headers/wtf/MallocCommon.h:28,
                 from /host/home/nzimmermann/Software/GitRepositories/WebKit/WebKitBuild/WPE/Release/WTF/Headers/wtf/FastMalloc.h:26,
                 from /host/home/nzimmermann/Software/GitRepositories/WebKit/Source/WebCore/config.h:47,
                 from /host/home/nzimmermann/Software/GitRepositories/WebKit/Source/WebCore/css/CSSValue.cpp:29,
                 from /host/home/nzimmermann/Software/GitRepositories/WebKit/WebKitBuild/WPE/Release/WebCore/DerivedSources/unified-sources/UnifiedSource-2f84417a-14.cpp:1:
In member function ‘void WTF::LazyNeverDestroyed< <template-parameter-1-1>, <template-parameter-1-2> >::constructWithoutAccessCheck(Args&& ...) [with Args = {WebCore::CSSValue::StaticCSSValueTag, WebCore::CSSPrimitiveValue::ImplicitInitialValueTag}; T = WebCore::CSSPrimitiveValue; AccessTraits = WTF::AnyThreadsAccessTraits]’,
    inlined from ‘void WTF::LazyNeverDestroyed< <template-parameter-1-1>, <template-parameter-1-2> >::construct(Args&& ...) [with Args = {WebCore::CSSValue::StaticCSSValueTag, WebCore::CSSPrimitiveValue::ImplicitInitialValueTag}; T = WebCore::CSSPrimitiveValue; AccessTraits = WTF::AnyThreadsAccessTraits]’ at /host/home/nzimmermann/Software/GitRepositories/WebKit/WebKitBuild/WPE/Release/WTF/Headers/wtf/NeverDestroyed.h:128:36,
    inlined from ‘WebCore::StaticCSSValuePool::StaticCSSValuePool()’ at /host/home/nzimmermann/Software/GitRepositories/WebKit/Source/WebCore/css/CSSValuePool.cpp:41:37:
/host/home/nzimmermann/Software/GitRepositories/WebKit/WebKitBuild/WPE/Release/WTF/Headers/wtf/NeverDestroyed.h:134:17: error: ‘*(WTF::LazyNeverDestroyed<WebCore::CSSPrimitiveValue, WTF::AnyThreadsAccessTraits>*)this.WTF::LazyNeverDestroyed<WebCore::CSSPrimitiveValue>::m_isConstructed’ is used uninitialized [-Werror=uninitialized]
  134 |         ASSERT(!m_isConstructed);
      |                 ^~~~~~~~~~~~~~~
/host/home/nzimmermann/Software/GitRepositories/WebKit/WebKitBuild/WPE/Release/WTF/Headers/wtf/Assertions.h:384:58: note: in definition of macro ‘UNLIKELY_FOR_C_ASSERTIONS’
  384 | #define UNLIKELY_FOR_C_ASSERTIONS(x) __builtin_expect(!!(x), 0)
      |                                                          ^
/host/home/nzimmermann/Software/GitRepositories/WebKit/WebKitBuild/WPE/Release/WTF/Headers/wtf/NeverDestroyed.h:134:9: note: in expansion of macro ‘ASSERT’
  134 |         ASSERT(!m_isConstructed);
      |         ^~~~~~
cc1plus: all warnings being treated as errors
```

That is because (Static)CSSValuePool is actually using non-static LazyNeverDestroyed objects (stored as members). While StaticCSSValuePool - as the name implies - is static itself, its members aren't and gcc caught that. Any ideas?