Bug 27759 – [V8] DOMData needs AtomicallyInitializedStatic

• Component: WebCore JavaScript
• Status: RESOLVED INVALID

[Adam Barth]

> +
> +DOMData* DOMData::getCurrent()
> +{
> +    if (WTF::isMainThread())
> +        return getCurrentMainThread();
> +
> +    DEFINE_STATIC_LOCAL(WTF::ThreadSpecific<ChildThreadDOMData>, childThreadDOMData, ());

This looks like it has race conditions.  Please add a comment about why it
isn't or consider switching to using AtomicallyInitializedStatic (from
wtf/Threading.h).

[Adam Barth]

I should say that those words in the issue report are Dave Levin's.  I don't understand the issue yet.

[David Levin]

This one too:

> Index: WebCore/bindings/v8/DOMData.h
> +WTF::Mutex& DOMDataStore::allStoresMutex()
> +{
> +    DEFINE_STATIC_LOCAL(WTF::Mutex, staticDOMDataListMutex, ());

This looks like it has race conditions.  Please add a comment about why it
isn't or consider switching to using AtomicallyInitializedStatic (from
wtf/Threading.h).

[anton muhin]

(In reply to comment #2)
> This one too:
> 
> > Index: WebCore/bindings/v8/DOMData.h
> > +WTF::Mutex& DOMDataStore::allStoresMutex()
> > +{
> > +    DEFINE_STATIC_LOCAL(WTF::Mutex, staticDOMDataListMutex, ());
> 
> This looks like it has race conditions.  Please add a comment about why it
> isn't or consider switching to using AtomicallyInitializedStatic (from
> wtf/Threading.h).

Just a minor comment.  If I did measurements right, AtomicallyInitializedStatic is rather expensive on Windows, so, please, be careful to measure performance implications.  Maybe sometimes it's easier to have plain static at least for main thread.

just my 2 cents.

[David Levin]

>  If I did measurements right, AtomicallyInitializedStatic is rather expensive on Windows

Likely true.  I simply want something that is safe though (for anyone using the bindings in a multithreaded fashion).  Of course, there could be a (compile time or runtime) flag that indicates whether this level of safetly is needed.

[Adam Barth]

I bet the main thread static is actually safe.  After all, you need to get a Worker wrapper before you can even create another thread.  :)

[Brian Burg]

Closing some V8-related work items.