Last Comment Bug 737463 - nsJARInputStream leaks if it is closed before the stream is inflated
: nsJARInputStream leaks if it is closed before the stream is inflated
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Networking: JAR (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla14
Assigned To: Chris Coulson
:
: Patrick McManus [:mcmanus]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-20 09:11 PDT by Chris Coulson
Modified: 2012-04-12 10:30 PDT (History)
5 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Don't leak in nsJARInputStream if the stream is closed before being fully inflated (2.01 KB, patch)
2012-03-20 09:14 PDT, Chris Coulson
no flags Details | Diff | Splinter Review
Don't leak in nsJARInputStream if the stream is closed before being fully inflated (v2) (730 bytes, patch)
2012-03-25 16:18 PDT, Chris Coulson
mwu.code: review+
Details | Diff | Splinter Review

Description Chris Coulson 2012-03-20 09:11:59 PDT
When running Firefox in valgrind, I see this (I only ran it for a minute or so here):

==32263== 150,192 bytes in 21 blocks are definitely lost in loss record 8,480 of 8,480
==32263==    at 0x4C2B6CD: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==32263==    by 0x953A7C4: MOZ_Z_inflateInit2_ (inflate.c:187)
==32263==    by 0x8154A25: gZlibInit(z_stream_s*) (nsZipArchive.cpp:122)
==32263==    by 0x81569ED: nsJARInputStream::InitFile(nsJAR*, nsZipItem*) (nsJARInputStream.cpp:80)
==32263==    by 0x815862A: nsJAR::GetInputStreamWithSpec(nsACString_internal const&, nsACString_internal const&, nsIInputStream**) (nsJAR.cpp:375)
==32263==    by 0x8EA1863: NS_InvokeByIndex_P (xptcinvoke_x86_64_unix.cpp:195)
==32263==    by 0x89C2B93: CallMethodHelper::Call() (XPCWrappedNative.cpp:3021)
==32263==    by 0x89C3157: XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) (XPCWrappedNative.cpp:2318)
==32263==    by 0x89C8CFB: XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) (XPCWrappedNativeJSOps.cpp:1539)
==32263==    by 0x91EE7AC: js::CallJSNative(JSContext*, int (*)(JSContext*, unsigned int, JS::Value*), js::CallArgs const&) (jscntxtinlines.h:314)
==32263==    by 0x9201B70: js::InvokeKernel(JSContext*, js::CallArgs, js::MaybeConstruct) (jsinterp.cpp:513)
==32263==    by 0x91FBB5A: js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) (jsinterp.cpp:2684)
==32263==    by 0x920178D: js::RunScript(JSContext*, JSScript*, js::StackFrame*) (jsinterp.cpp:469)
==32263==    by 0x9202AFB: js::ExecuteKernel(JSContext*, JSScript*, JSObject&, JS::Value const&, js::ExecuteType, js::StackFrame*, JS::Value*) (jsinterp.cpp:667)
==32263==    by 0x9233B70: EvalKernel(JSContext*, js::CallArgs const&, EvalType, js::StackFrame*, JSObject&) (jsobj.cpp:1052)
==32263==    by 0x92341D7: js::DirectEval(JSContext*, js::CallArgs const&) (jsobj.cpp:1117)
==32263==    by 0x91FC5DB: js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) (jsinterp.cpp:2643)
==32263==    by 0x920178D: js::RunScript(JSContext*, JSScript*, js::StackFrame*) (jsinterp.cpp:469)
==32263==    by 0x9202F22: js::Execute(JSContext*, JSScript*, JSObject&, JS::Value*) (jsinterp.cpp:667)
==32263==    by 0x90F9471: EvaluateUCScriptForPrincipalsCommon(JSContext*, JSObject*, JSPrincipals*, JSPrincipals*, unsigned short const*, unsigned int, char const*, unsigned int, JS::Value*, JSVersion) (jsapi.cpp:5266)
==32263==    by 0x90F9588: JS_EvaluateUCScriptForPrincipalsVersionOrigin (jsapi.cpp:5303)
==32263==    by 0x866DA99: nsJSContext::EvaluateString(nsAString_internal const&, JSObject*, nsIPrincipal*, nsIPrincipal*, char const*, unsigned int, unsigned int, nsAString_internal*, bool*) (nsJSEnvironment.cpp:1448)
==32263==    by 0x848E408: nsScriptLoader::EvaluateScript(nsScriptLoadRequest*, nsString const&) (nsScriptLoader.cpp:923)
==32263==    by 0x848E87C: nsScriptLoader::ProcessRequest(nsScriptLoadRequest*) (nsScriptLoader.cpp:816)
==32263==    by 0x849148E: nsScriptLoader::ProcessPendingRequests() (nsScriptLoader.cpp:983)
==32263==    by 0x8491835: nsScriptLoader::OnStreamComplete(nsIStreamLoader*, nsISupports*, unsigned int, unsigned int, unsigned char const*) (nsScriptLoader.cpp:1202)
==32263==    by 0x8074DD9: nsStreamLoader::OnStopRequest(nsIRequest*, nsISupports*, unsigned int) (nsStreamLoader.cpp:127)
==32263==    by 0x8046731: nsBaseChannel::OnStopRequest(nsIRequest*, nsISupports*, unsigned int) (nsBaseChannel.cpp:745)
==32263==    by 0x805228F: nsInputStreamPump::OnStateStop() (nsInputStreamPump.cpp:583)
==32263==    by 0x8052361: nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*) (nsInputStreamPump.cpp:405)
==32263==    by 0x8E76CE6: nsInputStreamReadyEvent::Run() (nsStreamUtils.cpp:114)
==32263==    by 0x8E8B08F: nsThread::ProcessNextEvent(bool, bool*) (nsThread.cpp:657)
==32263==    by 0x8E49CD3: NS_ProcessNextEvent_P(nsIThread*, bool) (nsThreadUtils.cpp:245)
==32263==    by 0x8D9122E: mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) (MessagePump.cpp:134)
==32263==    by 0x8EB6913: MessageLoop::RunInternal() (message_loop.cc:208)
==32263==    by 0x8EB693B: MessageLoop::Run() (message_loop.cc:201)
==32263==    by 0x8CB2F3A: nsBaseAppShell::Run() (nsBaseAppShell.cpp:189)
==32263==    by 0x8AF72EF: nsAppStartup::Run() (nsAppStartup.cpp:295)
==32263==    by 0x802726D: XRE_main (nsAppRunner.cpp:3703)
==32263==    by 0x4014A7: main (nsBrowserApp.cpp:190)

There's several more of these types of leak in the valgrind log too.

It seems to be trivially reproducible by installing the extension developer addon and doing this in the JS console:

file=Components.classes["@mozilla.org/file/local;1"].createInstance(Components.interfaces.nsILocalFile)
file.initWithPath("/usr/lib/firefox-trunk/omni.ja")
reader=Components.classes["@mozilla.org/libjar/zip-reader;1"].createInstance(Components.interfaces.nsIZipReader)
reader.open(file)
reader.getInputStream("components/browser.xpt").close()
Comment 1 Chris Coulson 2012-03-20 09:14:27 PDT
Created attachment 607577 [details] [diff] [review]
Don't leak in nsJARInputStream if the stream is closed before being fully inflated

This seems to fix it. Obviously with this patch it is no longer "aggressive about ending the inflation" (as per the comment in nsJARInputStream::ContinueInflate). If that matters, then I can take a different approach to it instead.
Comment 2 (dormant account) 2012-03-23 12:37:51 PDT
Comment on attachment 607577 [details] [diff] [review]
Don't leak in nsJARInputStream if the stream is closed before being fully inflated

cant review this until april. maybe mwu can
Comment 3 Michael Wu [:mwu] 2012-03-25 11:40:57 PDT
(In reply to Chris Coulson from comment #1)
> Created attachment 607577 [details] [diff] [review]
> Don't leak in nsJARInputStream if the stream is closed before being fully
> inflated
> 
> This seems to fix it. Obviously with this patch it is no longer "aggressive
> about ending the inflation" (as per the comment in
> nsJARInputStream::ContinueInflate). If that matters, then I can take a
> different approach to it instead.

I don't think calling inflateEnd twice really matters, at least according to https://hg.mozilla.org/mozilla-central/file/3e4735893504/modules/zlib/src/inflate.c#l1238 .
Comment 4 Chris Coulson 2012-03-25 16:01:06 PDT
Hi Michael,

Perhaps I'm missing something else, but would this not end up being a double-free?

https://hg.mozilla.org/mozilla-central/file/3e4735893504/modules/zlib/src/inflate.c#l1245

AFAICT, state->window is left dangling, which is why I thought it would be unsafe to call a second time.
Comment 5 Chris Coulson 2012-03-25 16:06:24 PDT
Errrr, actually, I see now ;)
Comment 6 Chris Coulson 2012-03-25 16:18:45 PDT
Created attachment 609167 [details] [diff] [review]
Don't leak in nsJARInputStream if the stream is closed before being fully inflated (v2)

Ok, this one just adds an extra call to inflateEnd() in nsJARInputStream::Close() and removes the changes in nsJARInputStream::ContinueInflate()
Comment 7 Michael Wu [:mwu] 2012-04-08 04:37:17 PDT
Comment on attachment 609167 [details] [diff] [review]
Don't leak in nsJARInputStream if the stream is closed before being fully inflated (v2)

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

Wow, I must've missed this review request during the craziness the last few weeks. Sorry about the delay.
Comment 8 Ryan VanderMeulen [:RyanVM] 2012-04-11 15:02:03 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/226937c24338
Comment 9 Richard Newman [:rnewman] 2012-04-11 15:38:19 PDT
Backed out whole merge for bustage per Yoric (Bug 743574):

https://hg.mozilla.org/integration/mozilla-inbound/rev/12e42fb8e321
Comment 10 Richard Newman [:rnewman] 2012-04-11 17:55:29 PDT
Disregard that; PEBKAC. Did not get backed out. I misread TBPL.

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