Bug 143404

Created attachment 250134 [details] Patch

Created attachment 250135 [details] Patch

Attachment 250135 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/SymbolConstructor.cpp:43: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 17 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 250183 [details] Patch

Attachment 250183 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/SymbolConstructor.cpp:43: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 17 files If any of these errors are false positives, please file a bug against check-webkit-style.

Could anyone take a look?

Comment on attachment 250183 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=250183&action=review > Source/JavaScriptCore/runtime/SymbolRegistry.cpp:51 > + Symbol* symbol = Symbol::create(exec, key); Is it spec-compliant that we'll return a new symbol object every time here? For example, is it spec-compliant that 'Symbol.for("a") !== Symbol.for("a")'? My reading of 19.4.2.1 says that it is not OK, and that we must return the same Symbol we returned last time, since the spec says "If SameValue(e.[[key]], stringKey) is true, return e.[[symbol]]". Also, the MDN documentation provides the example, "Symbol.for("bar") === Symbol.for("bar"); // true". Side question: If we are required to share a symbol across realms, how do we prevent information leakage across realms through the symbol prototype? I think this API might be broken by design, and not implementable in a web browser. > Source/JavaScriptCore/runtime/SymbolRegistry.h:39 > +class SymbolRegistry { This registry leaks memory because it has no way to remove UIDs once they are no longer in use: for (var i = 0; i < 10000000; ++i) Symbol.for(i); // Lots of memory now in use. You need a way to make the entries in this registry weak. WeakGCMap or HashMap<Weak<T>> might work for that purpose.

> Is it spec-compliant that we'll return a new symbol object every time here? > For example, is it spec-compliant that 'Symbol.for("a") !== Symbol.for("a")'? Ah, I see that you've modified ==, ===, Map, and Set to allow for equality across distinct symbol objects. So, my only concern here is the memory leak.

Comment on attachment 250183 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=250183&action=review Thank you for your review. >> Source/JavaScriptCore/runtime/SymbolRegistry.h:39 >> +class SymbolRegistry { > > This registry leaks memory because it has no way to remove UIDs once they are no longer in use: > > for (var i = 0; i < 10000000; ++i) > Symbol.for(i); > > // Lots of memory now in use. > > You need a way to make the entries in this registry weak. WeakGCMap or HashMap<Weak<T>> might work for that purpose. Right. Symbol.key always returns symbols. So users cannot determine that this symbols is newly created or looked up if users don't have reference to the previously registered symbol. So I think we can hold symbols in SymbolRegistry as weak references. I'll change it :D

Comment on attachment 250183 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=250183&action=review >>> Source/JavaScriptCore/runtime/SymbolRegistry.h:39 >>> +class SymbolRegistry { >> >> This registry leaks memory because it has no way to remove UIDs once they are no longer in use: >> >> for (var i = 0; i < 10000000; ++i) >> Symbol.for(i); >> >> // Lots of memory now in use. >> >> You need a way to make the entries in this registry weak. WeakGCMap or HashMap<Weak<T>> might work for that purpose. > > Right. > Symbol.key always returns symbols. So users cannot determine that this symbols is newly created or looked up if users don't have reference to the previously registered symbol. > So I think we can hold symbols in SymbolRegistry as weak references. > I'll change it :D After investigating it, I found difficult issues. Weak & WeakGCMap work for JSCell objects that's managed by GC, however, AtomicStringImpl is not GC-managed object. (Managed by RefCount) And since distinct Symbols can be equal, we cannot deal with it by holding Weak<Symbol>. For example, AtomicStringImpl* uid = ... Symbol* sym1 = Symbol::create(vm, uid); Symbol* sym2 = Symbol::create(vm, uid); // Now sym1 and sym2 are equal in JS context. Weak<Symbol> weak(sym1) // If weak becomes null, we can know sym1 is collected. // However, sym2 still may exist. We now allow for equality across distinct symbols to just store AtomicStringImpl* in PropertyTable. If we don't allow it, we need to store Symbol* into PropertyTable and GC need to mark them (so adding visitChildren for PropertyTable becomes necessary)

OK. I'll integrate WeakPtrFactory into StringImpl only if StringImpl is unique. This class may reside as if substringBuffer().

Created attachment 250460 [details] Patch

Attachment 250460 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/SymbolConstructor.cpp:44: Bad include order. Mixing system and custom headers. [build/include_order] [4] Total errors found: 1 in 21 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 250462 [details] Patch

Attachment 250462 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/SymbolConstructor.cpp:44: Bad include order. Mixing system and custom headers. [build/include_order] [4] Total errors found: 1 in 21 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 250478 [details] Patch

Attachment 250478 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/SymbolConstructor.cpp:44: Bad include order. Mixing system and custom headers. [build/include_order] [4] Total errors found: 1 in 23 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 250481 [details] Patch

Attachment 250481 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/SymbolConstructor.cpp:44: Bad include order. Mixing system and custom headers. [build/include_order] [4] Total errors found: 1 in 23 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 250486 [details] Patch

Attachment 250486 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/SymbolConstructor.cpp:44: Bad include order. Mixing system and custom headers. [build/include_order] [4] Total errors found: 1 in 23 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 250487 [details] Patch

Attachment 250487 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/SymbolConstructor.cpp:44: Bad include order. Mixing system and custom headers. [build/include_order] [4] Total errors found: 1 in 23 files If any of these errors are false positives, please file a bug against check-webkit-style.

OK, I've confirmed the patch passes the JSC tests. So the patch is ready for review.

Comment on attachment 250487 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=250487&action=review Test coverage seems a bit light. Does it really exercise all the code paths? Generally speaking, I think classes are easier to read if we keep function bodies out of them. We can still mark them inline, but the class itself is hard to read if it’s full of the function bodies and not just the declarations. > Source/JavaScriptCore/runtime/SymbolConstructor.cpp:117 > + JSValue sym = exec->argument(0); I suggest calling this local variable argument, symbolValue, or even just symbol, rather than sym. > Source/JavaScriptCore/runtime/SymbolConstructor.cpp:122 > + Symbol* symbol = asSymbol(sym); > + AtomicStringImpl* uid = symbol->privateName().uid(); I think this would read better in a single line rather than with a local variable. > Source/JavaScriptCore/runtime/VM.h:282 > + std::unique_ptr<WTF::SymbolRegistry> m_symbolRegistry; Why a pointer to a symbol registry instead of an entire symbol registry? Just costs us an extra pointer dereference all the time. > Source/JavaScriptCore/runtime/VM.h:293 > + WTF::SymbolRegistry* symbolRegistry() { return m_symbolRegistry.get(); } Should return a reference rather than a pointer, since it can never be null. > Source/WTF/wtf/text/StringImpl.cpp:295 > + RefPtr<StringImpl> rep = argRep; I don’t understand why we need this change. It looks to me like everything here would have worked if we just left rep as before. > Source/WTF/wtf/text/SymbolRegistry.cpp:40 > + WTF_MAKE_NONCOPYABLE(SymbolRegistryLocker); No needed. Deriving from SpinLockHolder already makes this non-copyable. > Source/WTF/wtf/text/SymbolRegistry.cpp:42 > + SymbolRegistryLocker(SymbolRegistry* symbolRegistry) Should take a reference rather than a pointer. > Source/WTF/wtf/text/SymbolRegistry.cpp:53 > + SymbolRegistryLocker(SymbolRegistry*) { } Should take a reference rather than a pointer. > Source/WTF/wtf/text/SymbolRegistry.cpp:59 > + : m_table() Why is this necessary? > Source/WTF/wtf/text/SymbolRegistry.cpp:61 > + , m_spinLock() Why is this necessary? > Source/WTF/wtf/text/SymbolRegistry.cpp:68 > + for (auto key : m_table) Should use auto& here instead of auto to avoid doing an extra ref/deref on every key. > Source/WTF/wtf/text/SymbolRegistry.cpp:72 > +Ref<StringImpl> SymbolRegistry::symbolForKey(String rep) This should take const String&, not String. Also not sure why we are taking String here and StringImpl elsewhere in the class. > Source/WTF/wtf/text/SymbolRegistry.cpp:77 > + if (addResult.isNewEntry) { I’d normally suggest putting the “not new” case first and doing an early return in that case. > Source/WTF/wtf/text/SymbolRegistry.cpp:86 > +String SymbolRegistry::keyForSymbol(StringImpl* uid) Should take a reference, not a pointer. > Source/WTF/wtf/text/SymbolRegistry.cpp:95 > +void SymbolRegistry::remove(StringImpl* uid) Should take a reference, not a pointer. > Source/WTF/wtf/text/SymbolRegistry.h:33 > +#include <wtf/HashSet.h> > +#include <wtf/WTFThreadData.h> > +#include <wtf/text/StringHash.h> > +#include <wtf/text/StringImpl.h> > +#include <wtf/text/WTFString.h> Too many includes. Don’t need to include headers that are included by others that are already in the list. > Source/WTF/wtf/text/SymbolRegistry.h:48 > + : m_impl() > + , m_hash() Shouldn’t need to do either of these. > Source/WTF/wtf/text/SymbolRegistry.h:52 > + explicit SymbolRegistryKey(StringImpl* uid) Should take a reference, not a pointer. > Source/WTF/wtf/text/SymbolRegistry.h:54 > + , m_hash(0) Strange to use m_hash() above and m_hash(0) here. I suggest using neither. > Source/WTF/wtf/text/SymbolRegistry.h:88 > + StringImpl* m_impl; > + unsigned m_hash; Should initialize both of these here: StringImpl* m_impl { nullptr }; unsigned m_hash { 0 }; > Source/WTF/wtf/text/SymbolRegistry.h:93 > + struct Hash : public StringHash { No need for public, since public is the default for struct inheritance. > Source/WTF/wtf/text/SymbolRegistry.h:114 > + WTF_MAKE_FAST_ALLOCATED; Should not use this, and should not allocate this on the heap. > Source/WTF/wtf/text/SymbolRegistry.h:121 > + WTF_EXPORT_PRIVATE SymbolRegistry(); > + WTF_EXPORT_PRIVATE ~SymbolRegistry(); > + > + WTF_EXPORT_PRIVATE Ref<StringImpl> symbolForKey(String); > + WTF_EXPORT_PRIVATE String keyForSymbol(StringImpl* uid); Are you sure all of these need to be exported?

Comment on attachment 250487 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=250487&action=review >> Source/JavaScriptCore/runtime/SymbolConstructor.cpp:117 >> + JSValue sym = exec->argument(0); > > I suggest calling this local variable argument, symbolValue, or even just symbol, rather than sym. OK, rename it to symbolValue. >> Source/JavaScriptCore/runtime/SymbolConstructor.cpp:122 >> + AtomicStringImpl* uid = symbol->privateName().uid(); > > I think this would read better in a single line rather than with a local variable. Fixed. >> Source/JavaScriptCore/runtime/VM.h:282 >> + std::unique_ptr<WTF::SymbolRegistry> m_symbolRegistry; > > Why a pointer to a symbol registry instead of an entire symbol registry? Just costs us an extra pointer dereference all the time. So, using SymbolRegistry directly instead of using a pointer. >> Source/JavaScriptCore/runtime/VM.h:293 >> + WTF::SymbolRegistry* symbolRegistry() { return m_symbolRegistry.get(); } > > Should return a reference rather than a pointer, since it can never be null. Fixed. >> Source/WTF/wtf/text/StringImpl.cpp:295 >> + RefPtr<StringImpl> rep = argRep; > > I don’t understand why we need this change. It looks to me like everything here would have worked if we just left rep as before. Oops, it's used for debugging purpose. I'll fix it. >> Source/WTF/wtf/text/SymbolRegistry.cpp:40 >> + WTF_MAKE_NONCOPYABLE(SymbolRegistryLocker); > > No needed. Deriving from SpinLockHolder already makes this non-copyable. OK, dropped. >> Source/WTF/wtf/text/SymbolRegistry.cpp:42 >> + SymbolRegistryLocker(SymbolRegistry* symbolRegistry) > > Should take a reference rather than a pointer. Changed. >> Source/WTF/wtf/text/SymbolRegistry.cpp:53 >> + SymbolRegistryLocker(SymbolRegistry*) { } > > Should take a reference rather than a pointer. Changed. >> Source/WTF/wtf/text/SymbolRegistry.cpp:59 >> + : m_table() > > Why is this necessary? OK, constructor is now dropped since there's no special construction. >> Source/WTF/wtf/text/SymbolRegistry.cpp:61 >> + , m_spinLock() > > Why is this necessary? Ditto. >> Source/WTF/wtf/text/SymbolRegistry.cpp:68 >> + for (auto key : m_table) > > Should use auto& here instead of auto to avoid doing an extra ref/deref on every key. Since SymbolRegistryKey doesn't have ownership to StringImpl* (when StringImpl* is destructed, SymbolRegistryKey is removed by SymbolRegistry::remove), so ref/deref doesn't occur. But, to align to the styles in WebKit, I'll use auto& :D >> Source/WTF/wtf/text/SymbolRegistry.cpp:72 >> +Ref<StringImpl> SymbolRegistry::symbolForKey(String rep) > > This should take const String&, not String. Also not sure why we are taking String here and StringImpl elsewhere in the class. OK, use const String&. This is because String represents just String, but StringImpl is used as uid (unique id) (like in JSC::Identifier class). SymbolRegistry::symbolForKey takes String and returnes Symbol (StringImpl uid). SymbolRegistry::keyForSymbol takes Symbol (StringImpl* uid) and returns String. >> Source/WTF/wtf/text/SymbolRegistry.cpp:77 >> + if (addResult.isNewEntry) { > > I’d normally suggest putting the “not new” case first and doing an early return in that case. Fixed :) >> Source/WTF/wtf/text/SymbolRegistry.cpp:86 >> +String SymbolRegistry::keyForSymbol(StringImpl* uid) > > Should take a reference, not a pointer. Here, StringImpl* is used as uid (StringImpl* uid), not String. So I think using StringImpl* is better (like in JSC::Identifier / JSC::PropertyName class). >> Source/WTF/wtf/text/SymbolRegistry.cpp:95 >> +void SymbolRegistry::remove(StringImpl* uid) > > Should take a reference, not a pointer. Ditto. >> Source/WTF/wtf/text/SymbolRegistry.h:33 >> +#include <wtf/text/WTFString.h> > > Too many includes. Don’t need to include headers that are included by others that are already in the list. Dropped several headers. >> Source/WTF/wtf/text/SymbolRegistry.h:48 >> + , m_hash() > > Shouldn’t need to do either of these. Replace it with SymbolRegistryKey() = default; >> Source/WTF/wtf/text/SymbolRegistry.h:52 >> + explicit SymbolRegistryKey(StringImpl* uid) > > Should take a reference, not a pointer. To align to the JSC::Identifier/JSC::PropertyName, use StringImpl* here (which represents uid). >> Source/WTF/wtf/text/SymbolRegistry.h:54 >> + , m_hash(0) > > Strange to use m_hash() above and m_hash(0) here. I suggest using neither. Use this style. >> Source/WTF/wtf/text/SymbolRegistry.h:88 >> + unsigned m_hash; > > Should initialize both of these here: > > StringImpl* m_impl { nullptr }; > unsigned m_hash { 0 }; Looks very nice. >> Source/WTF/wtf/text/SymbolRegistry.h:93 >> + struct Hash : public StringHash { > > No need for public, since public is the default for struct inheritance. Dropped. >> Source/WTF/wtf/text/SymbolRegistry.h:114 >> + WTF_MAKE_FAST_ALLOCATED; > > Should not use this, and should not allocate this on the heap. OK, dropped. >> Source/WTF/wtf/text/SymbolRegistry.h:121 >> + WTF_EXPORT_PRIVATE String keyForSymbol(StringImpl* uid); > > Are you sure all of these need to be exported? They are called from JSC.

Created attachment 250611 [details] Patch

Attachment 250611 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/SymbolConstructor.cpp:44: Bad include order. Mixing system and custom headers. [build/include_order] [4] Total errors found: 1 in 23 files If any of these errors are false positives, please file a bug against check-webkit-style.

Could you review this patch?

Comment on attachment 250611 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=250611&action=review I'm a bit worried about the WEB_THREAD thread safety here, but the rest of the patch looks good. > Source/WTF/wtf/text/SymbolRegistry.cpp:55 > +#if USE(WEB_THREAD) > + > +class SymbolRegistryLocker : public SpinLockHolder { > +public: > + SymbolRegistryLocker(SymbolRegistry& symbolRegistry) > + : SpinLockHolder(symbolRegistry.spinLock()) > + { > + } > +}; > + > +#else > + > +class SymbolRegistryLocker { > + WTF_MAKE_NONCOPYABLE(SymbolRegistryLocker); > +public: > + SymbolRegistryLocker(SymbolRegistry&) { } > +}; > + > +#endif // USE(WEB_THREAD) Why does the symbol registry need this locking, even though the identifier table does not?

Comment on attachment 250611 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=250611&action=review Thank you for your review! >> Source/WTF/wtf/text/SymbolRegistry.cpp:55 >> +#endif // USE(WEB_THREAD) > > Why does the symbol registry need this locking, even though the identifier table does not? Currently, AtomicString.cpp protects AtomicStringTable with AtomicStringTableLocker. It seems that it is introduced since WebCore in iOS is executed in 2 threads, the main UI thread and the WebThread. https://bugs.webkit.org/show_bug.cgi?id=108139 So in this time, I followed the design. Since this function can be called at the same time to AtomicString::remove. But seeing AtomicStringTable.cpp now, I think this gaurd for SymbolRegistry is not necessary because SymbolRegistry is owned by JSC VM and StringImpl* used for Symbol cannot be used outside the JSC world because it's hash value is special value. So I think we can drop this lock, is it correct?

> So I think we can drop this lock, is it correct? Yes, I think you are right.

(In reply to comment #32) > > So I think we can drop this lock, is it correct? > > Yes, I think you are right. Thanks! So I'll land it without locks ;)

Committed r182915: <http://trac.webkit.org/changeset/182915>

This broke Windows build: HeapInlines.h(234): error C2248: 'JSC::SymbolConstructor::StructureFlags' : cannot access protected member declared in class 'JSC::SymbolConstructor'

Oh, and Mac too. Is anyone available to fix now?

Created attachment 250972 [details] try to fix the build

Committed r182921: <http://trac.webkit.org/changeset/182921>

(In reply to comment #38) > Committed r182921: <http://trac.webkit.org/changeset/182921> Oops!!!! Thank you for notifying / fixing that. I missed it. Sorry for troubling you.