Closed
Bug 499570
Opened 15 years ago
Closed 15 years ago
Leak JSNativeEnumerator [@ js_Enumerate] with "Iterator({x:1});"
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: jruderman, Assigned: igor)
Details
(Keywords: memory-leak, testcase, valgrind, Whiteboard: fixed-in-tracemonkey)
Attachments
(1 file, 1 obsolete file)
2.00 KB,
patch
|
brendan
:
review+
|
Details | Diff | Splinter Review |
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/
Comment 1•15 years ago
|
||
Igor, must we mark rt->nativeEnumerators on GC_LAST_CONTEXT? Seems like we should skip it in that case. /be
Assignee: general → igor
Assignee | ||
Comment 2•15 years ago
|
||
(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.
Assignee | ||
Comment 3•15 years ago
|
||
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.
Assignee | ||
Updated•15 years ago
|
Attachment #384440 -
Attachment is patch: true
Attachment #384440 -
Attachment mime type: application/octet-stream → text/plain
Assignee | ||
Comment 4•15 years ago
|
||
(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.
Assignee | ||
Updated•15 years ago
|
Attachment #384440 -
Attachment description: v1 (with poke) → v1
Attachment #384440 -
Flags: review+
Assignee | ||
Updated•15 years ago
|
Attachment #384440 -
Flags: review+ → review?(brendan)
Updated•15 years ago
|
Attachment #384440 -
Flags: review?(brendan) → review+
Comment 5•15 years ago
|
||
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
Assignee | ||
Comment 6•15 years ago
|
||
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 7•15 years ago
|
||
Comment on attachment 384600 [details] [diff] [review] v2 Good catch, should have seen that. /be
Attachment #384600 -
Flags: review?(brendan) → review+
Assignee | ||
Comment 8•15 years ago
|
||
http://hg.mozilla.org/tracemonkey/rev/9ff7c152d17a
Whiteboard: fixed-in-tracemonkey
Comment 9•15 years ago
|
||
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.
Description
•