User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.5) Gecko/2009010509 Gentoo Firefox/3.0.5 Build Identifier: SpiderMonkey 1.7.0 See http://groups.google.com/group/mozilla.dev.tech.js-engine/browse_thread/thread/b1bf3460297f01e3 for the initial discussion about this. I have a multi-threaded application that periodically crashes. I maintain a pool of JSContexts: as they're requested from the pool JS_SetContextThread and JS_BeginRequest are called; when they're returned JS_EndRequest and JS_ClearContextThread are called. The crashes consistently occurs inside js_GC in the following code block: while ((acx = js_ContextIterator(rt, JS_FALSE, &iter)) != NULL) { if (!acx->thread || acx->thread == cx->thread) continue; memset(acx->thread->gcFreeLists, 0, sizeof acx->thread->gcFreeLists); GSN_CACHE_CLEAR(&acx->thread->gsnCache); } acx always appears to be valid but acx->thread == NULL when the application crashes (which may be in the memset or GSN_CACHE_CLEAR line). This shouldn't occur as these lines should be skipped if (!acx->thread).. What I suspect is happening is that one thread is calling JS_GC while a second is calling JS_EndRequest and JS_ClearContextThread (in returning a context to the pool). The call to JS_GC will block until JS_EndRequest finishes.. JS_GC then resumes.. but while JS_GC is running JS_ClearContextThread also runs (no locking is done in this?), modifying the value of acx->thread as the code above runs. acx->thread becomes NULL just before it gets dereferenced and the application exits. Reproducible: Always Steps to Reproduce: I've tried to put together the smallest bit of code to replicate the problem (and hope I haven't missed anything trimming it down). main() sets up an environment pretty much following the example in the User Guide then sits endlessly calling JS_GC. Before the loop it spawns one or more threads that create a new JSContext each and sit in their own loops beginning and ending requests for those contexts. If the child threads just call: JS_BeginRequest JS_EndRequest then the program runs and runs without any problems, as expected. If the child threads call: JS_SetContextThread JS_BeginRequest JS_EndRequest JS_ClearContextThread then the program crashes after a few seconds for me. If the child threads call: JS_SetContextThread JS_ClearContextThread the crashes happen almost instantly. 8<---- #include <pthread.h> #include <stdlib.h> #define XP_UNIX #define JS_THREADSAFE #include "jsapi.h" #define THREADS 1 static JSClass global_class = { "global", JSCLASS_GLOBAL_FLAGS, JS_PropertyStub, JS_PropertyStub, JS_PropertyStub, JS_PropertyStub, JS_EnumerateStub, JS_ResolveStub, JS_ConvertStub, JS_FinalizeStub, JSCLASS_NO_OPTIONAL_MEMBERS }; static JSRuntime *rt; static void * testfunc(void *ignored) { JSContext *cx = JS_NewContext(rt, 0x1000); if (cx == NULL) exit(1); for (;;) { // Fastest way to cause a crash.. JS_SetContextThread(cx); JS_ClearContextThread(cx); // Slower to crash, but more realistic use case? // JS_SetContextThread(cx); // JS_BeginRequest(cx); // JS_EndRequest(cx); // JS_ClearContextThread(cx); // Avoiding Set/ClearContextThread does not cause any crash.. // JS_BeginRequest(cx); // JS_EndRequest(cx); } } int main(void) { JSContext *cx; JSObject *global; rt = JS_NewRuntime(0x100000); if (rt == NULL) return 1; cx = JS_NewContext(rt, 0x1000); if (cx == NULL) return 1; global = JS_NewObject(cx, &global_class, NULL, NULL); if (global == NULL) return 1; if (! JS_InitStandardClasses(cx, global)) return 1; int i; for (i = 0; i < THREADS; i++) { pthread_t t; pthread_create(&t, NULL, testfunc, NULL); } for (;;) { // Does an isolated JS_GC need to be wrapped in a request? The // API doesn't explicitly state so, but we get the same // behaviour regardless of whether or not we use // JS_{Begin,End}Request anyway.. JS_BeginRequest(cx); JS_GC(cx); JS_EndRequest(cx); } } Actual Results: The application crashes as a NULL pointer is dereferenced in js_GC: Program received signal SIGSEGV, Segmentation fault. [Switching to Thread 0xb7c9d8d0 (LWP 3446)] 0xb7ec2bbd in js_GC (cx=0x804fed0, gckind=GC_NORMAL) at jsgc.c:2850 2850 GSN_CACHE_CLEAR(&acx->thread->gsnCache); (gdb) p acx $1 = (JSContext *) 0x806df78 (gdb) p acx->thread $2 = (JSThread *) 0x0 Expected Results: The program shouldn't crash :) The bug can be worked around by avoiding the use of JS_ClearContextThread and maintaining a 1:1 relation of thead:contexts, but this would only just be a workaround..

Can you upgrade to the current Engine and see if this was fixed?

There is a version of this bug that does not require an explicit JS_ClearContextThread. If during the GC another thread calls JS_DestroyContextNoGC(), then the context can be destroyed right in the middle of the context marking loop in js_GC(). Since this scenario is possible with the DOM workers implementation, I nominate this bug as 1.9.1 blocker. The fix for this should be straightforward: both js_DestroyContext and js_ClearContextThread must wait for the current GC to complete.

Hi Natch, Yes, it still happens in the latest version of the code. Compiling with debugging enabled I was also [rightly] reminded to wrap the JS_NewObject and JS_InitStandardClasses calls in a request; but even after that I still got the same problem: Program received signal SIGSEGV, Segmentation fault. [Switching to Thread 0xb7b026d0 (LWP 24095)] 0xb7e2015c in js_GC (cx=0x8052828, gckind=GC_NORMAL) at jsgc.cpp:3497 3497 GSN_CACHE_CLEAR(&acx->thread->gsnCache); Current language: auto; currently c++ (gdb) p acx $1 = (JSContext *) 0x807ad30 (gdb) p acx->thread $2 = (JSThread *) 0x0

(In reply to comment #2) > There is a version of this bug that does not require an explicit > JS_ClearContextThread. If during the GC another thread calls > JS_DestroyContextNoGC(), then the context can be destroyed right in the middle > of the context marking loop in js_GC(). Since this scenario is possible with > the DOM workers implementation, I nominate this bug as 1.9.1 blocker. > > The fix for this should be straightforward: both js_DestroyContext and > js_ClearContextThread must wait for the current GC to complete. Igor, it was never legal to use JS_THREADSAFE but not JS_BeginRequest(cx) before calling JS_DestroyContext*. JS_ClearContextThread is a different issue, arguably, although it too could be called in a request (on a different context, say a root or dummy context). As the comment in the test program suggests, the test program unnecessarily wraps a JS_BeginRequest and JS_EndRequest around JS_GC. Before we do anything in SpiderMonkey, people seeing this bug should fix their code to call JS_DestroyContext only from a request (it can be on the context to be destroyed), and try using a request on a different context around calls to JS_ClearContextThread. /be

Brendan, That just seems weird. I'm not running the trunk, but my pattern is: 1) JS_BeginRequest 2) do stuff... 3) JS_EndRequest 4) JS_GC or JS_MaybeGC 5) JS_ClearContextThread 6) JS_SetContextPrivate When I shut down I do this: 1) JS_SetContextThread 2) JS_SetGlobalObject(pContext, NULL); //Make sure globals are freed too. 3) JS_GC(pContext); 4) JS_DestroyContextMaybeGC(pContext); Wrapping a request around destroy context seems non-intuitive at best. Are you sure this is necessary?

(In reply to comment #4) > it was never legal to use JS_THREADSAFE but not JS_BeginRequest(cx) > before calling JS_DestroyContext*. Then we have a bug in xpconnect which does exactly that in http://mxr.mozilla.org/mozilla-central/source/js/src/xpconnect/src/nsXPConnect.cpp#2032 .

(In reply to comment #6) > (In reply to comment #4) > > it was never legal to use JS_THREADSAFE but not JS_BeginRequest(cx) > > before calling JS_DestroyContext*. > > Then we have a bug in xpconnect which does exactly that in > http://mxr.mozilla.org/mozilla-central/source/js/src/xpconnect/src/nsXPConnect.cpp#2032 Given that, IMO it is better to allow to destroy context outside the request as js_DestroyContext can detect that a GC is running and wait for its completion as necessary. This way ReleaseJSContext from xpc would not need to call JS_BeginRequest avoiding taking the GC lock an extra time.

brendan: there was code in JS which made it clear that you didn't have to call JS_DestroyContext from within a request, that it would (effectively) automatically create a request if it needed to. I remember people complaining about the balance or lack of it, and i think of late people forced me to added extra begins, but the comments and code history (from memory) were clear that it wasn't supposed to be necessary. That said, the bug's complaining about Clear (which I also use...). It seems odd that for each thread I'd be expected to have an extra context that i begin request on just so that i can reshare a context to another thread. And when I'm done w/ th thread, what do I do? begin request and then destroy the thread context?...

(In reply to comment #4) > JS_ClearContextThread is a different issue, arguably, although it too could be > called in a request (on a different context, say a root or dummy context). Calling JS_ClearContextThread within a new request does indeed seem to solve my problem. I'd have to agree with other comments about it being non-intuitive though. I don't think if I could ever use a single root/dummy context (lest I need to call JS_{Set,Clear}ContextThread on that and so end up in an infinite loop wrapping JS_ClearContextThread calls); I could use one context per thread -- but this ironically makes context pooling (and therefore any call to JS_ClearContextThread) redundant for me. This "proper" approach.. JS_SetContextThread(cx1) JS_BeginRequest(cx1) ... JS_EndRequest(cx1) JS_BeginRequest(cx2) JS_ClearContextThread(cx1) JS_EndRequest(cx2) ie, ending one request only to begin another simply to tidy up the first request's context -- is also certainly not as clear/intuitive as the current "usual code" example on https://developer.mozilla.org/en/SpiderMonkey/JSAPI_Reference/JS_ClearContextThread I'd be +1 for making JS_ClearContextThread block on the GC; but will leave it for you all. Thanks, Paul

(In reply to comment #5) > Brendan, That just seems weird. > I'm not running the trunk, but my pattern is: > > 1) JS_BeginRequest > 2) do stuff... > 3) JS_EndRequest > 4) JS_GC or JS_MaybeGC > 5) JS_ClearContextThread > 6) JS_SetContextPrivate I agree we need to help the JS_ClearContextThread not need to be surrounded by a (different-cx-based) request. > When I shut down I do this: > > 1) JS_SetContextThread > 2) JS_SetGlobalObject(pContext, NULL); //Make sure globals are freed too. > 3) JS_GC(pContext); > 4) JS_DestroyContextMaybeGC(pContext); This code violates the request model, which requires almost all JS API entry points to be called within a request. JS_GC is an exception to the rule, but the bundle of four above, taken as a request that must not race with the GC in whole (even though the JS_GC defends against such races by itself) is not exempt from the rule. > Wrapping a request around destroy context seems non-intuitive at best. No, it's the general rule: JS API entry points with a few exceptions need to be made within a request if JS_THREADSAFE. > Are you sure this is necessary? Yes, obviously from the analysis in this bug. Changing things to make an exception for JS_DestroyContext* could be done, maybe, but it's not how things stand now, or have worked for years (we're at the decade mark, if not past it). (In reply to comment #8) > brendan: there was code in JS which made it clear that you didn't have to call > JS_DestroyContext from within a request, that it would (effectively) > automatically create a request if it needed to. No. You might be reversing things: JS_DestroyContext* funnel into jscntxt.c's (now jscntxt.cpp's) js_DestroyContext, which will end any active requests: #ifdef JS_THREADSAFE /* * Destroying a context implicitly calls JS_EndRequest(). Also, we must * end our request here in case we are "last" -- in that event, another * js_DestroyContext that was not last might be waiting in the GC for our * request to end. We'll let it run below, just before we do the truly * final GC and then free atom state. * * At this point, cx must be inaccessible to other threads. It's off the * rt->contextList, and it should not be reachable via any object private * data structure. */ while (cx->requestDepth != 0) JS_EndRequest(cx); #endif > That said, the bug's complaining about Clear (which I also use...). It seems > odd that for each thread I'd be expected to have an extra context that i begin > request on just so that i can reshare a context to another thread. Yes, this is worth fixing as noted above. It's why we are here, so no need to belabor it. Paul, I'm +1 on fixing the bug as summarized. I'm not so keen on adding an implicit JS_BeginRequest to js_DestroyContext, though. That adds complexity and code, adds an exception to the rule, and may be unsound in general. Need to think about it more. Clearly every API could start its own request and reduce the obligation on the embedding to call JS_BeginRequest and JS_EndRequest. But this would be both badly inefficient, and in some scenarios racily unsafe. /be

(In reply to comment #7) > (In reply to comment #6) > > (In reply to comment #4) > > > it was never legal to use JS_THREADSAFE but not JS_BeginRequest(cx) > > > before calling JS_DestroyContext*. > > > > Then we have a bug in xpconnect which does exactly that in > > http://mxr.mozilla.org/mozilla-central/source/js/src/xpconnect/src/nsXPConnect.cpp#2032 > > Given that, IMO it is better to allow to destroy context outside the request as > js_DestroyContext can detect that a GC is running and wait for its completion > as necessary. This way ReleaseJSContext from xpc would not need to call > JS_BeginRequest avoiding taking the GC lock an extra time. Igor, could you split that out from this bug? Unless the two are really the same or should be fixed together. /be

The patch compiles and runs some trivial tests, but it requires more testing.

Comment on attachment 360736 [details] [diff] [review] v1 I have done testing and the patch works so it is a time to ask for a review. To properly support using JS_ClearContextThread and JS_SetContextThread (which also has this bug) outside JS requests the patch reuse the existing code from js_(Add|Remove)Root. That code is moved to js_WaitIfGC function. The patch refactors the context initialization/destruction to make sure that only JS_(Clear|Set)ContextThread takes the GC lock as there is no problem with lock-less manipulations of cx->thread when the context is not on runtime's list. The patch also fixes the existing bug in js_NewContext when the code ignored js_SetContextThread failures.

Igor, can you look at this bug I filed a while back. Its a crash and I'm thinking its related to this bug. Root cause might be the same... https://bugzilla.mozilla.org/show_bug.cgi?id=466182

(In reply to comment #14) > Igor, can you look at this bug I filed a while back. > Its a crash and I'm thinking its related to this bug. > Root cause might be the same... Yes, it looks like another incarnation of either this bug or bug 477021. For now I just recorder the dependencies leaving the duping for later.

Comment on attachment 360736 [details] [diff] [review] v1 Looks good, thanks for patching. This patch reminds me that js_GetCurrentThread(rt) does not use its rt param, and we have a bug on the consequences of not handling multiple runtimes per thread well. Nits below, r=me with them addressed. /be >+void >+js_InitContextThread(JSContext *cx, JSThread *thread) > { >- JSThread *thread = js_GetCurrentThread(cx->runtime); >+ JS_ASSERT(thread->id == js_CurrentThreadId()); Could use CURRENT_THREAD_IS_ME(thread) here in the assertion. Could even add a #define of that macro to expand to JS_TRUE or (better) C++ true in an #else clause for the #ifdef JS_THREADASAFE in jslock.h, to avoid #ifdef JS_THREADSAFE later in this patch. >+ /* >+ * At this point the cx is not on rt->contextList. Thus we do not need >+ * to prevent a race against a GC when adding cx to JSThread.contextList. English nits: s/the cx/cx/ and s/a GC/the GC/. >+ JS_ASSERT(cx->thread->id == js_CurrentThreadId()); >+#endif Here's another CURRENT_THREAD_IS_ME opportunity, which could be sans ifdefs. >@@ -470,20 +463,16 @@ js_DestroyContext(JSContext *cx, JSDestr > > #ifdef JS_THREADSAFE > /* > * Destroying a context implicitly calls JS_EndRequest(). Also, we must > * end our request here in case we are "last" -- in that event, another > * js_DestroyContext that was not last might be waiting in the GC for our > * request to end. We'll let it run below, just before we do the truly > * final GC and then free atom state. > */ > while (cx->requestDepth != 0) > JS_EndRequest(cx); > #endif FYI I do not think we can remove this -- I know of embeddings that count on it, it's an advertized part of the API. It does not seem important to remove, unless I'm missing something. >@@ -535,17 +524,23 @@ js_DestroyContext(JSContext *cx, JSDestr > while ((lrc = lrs->topChunk) != &lrs->firstChunk) { > lrs->topChunk = lrc->down; > JS_free(cx, lrc); > } > JS_free(cx, lrs); > } > > #ifdef JS_THREADSAFE >- js_ClearContextThread(cx); >+ /* >+ * Since the cx is not on rt->contextList, it cannot be accessed by a GC >+ * running on another thread. Thus we can safely unlink the cx from >+ * from JSThread.contextList without taking the GC lock or calls to >+ * JS_ClearContextThread. The second sentence is not grammatical -- faulty parallelism in "without taking ... or calls ...." What is the part after "without taking the GC lock" trying to say? >+js_WaitIfGC(JSRuntime *rt) js_WaitForGC is the better name, I think. >+{ >+ /* >+ * If the GC is running and we're called on another thread, wait for this >+ * GC activation to finish. We can safely wait here (in the case where we >+ * are called within a request on another thread's context) without fear >+ * of deadlock because the GC doesn't set rt->gcRunning until after it has >+ * waited for all active requests to end. >+ * >+ * We call here js_CurrentThreadId() after checking for rt->gcRunning to >+ * avoid expensive js_CurrentThreadId when the GC is not running. >+ */ Typically this major comment would go before the function definition, not first thing in its body. Hope it rewraps aesthetically! /be

The new version of the patch addresses the nits.

Comment on attachment 361027 [details] [diff] [review] v2 There is no need for new review per comment 16.

landed to TM - http://hg.mozilla.org/tracemonkey/rev/7b7fb6cf2299

Straightforward backport. There was a little drift, but not much.

(I want to land this on the branch because we're going to do a SpiderMonkey source release out of that branch, and I think this will fix bug 478336 comment 19. We'll verify that in the next day or two...)

Comment on attachment 366871 [details] [diff] [review] backport to 1.9.0 (for SpiderMonkey 1.8 source release) Can we get some unit tests for this change?

I dunno, Daniel... Igor might have some ideas but I wouldn't expect too much. The stack of 4 patches I've backported are to fix a bug that is important for other SpiderMonkey users, but which might not be able to crash Gecko at all. My first thought would be to look at the existing worker threads tests--but are those in the 1.9.0 branch? (I am on vacation until March 30, so I will be slow to respond.)

This patch is patch 1/4 to fix a crashing bug for embedders that blocks the SpiderMonkey 1.8 source release. See bug 478336 comment 21 for the full list. Daniel, I don't have enough time budgeted for SpiderMonkey 1.8 to do a lot more work on this, and unit tests for this would involve writing a new test harness (C++ code banging on SM in a way Gecko doesn't do). Igor can assess the risk of these backported patches. If they are too risky to land in 1.9.0.x without tests, that's ok--I will fork for SM1.8 (because that can be done quickly and we don't plan to do any SM point releases from CVS).

(In reply to comment #26) > Igor can assess the risk of these backported patches. If they are too risky to > land in 1.9.0.x without tests, that's ok--I will fork for SM1.8 The fork of SM1.8 from 1.9.0 would mean that security fixes from 1.9.0 would not find its way into SM1.8 automatically. But the patches are somewhat risky to land without extended testing. Surely they found their way into 1.9.1 without much hassle. Still they could potentially break if not FF itself then some extensions or, say XUL-runner users.

(In reply to comment #27) > The fork of SM1.8 from 1.9.0 would mean that security fixes from 1.9.0 would > not find its way into SM1.8 automatically. I thought of this. I can do this in Mercurial and merge fixes, but the real answer is that we don't have time to do SM1.8 point releases anyway. We never did any for SM1.7, despite numerous security fixes. Instead, we should just release SM1.8.1 from mozilla-central soon after the release of Firefox 3.5/Gecko 1.9.1.

(In reply to comment #28) > but the real answer is that we don't have time to do SM1.8 point releases anyway. Ok, so then lets create a branch, land all JS1.8 blockers there so interested folks can get the fully patched source from the CVS and merge the branch with 1.9.0 only once before JS1.8 release.

Comment on attachment 366871 [details] [diff] [review] backport to 1.9.0 (for SpiderMonkey 1.8 source release) Approved for 1.9.0.10, a=dveditz for release-drivers

landed to 1.9.0: Checking in jsapi.c; /cvsroot/mozilla/js/src/jsapi.c,v <-- jsapi.c new revision: 3.449; previous revision: 3.448 done Checking in jscntxt.c; /cvsroot/mozilla/js/src/jscntxt.c,v <-- jscntxt.c new revision: 3.138; previous revision: 3.137 done Checking in jscntxt.h; /cvsroot/mozilla/js/src/jscntxt.h,v <-- jscntxt.h new revision: 3.206; previous revision: 3.205 done Checking in jsgc.c; /cvsroot/mozilla/js/src/jsgc.c,v <-- jsgc.c new revision: 3.305; previous revision: 3.304 done Checking in jsgc.h; /cvsroot/mozilla/js/src/jsgc.h,v <-- jsgc.h new revision: 3.113; previous revision: 3.112 done

Backed out from 1.9.0 - the patch has caused compilation error on Windows.

The initial backport of the patch contains: + if (cx->thread) { + JS_ASSERT(cx->thread->id == js_CurrentThreadId()); + return cx->thread->id; + } + + JSRuntime *rt = cx->runtime; + JSThread *thread = js_GetCurrentThread(rt); That is, the code declares variables after code. This is legal in C++ and in C99, but it is not in C89 and older versions of MSVC does not allow that. The new patch moves that to the beginning of the function.

Comment on attachment 373456 [details] [diff] [review] backport to 1.9.0 v2 Approved for 1.9.0.10. a=ss

re-landed to 1.9.0 branch: Checking in jsapi.c; /cvsroot/mozilla/js/src/jsapi.c,v <-- jsapi.c new revision: 3.451; previous revision: 3.450 done Checking in jscntxt.c; /cvsroot/mozilla/js/src/jscntxt.c,v <-- jscntxt.c new revision: 3.142; previous revision: 3.141 done Checking in jscntxt.h; /cvsroot/mozilla/js/src/jscntxt.h,v <-- jscntxt.h new revision: 3.208; previous revision: 3.207 done Checking in jsgc.c; /cvsroot/mozilla/js/src/jsgc.c,v <-- jsgc.c new revision: 3.307; previous revision: 3.306 done Checking in jsgc.h; /cvsroot/mozilla/js/src/jsgc.h,v <-- jsgc.h new revision: 3.115; previous revision: 3.114 done