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
<rdar://problem/29168671>
Is this a regression?
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?
Solution should be simple. I'll work on it.
Created attachment 295231 [details] Patch WIP
Created attachment 295259 [details] Patch
Created attachment 295260 [details] Patch
Created attachment 295261 [details] Patch
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.
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.
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>.
Committed r208953: <http://trac.webkit.org/changeset/208953>
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.
(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
(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.
(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.
(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.