Bug 164898 – Crash in com.apple.JavaScriptCore: WTF::ThreadSpecific<WTF::WTFThreadData, + 142

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Ryan Haddad]

Encountered with LayoutTest inspector/debugger/stepping/stepping-try-catch-finally.html

https://build.webkit.org/results/Apple%20Sierra%20Release%20WK2%20(Tests)/r208861%20(1521)/results.html

https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=inspector%2Fdebugger%2Fstepping%2Fstepping-try-catch-finally.html

Thread 12 Crashed:: WTF::AutomaticThread
0   com.apple.JavaScriptCore      	0x00000001082d7ffe WTF::ThreadSpecific<WTF::WTFThreadData, (WTF::CanBeGCThread)0>::operator WTF::WTFThreadData*() + 142 (ThreadSpecific.h:150)
1   com.apple.JavaScriptCore      	0x0000000108b2a631 WTF::AtomicStringImpl::remove(WTF::AtomicStringImpl*) + 97 (WTFThreadData.h:64)
2   com.apple.JavaScriptCore      	0x0000000108b4fe93 WTF::StringImpl::~StringImpl() + 35 (StringImpl.h:486)
3   com.apple.JavaScriptCore      	0x0000000108b4fefe WTF::StringImpl::destroy(WTF::StringImpl*) + 14 (StringImpl.cpp:138)
4   com.apple.JavaScriptCore      	0x000000010825f945 JSC::TemplateRegistryKey::~TemplateRegistryKey() + 181 (Vector.h:59)
5   com.apple.JavaScriptCore      	0x0000000108a533e0 void WTF::HashTable<JSC::TemplateRegistryKey, WTF::KeyValuePair<JSC::TemplateRegistryKey, JSC::Weak<JSC::JSArray> >, WTF::KeyValuePairKeyExtractor<WTF::KeyValuePair<JSC::TemplateRegistryKey, JSC::Weak<JSC::JSArray> > >, JSC::TemplateRegistryKey::Hasher, WTF::HashMap<JSC::TemplateRegistryKey, JSC::Weak<JSC::JSArray>, JSC::TemplateRegistryKey::Hasher, WTF::HashTraits<JSC::TemplateRegistryKey>, WTF::HashTraits<JSC::Weak<JSC::JSArray> > >::KeyValuePairTraits, WTF::HashTraits<JSC::TemplateRegistryKey> >::removeIf<JSC::WeakGCMap<JSC::TemplateRegistryKey, JSC::JSArray, JSC::TemplateRegistryKey::Hasher, WTF::HashTraits<JSC::TemplateRegistryKey> >::pruneStaleEntries()::'lambda'(WTF::KeyValuePair<JSC::TemplateRegistryKey, JSC::Weak<JSC::JSArray> >&)>(JSC::WeakGCMap<JSC::TemplateRegistryKey, JSC::JSArray, JSC::TemplateRegistryKey::Hasher, WTF::HashTraits<JSC::TemplateRegistryKey> >::pruneStaleEntries()::'lambda'(WTF::KeyValuePair<JSC::TemplateRegistryKey, JSC::Weak<JSC::JSArray> >&) const&) + 160 (Vector.h:559)
6   com.apple.JavaScriptCore      	0x0000000108a53335 std::__1::__function::__func<JSC::WeakGCMap<JSC::TemplateRegistryKey, JSC::JSArray, JSC::TemplateRegistryKey::Hasher, WTF::HashTraits<JSC::TemplateRegistryKey> >::WeakGCMap(JSC::VM&)::'lambda'(), std::__1::allocator<JSC::WeakGCMap<JSC::TemplateRegistryKey, JSC::JSArray, JSC::TemplateRegistryKey::Hasher, WTF::HashTraits<JSC::TemplateRegistryKey> >::WeakGCMap(JSC::VM&)::'lambda'()>, void ()>::operator()() + 21 (functional:1437)
7   com.apple.JavaScriptCore      	0x0000000108626291 JSC::Heap::collectInThread() + 593 (HashTable.h:181)
8   com.apple.JavaScriptCore      	0x000000010862852d JSC::Heap::Thread::work() + 13 (Heap.cpp:257)
9   com.apple.JavaScriptCore      	0x0000000108b5d19f std::__1::__function::__func<WTF::AutomaticThread::start(WTF::Locker<WTF::LockBase> const&)::$_0, std::__1::allocator<WTF::AutomaticThread::start(WTF::Locker<WTF::LockBase> const&)::$_0>, void ()>::operator()() + 415 (AutomaticThread.cpp:195)
10  com.apple.JavaScriptCore      	0x0000000108b629c2 WTF::threadEntryPoint(void*) + 178 (functional:1766)
11  com.apple.JavaScriptCore      	0x0000000108b62d9f WTF::wtfThreadEntryPoint(void*) + 15 (memory:2723)
12  libsystem_pthread.dylib       	0x00007fffc327aabb _pthread_body + 180
13  libsystem_pthread.dylib       	0x00007fffc327aa07 _pthread_start + 286
14  libsystem_pthread.dylib       	0x00007fffc327a231 thread_start + 13

[Ryan Haddad]

<rdar://problem/29168671>

[Alexey Proskuryakov]

Is this a regression?

[Yusuke Suzuki]

I guess TemplateRegistry's WeakGCMap's visiting causes some ref()/deref() of StringImpl. It should not be done in the other thread. But now, collecting is done in its thread. Right?

[Yusuke Suzuki]

Solution should be simple. I'll work on it.

[Yusuke Suzuki]

Created attachment 295231 [details]
Patch

WIP

[Yusuke Suzuki]

Created attachment 295259 [details]
Patch

[Yusuke Suzuki]

Created attachment 295260 [details]
Patch

[Yusuke Suzuki]

Created attachment 295261 [details]
Patch

[Darin Adler]

Comment on attachment 295261 [details]
Patch

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

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3005
> -    JSTemplateRegistryKey*& templateRegistryKeyInMap = m_templateRegistryKeyMap.add(templateRegistryKey, nullptr).iterator->value;
> +    JSTemplateRegistryKey*& templateRegistryKeyInMap = m_templateRegistryKeyMap.add(templateRegistryKey.copyRef(), nullptr).iterator->value;
>      if (!templateRegistryKeyInMap) {
> -        templateRegistryKeyInMap = JSTemplateRegistryKey::create(*vm(), templateRegistryKey);
> +        templateRegistryKeyInMap = JSTemplateRegistryKey::create(*vm(), WTFMove(templateRegistryKey));
>          addConstantValue(templateRegistryKeyInMap);
>      }
>      return templateRegistryKeyInMap;

This can be written using HashMap::ensure instead. I think that idiom would be a little cleaner than this, but not entirely sure.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:888
> +        JSTemplateRegistryKey* addTemplateRegistryKeyConstant(Ref<TemplateRegistryKey>);

It’s not so common to use Ref as the type of a function argument. More common are const Ref& or Ref&&, depending on whether the caller is passing ownership or not.

If we keep the return value of createKey returning a Ref, then I think the type we want here is Ref&&. If we change createKey to return a const Ref&, then I think we want to take const Ref&.

> Source/JavaScriptCore/runtime/TemplateRegistry.h:31
> +#include <wtf/Ref.h>

I don’t understand why this include is needed. I don’t see any use of Ref in this header.

> Source/JavaScriptCore/runtime/TemplateRegistry.h:42
> +    JSArray* getTemplateObject(ExecState*, JSTemplateRegistryKey*);

If this code was in WebCore, I would suggest changing the argument type to JSTemplateRegistryKey&, but I am not sure if that style is welcomed in this directory.

> Source/JavaScriptCore/runtime/TemplateRegistryKey.h:69
> +        for (const String& string : rawStrings)
> +            hash += WTF::StringHash::hash(string);

This is not changing, but: Adding hashes together is not typically a great way to make a hash for a set of strings. Hashing the hashes of the strings together is one good way. Another good way is to hash all the characters of all the strings.

I also think this would be better if the function body was outside the class definition.

> Source/JavaScriptCore/runtime/TemplateRegistryKeyTable.cpp:47
> +    static unsigned hash(TemplateRegistryKey* generator)
> +    {
> +        return generator->hash();
> +    }
> +
> +    static inline bool equal(TemplateRegistryKey* key, TemplateRegistryKey* generator)
> +    {
> +        return *key == *generator;
> +    }
> +
> +    static void translate(TemplateRegistryKey*& location, TemplateRegistryKey* generator, unsigned)
> +    {
> +        location = generator;
> +    }

These would probably look better as one-liners.

> Source/JavaScriptCore/runtime/TemplateRegistryKeyTable.cpp:58
> +    Ref<TemplateRegistryKey> key = TemplateRegistryKey::create(rawStrings, cookedStrings);

auto would be better

> Source/JavaScriptCore/runtime/TemplateRegistryKeyTable.h:31
> +#include <wtf/Ref.h>

Header needs only Forward.h, not this.

> Source/JavaScriptCore/runtime/TemplateRegistryKeyTable.h:33
> +#include <wtf/text/WTFString.h>

Header needs only Forward.h, not this.

> Source/JavaScriptCore/runtime/TemplateRegistryKeyTable.h:40
> +    typedef Vector<String, 4> StringVector;

This should be using rather than typedef.

> Source/JavaScriptCore/runtime/TemplateRegistryKeyTable.h:44
> +    Ref<TemplateRegistryKey> createKey(const StringVector& rawStrings, const StringVector& cookedStrings);

The return value for this can be const Ref<TemplateRegistryKey>& since it always returns a key in the HashSet. And that reduces reference count churn a little bit, so I suggest we do it.

> Source/JavaScriptCore/runtime/TemplateRegistryKeyTable.h:46
> +    void unregister(TemplateRegistryKey*);

This should take a reference rather than a pointer.

> Source/JavaScriptCore/runtime/TemplateRegistryKeyTable.h:52
> +    struct KeyHash {
> +        static unsigned hash(const TemplateRegistryKey* key) { return key->hash(); }
> +        static bool equal(const TemplateRegistryKey* a, const TemplateRegistryKey* b) { return *a == *b; }
> +        static const bool safeToCompareToEmptyOrDeleted = false;
> +    };

This can be private.

[Yusuke Suzuki]

Comment on attachment 295261 [details]
Patch

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

Thanks!

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3005
>>      return templateRegistryKeyInMap;
> 
> This can be written using HashMap::ensure instead. I think that idiom would be a little cleaner than this, but not entirely sure.

Sounds nice. I'll change this with HashMap::ensure. Nice.

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:888
>> +        JSTemplateRegistryKey* addTemplateRegistryKeyConstant(Ref<TemplateRegistryKey>);
> 
> It’s not so common to use Ref as the type of a function argument. More common are const Ref& or Ref&&, depending on whether the caller is passing ownership or not.
> 
> If we keep the return value of createKey returning a Ref, then I think the type we want here is Ref&&. If we change createKey to return a const Ref&, then I think we want to take const Ref&.

Sounds nice. I changed it to `const Ref&` since now the createKey returns `const Ref&`.

>> Source/JavaScriptCore/runtime/TemplateRegistry.h:31
>> +#include <wtf/Ref.h>
> 
> I don’t understand why this include is needed. I don’t see any use of Ref in this header.

Oops. I used it in the previous version of this patch. Dropped.

>> Source/JavaScriptCore/runtime/TemplateRegistry.h:42
>> +    JSArray* getTemplateObject(ExecState*, JSTemplateRegistryKey*);
> 
> If this code was in WebCore, I would suggest changing the argument type to JSTemplateRegistryKey&, but I am not sure if that style is welcomed in this directory.

Personally I like this since `JSTemplateRegistryKey` is JS managed object and basically these objects are referenced in the pointer form in JSC.

>> Source/JavaScriptCore/runtime/TemplateRegistryKey.h:69
>> +            hash += WTF::StringHash::hash(string);
> 
> This is not changing, but: Adding hashes together is not typically a great way to make a hash for a set of strings. Hashing the hashes of the strings together is one good way. Another good way is to hash all the characters of all the strings.
> 
> I also think this would be better if the function body was outside the class definition.

Yeah. I've changed it to calculating the hash of all the characters.

>> Source/JavaScriptCore/runtime/TemplateRegistryKeyTable.cpp:47
>> +    }
> 
> These would probably look better as one-liners.

Changed.

>> Source/JavaScriptCore/runtime/TemplateRegistryKeyTable.cpp:58
>> +    Ref<TemplateRegistryKey> key = TemplateRegistryKey::create(rawStrings, cookedStrings);
> 
> auto would be better

Fixed.

>> Source/JavaScriptCore/runtime/TemplateRegistryKeyTable.h:31
>> +#include <wtf/Ref.h>
> 
> Header needs only Forward.h, not this.

Nice. Fixed.

>> Source/JavaScriptCore/runtime/TemplateRegistryKeyTable.h:33
>> +#include <wtf/text/WTFString.h>
> 
> Header needs only Forward.h, not this.

Fixed.

>> Source/JavaScriptCore/runtime/TemplateRegistryKeyTable.h:40
>> +    typedef Vector<String, 4> StringVector;
> 
> This should be using rather than typedef.

Fixed.

>> Source/JavaScriptCore/runtime/TemplateRegistryKeyTable.h:44
>> +    Ref<TemplateRegistryKey> createKey(const StringVector& rawStrings, const StringVector& cookedStrings);
> 
> The return value for this can be const Ref<TemplateRegistryKey>& since it always returns a key in the HashSet. And that reduces reference count churn a little bit, so I suggest we do it.

Sounds nice. Fixed.

>> Source/JavaScriptCore/runtime/TemplateRegistryKeyTable.h:46
>> +    void unregister(TemplateRegistryKey*);
> 
> This should take a reference rather than a pointer.

Fixed.

>> Source/JavaScriptCore/runtime/TemplateRegistryKeyTable.h:52
>> +    };
> 
> This can be private.

Fixed.

[Yusuke Suzuki]

Comment on attachment 295261 [details]
Patch

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

>>> Source/JavaScriptCore/runtime/TemplateRegistryKeyTable.h:44
>>> +    Ref<TemplateRegistryKey> createKey(const StringVector& rawStrings, const StringVector& cookedStrings);
>> 
>> The return value for this can be const Ref<TemplateRegistryKey>& since it always returns a key in the HashSet. And that reduces reference count churn a little bit, so I suggest we do it.
> 
> Sounds nice. Fixed.

We do not store it as Ref<> in the hash set. We just store it as TemplateRegistryKey* in the hash table because this table should not retain these keys.
So, we will use Ref<TemplateRegistryKey>.

[Yusuke Suzuki]

Committed r208953: <http://trac.webkit.org/changeset/208953>

[Mark Lam]

Looks like this patch (r208953) is causing an assertion failure in JSTests/stress/tagged-templates-syntax.js.  The test runs fine with r208952 and fails in r208954, and r208954 is not JSC related.

[Yusuke Suzuki]

(In reply to comment #13)
> Looks like this patch (r208953) is causing an assertion failure in
> JSTests/stress/tagged-templates-syntax.js.  The test runs fine with r208952
> and fails in r208954, and r208954 is not JSC related.

oops, looking

[Mark Lam]

(In reply to comment #14)
> (In reply to comment #13)
> > Looks like this patch (r208953) is causing an assertion failure in
> > JSTests/stress/tagged-templates-syntax.js.  The test runs fine with r208952
> > and fails in r208954, and r208954 is not JSC related.
> 
> oops, looking

I think I found the issue: Hasher::addCharacters() needs to check for legnth 0 and do nothing.  Currently, it doesn't.  I'm writing a test now.

[Mark Lam]

(In reply to comment #13)
> Looks like this patch (r208953) is causing an assertion failure in
> JSTests/stress/tagged-templates-syntax.js.  The test runs fine with r208952
> and fails in r208954, and r208954 is not JSC related.

I'll fix this in https://bugs.webkit.org/show_bug.cgi?id=165024.

[Yusuke Suzuki]

(In reply to comment #16)
> (In reply to comment #13)
> > Looks like this patch (r208953) is causing an assertion failure in
> > JSTests/stress/tagged-templates-syntax.js.  The test runs fine with r208952
> > and fails in r208954, and r208954 is not JSC related.
> 
> I'll fix this in https://bugs.webkit.org/show_bug.cgi?id=165024.

Oops, I've just returned in front of my keyboard.
r=me. Thanks, and sorry for troubling you.