Summary: | Extract ThreadFunctionInvocation into separate file and share between Apple Windows and Android | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Daniel Bates <dbates> | ||||||||||
Component: | Web Template Framework | Assignee: | Daniel Bates <dbates> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | aroben, benm, buildbot, eric | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | PC | ||||||||||||
OS: | Windows XP | ||||||||||||
Attachments: |
|
Description
Daniel Bates
2011-01-03 17:20:45 PST
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> |