From 26633b1aa8933e946807e2ff58ebee5a29063e22 Mon Sep 17 00:00:00 2001 From: "brendan%mozilla.org" Date: Tue, 2 Apr 2002 04:23:21 +0000 Subject: [PATCH] Fix next-to-last vs. last context GC race, plus ClaimScope vs. js_DestroyContext race; removed js_ForceGC from the FRIEND JS API (133773, sr=jband&shaver, a=asa). git-svn-id: svn://10.0.0.236/trunk@117899 18797224-902f-48f8-a5cc-f745e15eee43 --- mozilla/js/src/js.c | 2 +- mozilla/js/src/jsapi.c | 10 +++---- mozilla/js/src/jscntxt.c | 30 +++++++++++--------- mozilla/js/src/jscntxt.h | 17 ++++++++--- mozilla/js/src/jsgc.c | 21 +++++++++----- mozilla/js/src/jsgc.h | 6 ++-- mozilla/js/src/jslock.c | 2 +- mozilla/js/src/xpconnect/shell/xpcshell.cpp | 8 +++--- mozilla/js/src/xpconnect/src/nsXPConnect.cpp | 2 +- mozilla/js/src/xpconnect/tests/TestXPC.cpp | 5 ++-- mozilla/js2/tests/cpp/shamu.cpp | 6 ---- mozilla/modules/xml/js/test/js.c | 2 +- 12 files changed, 61 insertions(+), 50 deletions(-) diff --git a/mozilla/js/src/js.c b/mozilla/js/src/js.c index 47720aa45b3..e6ca33a09ab 100644 --- a/mozilla/js/src/js.c +++ b/mozilla/js/src/js.c @@ -706,7 +706,7 @@ GC(JSContext *cx, JSObject *obj, uintN argc, jsval *argv, jsval *rval) js_DumpGCHeap = stdout; } #endif - js_ForceGC(cx); + JS_GC(cx); #ifdef GC_MARK_DEBUG if (js_DumpGCHeap != stdout) fclose(js_DumpGCHeap); diff --git a/mozilla/js/src/jsapi.c b/mozilla/js/src/jsapi.c index 18c663254a8..8771046c258 100644 --- a/mozilla/js/src/jsapi.c +++ b/mozilla/js/src/jsapi.c @@ -652,7 +652,7 @@ JS_NewRuntime(uint32 maxbytes) rt->rtLock = JS_NEW_LOCK(); if (!rt->rtLock) goto bad; - rt->stateChange = JS_NEW_CONDVAR(rt->rtLock); + rt->stateChange = JS_NEW_CONDVAR(rt->gcLock); if (!rt->stateChange) goto bad; rt->setSlotLock = JS_NEW_LOCK(); @@ -684,10 +684,10 @@ JS_DestroyRuntime(JSRuntime *rt) { #ifdef DEBUG /* Don't hurt everyone in leaky ol' Mozilla with a fatal JS_ASSERT! */ - if (rt->contextList.next != &rt->contextList) { + if (!JS_CLIST_IS_EMPTY(&rt->contextList)) { JSContext *cx, *iter = NULL; uintN cxcount = 0; - while ((cx = js_ContextIterator(rt, &iter)) != NULL) + while ((cx = js_ContextIterator(rt, JS_TRUE, &iter)) != NULL) cxcount++; fprintf(stderr, "JS API usage error: %u contexts left in runtime upon JS_DestroyRuntime.\n", @@ -925,7 +925,7 @@ JS_GetRuntime(JSContext *cx) JS_PUBLIC_API(JSContext *) JS_ContextIterator(JSRuntime *rt, JSContext **iterp) { - return js_ContextIterator(rt, iterp); + return js_ContextIterator(rt, JS_TRUE, iterp); } JS_PUBLIC_API(JSVersion) @@ -1641,7 +1641,7 @@ JS_GC(JSContext *cx) JS_FinishArenaPool(&cx->stackPool); JS_FinishArenaPool(&cx->codePool); JS_FinishArenaPool(&cx->tempPool); - js_ForceGC(cx); + js_ForceGC(cx, 0); } JS_PUBLIC_API(void) diff --git a/mozilla/js/src/jscntxt.c b/mozilla/js/src/jscntxt.c index 076545ed477..231020b4804 100644 --- a/mozilla/js/src/jscntxt.c +++ b/mozilla/js/src/jscntxt.c @@ -74,7 +74,7 @@ js_NewContext(JSRuntime *rt, size_t stackChunkSize) js_InitContextForLocking(cx); #endif - JS_LOCK_RUNTIME(rt); + JS_LOCK_GC(rt); for (;;) { first = (rt->contextList.next == &rt->contextList); if (rt->state == JSRTS_UP) { @@ -89,7 +89,7 @@ js_NewContext(JSRuntime *rt, size_t stackChunkSize) JS_WAIT_CONDVAR(rt->stateChange, JS_NO_TIMEOUT); } JS_APPEND_LINK(&cx->links, &rt->contextList); - JS_UNLOCK_RUNTIME(rt); + JS_UNLOCK_GC(rt); /* * First we do the infallible, every-time per-context initializations. @@ -139,10 +139,10 @@ js_NewContext(JSRuntime *rt, size_t stackChunkSize) return NULL; } - JS_LOCK_RUNTIME(rt); + JS_LOCK_GC(rt); rt->state = JSRTS_UP; JS_NOTIFY_ALL_CONDVAR(rt->stateChange); - JS_UNLOCK_RUNTIME(rt); + JS_UNLOCK_GC(rt); } return cx; @@ -158,13 +158,13 @@ js_DestroyContext(JSContext *cx, JSGCMode gcmode) rt = cx->runtime; /* Remove cx from context list first. */ - JS_LOCK_RUNTIME(rt); + JS_LOCK_GC(rt); JS_ASSERT(rt->state == JSRTS_UP || rt->state == JSRTS_LAUNCHING); JS_REMOVE_LINK(&cx->links); last = (rt->contextList.next == &rt->contextList); if (last) rt->state = JSRTS_LANDING; - JS_UNLOCK_RUNTIME(rt); + JS_UNLOCK_GC(rt); if (last) { /* Unpin all pinned atoms before final GC. */ @@ -207,7 +207,7 @@ js_DestroyContext(JSContext *cx, JSGCMode gcmode) if (last) { /* Always force, so we wait for any racing GC to finish. */ - js_ForceGC(cx); + js_ForceGC(cx, GC_LAST_CONTEXT); /* Iterate until no finalizer removes a GC root or lock. */ while (rt->gcPoke) @@ -218,13 +218,13 @@ js_DestroyContext(JSContext *cx, JSGCMode gcmode) js_FreeAtomState(cx, &rt->atomState); /* Take the runtime down, now that it has no contexts or atoms. */ - JS_LOCK_RUNTIME(rt); + JS_LOCK_GC(rt); rt->state = JSRTS_DOWN; JS_NOTIFY_ALL_CONDVAR(rt->stateChange); - JS_UNLOCK_RUNTIME(rt); + JS_UNLOCK_GC(rt); } else { if (gcmode == JS_FORCE_GC) - js_ForceGC(cx); + js_ForceGC(cx, 0); else if (gcmode == JS_MAYBE_GC) JS_MaybeGC(cx); } @@ -256,7 +256,7 @@ js_DestroyContext(JSContext *cx, JSGCMode gcmode) } JSBool -js_LiveContext(JSRuntime *rt, JSContext *cx) +js_ValidContextPointer(JSRuntime *rt, JSContext *cx) { JSCList *cl; @@ -269,18 +269,20 @@ js_LiveContext(JSRuntime *rt, JSContext *cx) } JSContext * -js_ContextIterator(JSRuntime *rt, JSContext **iterp) +js_ContextIterator(JSRuntime *rt, JSBool unlocked, JSContext **iterp) { JSContext *cx = *iterp; - JS_LOCK_RUNTIME(rt); + if (unlocked) + JS_LOCK_GC(rt); if (!cx) cx = (JSContext *)&rt->contextList; cx = (JSContext *)cx->links.next; if (&cx->links == &rt->contextList) cx = NULL; *iterp = cx; - JS_UNLOCK_RUNTIME(rt); + if (unlocked) + JS_UNLOCK_GC(rt); return cx; } diff --git a/mozilla/js/src/jscntxt.h b/mozilla/js/src/jscntxt.h index 03687040d77..ea07ddd95ad 100644 --- a/mozilla/js/src/jscntxt.h +++ b/mozilla/js/src/jscntxt.h @@ -67,6 +67,7 @@ typedef struct JSPropertyTreeEntry { } JSPropertyTreeEntry; struct JSRuntime { + /* Runtime state, synchronized by the stateChange/gcLock condvar/lock. */ JSRuntimeState state; /* Garbage collector state, used by jsgc.c. */ @@ -107,7 +108,7 @@ struct JSRuntime { /* Empty string held for use by this runtime's contexts. */ JSString *emptyString; - /* List of active contexts sharing this runtime. */ + /* List of active contexts sharing this runtime; protected by gcLock. */ JSCList contextList; /* These are used for debugging -- see jsprvtd.h and jsdbgapi.h. */ @@ -157,7 +158,7 @@ struct JSRuntime { jsword rtLockOwner; #endif - /* Used to synchronize down/up state change; uses rtLock. */ + /* Used to synchronize down/up state change; protected by gcLock. */ PRCondVar *stateChange; /* Used to serialize cycle checks when setting __proto__ or __parent__. */ @@ -400,11 +401,19 @@ js_NewContext(JSRuntime *rt, size_t stackChunkSize); extern void js_DestroyContext(JSContext *cx, JSGCMode gcmode); +/* + * Return true if cx points to a context in rt->contextList, else return false. + * NB: the caller (see jslock.c:ClaimScope) must hold rt->gcLock. + */ extern JSBool -js_LiveContext(JSRuntime *rt, JSContext *cx); +js_ValidContextPointer(JSRuntime *rt, JSContext *cx); +/* + * If unlocked, acquire and release rt->gcLock around *iterp update; otherwise + * the caller must be holding rt->gcLock. + */ extern JSContext * -js_ContextIterator(JSRuntime *rt, JSContext **iterp); +js_ContextIterator(JSRuntime *rt, JSBool unlocked, JSContext **iterp); /* * Report an exception, which is currently realized as a printf-style format diff --git a/mozilla/js/src/jsgc.c b/mozilla/js/src/jsgc.c index 92f669e745a..b932116ebd4 100644 --- a/mozilla/js/src/jsgc.c +++ b/mozilla/js/src/jsgc.c @@ -968,15 +968,15 @@ gc_lock_marker(JSDHashTable *table, JSDHashEntryHdr *hdr, uint32 num, void *arg) return JS_DHASH_NEXT; } -JS_FRIEND_API(void) -js_ForceGC(JSContext *cx) +void +js_ForceGC(JSContext *cx, uintN gcflags) { uintN i; for (i = 0; i < GCX_NTYPES; i++) cx->newborn[i] = NULL; cx->runtime->gcPoke = JS_TRUE; - js_GC(cx, 0); + js_GC(cx, gcflags); JS_ArenaFinish(); } @@ -1015,9 +1015,16 @@ js_GC(JSContext *cx, uintN gcflags) JS_ASSERT(!JS_IS_RUNTIME_LOCKED(rt)); #endif - /* Don't run gc if it is disabled (unless this is the last context). */ - if (rt->gcDisabled && !(gcflags & GC_LAST_CONTEXT)) + /* + * Don't collect garbage if the runtime is down or if GC is disabled, and + * we're not the last context in the runtime. The last context must force + * a GC, and nothing should disable that final collection or there may be + * shutdown leaks, or runtime bloat until the next new context is created. + */ + if ((rt->state != JSRTS_UP || rt->gcDisabled) && + !(gcflags & GC_LAST_CONTEXT)) { return; + } /* * Let the API user decide to defer a GC if it wants to (unless this @@ -1065,7 +1072,7 @@ js_GC(JSContext *cx, uintN gcflags) * keep a sub-list of contexts having the same id? */ iter = NULL; - while ((acx = js_ContextIterator(rt, &iter)) != NULL) { + while ((acx = js_ContextIterator(rt, JS_FALSE, &iter)) != NULL) { if (acx->thread == cx->thread && acx->requestDepth) requestDebit++; } @@ -1157,7 +1164,7 @@ restart: JS_DHashTableEnumerate(rt->gcLocksHash, gc_lock_marker, cx); js_MarkAtomState(&rt->atomState, gcflags, gc_mark_atom_key_thing, cx); iter = NULL; - while ((acx = js_ContextIterator(rt, &iter)) != NULL) { + while ((acx = js_ContextIterator(rt, JS_TRUE, &iter)) != NULL) { /* * Iterate frame chain and dormant chains. Temporarily tack current * frame onto the head of the dormant list to ease iteration. diff --git a/mozilla/js/src/jsgc.h b/mozilla/js/src/jsgc.h index 0a0107dbd3e..5d219ba2b62 100644 --- a/mozilla/js/src/jsgc.h +++ b/mozilla/js/src/jsgc.h @@ -167,9 +167,6 @@ struct GCMarkNode { #endif /* !GC_MARK_DEBUG */ -extern JS_FRIEND_API(void) -js_ForceGC(JSContext *cx); - /* * Flags to modify how a GC marks and sweeps: * GC_KEEP_ATOMS Don't sweep unmarked atoms, they may be in use by the @@ -183,6 +180,9 @@ js_ForceGC(JSContext *cx); #define GC_KEEP_ATOMS 0x1 #define GC_LAST_CONTEXT 0x2 +extern void +js_ForceGC(JSContext *cx, uintN gcflags); + extern void js_GC(JSContext *cx, uintN gcflags); diff --git a/mozilla/js/src/jslock.c b/mozilla/js/src/jslock.c index 2752578acb1..1bda7d1c7fa 100644 --- a/mozilla/js/src/jslock.c +++ b/mozilla/js/src/jslock.c @@ -408,7 +408,7 @@ ClaimScope(JSScope *scope, JSContext *cx) * request before waiting on rt->scopeSharingDone). */ if (!scope->u.link && - (!js_LiveContext(rt, ownercx) || + (!js_ValidContextPointer(rt, ownercx) || !ownercx->requestDepth || ownercx->thread == cx->thread)) { JS_ASSERT(scope->u.count == 0); diff --git a/mozilla/js/src/xpconnect/shell/xpcshell.cpp b/mozilla/js/src/xpconnect/shell/xpcshell.cpp index cb3aedec26d..17bbed06753 100644 --- a/mozilla/js/src/xpconnect/shell/xpcshell.cpp +++ b/mozilla/js/src/xpconnect/shell/xpcshell.cpp @@ -310,7 +310,7 @@ GC(JSContext *cx, JSObject *obj, uintN argc, jsval *argv, jsval *rval) js_DumpGCHeap = stdout; } #endif - js_ForceGC(cx); + JS_GC(cx); #ifdef GC_MARK_DEBUG if (js_DumpGCHeap != stdout) fclose(js_DumpGCHeap); @@ -920,7 +920,7 @@ main(int argc, char **argv) PR_TRUE, getter_AddRefs(baz)); bar = nsnull; baz = nsnull; - js_ForceGC(foo); + JS_GC(foo); JS_DestroyContext(foo); #endif @@ -934,12 +934,12 @@ main(int argc, char **argv) #endif JS_ClearScope(jscontext, glob); - js_ForceGC(jscontext); + JS_GC(jscontext); JSContext *oldcx; cxstack->Pop(&oldcx); NS_ASSERTION(oldcx == jscontext, "JS thread context push/pop mismatch"); cxstack = nsnull; - js_ForceGC(jscontext); + JS_GC(jscontext); JS_DestroyContext(jscontext); xpc->SyncJSContexts(); xpc = nsnull; // force nsCOMPtr to Release the service diff --git a/mozilla/js/src/xpconnect/src/nsXPConnect.cpp b/mozilla/js/src/xpconnect/src/nsXPConnect.cpp index 0e4e5a4a32b..51649a7baef 100644 --- a/mozilla/js/src/xpconnect/src/nsXPConnect.cpp +++ b/mozilla/js/src/xpconnect/src/nsXPConnect.cpp @@ -221,7 +221,7 @@ nsXPConnect::ReleaseXPConnectSingleton() { FILE* oldFileHandle = js_DumpGCHeap; js_DumpGCHeap = stdout; - js_ForceGC(ccx); + JS_GC(ccx); js_DumpGCHeap = oldFileHandle; } #endif diff --git a/mozilla/js/src/xpconnect/tests/TestXPC.cpp b/mozilla/js/src/xpconnect/tests/TestXPC.cpp index 7265622f058..b920acde7d5 100644 --- a/mozilla/js/src/xpconnect/tests/TestXPC.cpp +++ b/mozilla/js/src/xpconnect/tests/TestXPC.cpp @@ -51,7 +51,6 @@ #include "nsIVariant.h" #include "jsapi.h" -#include "jsgc.h" // for js_ForceGC #include "xpctest.h" @@ -764,8 +763,8 @@ int main() DIE("FAILED to pop the current jscontext from the nsThreadJSContextStack service!\n"); JS_ClearScope(jscontext, glob); - js_ForceGC(jscontext); - js_ForceGC(jscontext); + JS_GC(jscontext); + JS_GC(jscontext); JS_DestroyContext(jscontext); xpc->SyncJSContexts(); xpc->DebugDump(4); diff --git a/mozilla/js2/tests/cpp/shamu.cpp b/mozilla/js2/tests/cpp/shamu.cpp index 29d17d7f337..7d64fe573ce 100644 --- a/mozilla/js2/tests/cpp/shamu.cpp +++ b/mozilla/js2/tests/cpp/shamu.cpp @@ -2486,12 +2486,6 @@ JS_ClearTrap(JSContext *cx, JSScript *script, jsbytecode *pc, { } -JS_FRIEND_API(void) -js_ForceGC(JSContext *cx) -{ - nyi(); -} - JS_PUBLIC_API(JSBool) JS_SetTrap(JSContext *cx, JSScript *script, jsbytecode *pc, JSTrapHandler handler, void *closure) diff --git a/mozilla/modules/xml/js/test/js.c b/mozilla/modules/xml/js/test/js.c index 01a56b50fea..8a767d475ab 100644 --- a/mozilla/modules/xml/js/test/js.c +++ b/mozilla/modules/xml/js/test/js.c @@ -252,7 +252,7 @@ GC(JSContext *cx, JSObject *obj, uintN argc, jsval *argv, jsval *rval) js_DumpGCHeap = stdout; } #endif - js_ForceGC(cx); + JS_GC(cx); #ifdef GC_MARK_DEBUG if (js_DumpGCHeap != stdout) fclose(js_DumpGCHeap);