ArgumentObjects memory leak

RESOLVED FIXED in Firefox 19

Status

()

Core
JavaScript Engine
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: darkxst, Assigned: darkxst)

Tracking

unspecified
mozilla21
x86_64
Linux
Points:
---

Firefox Tracking Flags

(firefox18 wontfix, firefox19 fixed, firefox20 fixed, firefox21 fixed, firefox-esr17 fixed, b2g18 wontfix)

Details

([MemShrink])

Attachments

(2 attachments)

(Assignee)

Description

4 years ago
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)
(Assignee)

Updated

4 years ago
Summary: memory leak → ArgumentObjects memory leak
(Assignee)

Comment 1

4 years ago
Created attachment 701528 [details] [diff] [review]
fix leak of data when jsobject fails.
Comment on attachment 701528 [details] [diff] [review]
fix leak of data when jsobject fails.

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

Good catch!
Attachment #701528 - Flags: review+
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! :)
https://hg.mozilla.org/integration/mozilla-inbound/rev/e7e4224b8c8d
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'
Assignee: general → darkxst
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
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.
https://hg.mozilla.org/mozilla-central/rev/9f56d7e548f1
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
(Assignee)

Comment 8

4 years ago
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 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
Attachment #701528 - Flags: approval-mozilla-release?
Attachment #701528 - Flags: approval-mozilla-esr17?
Attachment #701528 - Flags: approval-mozilla-beta?
Attachment #701528 - Flags: approval-mozilla-b2g18?
Attachment #701528 - Flags: approval-mozilla-aurora?
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.
Attachment #701528 - Flags: approval-mozilla-release?
Attachment #701528 - Flags: approval-mozilla-release-
Attachment #701528 - Flags: approval-mozilla-esr17?
Attachment #701528 - Flags: approval-mozilla-esr17+
Attachment #701528 - Flags: approval-mozilla-beta?
Attachment #701528 - Flags: approval-mozilla-beta+
Attachment #701528 - Flags: approval-mozilla-b2g18?
Attachment #701528 - Flags: approval-mozilla-b2g18-
Attachment #701528 - Flags: approval-mozilla-aurora?
Attachment #701528 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/d8040799b82a
https://hg.mozilla.org/releases/mozilla-beta/rev/c1b4639bd3b8
https://hg.mozilla.org/releases/mozilla-esr17/rev/fbd17599c5e5
status-b2g18: --- → wontfix
status-firefox18: --- → wontfix
status-firefox19: --- → fixed
status-firefox20: --- → fixed
status-firefox21: --- → fixed
status-firefox-esr17: --- → fixed
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
(In reply to darkxst from comment #0)

Can this bug fix be verified on released beta builds? How?
(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.
(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
Created attachment 706475 [details]
tbpl log

Unable to verify. (Unless this tbpl bug is a different one)
To track the tbpl bug, I've reopened bug 812423.
Whiteboard: [MemShrink]
(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.
The tbpl listing for Valgrind builds is at https://tbpl.mozilla.org/?noignore=1&jobname=valgrind
https://developer.mozilla.org/en-US/docs/Debugging_Mozilla_with_Valgrind may also be helpful.

Comment 21

4 years ago
could anybody please describe a procedure to get the valgrind log? Thank you in advance=)
You need to log in before you can comment on or make changes to this bug.