Closed
Bug 514647
Opened 15 years ago
Closed 15 years ago
multiple space leaks in jsshell TM 32138:66235f4c4eea
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
status1.9.2 | --- | beta1-fixed |
People
(Reporter: jseward, Assigned: jorendorff)
Details
(Whiteboard: fixed-in-tracemonkey)
Attachments
(1 file)
516 bytes,
patch
|
gal
:
review+
|
Details | Diff | Splinter Review |
For a release build of tracemonkey:32138:66235f4c4eea, there are various spaceleaks reported, and certainly we no longer have the desirable property that all allocated memory is freed. This is using the old monolithic trace-test.js since the new split-up version is much more problematic to valgrindise. valgrind-3.5.0 --smc-check=all --leak-check=full \ ./D32138/js/src/BuildR/js -j -e 'gSkipSlowTests=true' \ ./trace-test.js produces total heap usage: 177,184 allocs, 174,901 frees, 145,559,854 bytes allocated in use at exit: 163,963 bytes in 2,283 blocks Note: Memcheck prints the leaks in order of smallest-first, so scroll to the bottom to see the worst offenders. A quick check shows the following offenders (this is not comprehensive) which kind-of look like they have different underlying causes: 66,015 (65,787 direct, 228 indirect) bytes in 299 blocks are definitely lost in loss record 218 of 218 at 0x4C9EC1C: malloc (vg_replace_malloc.c:195) by 0x80F975F: js_NewScript (jsutil.h:186) by 0x80FAA9B: js_NewScriptFromCG (jsscript.cpp:1483) by 0x80895AB: js_EmitFunctionScript (jsemit.cpp:3499) by 0x808911A: js_EmitTree (jsemit.cpp:4293) by 0x80D4EE6: JSCompiler::compileScript(JSContext*, JSObject*, JSSta by 0x8051E49: JS_CompileFileHandleForPrincipals (jsapi.cpp:4718) by 0x8051EC9: JS_CompileFileHandle (jsapi.cpp:4704) by 0x804D512: Process(JSContext*, JSObject*, char*, int) (js.cpp:431) by 0x804DE64: main (js.cpp:843) 20,470 bytes in 427 blocks are definitely lost in loss record 217 of 218 at 0x4C9EC1C: malloc (vg_replace_malloc.c:195) by 0x810022F: js_ConcatStrings (jsutil.h:186) by 0x816E82D: js_Interpret (jsops.cpp:986) by 0x809A180: js_Execute (jsinterp.cpp:1622) by 0x8052039: JS_ExecuteScript (jsapi.cpp:4954) by 0x804C569: Load(JSContext*, JSObject*, unsigned int, int*, int*) (js.cpp:934) by 0x809AB2F: js_Invoke (jsinterp.cpp:1381) by 0x8168398: js_Interpret (jsops.cpp:2269) by 0x809A180: js_Execute (jsinterp.cpp:1622) by 0x8052039: JS_ExecuteScript (jsapi.cpp:4954) by 0x804D618: Process(JSContext*, JSObject*, char*, int) (js.cpp:435) by 0x804DE64: main (js.cpp:843) 10,164 bytes in 231 blocks are definitely lost in loss record 215 of 218 at 0x4C9EC1C: malloc (vg_replace_malloc.c:195) by 0x80F666F: JSScope::create(JSContext*, JSObjectOps*, JSClass*, JSO by 0x80F7399: js_GetMutableScope (jsscope.cpp:102) by 0x80A762E: js_SetPropertyHelper (jsobj.cpp:4567) by 0x8166F76: js_Interpret (jsops.cpp:1883) by 0x809A180: js_Execute (jsinterp.cpp:1622) by 0x8052039: JS_ExecuteScript (jsapi.cpp:4954) by 0x804D618: Process(JSContext*, JSObject*, char*, int) (js.cpp:435) by 0x804DE64: main (js.cpp:843) 8,192 bytes in 1 blocks are definitely lost in loss record 214 of 218 at 0x4C9ED22: realloc (vg_replace_malloc.c:476) by 0x805B058: ResizeSlots(JSContext*, JSObject*, unsigned int, unsign by 0x805C708: array_push1_dense(JSContext*, JSObject*, int, int*) (js by 0x805FC55: array_push(JSContext*, unsigned int, int*) (jsarray.cpp by 0x816D2EE: js_Interpret (jsops.cpp:2237) by 0x809A180: js_Execute (jsinterp.cpp:1622) by 0x8052039: JS_ExecuteScript (jsapi.cpp:4954) by 0x804C569: Load(JSContext*, JSObject*, unsigned int, int*, int*) by 0x809AB2F: js_Invoke (jsinterp.cpp:1381) by 0x8168398: js_Interpret (jsops.cpp:2269) by 0x809A180: js_Execute (jsinterp.cpp:1622) by 0x8052039: JS_ExecuteScript (jsapi.cpp:4954)
Comment 1•15 years ago
|
||
ftr, we have a zero tolerance leak policy. I thought these had been fixed when you mentioned them the other day.
Comment 2•15 years ago
|
||
Something is not getting GC'ed -- that could explain all the results. Can the test(s) required to reproduced be minimized/isolated? Hope it's a simple binary search and not a ordered subset finding problem. /be
Reporter | ||
Comment 3•15 years ago
|
||
Am doing automated bisection-y stuff now.
Reporter | ||
Comment 4•15 years ago
|
||
There's a large change in behaviour at r31950 in the TM repo: r31000 ==32708== in use at exit: 0 bytes in 0 blocks r31500 ==3201== in use at exit: 0 bytes in 0 blocks r31750 ==6181== in use at exit: 0 bytes in 0 blocks r31875 ==9070== in use at exit: 0 bytes in 0 blocks r31937 ==11894== in use at exit: 0 bytes in 0 blocks r31943 ==17673== in use at exit: 0 bytes in 0 blocks r31946 ==20538== in use at exit: 0 bytes in 0 blocks r31948 ==23373== in use at exit: 0 bytes in 0 blocks r31949 ==26190== in use at exit: 0 bytes in 0 blocks r31950 ==29102== in use at exit: 163,870 bytes in 2,281 blocks r32000 ==29841== in use at exit: 163,870 bytes in 2,281 blocks That's not the only leaker, but it's the biggest by far. I see the following log, but making sense of this exceeds my limited hg-foo to make sense of. Did we inherit something bad from some other branch at 31950? changeset: 31952:2f27256aa154 user: David Zbarsky <dzbarsky@gmail.com> date: Tue Aug 25 13:15:55 2009 -0400 summary: Bug 487023. Refactor IsCaseSensitive() into non-virtual inline IsHTML(). r=hsivonen, sr=jst changeset: 31951:bf78b6bb0b03 user: John Daggett <jdaggett@mozilla.com> date: Wed Aug 26 01:59:20 2009 +0900 summary: Bug 499621. Follow-up fixup to synthetic bolding on Windows. r=jkew. changeset: 31950:189759c41621 parent: 31949:a0b2a2ffa124 parent: 31808:1c95347e920c user: Robert Sayre <sayrer@gmail.com> date: Tue Aug 25 09:52:56 2009 -0700 summary: Merge tracemonkey to mozilla-central. changeset: 31949:a0b2a2ffa124 user: Benjamin Smedberg <benjamin@smedbergs.us> date: Tue Aug 25 08:59:31 2009 -0700 summary: Followup to bug 398573 - remove REQUIRES from the tree since it is no longer used... automatically generated patch, rs=ted changeset: 31948:4152186f5fda user: L. David Baron <dbaron@dbaron.org> date: Tue Aug 25 08:30:16 2009 -0700 summary: Fix test from bug 501569 to avoid triggering the tinderbox error parser.
Reporter | ||
Comment 5•15 years ago
|
||
Chased this into mozilla-central: r31808 ==4567== in use at exit: 0 bytes in 0 blocks r31814 ==14273== in use at exit: 0 bytes in 0 blocks r31878 ==17216== in use at exit: 0 bytes in 0 blocks r31882 ==29410== in use at exit: 0 bytes in 0 blocks r31884 ==32359== in use at exit: 0 bytes in 0 blocks r31885 ==2882== in use at exit: 163,870 bytes in 2,281 blocks r31886 ==26475== in use at exit: 163,870 bytes in 2,281 blocks r31894 ==23409== in use at exit: 163,870 bytes in 2,281 blocks r31910 ==20146== in use at exit: 163,870 bytes in 2,281 blocks r31942 ==11342== in use at exit: 163,870 bytes in 2,281 blocks r31943 ==8347== in use at exit: 163,870 bytes in 2,281 blocks So it's 31885: changeset: 31885:0ba3eb119a53 user: Andreas Gal <gal@mozilla.com> date: Tue Aug 18 16:31:20 2009 -0700 summary: Add an API to notify the JS engine that we are about to destroy the runtime (511252, r=brendan). See in particular https://bugzilla.mozilla.org/show_bug.cgi?id=511252#c8
Reporter | ||
Comment 6•15 years ago
|
||
Reporter | ||
Updated•15 years ago
|
Attachment #399366 -
Flags: review?(gal)
Comment 7•15 years ago
|
||
Comment on attachment 399366 [details] [diff] [review] seems to fix it; causes no (new) trace-tests failures ++jseward. Thanks a ton for looking into this.
Attachment #399366 -
Flags: review?(gal) → review+
Comment 8•15 years ago
|
||
Needed in xpcshell.cpp too, right? /be
Reporter | ||
Comment 9•15 years ago
|
||
(In reply to comment #8) > Needed in xpcshell.cpp too, right? A good question; however I believe that Andreas already did that; see https://bugzilla.mozilla.org/show_bug.cgi?id=511252#c6 for what was landed. Specifically: 5.1 --- a/js/src/xpconnect/src/nsXPConnect.cpp Fri Aug 14 16:10:58 2009 -0700 5.2 +++ b/js/src/xpconnect/src/nsXPConnect.cpp Tue Aug 18 16:31:20 2009 -0700 5.3 @@ -132,12 +132,15 @@ nsXPConnect::~nsXPConnect() 5.4 5.5 JSContext *cx = nsnull; 5.6 if (mRuntime) { 5.7 + // Tell the JS engine that we are about to destroy the runtime. 5.8 + JSRuntime *rt = mRuntime->GetJSRuntime(); 5.9 + JS_CommenceRuntimeShutDown(rt); 5.10 // Create our own JSContext rather than an XPCCallContext, since 5.11 // otherwise we will create a new safe JS context and attach a 5.12 // components object that won't get GCed. 5.13 // And do this before calling CleanupAllThreads, so that we 5.14 // don't create an extra xpcPerThreadData. 5.15 - cx = JS_NewContext(mRuntime->GetJSRuntime(), 8192); 5.16 + cx = JS_NewContext(rt, 8192); 5.17 } 5.18 Also, studying the leak numbers for some xpcshell runs, I cannot see any leaks that look like the jsshell ones that the patch fixes. So I think it's ok as-is. (famous last words ..)
Reporter | ||
Updated•15 years ago
|
Whiteboard: needs-checkin
Comment 10•15 years ago
|
||
http://hg.mozilla.org/tracemonkey/rev/1f9be3bbf522
Whiteboard: needs-checkin → fixed-in-tracemonkey
Comment 11•15 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/1f9be3bbf522
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Comment 12•15 years ago
|
||
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/0d8436480e7f
status1.9.2:
--- → beta1-fixed
Flags: wanted1.9.2+
You need to log in
before you can comment on or make changes to this bug.
Description
•