998069 - js_malloc can be called from libxul

Status: RESOLVED INVALID

(Core :: JavaScript Engine, defect)

Description

[(no longer active)]

I know that is not what we're doing today but I see nothing preventing against that.  Since mozjs.dll uses malloc and xul.dll uses jemalloc, freeing such a pointer in mozjs.dll would be catastrophic.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

FWIW, I need js_malloc to be callable from libxul in bug 987556 so that we can pass memory ownership to the JS engine.  The goal here is to avoid holding two copies of the UTF16 source buffer in memory while we compile the JS.  Suggestions for safer approaches welcome!

Comment 2

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Note that there's a difference between JS_malloc and js_malloc -- JS_malloc is fine; it's non-inlined and calls the right (JS's) allocator.  That's what you want to use.  js_malloc is not; it's an inline function that calls malloc(), which will lead to a different malloc if it's called from libxul code.

Comment 3

[Luke Wagner [:luke]]

Whoa, why does JS have a separate malloc heap?  If nothing else, that seems bad for fragmentation.

Comment 4

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Yes, that was the second bug that Ehsan was going to file :)

Comment 5

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Oh wait, we might be jumping to conclusions here.  the malloc() that mozjs imports is actually from mozglue.dll.  I assume that mozglue implements malloc using the right heap.  So maybe this is all OK?

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #2)
> Note that there's a difference between JS_malloc and js_malloc -- JS_malloc
> is fine; it's non-inlined and calls the right (JS's) allocator.  That's what
> you want to use.  js_malloc is not; it's an inline function that calls
> malloc(), which will lead to a different malloc if it's called from libxul
> code.

Hmm, that would be significant as I am definitely using js_malloc() as we thought this memory did not need to be associated with a particular JSContext.

(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #5)
> So maybe this is all OK?

That would be nice for me. :-)

Comment 7

[Nicholas Nethercote [inactive]]

I'm 99.8% certain that SpiderMonkey uses jemalloc when built as part of Firefox. glandium should be able to answer this definitively.

Comment 8

[Mike Hommey [:glandium]]

mozjs.dll uses malloc from mozglue.dll. Like xul.dll does.

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

So, this is a non-issue?

I did notice that JS_free() maps directly to js_free():

  http://dxr.mozilla.org/mozilla-central/source/js/src/jsapi.cpp#1445

Comment 10

[(no longer active)]

Hmm, yeah based on the above this seems like it's a non-issue.

Comment 11

[(no longer active)]

And finally, this started to bite me in the back tonight while trying to get address sanitizer builds on Windows.  :(

The issue is that I have had to turn off jemalloc, and got hit by this code which tries to free things allocated in the other library.  After trying to fix a dozen or so of these issues, I came across one which seemed extremely difficult to solve as we needed to free a XUL allocated buffer inside js, so I decided to give up and build with --disable-shared-js...

Comment 12

[Boris Zbarsky [:bzbarsky]]

Ehsan, what buffer was this?  Is it being allocated via js_malloc, and if so why can we not just use JS_malloc instead?

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Boris Zbarsky [:bz] from comment #12)
> Ehsan, what buffer was this?  Is it being allocated via js_malloc, and if so
> why can we not just use JS_malloc instead?

Is it the string that gets passed in to avoid copying sources in bug 987556?  I recall we did not always have a context available in order to JS_malloc.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Ah, right.  We should fix JS_malloc to work with a null cx, or some equivalent.

Comment 15

[(no longer active)]

(In reply to Ben Kelly [:bkelly] from comment #13)
> (In reply to Boris Zbarsky [:bz] from comment #12)
> > Ehsan, what buffer was this?  Is it being allocated via js_malloc, and if so
> > why can we not just use JS_malloc instead?
> 
> Is it the string that gets passed in to avoid copying sources in bug 987556?

Yes, that was when I decided that I'm going to give up on building with mozjs.dll...