This patch is intended to address the following situation: 1. It's not a good idea to build shells with --disable-threadsafe since it's not a very good approximation of actual Firefox behavior. 2. Building with --enable-threadsafe is pretty painful since it requires NSPR. I've never been able to build a 32-bit version of NSPR, and we very often need 32-bit shells for debugging fuzzbugs. I'm guessing that building for ARM is also difficult. 3. On most platforms that developers actually use, NSPR is (or should be) a very minimal wrapper over pthreads. This patch provides a replacement for NSPR on POSIX platforms. You can enable this code by compiling with --enable-posix-nspr-emulation. If you do this, and you're running on a POSIX platform, you won't need NSPR anymore. My expectation is that JS engine developers would use this feature when building the shell. I don't think you would ever want to build a browser this way. I've only tested with gcc and Linux so far, but I don't think there should be problems getting it running on MacOS and clang.

I've been able to build/install a 32 bit nspr using the instructions at https://developer.mozilla.org/en-US/docs/NSPR_build_instructions. The only problem I've had here when working with shells is that if I build/install a 64 bit nspr as well the two will overwrite each other.

Thanks Bill! In the best case, I was thinking, if you built with --enable-threadsafe (or, now that it's the default, failed to disable it) and you *didn't* specify --with-system-nspr or --with-nspr-* AND you were on a Unix, that --enable-posix-nspr-emulation would be on by default. That way, './configure' on Unices would just work.

Comment on attachment 822483 [details] [diff] [review] nspr-emulation Review of attachment 822483 [details] [diff] [review]: ----------------------------------------------------------------- Bill++; I wish I had thought of this. r=me, fwiw. ::: js/src/vm/PosixNSPR.cpp @@ +104,5 @@ > + > +void > +PR_SetCurrentThreadName(const char *name) > +{ > + /* Ignored. */ I looked this up before and it would be handy information to have in gdb now that we have umpteen threads: pthread_setname_np( #ifdef XP_UNIX gSelfThread, #endif name);

awesome

Comment on attachment 822483 [details] [diff] [review] nspr-emulation Review of attachment 822483 [details] [diff] [review]: ----------------------------------------------------------------- Good grief, why are we still carrying NSPR around? Thanks again. r=me with some stupid comments. ::: js/src/configure.in @@ +4098,5 @@ > + JS_POSIX_NSPR=1, > + JS_POSIX_NSPR= ) > +if test -n "$JS_POSIX_NSPR"; then > + AC_DEFINE(JS_POSIX_NSPR) > +fi Please make it so you can't configure with both --enable-posix-nspr-emulation and any of the other nspr-related configure flags. ::: js/src/vm/PosixNSPR.cpp @@ +180,5 @@ > + if (!lock) > + return nullptr; > + > + if (pthread_mutex_init(&lock->mutex(), nullptr)) > + return nullptr; `js_delete(lock);` in the error path. @@ +223,5 @@ > + if (!cvar) > + return nullptr; > + > + if (pthread_cond_init(&cvar->cond(), nullptr)) > + return nullptr; Same here. @@ +254,5 @@ > + > +uint32_t > +PR_MillisecondsToInterval(uint32_t milli) > +{ > + MOZ_ASSUME_UNREACHABLE("PR_MillisecondsToInterval unimplemented"); return milli; @@ +264,5 @@ > + > +uint32_t > +PR_TicksPerSecond() > +{ > + return 1000; return TicksPerSecond; ::: js/src/vm/PosixNSPR.h @@ +70,5 @@ > +PRThread * > +PR_GetCurrentThread(); > + > +void > +PR_SetCurrentThreadName(const char *name); The NSPR function returns PRStatus. I can't imagine it would ever matter, but might as well match it. @@ +109,5 @@ > +void > +PR_Lock(PRLock *lock); > + > +void > +PR_Unlock(PRLock *lock); Also returns PRStatus.

Build peer review is probably appropriate here.

This contains the main changes. Carrying over Jason's r+.

This patch does as Luke suggested in comment 2. If you don't specify any NSPR options, and you're on Linux, it enables --enable-posix-nspr-emulation by default. Hopefully someone can test this on Mac soon and we can add that to the list of platforms where it's on by default.

\o/

Comment on attachment 824231 [details] [diff] [review] nspr-emulation v2 Review of attachment 824231 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/PosixNSPR.cpp @@ +71,5 @@ > + if (pthread_create(&t->pthread(), &attr, &nspr::Thread::ThreadRoutine, t)) > + return nullptr; > + > + if (state == PR_UNJOINABLE_THREAD) { > + if (pthread_detach(t->pthread())) Do you need to js_delete(t) if pthread_create() or pthread_detach() fail? @@ +75,5 @@ > + if (pthread_detach(t->pthread())) > + return nullptr; > + } > + > + pthread_attr_destroy(&attr); pthread_attr_destroy() is typically an empty implementation, but you still might want to call pthread_attr_destroy() on PR_CreateThread()'s error return paths. @@ +117,5 @@ > +} > + > +static const size_t MaxTLSKeyCount = 32; > +size_t gTLSKeyCount; > +pthread_key_t gTLSKeys[MaxTLSKeyCount]; Should gTLSKeyCount and gTLSKeys be static? Otherwise they will be initialized to garbage values instead of zeros. gTLSKeyCount is used below without other initialization.

> Do you need to js_delete(t) if pthread_create() or pthread_detach() fail? I was pretty lazy with all the failure cases in this patch, but I can fix. > Should gTLSKeyCount and gTLSKeys be static? Otherwise they will be initialized to garbage > values instead of zeros. gTLSKeyCount is used below without other initialization. You're right that they should be static. However, all globals are initialized in C++ regardless of whether they're static.

Comment on attachment 824235 [details] [diff] [review] nspr-config-changes Review of attachment 824235 [details] [diff] [review]: ----------------------------------------------------------------- Is there an existing check that js-threadsafe has a NSPR present? (I didn't see one.) ::: js/src/configure.in @@ +2929,5 @@ > NSPR_LIBS=$withval) > AC_SUBST(NSPR_CFLAGS) > AC_SUBST(NSPR_LIBS) > > +if test "$_USE_SYSTEM_NSPR" || (test "$NSPR_CFLAGS" -o "$NSPR_LIBS"); then There's probably a better way to express this, but I don't remember shell syntax well enough.

I updated the build documentation[1] and added a short note to the 31 release notes[2]. [1]: https://developer.mozilla.org/en-US/docs/SpiderMonkey/Build_Documentation [2]: https://developer.mozilla.org/en-US/docs/SpiderMonkey/31