Closed Bug 514647 Opened 15 years ago Closed 15 years ago

multiple space leaks in jsshell TM 32138:66235f4c4eea

Categories

(Core :: JavaScript Engine, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
status1.9.2 --- beta1-fixed

People

(Reporter: jseward, Assigned: jorendorff)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file)

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)
ftr, we have a zero tolerance leak policy. I thought these had been fixed when you mentioned them the other day.
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
Am doing automated bisection-y stuff now.
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.
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
Attachment #399366 - Flags: review?(gal)
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+
Needed in xpcshell.cpp too, right?

/be
(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 ..)
Whiteboard: needs-checkin
http://hg.mozilla.org/tracemonkey/rev/1f9be3bbf522
Whiteboard: needs-checkin → fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/1f9be3bbf522
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: