Bug 39893

Summary: Explicitly use PTHREAD_MUTEX_NORMAL to create pthread mutex
Product: WebKit Reporter: Yong Li <yong.li.webkit>
Component: Web Template FrameworkAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, commit-queue, darin, eric, sam, staikos, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: Other   
Attachments:
Description Flags
the patch
sam: review-
Updated
darin: review-
use memset
darin: review-
updated none

Description Yong Li 2010-05-28 11:59:24 PDT
There're some places in webkit that assumes mutex cannot be recursive like this:

ASSERT(!mutex.tryLock());

However, PTHREAD_MUTEX_DEFAULT can be PTHREAD_MUTEX_RECURSIVE depending the implementation.

Let's explicitly make sure we're creating non-recursive mutex.
Comment 1 Yong Li 2010-05-28 12:05:21 PDT
Created attachment 57359 [details]
the patch
Comment 2 Eric Seidel (no email) 2010-05-28 12:12:30 PDT
Attachment 57359 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/2597047
Comment 3 Yong Li 2010-05-28 12:18:19 PDT
(In reply to comment #2)
> Attachment 57359 [details] did not build on mac:
> Build output: http://webkit-commit-queue.appspot.com/results/2597047

weird. struct T t = { 0 }; is a typical way to zero the struct
Comment 4 Alexey Proskuryakov 2010-05-28 14:32:55 PDT
The assertion checks that the mutex is not used recursively, it doesn't check how it's implemented.
Comment 5 Yong Li 2010-05-28 14:50:17 PDT
(In reply to comment #4)
> The assertion checks that the mutex is not used recursively, it doesn't check how it's implemented.

If the mutex is recursive (PTHREAD_MUTEX_RECURSIVE), the assertion will fail.
Comment 6 Alexey Proskuryakov 2010-05-28 14:53:55 PDT
Oh sorry, you're of course right.
Comment 7 Yong Li 2010-05-28 15:33:41 PDT
The other solution is:

1) add a flag in Mutex:

#ifndef NDEBUG
bool isLocked() const { return m_isLocked; }
bool m_isLocked;
#endif

2) set it in lock() and unlock()

3) change assertions to ASSERT(mutex.isLocked())
Comment 8 Sam Weinig 2010-05-29 10:06:55 PDT
Comment on attachment 57359 [details]
the patch

r- as this breaks the mac build.
Comment 9 Sam Weinig 2010-05-29 10:15:49 PDT
Can we do this as a compile time check.

#if PTHREAD_MUTEX_DEFAULT == PTHREAD_MUTEX_NORMAL
...old code
#else
...your code
#endif

I believe this is breaking the mac since the darwin implementation of pthread_t has a nested struct or array so you would have to initialize it with something like pt = {0, {0}}; I don't think depending on the implementations representation of pthread_t is a good idea though.
Comment 10 Yong Li 2010-05-29 17:23:29 PDT
(In reply to comment #9)
> Can we do this as a compile time check.
> 
> #if PTHREAD_MUTEX_DEFAULT == PTHREAD_MUTEX_NORMAL
> ...old code
> #else
> ...your code
> #endif

I thought about this, but it also assumes PTHREAD_MUTEX_NORMAL must be a constant. If PTHREAD_MUTEX_NORMAL is implemented by a function, this cannot build. Probably we can ignore this case?

> 
> I believe this is breaking the mac since the darwin implementation of pthread_t has a nested struct or array so you would have to initialize it with something like pt = {0, {0}}; I don't think depending on the implementations representation of pthread_t is a good idea though.

I can remove the initializer, because static variables are always zeroed.
Comment 11 Yong Li 2010-05-29 17:34:04 PDT
Created attachment 57417 [details]
Updated
Comment 12 Darin Adler 2010-05-29 21:01:14 PDT
Comment on attachment 57417 [details]
Updated

Patch looks generally good, but I have some concerns.

> +    // Make sure it's not the same value as PTHREAD_MUTEX_INITIALIZER.

This comment makes no sense. Why is it important to make sure that something is not the same value as PTHREAD_MUTEX_INITIALIZER? How is that relevant at all?

> +    // Static variables are always zeroed.
> +    static pthread_mutex_t s_zeroedMutex;
> +    m_mutex = s_zeroedMutex;

It seems really strange to do it that way. If you just want to zero something out, the best way I can think of is:

    memset(&m_mutex, 0, sizeof(m_mutex));

No need for a global variable.

But why does m_mutex need to be initialized twice? Doesn't pthread_mutex_init do a complete job? Why the need to initialize it before calling that?

review- because I don't think we should land this with the extra unneeded code and the mysterious comment
Comment 13 Yong Li 2010-05-31 07:14:00 PDT
(In reply to comment #12)
> (From update of attachment 57417 [details])
> Patch looks generally good, but I have some concerns.
> > +    // Make sure it's not the same value as PTHREAD_MUTEX_INITIALIZER.
> This comment makes no sense. Why is it important to make sure that something is not the same value as PTHREAD_MUTEX_INITIALIZER? How is that relevant at all?

pthread_mutex_init and pthread_mutex_destroy are not supposed to be called on PTHREAD_MUTEX_INITIALIZER. I cannot find any scription that defines the behavior. So I assume the behavior is undefined, and up to implementation.

> > +    // Static variables are always zeroed.
> > +    static pthread_mutex_t s_zeroedMutex;
> > +    m_mutex = s_zeroedMutex;
> It seems really strange to do it that way. If you just want to zero something out, the best way I can think of is:
>     memset(&m_mutex, 0, sizeof(m_mutex));
> No need for a global variable.

I can change it to memset
Comment 14 Yong Li 2010-05-31 07:36:15 PDT
Created attachment 57462 [details]
use memset
Comment 15 Darin Adler 2010-05-31 09:56:49 PDT
(In reply to comment #13)
> (In reply to comment #12)
> > > +    // Make sure it's not the same value as PTHREAD_MUTEX_INITIALIZER.
> > This comment makes no sense. Why is it important to make sure that something is not the same value as PTHREAD_MUTEX_INITIALIZER? How is that relevant at all?
> 
> pthread_mutex_init and pthread_mutex_destroy are not supposed to be called on PTHREAD_MUTEX_INITIALIZER. I cannot find any scription that defines the behavior. So I assume the behavior is undefined, and up to implementation.

I think you're misunderstanding this. I read over the various documentation, and yes, it's true that you should not use both PTHREAD_MUTEXT_INITIALIZER and pthread_mutex_init on the same mutex, but that does not mean that you have to pre-initialize the mutex before calling pthread_mutex_init. It just means that it's a programming mistake to both use PTHREAD_MUTEXT_INITIALIZER and pthread_mutex_init.
Comment 16 Darin Adler 2010-05-31 10:01:25 PDT
Comment on attachment 57462 [details]
use memset

> +    // Make sure it's not the same value as PTHREAD_MUTEX_INITIALIZER.
> +    memset(&m_mutex, 0, sizeof(pthread_mutex_t));

If we are doing this memset, I think it's better style to call sizeof(m_mutex) here instead of sizeof(pthread_mutex_t), because that way you can see looking at this line of code that it's correct without having to look up the type of m_mutex to double check that it's a pthread_mutex_t.

As I said in my previous comment in this bug, this comment and code are not needed, and I am almost certain the comment reflects a misunderstanding on your part. I don't want to confuse future programmers working on this code.

Initializing to zero does have a small good effect. It gives us more predictable behavior if pthread_mutex_init fails. Both the old code path and the new code path have the same failing in that they don't check the error return of pthread_mutex_init. But this issue is not specific to this new code path and applies just as much to the PTHREAD_MUTEX_NORMAL == PTHREAD_MUTEX_DEFAULT case.

Despite the fact that this patch is otherwise just fine, I'm going to say review- because I don't want to check in a mysterious comment or unneeded code.

The only difference between the two branches of the #if should be the attributes passed in to pthread_mutex_init.
Comment 17 Yong Li 2010-05-31 12:59:33 PDT
(In reply to comment #15)
> (In reply to comment #13)
> > (In reply to comment #12)
> > > > +    // Make sure it's not the same value as PTHREAD_MUTEX_INITIALIZER.
> > > This comment makes no sense. Why is it important to make sure that something is not the same value as PTHREAD_MUTEX_INITIALIZER? How is that relevant at all?
> > 
> > pthread_mutex_init and pthread_mutex_destroy are not supposed to be called on PTHREAD_MUTEX_INITIALIZER. I cannot find any scription that defines the behavior. So I assume the behavior is undefined, and up to implementation.
> 
> I think you're misunderstanding this. I read over the various documentation, and yes, it's true that you should not use both PTHREAD_MUTEXT_INITIALIZER and pthread_mutex_init on the same mutex, but that does not mean that you have to pre-initialize the mutex before calling pthread_mutex_init. It just means that it's a programming mistake to both use PTHREAD_MUTEXT_INITIALIZER and pthread_mutex_init.

If we don't initialize the mutex, what if the mutex just happens to be the same value as PTHREAD_MUTEXT_INITIALIZER? Although the chance if very small.

OK, I'll remove that code anyway.
Comment 18 Yong Li 2010-05-31 13:03:17 PDT
Created attachment 57492 [details]
updated
Comment 19 Darin Adler 2010-05-31 18:18:30 PDT
(In reply to comment #17)
> If we don't initialize the mutex, what if the mutex just happens to be the same value as PTHREAD_MUTEXT_INITIALIZER? Although the chance if very small.

Not important to this patch any more, but that question shows a misunderstanding. There's no way you could set the mutex to a value and guarantee it's not the same as PTHREAD_MUTEX_INITIALIZER, nor would you want to. I think you're just reading too much into some documentation you read. pthread_mutex_init does not require that before you call it you set the mutex up with a bit pattern that happens to not be equal to PTHREAD_MUTEX_INITIALIZER. And if it did require that I'm not sure how you would accomplish that portably anyway. There are two ways to initialize a pthreads mutex. One is to call pthread_mutex_init and the other is to use PTHREAD_MUTEX_INITIALIZER. In our case we are using pthread_mutex_init.
Comment 20 Yong Li 2010-06-01 08:56:59 PDT
(In reply to comment #19)
> (In reply to comment #17)
> > If we don't initialize the mutex, what if the mutex just happens to be the same value as PTHREAD_MUTEXT_INITIALIZER? Although the chance if very small.
> 
> Not important to this patch any more, but that question shows a misunderstanding. There's no way you could set the mutex to a value and guarantee it's not the same as PTHREAD_MUTEX_INITIALIZER, nor would you want to. I think you're just reading too much into some documentation you read. pthread_mutex_init does not require that before you call it you set the mutex up with a bit pattern that happens to not be equal to PTHREAD_MUTEX_INITIALIZER. And if it did require that I'm not sure how you would accomplish that portably anyway. There are two ways to initialize a pthreads mutex. One is to call pthread_mutex_init and the other is to use PTHREAD_MUTEX_INITIALIZER. In our case we are using pthread_mutex_init.

yeah. probably I have thought too much
Comment 21 WebKit Commit Bot 2010-06-01 10:46:53 PDT
Comment on attachment 57492 [details]
updated

Clearing flags on attachment: 57492

Committed r60487: <http://trac.webkit.org/changeset/60487>
Comment 22 WebKit Commit Bot 2010-06-01 10:47:03 PDT
All reviewed patches have been landed.  Closing bug.
Comment 23 Xan Lopez 2010-06-01 11:36:13 PDT
This is not quite working on GTK+:


../../JavaScriptCore/wtf/ThreadingPthreads.cpp:238:5: warning: "PTHREAD_MUTEX_NORMAL" is not defined
../../JavaScriptCore/wtf/ThreadingPthreads.cpp:238:29: warning: "PTHREAD_MUTEX_DEFAULT" is not defined
Comment 24 Darin Adler 2010-06-01 11:38:59 PDT
(In reply to comment #23)
> This is not quite working on GTK+:
>
> ../../JavaScriptCore/wtf/ThreadingPthreads.cpp:238:5: warning: "PTHREAD_MUTEX_NORMAL" is not defined
> ../../JavaScriptCore/wtf/ThreadingPthreads.cpp:238:29: warning: "PTHREAD_MUTEX_DEFAULT" is not defined

I suggest doing this for now to fix the build:

    #if !defined PTHREAD_MUTEX_NORMAL || PTHREAD_MUTEX_NORMAL == PTHREAD_MUTEX_DEFAULT

Later we can figure it out.
Comment 25 Yong Li 2010-06-01 12:53:44 PDT
(In reply to comment #23)
> This is not quite working on GTK+:
> ../../JavaScriptCore/wtf/ThreadingPthreads.cpp:238:5: warning: "PTHREAD_MUTEX_NORMAL" is not defined
> ../../JavaScriptCore/wtf/ThreadingPthreads.cpp:238:29: warning: "PTHREAD_MUTEX_DEFAULT" is not defined

Oops. sorry for this. but PTHREAD_MUTEX_NORMAL is supposed to be defined in pthread.h.

Are you fixing it as Darin sugguests now?