1022954 - ScriptSource leaks sourceMapURL_ sometimes

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Andrew McCreight [:mccr8]]

There are two places that ScriptSource assigns to sourceMapURL_ without freeing the existing value.  One was introduced by bug 765993, the other is pre-existing.

This showed up as an LSAN stack, and I was able to figure it out from there:

Direct leak of 22 byte(s) in 1 object(s) allocated from:
    #0 0x471d41 in malloc /builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:74
    #1 0x7f888d980ab0 in js_malloc /build/obj-firefox/js/src/../../dist/include/js/Utility.h:98
    #2 0x7f888d980ab0 in malloc_ /build/js/src/vm/MallocProvider.h:53
    #3 0x7f888d980ab0 in pod_malloc<char16_t> /build/js/src/vm/MallocProvider.h:105
    #4 0x7f888d980ab0 in js_strdup(js::ThreadSafeContext*, char16_t const*) /build/js/src/jsstr.cpp:4426
    #5 0x7f888d980f3f in js::ScriptSource::setSourceMapURL(js::ExclusiveContext*, char16_t const*) /build/js/src/jsscript.cpp:1984
    #6 0x7f888cf9a658 in SetSourceMap /build/js/src/frontend/BytecodeCompiler.cpp:58
    #7 0x7f888cf9a658 in js::frontend::CompileScript(js::ExclusiveContext*, js::LifoAlloc*, JS::Handle<JSObject*>, JS::Handle<JSScript*>, JS::ReadOnlyCompileOptions const&, JS::SourceBufferHolder&, JSString*, unsigned int, js::SourceCompressionTask*) /build/js/src/frontend/BytecodeCompiler.cpp:416
    #8 0x7f888d768d31 in Evaluate(JSContext*, JS::Handle<JSObject*>, JS::ReadOnlyCompileOptions const&, JS::SourceBufferHolder&, JS::Value*) /build/js/src/jsapi.cpp:4990

Comment 1

[Nicholas Nethercote [inactive]]

(In reply to Andrew McCreight [:mccr8] from comment #0)
> There are two places that ScriptSource assigns to sourceMapURL_ without
> freeing the existing value.  One was introduced by bug 765993

I guess that's the call to setSourceMapURL() in nsScriptLoader::FillCompileOptionsForReq()?

> the other is pre-existing.

Where is that one?

Comment 2

[Andrew McCreight [:mccr8]]

No, I'm talking about the sourceMapURL_ that is a field of ScriptSource.

Here are the two places:
http://mxr.mozilla.org/mozilla-central/source/js/src/jsscript.cpp#1800
http://mxr.mozilla.org/mozilla-central/source/js/src/jsscript.cpp#1984

Comment 3

[Andrew McCreight [:mccr8]]

Oh, I see what you mean.  Right.  I wasn't actually looking at any place these methods were called, just in the methods themselves.

Comment 4

[Nicholas Nethercote [inactive]]

> http://mxr.mozilla.org/mozilla-central/source/js/src/jsscript.cpp#1800

It's hard to tell if that's doing the right thing because of the fact that this code does either or decoding, depending on the |mode| parameter.

> http://mxr.mozilla.org/mozilla-central/source/js/src/jsscript.cpp#1984

Hmm, for that one it looks like if hasSourceMapURL() is true we probably shouldn't fall through to the code in the second half.

Comment 5

[Andrew McCreight [:mccr8]]

(In reply to Nicholas Nethercote [:njn] from comment #4)
> It's hard to tell if that's doing the right thing because of the fact that
> this code does either or decoding, depending on the |mode| parameter.
Well, the only way that can be right is if the value is always null going in, right?  Otherwise we're leaking.  Every time we assign to this field, we seem to own the memory.

> > http://mxr.mozilla.org/mozilla-central/source/js/src/jsscript.cpp#1984
> 
> Hmm, for that one it looks like if hasSourceMapURL() is true we probably
> shouldn't fall through to the code in the second half.

Yeah, that looked pretty odd to me, but honestly I've been filing so many leaks bugs these last few weeks I'm kind of tunnel visioning on the fixes.  If somebody wants to investigate more properly what should happen here, please go ahead.

Comment 6

[Nicholas Nethercote [inactive]]

> Well, the only way that can be right is if the value is always null going
> in, right?  Otherwise we're leaking.  Every time we assign to this field, we
> seem to own the memory.

Here's part of the code:

1789     uint8_t haveSourceMap = hasSourceMapURL();
1790     if (!xdr->codeUint8(&haveSourceMap))
1791         return false;

If |mode| is |XDR_DECODE|, then codeUint8() reads a value from file, overwriting whatever the result of hasSourceMapURL() is. I don't understand XDR enough to know whether sourceMapURL_ could be non-null prior to this point or not when decoding.

Comment 7

[Andrew McCreight [:mccr8]]

Attachment: ScriptSource leaks sourceMapURL_ sometimes.

Comment 8

[Andrew McCreight [:mccr8]]

Attachment: ScriptSource leaks sourceMapURL_ sometimes.

If I'm reading the code correctly, then we actually leak sourceMapURL_ every time we hit this error state, because JS_ReportErrorFlagsAndNumber returns |true| when it is reporting a warning, and I think we're always reporting a warning.  It seems to me we should just always return false in that case.  For the other one, maybe we can just assert if we're going to leak, and then hope people use it correctly.  If they aren't, then it could be freed.  This relies on bug 1020846 being fixed, because otherwise with this patch we throw an exception which isn't caught properly in a bunch of devtools tests, and the tests end up timing out.  With the patch from that bug, we don't throw the exception and things seem to work out okay.

For now, I'm just going to add this method to the suppression list for LSan.

Comment 9

[Andrew McCreight [:mccr8]]

With bug 1020846 fixed, this still fails content/base/test/chrome/test_bug765993.html.  In that test, there's a source map both in the header and the JS file.  Before my patch here, we'd set one, then overwrite it, ending up with the second one.  With this patch, we set one, then don't set the second one, so the result changes.

I think it is best to preserve existing behavior, so I think I'll just make the duplicate case free the old string and then set the new one.

Comment 10

[Andrew McCreight [:mccr8]]

Attachment: ScriptSource leaks sourceMapURL_ sometimes.

try run: https://tbpl.mozilla.org/?tree=Try&rev=04bde7a2f313
(M3 failure is unrelated)

Comment 11

[Jim Blandy :jimb]

Comment on attachment 8454083 [details] [diff] [review]
ScriptSource leaks sourceMapURL_ sometimes.

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

Looks good to me.

I think you're going to conflict with some of Waldo's UniquePtr work, here, which will eliminate some of these problems in a more reliable way. You might want to talk with him about that.

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5e318db257e7

Comment 13

[Andrew McCreight [:mccr8]]

Waldo, FYI this will conflict a little with your unique pointer stuff.  It will be nice to have a less brittle fix for this field. :)

Comment 14

[Jeff Walden [:Waldo]]

No conflict, I was changing the TokenStream fields.  I'll file a separate bug to convert these two fields as well to UniquePtr.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5e318db257e7