Bug 68429

Created attachment 107972 [details] Patch

Created attachment 107973 [details] Patch

Attachment 107973 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1 Source/JavaScriptCore/heap/MachineStackMarker.cpp:152: Missing space inside { }. [whitespace/braces] [5] Source/JavaScriptCore/heap/MachineStackMarker.cpp:153: Place brace on its own line for function definitions. [whitespace/braces] [4] Source/JavaScriptCore/heap/MachineStackMarker.cpp:158: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 3 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 107975 [details] Patch

Created attachment 115566 [details] Patch

Attachment 115566 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1 Source/JavaScriptCore/wtf/ThreadSpecificWin.cpp:39: This { should be at the end of the previous line [whitespace/braces] [4] Source/JavaScriptCore/wtf/ThreadSpecific.h:122: The parameter name "key" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptCore/wtf/ThreadSpecific.h:123: The parameter name "key" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptCore/wtf/ThreadSpecific.h:124: The parameter name "key" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptCore/wtf/ThreadSpecific.h:125: The parameter name "key" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 5 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 115566 [details] Patch Attachment 115566 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/10510175

Comment on attachment 115566 [details] Patch Attachment 115566 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/10509165

Created attachment 115572 [details] Patch

Attachment 115572 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1 Source/JavaScriptCore/wtf/ThreadSpecific.h:51: "pthread.h" already included at Source/JavaScriptCore/wtf/ThreadSpecific.h:48 [build/include] [4] Total errors found: 1 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 115572 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=115572&action=review > Source/JavaScriptCore/heap/MachineStackMarker.cpp:148 > + WTF::ThreadSpecificKeyDelete(m_threadSpecific); Why do you prefix with WTF here, but not in other cases in this file?

Created attachment 128011 [details] Patch

Comment on attachment 128011 [details] Patch Why can't we use WTF::ThreadSpecific (like the bug title suggests)?

(In reply to comment #13) > (From update of attachment 128011 [details]) > Why can't we use WTF::ThreadSpecific (like the bug title suggests)? "missing" constructor: http://trac.webkit.org/browser/trunk/Source/JavaScriptCore/wtf/ThreadSpecific.h?rev=107587#L79

Comment on attachment 128011 [details] Patch I wonder if this use of pthread_key_delete is needed.

It would be great not to have to introduce a new threading abstraction. If we can find a way to make WTF::ThreadSpecific work, that would be ideal. It sounds like we could use it if we don't need to support the existing pthread_key_delete behavior. Alexey and the JSC folks are the experts here.

Is there any thought on this? Currently this is one of the WinCE build breakers. If we can't use the ThreadSpecific class then is the desired direction to use the Win32 API for thread specific key creation?

I'm interested in this too. I have pthreads compiling for WinCE (not tested yet), but it would be nice to have the flexibility to not use pthreads on WinCE since the project is not being maintained.

*raises hand to show interest* Peter, any thoughts on the use of ThreadSpecific based on Adam's comments?

(In reply to comment #19) > *raises hand to show interest* > > Peter, any thoughts on the use of ThreadSpecific based on Adam's comments? Since I do not see any Peter in the CC list, I think that you mean PATRICK. Please don't get me wrong, but here some points: *) It is nice to get a "i want this too" every day, but since we are an open source project everybody can post a patch. *) It is hard do post a "correct" patch, when nobody says what's the "correct" way. *) The original authors of the pthread stuff i want to remove do not show much participation here. (first patch is from 2011-09) *) It's windows only. *) pthread works _somehow_ on windows so no real need in the moment. That it implies additional dependencies for other ports like Qt on windows too isn't a problem for the Apple folks. *) Like everybody else who works on WebKit in his free time, I prefer changes which get into trunk and make (more) fun to work on. If someone tells me the way to go, I'm willing to write a patch and post it for review here. *) Extending the current ThreadSpecific with a Destructor makes the current code even more tricky (unreadable) or adds additional Maps and Sets, which make threads more constable. *) IMHO MachineStackMarker needs some (bigger) changes to fit into the ThreadSpecific concept, since the current code does not use the pthread_key_t as a thread specific storage. It's used to detect when a thread ends. Once again: Please don't get me wrong.

Well if the goal is to simply remove the blatant pthread dependency in MachineStackMarker then I am happy to provide a patch that addresses that issue so that you can continue to build for Windows platforms without the requirement of a pthread cover library on top of the native threading API. My comment was less of a "hand up +1" than a question about how can we get this issue solved and moving forward ... and how can I help with that.

One again: The last comment wasn't agains anybody! (more a general "problem" with this bug) I had a discussion with Simon this morning about this in IRC and he agreed, that there is no easy/simple and clean solution. If we want a clean solution (and at WebKit we usually go this way) we need to investigate more on this issue. Of course is it possible to change only MachineStackMarker, but we should investigate a clean and generic solution, since we'll have similar requirements in FastMalloc too, if we want to get rid complete pthread stuff on windows. (In reply to comment #21) > ... and how can I help with that. Look into the code and post an idea how to solve this here. I'd suggest that we agree on a solution first (what code do we ant to change/extend) so we avoid any (hard and useless) patch creation. But feel free to do it like you want. :-) Maybe Simon has an idea of a clean solution in the meantime?

(In reply to comment #22) > One again: The last comment wasn't agains anybody! (more a general "problem" with this bug) > > I had a discussion with Simon this morning about this in IRC and he agreed, that there is no easy/simple and clean solution. If we want a clean solution (and at WebKit we usually go this way) we need to investigate more on this issue. Of course is it possible to change only MachineStackMarker, but we should investigate a clean and generic solution, since we'll have similar requirements in FastMalloc too, if we want to get rid complete pthread stuff on windows. > > (In reply to comment #21) > > ... and how can I help with that. > Look into the code and post an idea how to solve this here. I'd suggest that we agree on a solution first (what code do we ant to change/extend) so we avoid any (hard and useless) patch creation. But feel free to do it like you want. :-) > > Maybe Simon has an idea of a clean solution in the meantime? So I've hit this issue as well and wanted to make a potentially ill-informed comment. So the rub with ThreadSpecific boils down to the destructor not being implemented for a very good reason, undefined behavior. But something like a pthreads implementation of ThreadSpecific would end up calling pthread_key_delete as MachineThreads does in its destructor. The fact that pthread_key_delete is called this way makes it seem like this is okay here because of some knowledge on the lifetime of the pthread_key. So why not create something like ThreadSpecific<T> but with a destructor. So like a std::unique_ptr we know that only one thing owns it and we don't need to reference count it or do anything funky when determining when to delete it. Forgive me if I'm missing some larger issue as I'm a relative newbie with this codebase. I'm currently blocked on this issue so I figured I'd chime in. Cheers, Don

As an alternate suggestion, perhaps we could consider using http://locklessinc.com/articles/pthreads_on_windows/ for Windows? It's implemented entirely in a header file and is BSD licensed, meaning a) it works for both 32 and 64 bit platforms without problems (whereas there is no precompiled 64 bit version of pthreads-win32, as the name suggests). and b) it can be shipped with WebKit as source instead of being an external dependency.

(In reply to comment #24) > As an alternate suggestion, perhaps we could consider using > > http://locklessinc.com/articles/pthreads_on_windows/ > > for Windows? It's implemented entirely in a header file and is BSD licensed, meaning Does it work? IMHO it can't: Threads are created vie CreateThread(), so there is no place where the pthread code for "on thread exit" can be executed. When used as DLL there are some tricks for this, since DLLs get informed about such events and can handle them.

(In reply to comment #25) > (In reply to comment #24) > > As an alternate suggestion, perhaps we could consider using > > > > http://locklessinc.com/articles/pthreads_on_windows/ > > > > for Windows? It's implemented entirely in a header file and is BSD licensed, meaning > > Does it work? IMHO it can't: Threads are created vie CreateThread(), so there is no place where the pthread code for "on thread exit" can be executed. When used as DLL there are some tricks for this, since DLLs get informed about such events and can handle them. Since WebKit itself is a DLL, it could make a callback into the pthread implementation whenever a thread is created/destroyed.

(In reply to comment #26) > Since WebKit itself is a DLL, it could make a callback into the pthread implementation whenever a thread is created/destroyed. But then we need additonal ugly DLL hooks in the WebKit DLL and it won't work when sb does static builds. IMHO it doesn't make much sense to implement any "workarounds" like DLL hooks when there is an easy way to put it into the "normal" codebase.

The more I look into this the more I like Patrick's patch. MachineStackMarker is using two key aspects of the underlying TLS platform API: 1) Get notified when the thread exits 2) Store just a pointer in the TLS (no ownership assumed, no deletion required) When the thread that owns MachineStackMarker exits, then it will just destroy the TLS key to "unsubscribe" from any thread-termination notification. The destruction does not require any data deletion, because of (2). The WTF::ThreadSpecific interface is designed to store and own a piece of data - it allocates the template parameter T implicitly per thread and it also destroy the per-thread instance. Aspect (1) of MachineStackMarker could theoretically be mapped to the destructor of a wrapping type in MachineStackMarker, but there is no mechanism in ThreadSpecific to "unsubscribe" from termination deletion. It would be possible to just solve this with another indirection inside MachineStackMarker, but it comes at the price of the ThreadSpecific remaining alive until all the threads have terminated that were ever "adopted". So in a scenario with two long living threads, one creating and deleting MachineStackMarker instances and the other thread ending up using the JSC instance from time to time, we end up with slowly eating up TLS keys. One solution I see is to introduce a pointer specific specialization of ThreadSpecific (T*) that would _just_ store a pointer and would allow for a callback when the thread terminates. But that would be a C++/template interface to what Patrick already does with a few helper functions in his patch. And it would make the ThreadSpecific API slightly inconsistent depending on the specialization. Alexey, Adam: Would you be willing to reconsider Patrick's patch?

Comment on attachment 128011 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=128011&action=review > Source/JavaScriptCore/wtf/ThreadSpecific.h:170 > +class ThreadSpecificKeyValue; "KeyValue" is a very confusing name phrase. Our typical style is to use the word "platform": "PlatformThreadSpecificKey". > Source/JavaScriptCore/wtf/ThreadSpecific.h:176 > +void ThreadSpecificKeyCreate(ThreadSpecificKey*, void (*)(void *)); > +void ThreadSpecificKeyDelete(ThreadSpecificKey); > +void ThreadSpecificSet(ThreadSpecificKey, void*); > +void* ThreadSpecificGet(ThreadSpecificKey); Capitalization here doesn't match WebKit style. > Source/JavaScriptCore/wtf/ThreadSpecificWin.cpp:60 > + while (*next != this) { > + ASSERT(*next); > + next = &(*next)->m_next; > + } > + *next = (*next)->m_next; Linear search is so 1960s. Please use a doubly linked list with O(1) removal time. > Source/JavaScriptCore/wtf/ThreadSpecificWin.cpp:84 > + static ThreadSpecificKeyValue* m_first; WebKit style is to use the "s_" prefix for static data. "m_" means "member" data. Please don't call static data member data.

Created attachment 156016 [details] Patch

Comment on attachment 156016 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=156016&action=review > Source/WTF/wtf/ThreadSpecificWin.cpp:128 > + key->callDestructor(); > + key = key->next(); Do we expect the destructor to delete the PlatformThreadSpecificKey by calling threadSpecificKeyDelete()? If so, we need to fetch next() before calling the destructor, to avoid dereferencing deleted memory. If not, who do we expect to call threadSpecificKeyDelete()?

Comment on attachment 156016 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=156016&action=review >> Source/WTF/wtf/ThreadSpecificWin.cpp:128 >> + key = key->next(); > > Do we expect the destructor to delete the PlatformThreadSpecificKey by calling threadSpecificKeyDelete()? If so, we need to fetch next() before calling the destructor, to avoid dereferencing deleted memory. If not, who do we expect to call threadSpecificKeyDelete()? PlatformThreadSpecificKey will be created via threadSpecificKeyCreate and deleted via threadSpecificKeyDelete. The caller of threadSpecificKeyCreate is responsible to free it via threadSpecificKeyDelete. This is the same behavior as with pthread_key_create and pthread_key_delete. callDestructor() will call the the destructor function provided by threadSpecificKeyCreate, which is stored in m_destructor. The responsibility for freeing objects should be the same as in the pthread implementation. The only difference is, that the thread specific value won't be set to NULL and there is no "PTHREAD_DESTRUCTOR_ITERATIONS". To be honest: I'm not 100% happy with this API, but I want to get rid of the (silently introduced) pthread dependency. I'm open to change it follow up patches (if there are any good ideas), but I (and other) want this landed, because it's a blocker since 2011(!). The main idea behind this patch is, that it only adds an abstraction to the existing pthread functions.

> The main idea behind this patch is, that it only adds an abstraction to the existing pthread functions. I don't think you understood my comment. I'm not contesting the goal of this patch. I'm saying the implementation seems to have a "use after free" bug in it.

(In reply to comment #33) > > The main idea behind this patch is, that it only adds an abstraction to the existing pthread functions. > > I don't think you understood my comment. I'm not contesting the goal of this patch. I'm saying the implementation seems to have a "use after free" bug in it. key->callDestructor() does not free key. key stays valid until threadSpecificKeyDelete() gets called.

> key->callDestructor() does not free key. key stays valid until threadSpecificKeyDelete() gets called. What happens if a destructor calls threadSpecificKeyDelete()? From the pthread_key_delete man page: "The pthread_key_delete() function is callable from within destructor functions."

Created attachment 156527 [details] Patch

Comment on attachment 156527 [details] Patch r=me

Comment on attachment 156527 [details] Patch Clearing flags on attachment: 156527 Committed r124823: <http://trac.webkit.org/changeset/124823>

All reviewed patches have been landed. Closing bug.

Thank you Geoffrey and Patrick!