Summary: | Crash in com.apple.JavaScriptCore: WTF::ThreadSpecific<WTF::WTFThreadData, + 142 | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ryan Haddad <ryanhaddad> | ||||||||||
Component: | JavaScriptCore | Assignee: | Yusuke Suzuki <ysuzuki> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | ap, darin, joepeck, mark.lam, msaboff, webkit-bug-importer, ysuzuki | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | Other | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Bug Depends on: | |||||||||||||
Bug Blocks: | 188331 | ||||||||||||
Attachments: |
|
Description
Ryan Haddad
2016-11-17 16:43:23 PST
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. |