Both the Apple Windows and Android ports implement a similar adapter structure, called ThreadFunctionInvocation and ThreadData respectively, as part of their thread creation process. We should share such an adapter structure and remove duplicate code.
Created attachment 77855 [details] Patch This patch also modifies createThreadInternal() in wtf/ThreadingWin.cpp so that we deallocate the instantiated ThreadFunctionInvocation if the call to CreateThread() fails.
Created attachment 77860 [details] Patch I thought it would be better to extract the fix for the memory leak into a separate bug, bug #51860.
Created attachment 77861 [details] Patch Substitute typedef for ThreadFunction for #include "Threading.h" in ThreadFunctionInvocation.h
Comment on attachment 77861 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=77861&action=review > Source/JavaScriptCore/wtf/ThreadingPthreads.cpp:-155 > - ThreadData* data = static_cast<ThreadData*>(arg); > + ThreadFunctionInvocation invocation = *static_cast<ThreadFunctionInvocation*>(arg); > + delete static_cast<ThreadFunctionInvocation*>(arg); > + > JavaVM* vm = JSC::Bindings::getJavaVM(); > JNIEnv* env; > void* ret = 0; > if (vm->AttachCurrentThread(&env, 0) == JNI_OK) { > - ret = data->entryPoint(data->arg); > + ret = invocation.function(invocation.data); > vm->DetachCurrentThread(); > } > - delete data; This is a little bizarre. How about just adopting arg into an OwnPtr<ThreadFunctionInvocation> and using that to invoke the function? > Source/JavaScriptCore/wtf/ThreadingPthreads.cpp:162 > + ThreadFunctionInvocation* invocation = new ThreadFunctionInvocation(entryPoint, data); // Deallocated in runThreadWithRegistration() if a thread is created. > + > + if (pthread_create(&threadHandle, 0, runThreadWithRegistration, invocation)) { It would be nicer to use adoptPtr/leakPtr here.
(In reply to comment #4) > (From update of attachment 77861 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=77861&action=review > > > Source/JavaScriptCore/wtf/ThreadingPthreads.cpp:-155 > > - ThreadData* data = static_cast<ThreadData*>(arg); > > + ThreadFunctionInvocation invocation = *static_cast<ThreadFunctionInvocation*>(arg); > > + delete static_cast<ThreadFunctionInvocation*>(arg); > > + > > JavaVM* vm = JSC::Bindings::getJavaVM(); > > JNIEnv* env; > > void* ret = 0; > > if (vm->AttachCurrentThread(&env, 0) == JNI_OK) { > > - ret = data->entryPoint(data->arg); > > + ret = invocation.function(invocation.data); > > vm->DetachCurrentThread(); > > } > > - delete data; > > This is a little bizarre. How about just adopting arg into an OwnPtr<ThreadFunctionInvocation> and using that to invoke the function? Will change. > > > Source/JavaScriptCore/wtf/ThreadingPthreads.cpp:162 > > + ThreadFunctionInvocation* invocation = new ThreadFunctionInvocation(entryPoint, data); // Deallocated in runThreadWithRegistration() if a thread is created. > > + > > + if (pthread_create(&threadHandle, 0, runThreadWithRegistration, invocation)) { > > It would be nicer to use adoptPtr/leakPtr here. Is it safe to use adoptPtr/leakPtr for <https://bugs.webkit.org/show_bug.cgi?id=51855>. From my understanding, pthread_create() does not take ownership of the parameter arg. Assume we use adoptPtr/leakPtr and suppose pthread_create() returns an error (say EAGAIN - system lacked the necessary resources to create another thread). Then we will not delete the instance of ThreadFunctionInvocation because OwnPtr::leakPtr() sets OwnPtr::m_ptr to 0 and hence ~OwnPtr() will call delete(0).
Comment on attachment 77861 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=77861&action=review >>> Source/JavaScriptCore/wtf/ThreadingPthreads.cpp:162 >>> + if (pthread_create(&threadHandle, 0, runThreadWithRegistration, invocation)) { >> >> It would be nicer to use adoptPtr/leakPtr here. > > Is it safe to use adoptPtr/leakPtr for <https://bugs.webkit.org/show_bug.cgi?id=51855>. From my understanding, pthread_create() does not take ownership of the parameter arg. Assume we use adoptPtr/leakPtr and suppose pthread_create() returns an error (say EAGAIN - system lacked the necessary resources to create another thread). Then we will not delete the instance of ThreadFunctionInvocation because OwnPtr::leakPtr() sets OwnPtr::m_ptr to 0 and hence ~OwnPtr() will call delete(0). You'd have to do something like: OwnPtr<> invocation = adoptPtr(…); if (pthread_create(…, invocation.get()) return 0; // The thread will take ownership of invocation. invocation.leakPtr(); Maybe that's not much better than what you have now.
Created attachment 77900 [details] Patch Updated patch based on Adam Roben's suggestions.
Comment on attachment 77900 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=77900&action=review > Source/JavaScriptCore/wtf/ThreadingPthreads.cpp:161 > + if (pthread_create(&threadHandle, 0, runThreadWithRegistration, invocation)) { This shouldn't compile. If it does, we should find out why. You should be passing invocation.get().
(In reply to comment #8) > (From update of attachment 77900 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=77900&action=review > > > Source/JavaScriptCore/wtf/ThreadingPthreads.cpp:161 > > + if (pthread_create(&threadHandle, 0, runThreadWithRegistration, invocation)) { > > This shouldn't compile. If it does, we should find out why. You should be passing invocation.get(). Right. Will fix before I land.
Attachment 77900 [details] did not build on win: Build output: http://queues.webkit.org/results/7346026
Committed r74975: <http://trac.webkit.org/changeset/74975>