Currently in the JS engine, threads spawned for off-main-thread workloads like compilation and parsing, as well as PJS threads are spawned with the PR_LOCAL_THREAD flag. Here's the NSPR comment about this flag: http://mxr.mozilla.org/mozilla-central/source/nsprpub/pr/include/prthread.h#22 And I quote, "An arbitrary number of such LOCAL threads can be assigned to a single GLOBAL thread." But guess what! We expect TLS to be, well, unique per thread. For the OMT workloads, we store different PerThreadData * in there. For PJS, we store different ForkJoinContext * in there. On Linux and OS X, it seems like PR_LOCAL_THREADS are spawned as different OS threads. On Windows though, you end up with ***distinct NSPR threads that share a TLS***. This probably exploitable. This is completely insane, and we should change all uses of PR_LOCAL_THREAD to PR_GLOBAL_THREAD.

Here's an unreduced test case that led me to this bug. Crashes about 10% of the time for me on Win8.1.

Brian, is there a reason

Oops, message cut off. Brian, was PR_LOCAL_THREAD intentional? It's causing crashes in jsworkers, so we should switch to PR_GLOBAL_THREAD.

NI Ben so he can look at where we're doing this in Gecko and we shouldn't be.

Speaking for PJS, we should definitely switch to PR_GLOBAL_THREAD asap. I'm pretty sure that NSPR code was cribbed from elsewhere without much thought given to this flag.

(In reply to Shu-yu Guo [:shu] from comment #1) > Created attachment 8398776 [details] > unreduced crash > > Here's an unreduced test case that led me to this bug. Crashes about 10% of > the time for me on Win8.1. The test case should be run in a DEBUG shell with flags --ion-eager --ion-parallel-compile=off

Did a little more digging. The good news is that this doesn't affect official builds. The NSPR in those builds are built --with-win32-target=WIN95, which makes all threads global threads. Unfortunately when you build NSPR under mozilla-build, the default parameter is --with-win32-target=WINNT, which gives you this insane local thread behavior. We should still change all PR_LOCAL_THREAD uses to PR_GLOBAL_THREAD in the codebase, since currently we are horribly broken if we ever link against a version of NSPR that supports local threads. Clearing bhackett's NI since it's very unlikely he intended to use local threads.

(In reply to Shu-yu Guo [:shu] from comment #7) > Did a little more digging. The good news is that this doesn't affect > official builds. The NSPR in those builds are built > --with-win32-target=WIN95, which makes all threads global threads. > > Unfortunately when you build NSPR under mozilla-build, the default parameter > is --with-win32-target=WINNT, which gives you this insane local thread > behavior. Uh, how did that happen? That's definitely wrong.

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #8) > (In reply to Shu-yu Guo [:shu] from comment #7) > > Did a little more digging. The good news is that this doesn't affect > > official builds. The NSPR in those builds are built > > --with-win32-target=WIN95, which makes all threads global threads. > > > > Unfortunately when you build NSPR under mozilla-build, the default parameter > > is --with-win32-target=WINNT, which gives you this insane local thread > > behavior. > > Uh, how did that happen? That's definitely wrong. I mean if you build NSPR independently (I was building it for the js shell) without manually giving --with-win32-target=WIN95 to configure, you get the crazy fiber stuff.

I'm sure I copy pasted that PR_LOCAL_THREAD from somewhere else in the JS engine.

Can you file a separate bug to switch the official builds to --with-win32-target=WINNT? I'm not too familiar with NSPR but it sounds like that's what we want (after we fix this fiber thing of course).

(In reply to Jan de Mooij [:jandem] from comment #11) > Can you file a separate bug to switch the official builds to > --with-win32-target=WINNT? I'm not too familiar with NSPR but it sounds like > that's what we want (after we fix this fiber thing of course). I don't think we actually want --with-win32-target=WINNT, it's just poorly named or something.

We absolutely do not want the WINNT NSPR. See https://bugzilla.mozilla.org/show_bug.cgi?id=830785#c12 and the following comment.

So in practice is it only with the WINNT NSPR variant that there's a difference between PR_LOCAL and PR_GLOBAL? If that's the case then we can just do a search/replace on the whole tree without any side effects I guess.

Not sure who to flag r? for the non-js parts. I also didn't touch security/nss, which seems to be from before the flood.

Comment on attachment 8400953 [details] [diff] [review] Part 1: js/ weeeee

Btw, this probably isn't s-s, yes?

Comment on attachment 8400954 [details] [diff] [review] Part 2: dom/, netwerk/, startupcache/, and xpcom/ Ben is probably as reasonable a reviewer for this as anybody.

Comment on attachment 8400954 [details] [diff] [review] Part 2: dom/, netwerk/, startupcache/, and xpcom/ Review of attachment 8400954 [details] [diff] [review]: ----------------------------------------------------------------- I would actually request review from the folks who added these just to make sure they didn't think they were getting something that they're not. I'll stamp the TestThreadedIO.cpp change, it has existed since 2000... Honza, can you look at DOMStorageDBThread.cpp? Taras, can you look over StartupCache.cpp? Gabriele, can you look at TimeStamp_posix.cpp? Dave, can you look at nsProcessCommon.cpp?

Comment on attachment 8400954 [details] [diff] [review] Part 2: dom/, netwerk/, startupcache/, and xpcom/ The TimeStamp_posix.cpp part was copied from the existing code when those files were refactored and it doesn't have any reason to request special treatment of TLS variables as the spawned thread is not using any.

Comment on attachment 8400954 [details] [diff] [review] Part 2: dom/, netwerk/, startupcache/, and xpcom/ Aklotz is startup cache

Comment on attachment 8400954 [details] [diff] [review] Part 2: dom/, netwerk/, startupcache/, and xpcom/ Review of attachment 8400954 [details] [diff] [review]: ----------------------------------------------------------------- r+ for StartupCache

Comment on attachment 8400954 [details] [diff] [review] Part 2: dom/, netwerk/, startupcache/, and xpcom/ Review of attachment 8400954 [details] [diff] [review]: ----------------------------------------------------------------- Pretty sure I just copied this from somewhere else in the code.

Comment on attachment 8400954 [details] [diff] [review] Part 2: dom/, netwerk/, startupcache/, and xpcom/ Review of attachment 8400954 [details] [diff] [review]: ----------------------------------------------------------------- r+ for netwerk/

Comment on attachment 8400955 [details] [diff] [review] Part 3: security/ You should be actually the person giving the final r+

Comment on attachment 8400955 [details] [diff] [review] Part 3: security/ Review of attachment 8400955 [details] [diff] [review]: ----------------------------------------------------------------- As far as I've seen, using "global" threads is the correct thing nowadays.

remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/0713dda36318 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/f3ef20919f35 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/bd8ca323fed8

FWIW given shu's investigation I don't think this needs to be s-s either.

Opening.

https://hg.mozilla.org/mozilla-central/rev/0713dda36318 https://hg.mozilla.org/mozilla-central/rev/f3ef20919f35 https://hg.mozilla.org/mozilla-central/rev/bd8ca323fed8