Thread-unsafe access to lock->owner in PR_Lock

Hi NSPR developers,

while running Chromium tests under ThreadSanitizer (https://code.google.com/p/data-race-test/) we've encountered a report about a data race between PR_Lock and PR_Unlock: http://crbug.com/328521

It turns out that the lock->owner variable is being read by PR_Lock at the same time other threads read or write it in PR_Lock or PR_Unlock:

189 PR_IMPLEMENT(void) PR_Lock(PRLock *lock)
190 {
191     PRThread *me = _PR_MD_CURRENT_THREAD();
192     PRIntn is;
193     PRThread *t;
194     PRCList *q;
195 
196     PR_ASSERT(me != suspendAllThread);
197     PR_ASSERT(!(me->flags & _PR_IDLE_THREAD));
198     PR_ASSERT(lock != NULL);
199 #ifdef _PR_GLOBAL_THREADS_ONLY 
200     PR_ASSERT(lock->owner != me);
201     _PR_MD_LOCK(&lock->ilock);
202     lock->owner = me;
203     return;
204 #else  /* _PR_GLOBAL_THREADS_ONLY */

(code from nspr-4.9.5/mozilla/nsprpub/pr/src/threads/combined/prulock.c in Ubuntu Precise)

In the case the optimizer is conservative enough nothing will break (during the race lock->owner will be either another thread or NULL, but not |me|), but this is undefined behavior in C++11 and can possibly break in newer compilers.

Is there a reason to do this check before the lock is taken? I'm guessing it's there to avoid reentrancy, so it should be fine to swap lines 200 and 201.

WBR, 
Alexander Potapenko
Software Engineer
Google Moscow