Bug 51855 – Extract ThreadFunctionInvocation into separate file and share between Apple Windows and Android

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Daniel Bates]

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.

[Daniel Bates]

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.

[Daniel Bates]

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.

[Daniel Bates]

Created attachment 77861 [details]
Patch

Substitute typedef for ThreadFunction for #include "Threading.h" in ThreadFunctionInvocation.h

[Adam Roben (:aroben)]

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.

[Daniel Bates]

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

[Adam Roben (:aroben)]

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.

[Daniel Bates]

Created attachment 77900 [details]
Patch

Updated patch based on Adam Roben's suggestions.

[Adam Roben (:aroben)]

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

[Daniel Bates]

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

[Build Bot]

Attachment 77900 [details] did not build on win:
Build output: http://queues.webkit.org/results/7346026

[Daniel Bates]

Committed r74975: <http://trac.webkit.org/changeset/74975>