Needed for bug 365851, split out of the latest patch there. This eliminates complex deadlock avoidance js{lock,obj}.c code and JSRuntime data members, and it is needed because the complexity is intolerable (I'm not sure I can even maintain property cache consistency and avoid deadlock) with the patch for bug 365851. Splitting this out, the review load is easier if I split this out, and of course testing should confirm no regressions or impurities (the whole patch is already known good). /be

Oops, had proto-patch for bug 413097 mixed in. /be

-restart: + restart: Style curiosity: Is the extra beginning space intended?

Comment on attachment 299847 [details] [diff] [review] patch for this bug >+ /* >+ * We assume here that killing links to parent and prototype objects >+ * does not create garbage (such objects typically are long-lived and >+ * widely shared, e.g. global objects, Function.prototype, etc.). We >+ * collect garbage only if a racing thread attempted GC and is waiting >+ * for us to finish (gcLevel > 1) or if someone already poked us. >+ */ >+ if (rt->gcLevel == 1 && !rt->gcPoke) >+ goto done_running; >+ rt->gcLevel = 1; >+ rt->gcPoke = JS_FALSE; >+ } This suggests to have at least internal API for serializing generic actions. It would be useful to do the non-gc tracing properly. Currently that debug or js shell code does not bother with any locking but a proper solution requires something similar to the above fragment. But this is for later. I will finish reviewing the patch later today.

Comment on attachment 299847 [details] [diff] [review] patch for this bug >Index: jsobj.c >=================================================================== ... > JSBool > js_SetProtoOrParent(JSContext *cx, JSObject *obj, uint32 slot, JSObject *pobj) > { ... >+ if (!pobj) { >+ OBJ_SET_SLOT(cx, obj, slot, JSVAL_NULL); >+ return JS_TRUE; > } What if obj shares the scope with its prototype? Shouldn't the code do js_DropObjectMap in this case?

Should have seen that, thanks. Still believe it is worth the code to optimize the obj = {__proto__:null, ...} and similar case to avoid js_GC costs, and it is not necessary on cycle detection grounds. For generality and fool-proofing, I left the !pobj logic in ProcessSetSlotRequest intact. /be

Comment on attachment 299875 [details] [diff] [review] patch, v2 >Index: jsobj.c ... > JSBool > js_SetProtoOrParent(JSContext *cx, JSObject *obj, uint32 slot, JSObject *pobj) > { >+ JSSetSlotRequest ssr; >+ /* Optimize the null case to avoid the unnecessary overhead of js_GC. */ >+ if (!pobj) { >+ JS_LOCK_OBJ(cx, obj); >+ if (OBJ_SCOPE(obj)->object != obj && !js_GetMutableScope(cx, obj)) { This has to be done only when slot == JSSLOT_PROTO. r+ with this fixed.

Thanks -- I also avoided over-optimizing this case by hoisting the OBJ_SCOPE(obj)->object == obj test out of js_GetMutableScope. Hope that is ok, marking r+. /be

Fixed: js/src/jsapi.c 3.394 js/src/jscntxt.h 3.183 js/src/jsgc.c 3.261 js/src/jsgc.h 3.94 js/src/jslock.c 3.71 js/src/jsobj.c 3.420 /be

Caused orange mochitest, assertbotching here: Assertion failure: sprop != child && SPROP_MATCH(sprop, child), at jsscope.c:594 /be

Have to avoid self-deadlocks of this form: #0 0x900248c7 in semaphore_wait_signal_trap () #1 0x90001582 in pthread_mutex_lock () #2 0x20019fbf in PR_Lock (lock=0x11378c0) at ptsynch.c:206 #3 0x230185b2 in js_ContextIterator (rt=0x181ac00, unlocked=1, iterp=0xbfffd6fc) at jscntxt.c:495 #4 0x230015f5 in JS_ContextIterator (rt=0x181ac00, iterp=0xbfffd6fc) at jsapi.c:1037 #5 0x1a01aed7 in XPCJSRuntime::UnsetContextGlobals (this=0x1138930) at xpcjsruntime.cpp:452 #6 0x19ff8163 in XPCCycleCollectGCCallback (cx=0x24a23210, status=JSGC_BEGIN) at nsXPConnect.cpp:458 #7 0x2303c9e0 in js_GC (cx=0x24a23210, gckind=GC_LOCK_HELD) at jsgc.c:2449 #8 0x23057eb9 in js_SetProtoOrParent (cx=0x24a23210, obj=0x1e203e40, slot=0, pobj=0x1e203780) at jsobj.c:301 #9 0x23002da4 in JS_SetPrototype (cx=0x24a23210, obj=0x1e203e40, proto=0x1e203780) at jsapi.c:2835 #10 0x2d98da6a in nsWindowSH::InvalidateGlobalScopePolluter (cx=0x24a23210, obj=0x1e203e40) at nsDOMClassInfo.cpp:4314 Real fix will come in bug 413097 but I would like to get this patch landed first, to lighten review and maintenance loads in the short run. /be

Comment on attachment 300252 [details] [diff] [review] important hack-fix to bug 413097 Forgot to say that I pass the mochitests with this patch applied. The orange last night was due to hangs on rt->gcLock as shown by that stack trace fragment. /be

I lied, the fix needed to avoid a self-hang is now split out in bug 413097. /be

Comment on attachment 300266 [details] [diff] [review] final patch, now that 413097 has been fixed separately r=shaver

Fixed: js/src/jsapi.c 3.398 js/src/jscntxt.h 3.187 js/src/jsgc.c 3.268 js/src/jsgc.h 3.97 js/src/jslock.c 3.73 js/src/jsobj.c 3.422 /be

Regressed Ts, going to try a followup fix. /be

Comment on attachment 300310 [details] [diff] [review] avoid JSGC_BEGIN callback when *only* setting proto/parent via js_GC Lo, it is r=shaver.

Comment on attachment 300317 [details] [diff] [review] fix dumb bug in gckind encoding will tonight never end?

Tweak gckind encoding to use more obvious order to low bits. /be

There are too many JS_SetPrototype and JS_SetParent calls in xpconnect. These should use STOBJ_SET_PROTO/PARENT. I'll file a followup bug tomorrow. This should win back any remaining Ts. /be

Many of the JS_SetPrototype and JS_SetParent calls are covered by bug 408871, which we should think about fixing soon. The rest could use a new bug, unless jst or mrbkap has one handy already. /be

(In reply to comment #22) > There are too many JS_SetPrototype and JS_SetParent calls in xpconnect. These > should use STOBJ_SET_PROTO/PARENT. I'll file a followup bug tomorrow. This > should win back any remaining Ts. But what about scope sharing with STOBJ_SET_PROTO? Wouldn't it require to unshare the scope?

(In reply to comment #24) > (In reply to comment #22) > But what about scope sharing with STOBJ_SET_PROTO? Wouldn't it require to > unshare the scope? Right -- can't forget that! Even better would be to pass the right proto in from the start. If proto and parent are non-null, they will be used as-is. It's only if a null is passed that defaulting occurs. /be

I'm going to avoid filing a new bug, and work on bug 408871, which may help enough to declare victory. Another idea is to make the JS_SetPrototype and JS_SetParent APIs not do cycle detection (except in DEBUG builds, maybe) and declare them sharp tools. This is an incompatible API change. /be