The attached logfile shows a thread/data race (mozilla-central revision 626d99c084cb) detected by TSan (ThreadSanitizer). Typically, races reported by TSan are not false positives, but it is possible that the race is benign. Even in this case though, we should try to come up with a fix unless this would cause inacceptable performance issues. Also note that seemingly benign races can possibly be harmful (also depending on the compiler and the architecture) [1]. If the bug cannot be fixed, then this bug should be used to either make a compile-time annotation for blacklisting or add an entry to the runtime blacklist. [1] http://software.intel.com/en-us/blogs/2013/01/06/benign-data-races-what-could-possibly-go-wrong

The problematic code is the read of _pr_tpd_highwater on line 146: 137 PR_IMPLEMENT(PRStatus) PR_SetThreadPrivate(PRUintn index, void *priv) 138 { 139 PRThread *self = PR_GetCurrentThread(); 140 141 /* 142 ** The index being set might not have a sufficient vector in this 143 ** thread. But if the index has been allocated, it's okay to go 144 ** ahead and extend this one now. 145 */ 146 if ((index >= _PR_TPD_LIMIT) || (index >= _pr_tpd_highwater)) 147 { 148 PR_SetError(PR_TPD_RANGE_ERROR, 0); 149 return PR_FAILURE; 150 } _pr_tpd_highwater is maintained by PR_ATOMIC_INCREMENT. It should not be read directly. I think we should remove the index >= _pr_tpd_highwater check on line 146. The comment on lines 142-144 can be deleted.

Christian, could you review and test this patch? Thanks. The fix is to only access the _pr_tpd_highwater variable using NSPR atomic functions. This means we can't do a check like index >= _pr_tpd_highwater POSIX Threads also allows skipping this test to improve performance: http://pubs.opengroup.org/onlinepubs/007904975/functions/pthread_getspecific.html The equivalent error for pthread_setspecific is EINVAL, "The key value is invalid.". Note that the man page uses "may" as opposed to "shall" for that error. Also the Rationale section says: Performance and ease-of-use of pthread_getspecific() are critical for functions that rely on maintaining state in thread-specific data. Since no errors are required to be detected by it, and since the only error that could be detected is the use of an invalid key, the function to pthread_getspecific() has been designed to favor speed and simplicity over error reporting.

Comment on attachment 8395305 [details] [diff] [review] Only access _pr_tpd_highwater using atomic functions Review of attachment 8395305 [details] [diff] [review]: ----------------------------------------------------------------- I'm not sure if I can review this and I'd prefer to have it reviewed by someone who is actually familiar with NSPR code. Joshua, can you review this, since you reviewed the last TSan NSPR patch? Thanks.

Comment on attachment 8395305 [details] [diff] [review] Only access _pr_tpd_highwater using atomic functions Review of attachment 8395305 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for the delay in reviewing this patch, but "thread-safety" and "NSPR" in my mind equate to "scary patch, takes forever to review" and hence kept getting put off until I could find the time to review it. I also had to take the time to convince myself that the deletion of the check was safe, in that while its behavior may not notionally be well-defined, its actual runtime behavior should still avoid causing problems in the first place.

Comment on attachment 8395305 [details] [diff] [review] Only access _pr_tpd_highwater using atomic functions Joshua: thanks for the careful review! Patch checked in: https://hg.mozilla.org/projects/nspr/rev/3fa631505238

I updated NSPR on inbound to take this fix: https://hg.mozilla.org/integration/mozilla-inbound/rev/51895ae51261