634417 - Remove uses of vanilla malloc/calloc/realloc/free

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

Bug 624878 removed uses of vanilla new/new[] from SpiderMonkey.  This bug is for removing uses of vanilla malloc/calloc/realloc/free, the presence of which makes life difficult for embedders who use JS_USE_CUSTOM_ALLOCATOR.

js_malloc/js_calloc/js_realloc/js_free should be used instead.  find_vanilla_new_calls should be augmented to search for malloc/calloc/realloc as well, which should be easy.

Comment 1

[Nicholas Nethercote [inactive]]

There are some uses in nanojit/avmplus.h, ctypes/*, assembler/assembler/AssemblerBuffer{,WithConstantPool}.h, jsprf.cpp, shell/js.cpp, and probably some others.

Comment 2

[Andrew Paprocki]

It would be awesome if 2.0 ships with a completely working JS_USE_CUSTOM_ALLOCATOR. This, in my mind, means you can build a js shell with JS_USE_CUSTOM_ALLOCATOR and run the test suite successfully with an LD_PRELOAD of an .so which defined malloc/calloc/realloc/free/new/new[]/delete/strdup and makes them assert().

Comment 3

[Brendan Eich [:brendan]]

Embedders will rejoice all around.

/be

Comment 4

[Paul Biggar]

Based on bug 634155, (js_new should be cx->new) I guess mallocs should become cx->malloc rather than js_malloc whenever possible (this keeps the GC accounting up to date).

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: Remove inappropriate uses of vanilla malloc/calloc/realloc/free/strdup from SpiderMonkey.

Some notes.

- I used js_malloc et al instead of cx->malloc_ et al everywhere, because in
  all the relevant places we didn't have a |cx| handy.

- In Profilers.cpp I avoided strdup() by just manually emulating it.  Eh.

- mozilla::Vector uses mozilla::MallocAllocPolicy by default, so we should use
  js::Vector instead.

If you're wondering how I found these, it was partly with grep, and partly with
a Python script I wrote that grovels through the output of |nm|.

I was hoping to land that script in order to prevent backsliding.  But in the
end I can't get it to work sufficiently reliably with a normal build of
SpiderMonkey.  If I build SpiderMonkey with custom versions of js_malloc (et
al) that don't call system malloc (et al) then it is reliable... but that's not
an easily testable configuration, unfortunately.

Comment 6

[Nathan Froyd [:froydnj]]

Is this worth shipping in the 24 release of the engine?

Comment 7

[Nicholas Nethercote [inactive]]

> Is this worth shipping in the 24 release of the engine?

You mean, backporting to ESR 24?

Comment 8

[Nathan Froyd [:froydnj]]

(In reply to Nicholas Nethercote [:njn] from comment #7)
> > Is this worth shipping in the 24 release of the engine?
> 
> You mean, backporting to ESR 24?

I didn't know whether the standalone engine release was coming off the branch or a completely different repository.  So yes, I suppose I do.

Comment 9

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/de3af7ac0598

Comment 10

[Nicholas Nethercote [inactive]]

sstangl, is it worth trying to sneak this into the SpiderMonkey 24 standalone release?  AIUI, there's a few days until that release's deadline is hit.

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/de3af7ac0598

Comment 12

[Sean Stangl [:sstangl]]

(In reply to Nicholas Nethercote [:njn] from comment #10)
> sstangl, is it worth trying to sneak this into the SpiderMonkey 24
> standalone release?  AIUI, there's a few days until that release's deadline
> is hit.

Release is being postponed for other reasons, so I'll include this patch. Thanks!

Comment 13

[Sean Stangl [:sstangl]]

Attachment: ESR24 Backport Part 1/2 - Backport js_strdup().

Comment 14

[Sean Stangl [:sstangl]]

Attachment: ESR24 Backport Part 2/2 - Remove vanilla alloc