352855 - JS_New(UC)?String allows easy mixing of allocators

Status: RESOLVED FIXED

(Developer Documentation Graveyard :: General, defect)

Description

[Boris Zbarsky [:bzbarsky]]

JS_New(UC)?String takes ownership of the buffer and then later uses free() to free it.  But nowhere is it documented what allocator will be used to deallocate and whether there is a way to control it.  As a result, we have various Gecko code that uses nsMemory (so the NSPR allocator) to allocate buffers and then passes them to JS_NewUCString.  Which crashes if NSPR does anything interesting allocator-wise (and this last part is not really under Gecko's control).

The solution is probably either to allow specifying a deallocator, or to document that JS_New(UC)?String should only get passed buffers allocated by malloc...

Comment 1

[Brendan Eich [:brendan]]

JS always uses malloc, period.  If you want a JSString whose storage is allocated otherwise, use JS_NewExternalString, after registering an external string GC-thing type with JS_AddExternalStringFinalizer.  Those APIs let the embedding client of SpiderMonkey control allocation.

Doc bugs don't go in b.m.o, AFAIK.  There's no code bug to fix unless you want more comments in jsapi.h.  Deb, who might be the best person to edit MDC?

/be

Comment 2

[Boris Zbarsky [:bzbarsky]]

More comments in jsapi.h would be great if we're doing that nowadays, yes...

Comment 3

[Deb Richardson [:dria] (plz NEEDINFO)]

Likely Nickolay or Sheppy.  Cc'ing them.

Comment 4

[Brendan Eich [:brendan]]

I am not sure more jsapi.h comments are helpful.  Last night on IRC, you noted how the file is big (2201 lines at my count), and it was clear no one was reading the relevant comments for the growable/dependent vs. immutable/independent string API (not saying anyone should have -- in fact that's my point ;-).

Making the .h bigger is not going to help people as much as fixing and completing the API docs on MDC.  So I think this is a doc bug, which does not belong in b.m.o AFAIK (someone please correct me if I'm wrong).

/be

Comment 5

[Eric Shepherd [:sheppy]]

We do actually like doc bugs to be filed in Bugzilla.  At least I do.  :)

Comment 6

[Boris Zbarsky [:bzbarsky]]

OK.  I'm willing to buy that it's a doc bug.  Over to the Documentation product. ;)

I guess I should file separate bugs on existing allocator abuses, right?

Comment 7

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

On Windows, is malloc in one library always equivalent to malloc in another?  I thought that it wasn't was one of the reasons we have interesting mixed-allocator bugs.

Comment 8

[Boris Zbarsky [:bzbarsky]]

> On Windows, is malloc in one library always equivalent to malloc in another?

No.  It's not, last I checked.

Comment 9

[Brendan Eich [:brendan]]

You guys are not Windows users.  But neither am I.  FWIW:

The only different I know of is debug vs. release built DLLs.  "Don't do that" is the prescribed cure.

Any real Windows(tm) hackers know more?

/be

Comment 10

[Nickolay_Ponomarev]

(comment #1)
> Deb, who might be the best person to edit MDC?

I think someone familiar with spidermonkey would do a better job editing the docs.

Anyways, http://developer.mozilla.org/en/docs/JS_NewString currently says that "the char array, |bytes|, *must* be allocated on the heap using JS_malloc". Is that true (or the caller may simply use malloc)? Would adding the same sentence to http://developer.mozilla.org/en/docs/JS_NewUCString fix this bug?

Comment 11

[timeless]

malloc is per library each library can and easily may get its own.

do *not* require people to use malloc or free to pass data across libraries because they in general can *not* do so.

that's the entire story.

each compiler (vc42, 5, 6, 7, 71, 8), digital mars, borland (...), will link libraries with its own crt (that's ignoring the fact that each compiler can choose a debug or release crt).

Comment 12

[Boris Zbarsky [:bzbarsky]]

> Would adding the same sentence 
> to http://developer.mozilla.org/en/docs/JS_NewUCString fix this bug?

I believe so, yes.  Then we'll need to file bugs on the existing JS_NewUCString callers in Gecko code -- most of them don't do that.

Comment 13

[Nickolay_Ponomarev]

I made the proposed change: http://developer.mozilla.org/en/docs/index.php?title=JS_NewUCString&diff=39967&oldid=19883

Comment 14

[Boris Zbarsky [:bzbarsky]]

Filed bug 353107, bug 353108, bug 353109.

The fourth and last caller of JS_NewUCString was OK.  It looks like all JS_NewString callers are in the JS engine itself.