In a process that uses SpiderMonkey, whenever a thread exits, js_ThreadDestructorCB is called. This clears the thread's GSN cache and frees its JSThread struct. I think there are two problems here: 1. It looks like this happens outside of any request. So this is not threadsafe. (For example, if another thread is in js_GC, it may be clearing the same GSN cache at the same time.) 2. JSContexts may still have cx->thread pointing to that JSThread, even after it is freed. This violates some assumptions in js_GC (see the GSN cache clearing code again) and possibly elsewhere. Problem 2 is causing my ActionMonkey tests to crash; they use the pattern "create N JSContexts; spawn N threads to use them; join the threads; destroy the JSContexts". I can work around it by changing the tests.

For problem 2, it seems like js_ThreadDestructorCB should just assert that the list is empty (i.e., the embedder must JS_ClearContextThread before the thread exits). That still leaves problem 1, though.

Aha, problem 1 can't happen if the embedder is following the rules either. So really this is always the embedder's bug. Changing the summary. I wasn't aware of this rule. I'll post a patch that enforces it with a couple of asserts.

Comment on attachment 292863 [details] [diff] [review] v1 >+ JS_ASSERT(cx->thread == thread || cx->thread == NULL); Basis case first, and prevailing style null-tests via !. >+ if (cx->thread == NULL) ! again. r/a=me with these changes. Thanks for catching this! /be

This needs a new review: I added another assertion. JS_ClearContextThread also manipulates cx->thread's contextList and therefore must only be called from the owning thread (that is, from cx->thread itself). I also added a comment on each assertion-- to aid the poor embedder who, after upgrading to the latest SpiderMonkey, hits assertion failures in code that "used to work just fine" (with horrible race conditions). I think it's worth the space; your call. This also addresses Brendan's nits.

Yes! I love comments near assertions like these. It's a lot easier than asking shaver why your embedding doesn't work (as I used to do).

Comment on attachment 292974 [details] [diff] [review] v2 > void > js_ClearContextThread(JSContext *cx) > { >+ /* This must be called only from the owning thread. */ >+ JS_ASSERT(cx->thread == js_GetCurrentThread(cx->runtime) || !cx->thread); Comment could say something about idempotency, to 'splain the right operand of || in the assertion. r+a=me with that. /be

It now says, "If cx is associated with a thread, this must be called only from that thread. If not, this is a harmless no-op."

Checking in js/src/jscntxt.c; /cvsroot/mozilla/js/src/jscntxt.c,v <-- jscntxt.c new revision: 3.124; previous revision: 3.123 done