616834 - Leak in js/src/xpconnect/loader/mozJSComponentLoader.cpp Atob & Btoa

Status: RESOLVED DUPLICATE of bug 612642

(Core :: XPConnect, defect)

Description

[Julian Seward [:jseward]]

Leak in js/src/xpconnect/loader/mozJSComponentLoader.cpp Atob & Btoa

TM current (but looks like MC has the same problem)

x86_64-linux, loading 30 random tabs from www.cad-comic.com/cad

Memcheck reports at program exit, 2331kB of hard leaks (blocks to
which no pointer could be found).  Of these, almost all of that
is from

  Atob(JSContext *cx, uintN argc, jsval *vp) and
  Btoa(JSContext *cx, uintN argc, jsval *vp)

as per stacks below.

Considering that the peak heap size in this run is around 320MB,
constitutes a leak of around 0.7% of it.

722,809 bytes in 473 blocks are definitely lost in loss record 5,728 of 5,729
   at 0x4C27878: malloc (vg_replace_malloc.c:236)
   by 0x647BA74: NS_Alloc_P (nsMemoryImpl.cpp:210)
   by 0x5FD4234: Atob(JSContext*, unsigned int, unsigned long*) (nsMemor
   by 0x676D3E6: js::Interpret(JSContext*, JSStackFrame*, unsigned int, 
   by 0x65F0436: js::RunScript(JSContext*, JSScript*, JSStackFrame*) (js
   by 0x65F10C1: js::Invoke(JSContext*, js::CallArgs const&, unsigned in
   by 0x65F2404: js::ExternalInvoke(JSContext*, js::Value const&, js::Va
   by 0x6598610: JS_CallFunctionValue (jsinterp.h:962)
   by 0x5BBE960: nsJSContext::CallEventHandler(nsISupports*, void*, void
   by 0x5BD5D10: nsGlobalWindow::RunTimeout(nsTimeout*) (nsGlobalWindow.
   by 0x5BD6150: nsGlobalWindow::TimerCallback(nsITimer*, void*) (nsGlob
   by 0x6475A55: nsTimerImpl::Fire() (nsTimerImpl.cpp:425)

1,552,009 bytes in 1,355 blocks are definitely lost in loss record 5,729 of 5,729
   at 0x4C27878: malloc (vg_replace_malloc.c:236)
   by 0x647BA74: NS_Alloc_P (nsMemoryImpl.cpp:210)
   by 0x5FD4074: Btoa(JSContext*, unsigned int, unsigned long*) (nsMemor
   by 0x676D3E6: js::Interpret(JSContext*, JSStackFrame*, unsigned int, 
   by 0x65F0436: js::RunScript(JSContext*, JSScript*, JSStackFrame*) (js
   by 0x65F10C1: js::Invoke(JSContext*, js::CallArgs const&, unsigned in
   by 0x65DC01E: js_fun_call(JSContext*, unsigned int, js::Value*) (jsfu
   by 0x676D3E6: js::Interpret(JSContext*, JSStackFrame*, unsigned int, 
   by 0x65F0436: js::RunScript(JSContext*, JSScript*, JSStackFrame*) (js
   by 0x65F10C1: js::Invoke(JSContext*, js::CallArgs const&, unsigned in
   by 0x65F2404: js::ExternalInvoke(JSContext*, js::Value const&, js::Va
   by 0x6598610: JS_CallFunctionValue (jsinterp.h:962)

LEAK SUMMARY:
   definitely lost: 2,331,837 bytes in 2,828 blocks
   indirectly lost: 12,296 bytes in 380 blocks
     possibly lost: 443,045 bytes in 1,427 blocks
   still reachable: 1,616,382 bytes in 11,096 blocks
        suppressed: 0 bytes in 0 blocks

Comment 1

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Introduced with the merge in bug 612642.

Comment 2

[Julian Seward [:jseward]]

Attachment: a possible fix

This gets rid of the leaks, and doesn't create any obvious
access-after-free errors.  So it might be correct.  Not sure
though -- it assumes that the calls to nsDependentCString
copy their first arg by value rather than merely stashing
the pointer.

Comment 3

[Boris Zbarsky [:bzbarsky]]

nsDependentCString just takes a |const char*| and stores that value in a member.

Also, we have RAII classes for managing this sort of thing.  So instead of the nsDependentCString thing, do:

nsCString string;
string.Adopt(buffer, JS_GetStringLength(str));

and then not have to worry about having to make explicit free calls, since the |string| destructor will take care of it?

Comment 4

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 495407 [details] [diff] [review]
a possible fix

Actually, I'd recommend making |buffer| be another nsCAutoString, and call SetLength on it rather than the NS_Alloc that we've got here.

Comment 5

[Ben Turner (not reading bugmail, use the needinfo flag!)]

I'm going to dupe this to bug bug 612642, fix it there.

Comment 6

[LegNeato]

As per today's meeting, beta 9 will be a time-based release. Marking these all betaN+. Please move it back to beta9+ if you believe it MUST be in the next beta (ie: trunk is in an unshippable state without this)