Running Valgrind like this on 'js' on my Linux box: valgrind --smc-check=all --leak-check=full js-debug/js -j -e 'gSkipSlowTests=true' trace-test.js I get this: ==29456== 164 bytes in 1 blocks are definitely lost in loss record 1 of 1 ==29456== at 0x4AF0F3E: calloc (vg_replace_malloc.c:414) ==29456== by 0x813B1DA: avmplus::GCObject::operator new(unsigned int, avmplus::GC*) (avmplus.h:166) ==29456== by 0x8198F89: getAnchor(JSTraceMonitor*, void const*, JSObject*, unsigned int, unsigned int) (jstracer.cpp:644) ==29456== by 0x81A45D2: js_MonitorLoopEdge(JSContext*, unsigned int&) (jstracer.cpp:4824) ==29456== by 0x80BBC02: js_Interpret (jsinterp.cpp:3875) ==29456== by 0x80DE9CD: js_Execute (jsinterp.cpp:1622) ==29456== by 0x8055931: JS_ExecuteScript (jsapi.cpp:5036) ==29456== by 0x80517DB: Process(JSContext*, JSObject*, char*, int) (js.cpp:407) ==29456== by 0x80524FD: ProcessArgs(JSContext*, JSObject*, char**, int) (js.cpp:767) ==29456== by 0x80528C4: main (js.cpp:4696) It occurs both on tracemonkey/ and mozilla-1.9.1/, and so might be worth consideration as a 1.9.1 blocker. I haven't checked if it occurs with a full Firefox build. Graydon volunteered to look at it.
Yeah, this is actually bad. Unfortunately it seems bug 496391 was mis-fixed and it winds up leaking every VMFragment by prematurely disconnecting them on the first (trashing, not deleting) pass over the VMFragments hashtable. And I reviewed that! Ach. Anyway, patch forthcoming in a few minutes. Meanwhile I'll nominate this to block, unfortunately. It's a guaranteed and reasonably rapid leak. It should be plugged.
Created attachment 384500 [details] [diff] [review] Fix the bug I'm not sure using 'bool' to indicate 'yeah, go ahead and disconnect the fragment' is the most obvious interface, but it's easy and works. Valgrind reports no leaks with this patch. Running through try servers now.
Are we sure this blocks? FWIW, this leak has not popped up in the membuster testing we've been running (loads webpages in 30 tabs, cycles through them, closes all tabs).
> I'm not sure using 'bool' to indicate 'yeah, go ahead and disconnect the > fragment' is the most obvious interface, but it's easy and works. Valgrind > reports no leaks with this patch. Running through try servers now. A possibly more explicit alternative would be to add a boolean template parameter.
(In reply to comment #3) > Are we sure this blocks? FWIW, this leak has not popped up in the membuster > testing we've been running (loads webpages in 30 tabs, cycles through them, > closes all tabs). Ok, 'rapid' in a manner of speaking: it leaks a single 164-byte structure for every loop that was traced and executed, when the originating script unloads. So this could be as few as 0 per page or as many as hundreds, but it's unlikely to be noticed at broad granularity. A few kb per page?
1.9.1 tree appears closed at the moment, but I'm happy to land this there whenever that changes.
What is the regression-risk of this patch? Is the code path localized (as in, all that could happen is it doesn't actually stop the leak, or expose another) or would we need to re-run our entire beta/QA period again to make sure we didn't alter other things?
Graydon and I talked on IRC, a191=beltzner, please land using "CLOSED TREE" in your commit comment to get past the sentry.
The code path is pretty localized. The leaked objects are headers for subgraphs of the JIT cache that (a) are already nulled-out and cannot be used if found, and (b) are already disconnected from the object graph anyways, so there should be no way of even noticing the leak short of an allocation-tracker like valgrind. There should be no other observable affect. The only caveat I'd provide is the word "should". Because, you know, it's software. And because we were sure we got this path right .. last time :(
Please remember to get this fixed on trunk, as well.
(In reply to comment #12) > Please remember to get this fixed on trunk, as well. Landed in tracemonkey first; it'll flow to trunk on the next merge.
there was a merge recently; did this flow across?
I'm not seeing it in the m-c logs yet, no.
Mass change: adding fixed1.9.2 keyword (This bug was identified as a mozilla1.9.2 blocker which was fixed before the mozilla-1.9.2 repository was branched (August 13th, 2009) as per this query: http://is.gd/2ydcb - if this bug is not actually fixed on mozilla1.9.2, please remove the keyword. Apologies for the bugspam)