Last Comment Bug 829421 - ArgumentObjects memory leak
: ArgumentObjects memory leak
Status: RESOLVED FIXED
[MemShrink]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: mozilla21
Assigned To: darkxst
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-01-10 20:05 PST by darkxst
Modified: 2013-06-24 10:23 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
fixed
fixed
fixed
fixed
wontfix


Attachments
fix leak of data when jsobject fails. (634 bytes, patch)
2013-01-12 19:20 PST, darkxst
nicolas.b.pierron: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
akeybl: approval‑mozilla‑release-
akeybl: approval‑mozilla‑esr17+
akeybl: approval‑mozilla‑b2g18-
Details | Diff | Review
tbpl log (34.73 KB, text/plain)
2013-01-25 10:38 PST, Gary Kwong [:gkw] [:nth10sd]
no flags Details

Description darkxst 2013-01-10 20:05:18 PST
Been seeing this leak for some time. This particular trace is from gjs built against the ESR17 engine.

==32610== 340,672 bytes in 6,084 blocks are definitely lost in loss record 21,799 of 21,837
==32610==    at 0x4C2CD7B: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==32610==    by 0x7D584D1: js::ArgumentsObject::create(JSContext*, js::StackFrame*) (Utility.h:157)
==32610==    by 0x7D58D08: js::ArgumentsObject::createExpected(JSContext*, js::StackFrame*) (ArgumentsObject.cpp:103)
==32610==    by 0x7C85F92: js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) (jsinterp.cpp:2721)
==32610==    by 0x7C88D1C: js::RunScript(JSContext*, JSScript*, js::StackFrame*) (jsinterp.cpp:309)
==32610==    by 0x7C8969C: js::InvokeKernel(JSContext*, JS::CallArgs, js::MaybeConstruct) (jsinterp.cpp:363)
==32610==    by 0x7C50720: js_fun_apply(JSContext*, unsigned int, JS::Value*) (jsinterp.h:119)
==32610==    by 0x7C89537: js::InvokeKernel(JSContext*, JS::CallArgs, js::MaybeConstruct) (jscntxtinlines.h:372)
==32610==    by 0x7C82778: js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) (jsinterp.cpp:2414)
==32610==    by 0x7C88D1C: js::RunScript(JSContext*, JSScript*, js::StackFrame*) (jsinterp.cpp:309)
==32610==    by 0x7C8969C: js::InvokeKernel(JSContext*, JS::CallArgs, js::MaybeConstruct) (jsinterp.cpp:363)
==32610==    by 0x7C50720: js_fun_apply(JSContext*, unsigned int, JS::Value*) (jsinterp.h:119)
Comment 1 darkxst 2013-01-12 19:20:00 PST
Created attachment 701528 [details] [diff] [review]
fix leak of data when jsobject fails.
Comment 2 Nicolas B. Pierron [:nbp] 2013-01-12 20:28:37 PST
Comment on attachment 701528 [details] [diff] [review]
fix leak of data when jsobject fails.

Review of attachment 701528 [details] [diff] [review]:
-----------------------------------------------------------------

Good catch!
Comment 3 Nicholas Nethercote [:njn] (on vacation until July 11) 2013-01-14 17:29:21 PST
Comment on attachment 701528 [details] [diff] [review]
fix leak of data when jsobject fails.

Review of attachment 701528 [details] [diff] [review]:
-----------------------------------------------------------------

Please use eight lines of context in your diffs! :)
Comment 4 Nicolas B. Pierron [:nbp] 2013-01-17 10:27:42 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/e7e4224b8c8d
Comment 5 Marco Bonardo [::mak] 2013-01-17 10:55:48 PST
backed out for butage on all platforms

https://hg.mozilla.org/integration/mozilla-inbound/rev/dbaf061c64fb

e:/builds/moz2_slave/m-in-w32/build/js/src/vm/ArgumentsObject.cpp(165) : error C2039: 'free_' : is not a member of 'JSContext'
Comment 6 Nicolas B. Pierron [:nbp] 2013-01-17 12:05:16 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f56d7e548f1

Apparently at one point, we changed from cx->free_ to js_free.  I just updated the patch accordingly to push it to inbound.
Comment 8 darkxst 2013-01-19 15:00:28 PST
Would it be possible to get this backported to the 17 branch, sometime before the next release of standalone spidermonkey happens.

This leak appears to have quite a large impact on memory usage in gnome-shell, and is in fact one of the larger remaining leaks from the valgrind logs.
Comment 9 Nicolas B. Pierron [:nbp] 2013-01-19 15:44:25 PST
Comment on attachment 701528 [details] [diff] [review]
fix leak of data when jsobject fails.

[Approval Request Comment]
Bug caused by (feature/regressing bug #):
  Bug 684507, by moving the object allocation after the ArgumentsData structure.

User impact if declined: (same as below)
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
  Leak, apparently important in the gnome-shell which rely on ESR versions.

Fix Landed on Version:
  Nightly 9f56d7e548f1

Risk to taking this patch (and alternatives if risky):
  None, just need to take care to use cx->free_ on oldest version and use js_free on recent one.

String or UUID changes made by this patch:
  N/A
Comment 10 Alex Keybl [:akeybl] 2013-01-22 06:35:32 PST
Comment on attachment 701528 [details] [diff] [review]
fix leak of data when jsobject fails.

Approving for ESR and Aurora/Beta given where we are in the cycle and the risk profile here. It's not clear that this fix is needed on the B2G branch for the time being.
Comment 12 Ryan VanderMeulen [:RyanVM] 2013-01-23 11:53:41 PST
Backed out on esr17 for bustage:
https://hg.mozilla.org/releases/mozilla-esr17/rev/95405d6f2621

Re-landed, this time with feeling:
https://hg.mozilla.org/releases/mozilla-esr17/rev/946a0d000fbe
Comment 13 Mihai Morar, (:MihaiMorar) 2013-01-24 04:56:37 PST
(In reply to darkxst from comment #0)

Can this bug fix be verified on released beta builds? How?
Comment 14 Nicolas B. Pierron [:nbp] 2013-01-24 10:48:51 PST
(In reply to MarioMi from comment #13)
> (In reply to darkxst from comment #0)
> 
> Can this bug fix be verified on released beta builds? How?

Run tbpl valgrind builds with the modification of the suppression file (ask gkw for details about both) to see if this function signature appears or not.
Comment 15 Gary Kwong [:gkw] [:nth10sd] 2013-01-25 10:37:24 PST
(In reply to Nicolas B. Pierron [:pierron] [:nbp] from comment #14)
> (In reply to MarioMi from comment #13)
> > (In reply to darkxst from comment #0)
> > 
> > Can this bug fix be verified on released beta builds? How?
> 
> Run tbpl valgrind builds with the modification of the suppression file (ask
> gkw for details about both) to see if this function signature appears or not.

I removed the suppression in bug 812423 and it seems like this is still happening:

https://tbpl.mozilla.org/php/getParsedLog.php?id=19127812&tree=Firefox&full=1
Comment 16 Gary Kwong [:gkw] [:nth10sd] 2013-01-25 10:38:47 PST
Created attachment 706475 [details]
tbpl log

Unable to verify. (Unless this tbpl bug is a different one)
Comment 17 Gary Kwong [:gkw] [:nth10sd] 2013-01-25 10:43:21 PST
To track the tbpl bug, I've reopened bug 812423.
Comment 18 Mihai Morar, (:MihaiMorar) 2013-02-27 07:33:33 PST
(In reply to Nicolas B. Pierron [:pierron] [:nbp] from comment #14)
> (In reply to MarioMi from comment #13)
> Run tbpl valgrind builds with the modification of the suppression file (ask
> gkw for details about both) to see if this function signature appears or not.

Where can I find support in running tbpl valgrind builds because I haven't worked with it yet but I'm interested in doing that.
Comment 19 Gary Kwong [:gkw] [:nth10sd] 2013-02-27 10:49:59 PST
The tbpl listing for Valgrind builds is at https://tbpl.mozilla.org/?noignore=1&jobname=valgrind
Comment 20 Nicholas Nethercote [:njn] (on vacation until July 11) 2013-02-27 14:06:54 PST
https://developer.mozilla.org/en-US/docs/Debugging_Mozilla_with_Valgrind may also be helpful.
Comment 21 yoshi yokotani 2013-06-24 10:23:16 PDT
could anybody please describe a procedure to get the valgrind log? Thank you in advance=)

Note You need to log in before you can comment on or make changes to this bug.