Daynamic hash table in DOMObjectHashTableMap is copied from the static table which may be created by other thread and contains thread specific identifiers.
Created attachment 143706 [details] Patch
Comment on attachment 143706 [details] Patch Attachment 143706 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12804010
Comment on attachment 143706 [details] Patch Attachment 143706 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/12795046
Comment on attachment 143706 [details] Patch Attachment 143706 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12805006
Created attachment 143714 [details] Patch v2 Rebased to fix build.
Comment on attachment 143714 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=143714&action=review > Source/WebCore/bindings/js/DOMObjectHashTableMap.h:56 > + // Don't copy dynamic allocated table which may be allocated on other thread and contains thread specific identifiers. > + // For example, a JSEntryArray's hash map was first initialized on a worker thread, and then the user reloaded > + // the page, another worker thread is created due to reload, the dynamic allocated table in *staticTable* is specific > + // to the first worker thread which has died. If the user reload the page again, the dynamic table will be freed > + // and memory corruption will occur. > + table->table = 0; Is there any other code that copies a JSC::HashTable and needs this fix? I'd prefer to see this logic in a JSC::HashTable copy constructor. That's the best way to ensure that we get this idiom right in all uses of JSC::HashTable.
Generally, I would not expect anything with "DOM" in its name to be used from non-main threads. Why is this an exception?
(In reply to comment #6) > (From update of attachment 143714 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=143714&action=review > > > Source/WebCore/bindings/js/DOMObjectHashTableMap.h:56 > > + // Don't copy dynamic allocated table which may be allocated on other thread and contains thread specific identifiers. > > + // For example, a JSEntryArray's hash map was first initialized on a worker thread, and then the user reloaded > > + // the page, another worker thread is created due to reload, the dynamic allocated table in *staticTable* is specific > > + // to the first worker thread which has died. If the user reload the page again, the dynamic table will be freed > > + // and memory corruption will occur. > > + table->table = 0; > > Is there any other code that copies a JSC::HashTable and needs this fix? I don't find. > > I'd prefer to see this logic in a JSC::HashTable copy constructor. That's the best way to ensure that we get this idiom right in all uses of JSC::HashTable. Auto-generated Lexer.lut.h is using POD style initializer for JSC::HasTable. It will fail compile if we add a constructor to JSC::HashTable.
(In reply to comment #7) > Generally, I would not expect anything with "DOM" in its name to be used from non-main threads. Why is this an exception? Actually, many idl files (like WebCore/Modules/filesystem/*.idl) which are not part of "DOM" is being used in multiple thread (for example, in worker thread).
> Auto-generated Lexer.lut.h is using POD style initializer for JSC::HasTable. It will fail compile if we add a constructor to JSC::HashTable. Is there some reason this can't change?
I'd prefer to keep the default bit-wise copy semantics. The copy constructor is being called implicitly when copy semantics is needed, for example when WTF::HashMap with value type JSC::HashTable moves values internally. For these cases bit-wise copy semantics is the right one. When we really don't need bit-wise copy we can do what we want explicitly just as the patch does.
> > Generally, I would not expect anything with "DOM" in its name to be used from non-main threads. Why is this an exception? > > Actually, many idl files (like WebCore/Modules/filesystem/*.idl) which are not part of "DOM" is being used in multiple thread (for example, in worker thread). OK, but what's DOMObjectHashTableMap anyway? "DOM Object" makes no sense if that includes both nodes and things like DOMFileSystem.idl - these objects have nothing in common.
> The copy constructor is being called implicitly when copy semantics is needed, for example when > WTF::HashMap with value type JSC::HashTable moves values internally. For these cases bit-wise copy > semantics is the right one. You can get a bitwise copy in HashMap using HashTraits. See, for example, the traits for OwnPtr. > When we really don't need bit-wise copy we can do what we want explicitly just as the patch does. My problem is with the word "we" here. Currently, there's no "we". There's only one person in the WebKit project who understands the subtle thread-related problems that can arise when copying a JSC::HashTable: you. If you give JSC::HashTable a copy constructor with correct semantics, along with a comment in the constructor explaining the semantics, then there is a "we": anyone who tries to use the class incorrectly will get a compile error, which will point them at the constructor function, and a comment explaining its behavior.
(In reply to comment #13) > > The copy constructor is being called implicitly when copy semantics is needed, for example when > > WTF::HashMap with value type JSC::HashTable moves values internally. For these cases bit-wise copy > > semantics is the right one. > > You can get a bitwise copy in HashMap using HashTraits. See, for example, the traits for OwnPtr. I can't see how to get bitwise copy when a copy constructor is not bitwise copy where copy semantics is need for the compiler. For example, how to get bitwise copy for a return value (e.g. return value of HashTraits::passOut) when the copy constructor for JSC::HashTable is not bitwise semantically? > > > When we really don't need bit-wise copy we can do what we want explicitly just as the patch does. > > My problem is with the word "we" here. Currently, there's no "we". There's only one person in the WebKit project who understands the subtle thread-related problems that can arise when copying a JSC::HashTable: you. > > If you give JSC::HashTable a copy constructor with correct semantics, along with a comment in the constructor explaining the semantics, then there is a "we": anyone who tries to use the class incorrectly will get a compile error, which will point them at the constructor function, and a comment explaining its behavior.
> For example, how to get bitwise copy for a return value (e.g. return value of HashTraits::passOut) Are you asking how to implement DOMObjectHashTableMap::get()? Since that function already operates in terms of pointers and iterators, I don't think it requires an implementation of HashTraits::passOut(). If you need HashTraits::passOut() for JSC::HashTable, I'd recommend using JSC::HashTable* as the PassOut type, just like OwnPtr<T> uses T* as the PassOut type.
I would choose OwnPtr<JSC::HashTable> as value type of the hash map.
Created attachment 145927 [details] Patch v3
Comment on attachment 145927 [details] Patch v3 Attachment 145927 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12902703
(In reply to comment #18) > (From update of attachment 145927 [details]) > Attachment 145927 [details] did not pass mac-ews (mac): > Output: http://queues.webkit.org/results/12902703 It seems that Mac build doesn't allow global initializer for most files. Tools/Scripts/check-for-global-initializers was generating errors.
Comment on attachment 145927 [details] Patch v3 View in context: https://bugs.webkit.org/attachment.cgi?id=145927&action=review > It seems that Mac build doesn't allow global initializer for most files. Indeed, it doesn't. I'm surprised that constant initialization requires a global initializer. I guess we have to go back to struct initialization. Let's use a "copy" function in JSC::HashTable instead of a copy constructor. > Source/JavaScriptCore/runtime/Lookup.h:122 > + { } WebKit style is to put a newline between "{" and "}". > Source/JavaScriptCore/runtime/Lookup.h:127 > + if (this == &other) > + return; Since this is not the assignment operator, this == &other should be impossible. > Source/JavaScriptCore/runtime/Lookup.h:132 > + compactSize = other.compactSize; > + compactHashSizeMask = other.compactHashSizeMask; > + values = other.values; > + // Don't copy dynamic table since it's thread specific. > + table = 0; Please use initializer syntax here.
Created attachment 146181 [details] Patch v4
Could someone review the updated patch?
Comment on attachment 146181 [details] Patch v4 r=me
Would be nice, in clang builds, to delete the copy constructor from the class.
Comment on attachment 146181 [details] Patch v4 Clearing flags on attachment: 146181 Committed r120143: <http://trac.webkit.org/changeset/120143>
All reviewed patches have been landed. Closing bug.
Thanks for your review Geoffrey.
(In reply to comment #24) > Would be nice, in clang builds, to delete the copy constructor from the class. I'm not familiar with that part. Which files need to be updated?
> I'm not familiar with that part. Which files need to be updated? The JSC::HashTable declaration would need a declaration similar to the one in <wtf/Noncopyable.h>. The WTF_MAKE_NONCOPYABLE macro shows how to use "delete" syntax to remove the copy constructor for a class. I believe we could do this in clang builds without causing the problems created by adding private or special copy constructors.
(In reply to comment #29) > > I'm not familiar with that part. Which files need to be updated? > > The JSC::HashTable declaration would need a declaration similar to the one in <wtf/Noncopyable.h>. > > The WTF_MAKE_NONCOPYABLE macro shows how to use "delete" syntax to remove the copy constructor for a class. I believe we could do this in clang builds without causing the problems created by adding private or special copy constructors. I tried to use "HashTable(const HashTable&) = delete;" to delete the copy constructor. But it will cause compiling problems. 1. The deleted copy constructor will prevent POD style initializer. The error message in gcc 4.7 is like "error: could not convert ‘{133, 127, JSC::mainTableValues, 0}’ from ‘<brace-enclosed initializer list>’ to ‘const JSC::HashTable’" and "error: use of deleted function ‘JSC::HashTable::HashTable(const JSC::HashTable&)’". 2. JavaScriptCore/parser/Lexer.cpp is using bitwise copy semantics for m_keywordTable. But this problem can be resolved by adding a copy function that does bitwise copy. In summary, disallowing global initializer (as mentioned in previous comment) prevents us removing POD style initializer that prevents us removing copy constructor.
Darn. Thanks for looking into this.