Closed Bug 499570 Opened 15 years ago Closed 15 years ago

Leak JSNativeEnumerator [@ js_Enumerate] with "Iterator({x:1});"

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: jruderman, Assigned: igor)

Details

(Keywords: memory-leak, testcase, valgrind, Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file, 1 obsolete file)

rtleak.js:
    Iterator({x:1});

valgrind 
    --num-callers=40 
    --auto-run-dsymutil=yes 
    --leak-check=full 
    ~/tracemonkey/js/src/debug/js
    rtleak.js

20 bytes in 1 blocks are definitely lost in loss record 29 of 107
   at 0x271516: malloc (vg_replace_malloc.c:193)
   by 0x222B6: JS_malloc (jsapi.cpp:1839)
   by 0xB3F4F: js_Enumerate (jsobj.cpp:4938)
   by 0x9E040: InitNativeIterator(JSContext*, JSObject*, JSObject*, unsigned int) (jsiter.cpp:150)
   by 0x9E8CE: js_ValueToIterator (jsiter.cpp:398)
   by 0x9EAD5: Iterator(JSContext*, JSObject*, unsigned int, long*, long*) (jsiter.cpp:192)
   by 0x9C8E5: js_Invoke (jsinterp.cpp:1380)
   by 0x86F37: js_Interpret (jsinterp.cpp:5209)
   by 0x9B29E: js_Execute (jsinterp.cpp:1629)
   by 0x1E51B: JS_ExecuteScript (jsapi.cpp:5046)
   by 0x8474: Process(JSContext*, JSObject*, char*, int) (shell/js.cpp:412)
   by 0x9CA9: ProcessArgs(JSContext*, JSObject*, char**, int) (shell/js.cpp:820)
   by 0xB153: main (shell/js.cpp:4765)

To install Valgrind on Mac: http://blog.mozilla.com/nnethercote/2009/05/28/mac-os-x-now-supported-on-the-valgrind-trunk/
Igor, must we mark rt->nativeEnumerators on GC_LAST_CONTEXT? Seems like we should skip it in that case.

/be
Assignee: general → igor
(In reply to comment #1)
> Igor, must we mark rt->nativeEnumerators on GC_LAST_CONTEXT? 

The bug is that finalizing of the native iterators requires an extra GC cycle. The first cycle during the sweep phase calls js_CloseNativeIterator. But that does not remove the corresponding JSNativeEnumerator from rt->nativeEnumerators. That would happen during a later cycle when the GC calls and the function js_TraceNativeEnumerators detects closed iterator on the list and removes it.

A simple way to fix this is to set rt->gcPoke in js_CloseNativeIterator during GC_LAST_CONTEXT forcing the necessary second cycle.
Attached patch v1 (obsolete) — Splinter Review
The fix sets gcPoke in js_CloseNativeIterator. The patch sets gcPoke only on shutdown to prevent extra GC cycles due to not-explicitly closed iterators. 

An alternative to this is to split js_TraceNativeEnumerators into two functions, one to mark not-yet closed JSNativeEnumerator instances, and another to destroy the closed ones. This way JSNativeEnumerator instances can be destroyed after the iterators is finalized. I will try this in another patch to judge the code complexity.
Attachment #384440 - Attachment is patch: true
Attachment #384440 - Attachment mime type: application/octet-stream → text/plain
(In reply to comment #3)
> An alternative to this is to split js_TraceNativeEnumerators into two
> functions, one to mark not-yet closed JSNativeEnumerator instances, and another
> to destroy the closed ones. This way JSNativeEnumerator instances can be
> destroyed after the iterators is finalized. I will try this in another patch to
> judge the code complexity.

That does not work since the above split would still require a separated GC cycle to collect the ids of the enumerators that corresponds to unreachable, but not-yet-closed, iterator objects.
Attachment #384440 - Attachment description: v1 (with poke) → v1
Attachment #384440 - Flags: review+
Attachment #384440 - Flags: review+ → review?(brendan)
Attachment #384440 - Flags: review?(brendan) → review+
Comment on attachment 384440 [details] [diff] [review]
v1

>+             * Force on shutdown an extra GC cycle so all native enumerators
>+             * on the rt->nativeEnumerators list will be removed when the GC
>+             * calls js_TraceNativeEnumerators from jsobj.cpp. See bug 499570.

Lose the "from jsobj.cpp", it's not needed, and r=me.

/be
Attached patch v2Splinter Review
The new version moves setting of gcPoke to the JSENUMERATE_DESTROY case of js_Enumerate. This makes sure that a enumerator created for any reason, not only as a part of the iterator implementation, and destroyed during the finalization phase does not leak on shutdown the JSNativeEnumerator instance or property ids the instance roots.
Attachment #384440 - Attachment is obsolete: true
Attachment #384600 - Flags: review?(brendan)
Comment on attachment 384600 [details] [diff] [review]
v2

Good catch, should have seen that.

/be
Attachment #384600 - Flags: review?(brendan) → review+
http://hg.mozilla.org/tracemonkey/rev/9ff7c152d17a
Whiteboard: fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/9ff7c152d17a
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

Creator:
Created:
Updated:
Size: