Bug 87334 – Dynamic hash table in DOMObjectHashTableMap is wrong in multiple threads

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Leo Yang]

Daynamic hash table in DOMObjectHashTableMap is copied from the static table which may be created by other thread and contains thread specific identifiers.

[Leo Yang]

Created attachment 143706 [details]
Patch

[Early Warning System Bot]

Comment on attachment 143706 [details]
Patch

Attachment 143706 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12804010

[Early Warning System Bot]

Comment on attachment 143706 [details]
Patch

Attachment 143706 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12795046

[Build Bot]

Comment on attachment 143706 [details]
Patch

Attachment 143706 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12805006

[Leo Yang]

Created attachment 143714 [details]
Patch v2

Rebased to fix build.

[Geoffrey Garen]

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.

[Alexey Proskuryakov]

Generally, I would not expect anything with "DOM" in its name to be used from non-main threads. Why is this an exception?

[Leo Yang]

(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.

[Leo Yang]

(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).

[Geoffrey Garen]

> 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?

[Leo Yang]

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.

[Alexey Proskuryakov]

> > 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.

[Geoffrey Garen]

> 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.

[Leo Yang]

(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.

[Geoffrey Garen]

> 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.

[Leo Yang]

I would choose OwnPtr<JSC::HashTable> as value type of the hash map.

[Leo Yang]

Created attachment 145927 [details]
Patch v3

[Build Bot]

Comment on attachment 145927 [details]
Patch v3

Attachment 145927 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12902703

[Leo Yang]

(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.

[Geoffrey Garen]

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.

[Leo Yang]

Created attachment 146181 [details]
Patch v4

[Leo Yang]

Could someone review the updated patch?

[Geoffrey Garen]

Comment on attachment 146181 [details]
Patch v4

r=me

[Geoffrey Garen]

Would be nice, in clang builds, to delete the copy constructor from the class.

[WebKit Review Bot]

Comment on attachment 146181 [details]
Patch v4

Clearing flags on attachment: 146181

Committed r120143: <http://trac.webkit.org/changeset/120143>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Leo Yang]

Thanks for your review Geoffrey.

[Leo Yang]

(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?

[Geoffrey Garen]

> 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.

[Leo Yang]

(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.

[Geoffrey Garen]

Darn. Thanks for looking into this.