Last Comment Bug 656991 - JSStructuredCloneWriter leaks memory
: JSStructuredCloneWriter leaks memory
Status: RESOLVED FIXED
bz nominated near comment 1
: mlk, regression
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: x86_64 Linux
: -- normal (vote)
: mozilla6
Assigned To: Justin Lebar (not reading bugmail)
:
Mentors:
Depends on: 657458
Blocks: 551225 mlk-fx6
  Show dependency treegraph
 
Reported: 2011-05-13 12:42 PDT by Julian Seward [:jseward]
Modified: 2013-12-27 14:23 PST (History)
12 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected


Attachments
Patch v1 (1.09 KB, patch)
2011-05-13 21:42 PDT, Justin Lebar (not reading bugmail)
bzbarsky: review+
Details | Diff | Splinter Review
Patch v2 (1.08 KB, patch)
2011-05-15 15:23 PDT, Justin Lebar (not reading bugmail)
igor: review+
Details | Diff | Splinter Review

Description Julian Seward [:jseward] 2011-05-13 12:42:48 PDT
STR: I don't know specifically.  These were picked up during a
complete run of Mochitests on Valgrind.  I don't know which test
caused them.  I can try to narrow it down.

3,008 bytes in 47 blocks are definitely lost in loss record 8,069 of 8,258
   at 0x4C27972: realloc (vg_replace_malloc.c:525)
   by 0x66CD047: js::Vector<unsigned long, 0ul, js::ContextAllocPolicy>::growStorageBy(unsigned long) (jsutil.h:249)
   by 0x66CADCD: js::SCOutput::write(unsigned long) (jsvector.h:643)
   by 0x66CAE7F: JSStructuredCloneWriter::writeString(unsigned int, JSString*) (jsclone.cpp:362)
   by 0x66CBBA2: JSStructuredCloneWriter::write(js::Value const&) (jsclone.cpp:577)
   by 0x66CC196: js::WriteStructuredClone(JSContext*, js::Value const&, unsigned long**, unsigned long*, JSStructuredCloneCallbacks const*, void*) (jsclone.cpp:57)
   by 0x66AF7D7: JS_WriteStructuredClone (jsapi.cpp:5587)
   by 0x5C46AE1: nsStructuredCloneContainer::InitFromVariant(nsIVariant*, JSContext*) (nsStructuredCloneContainer.cpp:91)
   by 0x6048565: nsDocShell::AddState(nsIVariant*, nsAString_internal const&, nsAString_internal const&, int, JSContext*) (nsDocShell.cpp:9477)
   by 0x5C276F9: nsHistory::ReplaceState(nsIVariant*, nsAString_internal const&, nsAString_internal const&, JSContext*) (nsHistory.cpp:337)
   by 0x6477C31: NS_InvokeByIndex_P (xptcinvoke_x86_64_unix.cpp:195)
   by 0x5F7721F: XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) (xpcwrappednative.cpp:3141)

plus two other reports, essentially identical.
Comment 1 Boris Zbarsky [:bz] 2011-05-13 21:00:08 PDT
As far as I can tell, nsStructuredCloneContainer::InitFromVariant is just buggy.  It never frees jsBytes!  Am I just missing something?  Justin?  Jonas?
Comment 2 Justin Lebar (not reading bugmail) 2011-05-13 21:24:36 PDT
(In reply to comment #1)
> As far as I can tell, nsStructuredCloneContainer::InitFromVariant is just
> buggy.  It never frees jsBytes!

Indeed!  I'll post a fix in a moment.
Comment 3 Justin Lebar (not reading bugmail) 2011-05-13 21:42:16 PDT
Created attachment 532414 [details] [diff] [review]
Patch v1
Comment 4 Boris Zbarsky [:bz] 2011-05-13 21:49:17 PDT
Comment on attachment 532414 [details] [diff] [review]
Patch v1

Those are the right places to call free(), but I can't tell whether this is the right free() to call.  The jsapi docs claim that js::Foreground::free is the right thing, but is that public?  The JS_free calls you're making here can end up on the background thread (or in the foreground, depending on how the JSContext feels today).

Paul, what's the story here?
Comment 5 Justin Lebar (not reading bugmail) 2011-05-13 21:57:39 PDT
FWIW, JS_free is what JSAutoStructuredCloneBuffer::clear uses.
Comment 6 Nicholas Nethercote [:njn] 2011-05-13 22:00:19 PDT
CC'ing pbiggar (see comment 4).
Comment 7 Paul Biggar 2011-05-15 09:41:22 PDT
It's acceptable (though sometimes not ideal) to call any of the free() methods on any allocated memory.

That we don't expose these calls into the API would be a bug, except that we need to clarify how we use deallocators, and what their semantics are, which Igor is doing in bug 647103. (Igor, please correct me if I'm wrong here).
Comment 8 Igor Bukanov 2011-05-15 10:50:41 PDT
(In reply to comment #7)
> It's acceptable (though sometimes not ideal) to call any of the free()
> methods on any allocated memory.

The code should call Foreground::free or free as it is not invoked from the finalizer. So JS_free should be avoided as it just consumes useless cycles checking for the finalization.

> That we don't expose these calls into the API would be a bug, except that we
> need to clarify how we use deallocators, and what their semantics are, which
> Igor is doing in bug 647103.

Well, with the background finalization we may not need any background free for the GC and may well end up using a single wrapper for the free in the engine. But that is for latter. For now I would suggest just to call free.
Comment 9 Justin Lebar (not reading bugmail) 2011-05-15 15:23:51 PDT
Created attachment 532535 [details] [diff] [review]
Patch v2

s/JS_free/free, and verified that the leak doesn't appear when I run test_bug500328.html under valgrind.
Comment 10 Boris Zbarsky [:bz] 2011-05-15 18:06:24 PDT
Comment on attachment 532535 [details] [diff] [review]
Patch v2

I think we really do want JS_free here; there's no guarantee that the |free| in libmozjs and libxul is the same |free|.  :(
Comment 11 Justin Lebar (not reading bugmail) 2011-05-15 18:08:35 PDT
http://hg.mozilla.org/mozilla-central/rev/1ba51bb439e0  :)

Would you like me to push a followup, or would you like someone else to weigh in first?
Comment 12 Boris Zbarsky [:bz] 2011-05-15 19:09:18 PDT
Push a followup with a FIXME comment explaining why it's JS_free and file a bug to update to Foreground::free once that's exposed?
Comment 13 Julian Seward [:jseward] 2011-05-16 06:39:29 PDT
(In reply to comment #9)
> s/JS_free/free, and verified that the leak doesn't appear when I run
> test_bug500328.html under valgrind.

Also verified here.  Can we get this in Fx 5  ?  This is the second
largest leak in mochitests, after bug 655435.
Comment 14 Justin Lebar (not reading bugmail) 2011-05-16 06:43:50 PDT
nsStructuredCloneContainer was added in rev 0221eb8660f4 on April 24.  That's after the Aurora 5 branch on April 12 (tag a95d42642281).
Comment 15 Justin Lebar (not reading bugmail) 2011-05-16 13:38:34 PDT
Pushed followup to use JS_free instead of free.

http://hg.mozilla.org/mozilla-central/rev/794c73cbfca6
Comment 16 Asa Dotzler [:asa] 2011-05-26 14:46:00 PDT
unsetting request because this appears to have landed before the aurora migration. if it's not, please nominate the patch.

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