From d5f7334631674d78664e7ad6232e3b4b4ad63d87 Mon Sep 17 00:00:00 2001 From: Igor Bukanov Date: Tue, 21 Sep 2010 14:58:19 +0200 Subject: [PATCH] bug 597736 - fixing TreeFragment leak. r=gal --- js/src/jsapi.cpp | 6 -- js/src/jsapi.h | 4 +- js/src/jscntxt.cpp | 3 - js/src/jscntxt.h | 13 ++- js/src/jsgc.cpp | 5 + js/src/jsscope.h | 1 + js/src/jstracer.cpp | 94 ++++++++++++------- js/src/jstracer.h | 3 +- js/src/shell/js.cpp | 2 - .../trace-test/tests/basic/testBug597736.js | 32 +++++++ js/src/xpconnect/src/nsXPConnect.cpp | 11 --- js/src/xpconnect/src/xpcprivate.h | 1 - xpcom/base/nsCycleCollector.cpp | 9 -- xpcom/base/nsCycleCollector.h | 1 - 14 files changed, 110 insertions(+), 75 deletions(-) create mode 100644 js/src/trace-test/tests/basic/testBug597736.js diff --git a/js/src/jsapi.cpp b/js/src/jsapi.cpp index 34f8a6f01f4d..9bd1c3a045ea 100644 --- a/js/src/jsapi.cpp +++ b/js/src/jsapi.cpp @@ -761,12 +761,6 @@ JS_NewRuntime(uint32 maxbytes) return rt; } -JS_PUBLIC_API(void) -JS_CommenceRuntimeShutDown(JSRuntime *rt) -{ - rt->gcFlushCodeCaches = true; -} - JS_PUBLIC_API(void) JS_DestroyRuntime(JSRuntime *rt) { diff --git a/js/src/jsapi.h b/js/src/jsapi.h index a406b86ae6bd..6e4ab1b4ef26 100644 --- a/js/src/jsapi.h +++ b/js/src/jsapi.h @@ -717,8 +717,8 @@ JS_SameValue(JSContext *cx, jsval v1, jsval v2); extern JS_PUBLIC_API(JSRuntime *) JS_NewRuntime(uint32 maxbytes); -extern JS_PUBLIC_API(void) -JS_CommenceRuntimeShutDown(JSRuntime *rt); +/* Deprecated. */ +#define JS_CommenceRuntimeShutDown(rt) ((void) 0) extern JS_PUBLIC_API(void) JS_DestroyRuntime(JSRuntime *rt); diff --git a/js/src/jscntxt.cpp b/js/src/jscntxt.cpp index 63bdfa137a17..9d9f9f9e5df4 100644 --- a/js/src/jscntxt.cpp +++ b/js/src/jscntxt.cpp @@ -529,9 +529,6 @@ void JSThreadData::mark(JSTracer *trc) { stackSpace.mark(trc); -#ifdef JS_TRACER - traceMonitor.mark(trc); -#endif } void diff --git a/js/src/jscntxt.h b/js/src/jscntxt.h index 861f02e2242b..fa745a547878 100644 --- a/js/src/jscntxt.h +++ b/js/src/jscntxt.h @@ -1031,11 +1031,15 @@ struct TraceMonitor { FragStatsMap* profTab; #endif + bool ontrace() const { + return !!tracecx; + } + /* Flush the JIT cache. */ void flush(); - /* Mark all objects baked into native code in the code cache. */ - void mark(JSTracer *trc); + /* Sweep any cache entry pointing to dead GC things. */ + void sweep(); bool outOfMemory() const; }; @@ -1048,9 +1052,9 @@ struct TraceMonitor { * executing. cx must be a context on the current thread. */ #ifdef JS_TRACER -# define JS_ON_TRACE(cx) (JS_TRACE_MONITOR(cx).tracecx != NULL) +# define JS_ON_TRACE(cx) (JS_TRACE_MONITOR(cx).ontrace()) #else -# define JS_ON_TRACE(cx) JS_FALSE +# define JS_ON_TRACE(cx) false #endif /* Number of potentially reusable scriptsToGC to search for the eval cache. */ @@ -1343,7 +1347,6 @@ struct JSRuntime { uint32 gcTriggerFactor; size_t gcTriggerBytes; volatile JSBool gcIsNeeded; - volatile JSBool gcFlushCodeCaches; /* * NB: do not pack another flag here by claiming gcPadding unless the new diff --git a/js/src/jsgc.cpp b/js/src/jsgc.cpp index 29906532507f..a04a477de605 100644 --- a/js/src/jsgc.cpp +++ b/js/src/jsgc.cpp @@ -2332,6 +2332,11 @@ MarkAndSweep(JSContext *cx, JSGCInvocationKind gckind GCTIMER_PARAM) rt->liveObjectPropsPreSweep = rt->liveObjectProps; #endif +#ifdef JS_TRACER + for (ThreadDataIter i(rt); !i.empty(); i.popFront()) + i.threadData()->traceMonitor.sweep(); +#endif + #ifdef JS_METHODJIT /* Fix-up call ICs guarding against unreachable objects. */ mjit::SweepCallICs(cx); diff --git a/js/src/jsscope.h b/js/src/jsscope.h index 8604ef073476..2adf18bc3ca0 100644 --- a/js/src/jsscope.h +++ b/js/src/jsscope.h @@ -291,6 +291,7 @@ struct Shape : public JSObjectMap friend struct ::JSObject; friend struct ::JSFunction; friend class js::PropertyTree; + friend bool HasUnreachableGCThings(TreeFragment *f); protected: mutable js::PropertyTable *table; diff --git a/js/src/jstracer.cpp b/js/src/jstracer.cpp index ecd7da8eac2e..4be3b77bddc0 100644 --- a/js/src/jstracer.cpp +++ b/js/src/jstracer.cpp @@ -2298,7 +2298,7 @@ FlushJITCache(JSContext *cx) } static void -TrashTree(JSContext* cx, TreeFragment* f); +TrashTree(TreeFragment* f); template static T& @@ -2537,10 +2537,10 @@ TraceRecorder::~TraceRecorder() JS_ASSERT(traceMonitor->recorder != this); if (trashSelf) - TrashTree(cx, fragment->root); + TrashTree(fragment->root); for (unsigned int i = 0; i < whichTreesToTrash.length(); i++) - TrashTree(cx, whichTreesToTrash[i]); + TrashTree(whichTreesToTrash[i]); /* Purge the tempAlloc used during recording. */ tempAlloc().reset(); @@ -2615,7 +2615,7 @@ TraceRecorder::finishAbort(const char* reason) * Otherwise, we may be throwing away another recorder's valid side exits. */ if (fragment->root == fragment) { - TrashTree(cx, fragment->toTreeFragment()); + TrashTree(fragment->toTreeFragment()); } else { JS_ASSERT(numSideExitsBefore <= fragment->root->sideExits.length()); fragment->root->sideExits.setLength(numSideExitsBefore); @@ -2921,46 +2921,67 @@ TraceMonitor::flush() needFlush = JS_FALSE; } -static inline void -MarkTree(JSTracer* trc, TreeFragment *f) +inline bool +HasUnreachableGCThings(TreeFragment *f) { + /* + * We do not check here for dead scripts as JSScript is not a GC thing. + * Instead PurgeScriptFragments is used to remove dead script fragments. + * See bug 584860. + */ + if (IsAboutToBeFinalized(f->globalObj)) + return true; Value* vp = f->gcthings.data(); - unsigned len = f->gcthings.length(); - while (len--) { + for (unsigned len = f->gcthings.length(); len; --len) { Value &v = *vp++; - JS_SET_TRACING_NAME(trc, "jitgcthing"); JS_ASSERT(v.isMarkable()); - MarkGCThing(trc, v.toGCThing(), v.gcKind()); + if (IsAboutToBeFinalized(v.toGCThing())) + return true; } const Shape** shapep = f->shapes.data(); - len = f->shapes.length(); - while (len--) { + for (unsigned len = f->shapes.length(); len; --len) { const Shape* shape = *shapep++; - shape->trace(trc); + if (!shape->marked()) + return true; } + return false; } void -TraceMonitor::mark(JSTracer* trc) +TraceMonitor::sweep() { - if (!trc->context->runtime->gcFlushCodeCaches) { - for (size_t i = 0; i < FRAGMENT_TABLE_SIZE; ++i) { - TreeFragment* f = vmfragments[i]; - while (f) { - if (f->code()) - MarkTree(trc, f); - TreeFragment* peer = f->peer; - while (peer) { - if (peer->code()) - MarkTree(trc, peer); - peer = peer->peer; - } - f = f->next; + JS_ASSERT(!ontrace()); + debug_only_print0(LC_TMTracer, "Purging fragments with dead things"); + + for (size_t i = 0; i < FRAGMENT_TABLE_SIZE; ++i) { + TreeFragment** fragp = &vmfragments[i]; + while (TreeFragment* frag = *fragp) { + TreeFragment* peer = frag; + do { + if (HasUnreachableGCThings(peer)) + break; + peer = peer->peer; + } while (peer); + if (peer) { + debug_only_printf(LC_TMTracer, + "TreeFragment peer %p has dead gc thing." + "Disconnecting tree %p with ip %p\n", + (void *) peer, (void *) frag, frag->ip); + JS_ASSERT(frag->root == frag); + *fragp = frag->next; + do { + verbose_only( FragProfiling_FragFinalizer(frag, this); ) + TrashTree(frag); + frag = frag->peer; + } while (frag); + } else { + fragp = &frag->next; } } - if (recorder) - MarkTree(trc, recorder->getTree()); } + + if (recorder && HasUnreachableGCThings(recorder->getTree())) + recorder->finishAbort("dead GC things"); } /* @@ -5672,7 +5693,7 @@ TraceRecorder::startRecorder(JSContext* cx, VMSideExit* anchor, VMFragment* f, } static void -TrashTree(JSContext* cx, TreeFragment* f) +TrashTree(TreeFragment* f) { JS_ASSERT(f == f->root); debug_only_printf(LC_TMTreeVis, "TREEVIS TRASH FRAG=%p\n", (void*)f); @@ -5685,11 +5706,11 @@ TrashTree(JSContext* cx, TreeFragment* f) TreeFragment** data = f->dependentTrees.data(); unsigned length = f->dependentTrees.length(); for (unsigned n = 0; n < length; ++n) - TrashTree(cx, data[n]); + TrashTree(data[n]); data = f->linkedTrees.data(); length = f->linkedTrees.length(); for (unsigned n = 0; n < length; ++n) - TrashTree(cx, data[n]); + TrashTree(data[n]); } static void @@ -5901,7 +5922,7 @@ AttemptToStabilizeTree(JSContext* cx, JSObject* globalObj, VMSideExit* exit, jsb return false; } else if (consensus == TypeConsensus_Undemotes) { /* The original tree is unconnectable, so trash it. */ - TrashTree(cx, peer); + TrashTree(peer); return false; } @@ -7813,6 +7834,11 @@ PurgeScriptFragments(JSContext* cx, JSScript* script) "Purging fragments for JSScript %p.\n", (void*)script); TraceMonitor* tm = &JS_TRACE_MONITOR(cx); + + /* A recorder script is being evaluated and can not be destroyed or GC-ed. */ + JS_ASSERT_IF(tm->recorder, + JS_UPTRDIFF(tm->recorder->getTree()->ip, script->code) >= script->length); + for (size_t i = 0; i < FRAGMENT_TABLE_SIZE; ++i) { TreeFragment** fragp = &tm->vmfragments[i]; while (TreeFragment* frag = *fragp) { @@ -7828,7 +7854,7 @@ PurgeScriptFragments(JSContext* cx, JSScript* script) *fragp = frag->next; do { verbose_only( FragProfiling_FragFinalizer(frag, tm); ) - TrashTree(cx, frag); + TrashTree(frag); } while ((frag = frag->peer) != NULL); continue; } diff --git a/js/src/jstracer.h b/js/src/jstracer.h index a41667c8e19a..4bee97e32e85 100644 --- a/js/src/jstracer.h +++ b/js/src/jstracer.h @@ -1424,8 +1424,9 @@ class TraceRecorder bool &blacklist); friend void AbortRecording(JSContext*, const char*); friend class BoxArg; + friend void TraceMonitor::sweep(); -public: + public: static bool JS_REQUIRES_STACK startRecorder(JSContext*, VMSideExit*, VMFragment*, unsigned stackSlots, unsigned ngslots, JSValueType* typeMap, diff --git a/js/src/shell/js.cpp b/js/src/shell/js.cpp index 409e8c8af4ea..3625d305a37d 100644 --- a/js/src/shell/js.cpp +++ b/js/src/shell/js.cpp @@ -5348,8 +5348,6 @@ main(int argc, char **argv, char **envp) result = shell(cx, argc, argv, envp); - JS_CommenceRuntimeShutDown(rt); - DestroyContext(cx, true); KillWatchdog(); diff --git a/js/src/trace-test/tests/basic/testBug597736.js b/js/src/trace-test/tests/basic/testBug597736.js new file mode 100644 index 000000000000..feeadcecdab3 --- /dev/null +++ b/js/src/trace-test/tests/basic/testBug597736.js @@ -0,0 +1,32 @@ +function leak_test() { + // Create a reference loop function->script->traceFragment->object->function + // that GC must be able to break. To embedd object into the fragment the + // code use prototype chain of depth 2 which caches obj.__proto__.__proto__ + // into the fragment. + + // To make sure that we have no references to the function f after this + // function returns due via the conservative scan of the native stack we + // loop here twice overwriting the stack and registers with new garabge. + for (var j = 0; j != 2; ++j) { + var f = Function("a", "var s = 0; for (var i = 0; i != 100; ++i) s += a.b; return s;"); + var c = {b: 1, f: f, leakDetection: makeFinalizeObserver()}; + f({ __proto__: { __proto__: c}}); + f = c = a = null; + gc(); + } +} + +function test() +{ + if (typeof finalizeCount != "function") + return; + + var base = finalizeCount(); + leak_test(); + gc(); + gc(); + var n = finalizeCount(); + assertEq(base < finalizeCount(), true, "Some finalizations must happen"); +} + +test(); diff --git a/js/src/xpconnect/src/nsXPConnect.cpp b/js/src/xpconnect/src/nsXPConnect.cpp index 11b498ec992e..3dd8175d5e93 100644 --- a/js/src/xpconnect/src/nsXPConnect.cpp +++ b/js/src/xpconnect/src/nsXPConnect.cpp @@ -512,17 +512,6 @@ nsXPConnect::ToParticipant(void *p) return this; } -void -nsXPConnect::CommenceShutdown() -{ -#ifdef DEBUG - fprintf(stderr, "nsXPConnect::CommenceShutdown()\n"); -#endif - // Tell the JS engine that we are about to destroy the runtime. - JSRuntime* rt = mRuntime->GetJSRuntime(); - JS_CommenceRuntimeShutDown(rt); -} - NS_IMETHODIMP nsXPConnect::RootAndUnlinkJSObjects(void *p) { diff --git a/js/src/xpconnect/src/xpcprivate.h b/js/src/xpconnect/src/xpcprivate.h index ef55a112bceb..ce674d13e209 100644 --- a/js/src/xpconnect/src/xpcprivate.h +++ b/js/src/xpconnect/src/xpcprivate.h @@ -496,7 +496,6 @@ public: bool explainExpectedLiveGarbage); virtual nsresult FinishCycleCollection(); virtual nsCycleCollectionParticipant *ToParticipant(void *p); - virtual void CommenceShutdown(); virtual void Collect(); #ifdef DEBUG_CC virtual void PrintAllReferencesTo(void *p); diff --git a/xpcom/base/nsCycleCollector.cpp b/xpcom/base/nsCycleCollector.cpp index aa3fe1551af5..d7f8afdab00d 100644 --- a/xpcom/base/nsCycleCollector.cpp +++ b/xpcom/base/nsCycleCollector.cpp @@ -941,10 +941,6 @@ struct nsCycleCollectionXPCOMRuntime : inline nsCycleCollectionParticipant *ToParticipant(void *p); - void CommenceShutdown() - { - } - #ifdef DEBUG_CC virtual void PrintAllReferencesTo(void *p) {} #endif @@ -2727,11 +2723,6 @@ nsCycleCollector::Shutdown() // Here we want to run a final collection and then permanently // disable the collector because the program is shutting down. - for (PRUint32 i = 0; i <= nsIProgrammingLanguage::MAX; ++i) { - if (mRuntimes[i]) - mRuntimes[i]->CommenceShutdown(); - } - Collect(SHUTDOWN_COLLECTIONS(mParams), nsnull); #ifdef DEBUG_CC diff --git a/xpcom/base/nsCycleCollector.h b/xpcom/base/nsCycleCollector.h index 099f6eb8917f..0ce6665077d4 100644 --- a/xpcom/base/nsCycleCollector.h +++ b/xpcom/base/nsCycleCollector.h @@ -56,7 +56,6 @@ struct nsCycleCollectionLanguageRuntime bool explainLiveExpectedGarbage) = 0; virtual nsresult FinishCycleCollection() = 0; virtual nsCycleCollectionParticipant *ToParticipant(void *p) = 0; - virtual void CommenceShutdown() = 0; #ifdef DEBUG_CC virtual void PrintAllReferencesTo(void *p) = 0; #endif