829421 - ArgumentObjects memory leak

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[darkxst]

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]

Attachment: fix leak of data when jsobject fails.

Comment 2

[Nicolas B. Pierron [:nbp]]

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 [inactive]]

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]]

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

Comment 5

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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]]

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 7

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/9f56d7e548f1

Comment 8

[darkxst]

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]]

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]]

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 11

[Ryan VanderMeulen [:RyanVM]]

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

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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)]

(In reply to darkxst from comment #0)

Can this bug fix be verified on released beta builds? How?

Comment 14

[Nicolas B. Pierron [:nbp]]

(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] (NOT official MoCo now)]

(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] (NOT official MoCo now)]

Attachment: tbpl log

Unable to verify. (Unless this tbpl bug is a different one)

Comment 17

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

To track the tbpl bug, I've reopened bug 812423.

Comment 18

[Mihai Morar, (:MihaiMorar)]

(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] (NOT official MoCo now)]

The tbpl listing for Valgrind builds is at https://tbpl.mozilla.org/?noignore=1&jobname=valgrind

Comment 20

[Nicholas Nethercote [inactive]]

https://developer.mozilla.org/en-US/docs/Debugging_Mozilla_with_Valgrind may also be helpful.

Comment 21

[yoshi yokotani]

could anybody please describe a procedure to get the valgrind log? Thank you in advance=)