Bug 26839 – Reference counting in JSC::UString is not thread safe

• Status: RESOLVED
• Resolution: WONTFIX
• Component: JavaScriptCore

[Martin Zoubek]

In multithread build of JavaScriptCore helgrind reports a lot of unprotected concurrent accesses to reference counters in class JSC::UString, e.g.:

==24999== Possible data race during read of size 4 at 0x6b386b8 by thread #2
==24999==    at 0x4B550E4: JSC::UString::Rep::ref() (UString.h:133)
==24999==    by 0x4B57817: WTF::RefPtr<JSC::UString::Rep>::RefPtr(WTF::RefPtr<JSC::UString::Rep> const&) (RefPtr.h:40)
==24999==    by 0x4B57836: JSC::UString::UString(JSC::UString const&) (UString.h:242)
==24999==    by 0x4B7C115: OpaqueJSString::ustring() const (OpaqueJSString.cpp:46)
==24999==    by 0x4B5E456: JSEvaluateScript (JSBase.cpp:53)
==24999==    by 0x400FE4: worker (jstest.c:61)
==24999==    by 0x4A1ECE4: mythread_wrapper (hg_intercepts.c:194)
==24999==    by 0x514AC2A: start_thread (in /lib64/tls/libpthread-0.60.so)
==24999==    by 0x4FD903F: clone (in /lib64/tls/libc-2.3.2.so)
==24999==  This conflicts with a previous write of size 4 by thread #1
==24999==    at 0x4B550EE: JSC::UString::Rep::ref() (UString.h:133)
==24999==    by 0x4B579E3: WTF::RefPtr<JSC::UString::Rep>::RefPtr(JSC::UString::Rep*) (RefPtr.h:39)
==24999==    by 0x4B57A02: JSC::UString::UString() (UString.h:493)
==24999==    by 0x4C8CEA6: JSC::RegExpConstructorPrivate::RegExpConstructorPrivate() (RegExpConstructor.cpp:95)
==24999==    by 0x4C8C8F3: JSC::RegExpConstructor::RegExpConstructor(JSC::ExecState*, WTF::PassRefPtr<JSC::Structure>, JSC::RegExpPrototype*) (RegExpConstructor.cpp:108)
==24999==    by 0x4C697A9: JSC::JSGlobalObject::reset(JSC::JSValue) (JSGlobalObject.cpp:267)
==24999==    by 0x4C6C05B: JSC::JSGlobalObject::init(JSC::JSObject*) (JSGlobalObject.cpp:149)
==24999==    by 0x4B71868: JSC::JSGlobalObject::JSGlobalObject() (JSGlobalObject.h:156)

Attached patch fixes this by using atomic operations provided by Threading.h.
For single-threaded builds the original code is preserved, therefore there should be no performance penalty.

[Martin Zoubek]

Created attachment 32062 [details]
WebKit-r45357-UString-multithread-reference-counting.diff

[Jan Alonzo]

Hi Martin

Thanks for the patch. This also needs a ChangeLog and review flag set to r?. See http://webkit.org/coding/contributing.html for more info.

[Mark Rowe (bdash)]

The comment alongside the variable declaration that you modified explicitly states that ref() and deref() are not intended to be thread safe.  It is safe to use the null and empty strings from multiple threads as there is a special-case in ::destroy that prevents them from being deleted irrespective of their reference count.  UStrings other than the null and empty strings are not intended to be used on multiple threads concurrently.  If you wish to do this, you need to use an explicit synchronization mechanism.

[Sam Weinig]

UString is not intended to be thread safe.

[Jaroslav Franek]

Hi gents,

We probably have a misunderstanding here. The purpose of this patch is not to make UString thread-safe. The sole purpose is to make the increment & decrement of the reference counter thread-safe in MT environment.

Clearly the atomic operations used in the patch would not provide thread safety for the Ustring instance. Rather they are protecting only the 'rc' member.

This fix eliminates millions of valgrind reports that otherwise overwhelm the valgrind output log. Please see the following summary got on the same test case:

original code:
==9392== ERROR SUMMARY: 3511545 errors from 365 contexts (suppressed: 3670978 from 6)

patched code:
==7902== ERROR SUMMARY: 2470023 errors from 91 contexts (suppressed: 3689267 from 6)

gain: 1041522 less errors (30%)

Our main goal is to use the WebKit on Linux x86_64 platform in multi-thread environment. The patch is important for us. Please reconsider your decision. Thank you.