This patch adds a PosixNSPR.cpp implemented using native win32 APIs. This should really be called MiniNSPR or something, but I didn't want to rename all the files and configure variables for this. I can do that if whoever reviews this would prefer -- MiniNSPR.h, PosixMiniNSPR.cpp, WinMiniNSPR.cpp. The only issue is that on Windows, native CONDITION_VARIABLE support is available only on Vista and above. This would make non-nspr standalone JS builds viable on Vista and above only, which doesn't seem like a major issue. It would be interesting to do some perf comparisons between a standalone Windows JS with this and with full NSPR.

Comment on attachment 8705327 [details] [diff] [review] Windows mini NSPR impl Interesting! This made me think of bug 956899, but I guess the scope is smaller. When calling CreateThread, you should pass the STACK_SIZE_PARAM_IS_A_RESERVATION flag, otherwise a non-zero stackSize indicates the *committed* size. NSPR uses _beginthreadex instead of CreateThread [1], but I always forget what the difference between those two is. [1] https://dxr.mozilla.org/mozilla-central/source/nsprpub/pr/src/md/windows/ntthread.c#180

Comment on attachment 8705327 [details] [diff] [review] Windows mini NSPR impl Review of attachment 8705327 [details] [diff] [review]: ----------------------------------------------------------------- Review comments are mostly style nits (please fix those throughout the file before re-submitting), but there are a couple larger problems as well. First, the TLS implementation is fine enough, but I'd much rather just change our existing usage of PR_GetThreadPrivate/PR_SetThreadPrivate to mfbt's ThreadLocal<T>. In fact, I'm not seeing any usage of these TLS functions in SpiderMonkey in dxr. Can we just remove the TLS function defs from PosixNSPR? Secondly, this desperately needs tests. See the existing tests in js/src/jsapi-tests/. ::: mozjs/js/src/vm/WinNSPR.cpp @@ +62,5 @@ > +{ > + HANDLE thread; > + DWORD threadId; > + void (*start)(void *arg); > + void *arg; * associates to the left here and above. @@ +64,5 @@ > + DWORD threadId; > + void (*start)(void *arg); > + void *arg; > + bool joinable; > + Remove the whitespace on this empty line. @@ +65,5 @@ > + void (*start)(void *arg); > + void *arg; > + bool joinable; > + > + Thread(void (*start)(void *arg), void *arg, bool joinable) * to the left. @@ +66,5 @@ > + void *arg; > + bool joinable; > + > + Thread(void (*start)(void *arg), void *arg, bool joinable) > + : thread(0), threadId(0), start(start), arg(arg), joinable(joinable) {} {} on next line. @@ +71,5 @@ > + > + ~Thread() { > + if (thread) { > + CloseHandle(thread); > + } No {} around a single statement. @@ +92,5 @@ > + Thread *self = static_cast<Thread *>(arg); > + TlsSetValue(gSelfThreadIndex, self); > + self->start(self->arg); > + > + // call Tls destructors Capitalize Call and put a period at the end. @@ +97,5 @@ > + for (size_t i = 0; i < gTLSKeyCount; ++i) { > + void *tlsVal = TlsGetValue(gTLSKeys[i]); > + if (tlsVal && gTLSDestructors[i]) { > + gTLSDestructors[i](tlsVal); > + } Remove {}. @@ +102,5 @@ > + } > + > + if (!self->joinable) { > + js_delete(self); > + } Remove {}. @@ +123,5 @@ > + > + TlsSetValue(gSelfThreadIndex, &gMainThread); > +} > + > +PRThread * * to left. @@ +139,5 @@ > + if (!gInitialized) { > + /* > + * We assume that the first call to PR_CreateThread happens on the main > + * thread. > + */ // C++ style comments please. @@ +144,5 @@ > + Initialize(); > + } > + > + nspr::Thread *t = js_new<nspr::Thread>(start, arg, > + state != PR_UNJOINABLE_THREAD); * to left. SM line width is 100 chars, so this should fit on one line. Please use mozilla::UniquePtr to automate cleanup in failure paths. @@ +147,5 @@ > + nspr::Thread *t = js_new<nspr::Thread>(start, arg, > + state != PR_UNJOINABLE_THREAD); > + if (!t) { > + return nullptr; > + } And no {} here. @@ +154,5 @@ > + stackSize, > + &nspr::Thread::ThreadRoutine, > + t, > + 0, > + &t->threadId); Don't bother stacking the args unless you're going to add a comment to each line or something. @@ +156,5 @@ > + t, > + 0, > + &t->threadId); > + if (!t->thread) { > + js_delete(t); Let the UniquePtr do the work. @@ +158,5 @@ > + &t->threadId); > + if (!t->thread) { > + js_delete(t); > + return nullptr; > + } Which will allow us to remove the {}. @@ +160,5 @@ > + js_delete(t); > + return nullptr; > + } > + > + return t; return t.release(); @@ +164,5 @@ > + return t; > +} > + > +PRStatus > +PR_JoinThread(PRThread *thread) * to the left. @@ +186,5 @@ > + PRThread *thread = (PRThread *)TlsGetValue(gSelfThreadIndex); > + if (!thread) { > + thread = js_new<nspr::Thread>(nullptr, nullptr, false); > + if (!thread) > + MOZ_CRASH(); Please use AutoEnterOOMUnsafeRegion here. Otherwise you will be getting a pile of OOM fuzz bugs here shortly. @@ +212,5 @@ > + > + DWORD key = TlsAlloc(); > + if (key == TLS_OUT_OF_INDEXES) > + return PR_FAILURE; > + Remove extra whitespace.

(In reply to Terrence Cole [:terrence] from comment #2) > Review comments are mostly style nits (please fix those throughout the file > before re-submitting), but there are a couple larger problems as well. > > First, the TLS implementation is fine enough, but I'd much rather just > change our existing usage of PR_GetThreadPrivate/PR_SetThreadPrivate to > mfbt's ThreadLocal<T>. In fact, I'm not seeing any usage of these TLS > functions in SpiderMonkey in dxr. Can we just remove the TLS function defs > from PosixNSPR? Yup, will do. > Secondly, this desperately needs tests. See the existing tests in > js/src/jsapi-tests/. Shouldn't this be testable using the existing tests? > @@ +186,5 @@ > > + PRThread *thread = (PRThread *)TlsGetValue(gSelfThreadIndex); > > + if (!thread) { > > + thread = js_new<nspr::Thread>(nullptr, nullptr, false); > > + if (!thread) > > + MOZ_CRASH(); > > Please use AutoEnterOOMUnsafeRegion here. Otherwise you will be getting a > pile of OOM fuzz bugs here shortly. Ack ok, the SM that Servo is using doesn't yet have that. I'll prep a patch against SM trunk tip.

(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #3) > (In reply to Terrence Cole [:terrence] from comment #2) > > Secondly, this desperately needs tests. See the existing tests in > > js/src/jsapi-tests/. > > Shouldn't this be testable using the existing tests? Yeah, that's pretty unclear. I made a quick check for existing direct tests of the posix NSPR wrappers and didn't see anything, but maybe I missed it. What I meant so convey was something like: "See js/src/jsapi-tests/ for examples of how to create simple C++ tests for SpiderMonkey and add a few that use these classes directly." > > @@ +186,5 @@ > > > + PRThread *thread = (PRThread *)TlsGetValue(gSelfThreadIndex); > > > + if (!thread) { > > > + thread = js_new<nspr::Thread>(nullptr, nullptr, false); > > > + if (!thread) > > > + MOZ_CRASH(); > > > > Please use AutoEnterOOMUnsafeRegion here. Otherwise you will be getting a > > pile of OOM fuzz bugs here shortly. > > Ack ok, the SM that Servo is using doesn't yet have that. I'll prep a patch > against SM trunk tip. Thanks! We should really do a better of keeping these in sync.

We still need this on Servo and the patch doesn't apply anymore on SM48. Could it be unbitrotten?

Updated, and now with a test.

Comment on attachment 8747951 [details] [diff] [review] Windows mini NSPR impl, v2 Review of attachment 8747951 [details] [diff] [review]: ----------------------------------------------------------------- I'd like to see a version that works on WinXP using js::ConditionVariable. ::: js/src/jsapi-tests/testPosixNSPR.cpp @@ +9,5 @@ > + > +static void > +ThreadFunc(void *arg) > +{ > + void **lockAndCvar = static_cast<void**>(arg); Pointer syntax (*) is type-associative in SpiderMonkey. Please move these **'s and all the others to the left. @@ +17,5 @@ > + // Wait until the main thread is actually sleeping on the cvar > + PR_Lock(lock); > + PR_Unlock(lock); > + > + PR_NotifyCondVar(cvar); Did you forget to qref? Generally, condition variable methods must be called with the owning lock held. And indeed, these are none different; from the documentation: "The calling thread must hold the lock that protects the condition, as well as the invariants that are tightly bound to the condition." @@ +45,5 @@ > + > + // Lock this, so that the thread can wait on it > + PR_Lock(lock); > + > + void *arg[2] = { lock, cv }; Please also pass a bool* that gets set by ThreadFunc to prove that the condvar is actually synchronizing access. @@ +60,5 @@ > + CHECK(PR_Unlock(lock) == PR_SUCCESS); > + > + PR_DestroyCondVar(cv); > + > + CHECK(PR_JoinThread(newThread) == PR_SUCCESS); It wouldn't matter if the CondVar was properly locked above, but because it's not, destroying the condvar while the other thread is still running will result in a UAF. Please join all threads that might possibly touch a resource before destroying those resources: it's just good habit. ::: js/src/vm/Initialization.cpp @@ -149,5 @@ > - // "reset" the called-once status -- doable, but more trouble than it's > - // worth now.) Initializing that subsystem from JS_Init eliminates the > - // problem, but initialization can take a comparatively long time (15ms or > - // so), so we really don't want to do it in JS_Init, and we really do want > - // to do it only when PRMJ_Now is eventually called. I guess this is not true anymore? If so, awesome! ::: js/src/vm/WinNSPR.cpp @@ +10,5 @@ > + * This NSPR implementation requires Windows Vista or higher; the rest of Gecko/SM > + * is built only to require WinXP at compile time (other functions are dynamically > + * loaded). So, for this file only, we bump _WIN32_WINNT to indicate that we want > + * Vista+ functionality (0x0600). For more fun, the location of the synchronization > + * primitives changed -- in the Windows 8 SDK, it moved to <SynchAPI.h>. js::Mutex and js::ConditionVariable (in js/src/threading/) support WinXP. If possible, could we instead use those to implement these? @@ +71,5 @@ > +#endif > + > +class nspr::Thread > +{ > +public: 2 spaces indent here. The diff command is fairly dumb in that it will report as context the first non-indented line it sees above the patched block. Half indentation is ugly, but it makes patches much easier to review by providing real context instead of just "public:". @@ +215,5 @@ > + AcquireSRWLockExclusive(&lock->lock()); > +} > + > +PRStatus > +PR_Unlock(PRLock *lock) Please use type associative *'s everywhere. @@ +294,5 @@ > + if (timeout == PR_INTERVAL_NO_TIMEOUT) { > + msTimeout = INFINITE; > + } else { > + msTimeout = timeout; > + } Remove the braces from this if/else. Or just make it a ternary.

(In reply to Terrence Cole [:terrence] from comment #7) > Comment on attachment 8747951 [details] [diff] [review] > Windows mini NSPR impl, v2 > > Review of attachment 8747951 [details] [diff] [review]: > ----------------------------------------------------------------- > > I'd like to see a version that works on WinXP using js::ConditionVariable. That's.. not a thing that exists? Can that be left as an exercise for the reader? WinXP support exists in the form of NSPR already. > ::: js/src/jsapi-tests/testPosixNSPR.cpp > @@ +9,5 @@ > > + > > +static void > > +ThreadFunc(void *arg) > > +{ > > + void **lockAndCvar = static_cast<void**>(arg); > > Pointer syntax (*) is type-associative in SpiderMonkey. Please move these > **'s and all the others to the left. Will fix this and the other issues below. > @@ +17,5 @@ > > + // Wait until the main thread is actually sleeping on the cvar > > + PR_Lock(lock); > > + PR_Unlock(lock); > > + > > + PR_NotifyCondVar(cvar); > > Did you forget to qref? Ugh. Copypasta that I missed amending. > ::: js/src/vm/Initialization.cpp > @@ -149,5 @@ > > - // "reset" the called-once status -- doable, but more trouble than it's > > - // worth now.) Initializing that subsystem from JS_Init eliminates the > > - // problem, but initialization can take a comparatively long time (15ms or > > - // so), so we really don't want to do it in JS_Init, and we really do want > > - // to do it only when PRMJ_Now is eventually called. > > I guess this is not true anymore? If so, awesome! Well... nothing was actually using PR_CallOnce. That said, the limitation is still there; PRMJ_NowInit initializes static data, including a static critical section, which PRMJ_NowShutdown destroys. But there is no protection around PRMJ_NowInit/NowShutdown being called multiple times.

(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #8) > (In reply to Terrence Cole [:terrence] from comment #7) > > Comment on attachment 8747951 [details] [diff] [review] > > Windows mini NSPR impl, v2 > > > > Review of attachment 8747951 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > I'd like to see a version that works on WinXP using js::ConditionVariable. > > That's.. not a thing that exists? Can that be left as an exercise for the > reader? WinXP support exists in the form of NSPR already. https://dxr.mozilla.org/mozilla-central/source/js/src/threading/ConditionVariable.h The plan is to convert everything in SpiderMonkey over to the new Mutex/ConditionVariable/Thread implementation and drop NSPR completely. I thought you might want WinXP support for free now rather than later, but I really don't have a strong opinion either way. > > ::: js/src/vm/Initialization.cpp > > @@ -149,5 @@ > > > - // "reset" the called-once status -- doable, but more trouble than it's > > > - // worth now.) Initializing that subsystem from JS_Init eliminates the > > > - // problem, but initialization can take a comparatively long time (15ms or > > > - // so), so we really don't want to do it in JS_Init, and we really do want > > > - // to do it only when PRMJ_Now is eventually called. > > > > I guess this is not true anymore? If so, awesome! > > Well... nothing was actually using PR_CallOnce. That said, the limitation > is still there; PRMJ_NowInit initializes static data, including a static > critical section, which PRMJ_NowShutdown destroys. But there is no > protection around PRMJ_NowInit/NowShutdown being called multiple times. Fair enough. All of this should probably just be in static initializers, regardless.