413097 - JSGC_BEGIN callback can be invoked while rt->gcLock held

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Brendan Eich [:brendan]]

This leads to self-lock pretty directly:

#0  0x9002493f in semaphore_wait_trap ()#1  0x9000158f in pthread_mutex_lock ()#2  0x20019f27 in PR_Lock (lock=0x1137890) at ptsynch.c:206
#3  0x23018132 in js_ContextIterator (rt=0x1819a00, unlocked=1, iterp=0xbfffc1dc) at jscntxt.c:495#4  0x23001345 in JS_ContextIterator (rt=0x1819a00, iterp=0xbfffc1dc) at jsapi.c:1038#5  0x19ec7e2b in XPCJSRuntime::UnsetContextGlobals (this=0x1138860) at xpcjsruntime.cpp:452
#6  0x19ea4df5 in XPCCycleCollectGCCallback (cx=0x1137b00, status=JSGC_BEGIN) at nsXPConnect.cpp:458#7  0x2303c16c in js_GC (cx=0x1137b00, gckind=GC_LOCK_HELD) at jsgc.c:2442
#8  0x2305ae86 in js_SetProtoOrParent (cx=0x1137b00, obj=0x1f70e1a0, slot=0, pobj=0x1b2c8060) at jsobj.c:294

This stack shows a new call to js_GC with rt->gcLock held, at frame 8, from the patch I'm working on for bug 365851, but the same thing could happen in the last ditch case where js_NewGCThing finds no memory available and calls js_GC with rt->gcLock held.

/be

Comment 1

[Peter Van der Beken [:peterv]]

Hmm, tricky. We need to make sure we only call UnsetContextGlobals/ResetContextGlobals once, probably ignoring it in nested calls to XPCCycleCollectGCCallback. Could new JSContexts be created during the "outer" js_GC but before the nested call to js_GC?

Comment 2

[Mike Schroepfer]

Matching blocking1.9 status with blocked bug 365851

Comment 3

[Brendan Eich [:brendan]]

Attachment: simple racy fix, same guarantees as before and no selflock

This is as good as it gets, I'm afraid. JSGC_BEGIN is normally called without rt->gcLock held, and it can race among threads. Gecko uses it only to veto GC attempts on non-main threads, which is a static thread-id comparison against the current thread's id.

/be

Comment 4

[Mike Shaver (:shaver emeritus)]

Comment on attachment 300256 [details] [diff] [review]
simple racy fix, same guarantees as before and no selflock

r-, new patch coming with safer sampling of rt->gcCallback.

Comment 5

[Brendan Eich [:brendan]]

Attachment: better fix, thanks to shaver

Comment 6

[Mike Shaver (:shaver emeritus)]

Comment on attachment 300261 [details] [diff] [review]
better fix, thanks to shaver

r=shaver.

Comment 7

[Brendan Eich [:brendan]]

Fixed:

js/src/jsapi.c 3.397
js/src/jsgc.c 3.265

/be

Comment 8

[Brendan Eich [:brendan]]

Ahem:

$ cvs log -r3.266 jsgc.c
[...]
revision 3.266
date: 2008/01/30 05:12:45;  author: brendan%mozilla.org;  state: Exp;  lines: +2 -2
Oops.
=============================================================================
$ cvs log -r3.267 jsgc.c
[...]
revision 3.267
date: 2008/01/30 05:14:55;  author: brendan%mozilla.org;  state: Exp;  lines: +2 -2
Dammit.

/be

Comment 9

[Brian Crowder]

Attachment: test patch to backout this patch (too late to back out cleanly, and some post-review fixes?)

I'm just recording this patch here as the "reversal" patch for the changes documented in this bug, as I am testing its removal in response to bug 415393 comment 8.  Results soon.

Comment 10

[Brian Crowder]

Attachment: results of backout

This is the result of backing out the patch in this bug, for SunSpider.  I think overall, it's pretty much a wash, assuming I did the backout patch correctly.

Comment 11

[Brendan Eich [:brendan]]

Smart money (shaver and me, at any rate) says its bug 414452 that dinged in-browser-only SS perf -- but we can get that back by fixing bug 408871 and (if necessary) making JS_SetPrototype not to cycle detection unless #ifdef DEBUG.

/be