Bug 39893 – Explicitly use PTHREAD_MUTEX_NORMAL to create pthread mutex

• Component: Web Template Framework
• Status: RESOLVED FIXED

[Yong Li]

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.

[Yong Li]

Created attachment 57359 [details]
the patch

[Eric Seidel (no email)]

Attachment 57359 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/2597047

[Yong Li]

(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

[Alexey Proskuryakov]

The assertion checks that the mutex is not used recursively, it doesn't check how it's implemented.

[Yong Li]

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

[Alexey Proskuryakov]

Oh sorry, you're of course right.

[Yong Li]

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

[Sam Weinig]

Comment on attachment 57359 [details]
the patch

r- as this breaks the mac build.

[Sam Weinig]

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.

[Yong Li]

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

[Yong Li]

Created attachment 57417 [details]
Updated

[Darin Adler]

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

[Yong Li]

(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

[Yong Li]

Created attachment 57462 [details]
use memset

[Darin Adler]

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

[Darin Adler]

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.

[Yong Li]

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

[Yong Li]

Created attachment 57492 [details]
updated

[Darin Adler]

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

[Yong Li]

(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

[WebKit Commit Bot]

Comment on attachment 57492 [details]
updated

Clearing flags on attachment: 57492

Committed r60487: <http://trac.webkit.org/changeset/60487>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Xan Lopez]

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

[Darin Adler]

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

[Yong Li]

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