617215 - JS_NewString leaks

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

In particular, since we no longer have a deflated string cache we're not actually taking ownership if the passed-in buffer anywhere.

Comment 1

[Igor Bukanov]

Thanks for catching this. JS_NewString should definitely free the passed buffer.

Comment 2

[Igor Bukanov]

Attachment: removal of JS_NewString

With the deflated cache removed JS_NewString no longer make sense as it immediately releases the passed char buffer. So the patch removes it replacing its uses by JS_NewStringCopyZ/JS_NewStringCopyN.

Comment 3

[Igor Bukanov]

Brendan: is the above JS_NewString removal OK?

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 495901 [details] [diff] [review]
removal of JS_NewString

The jsopcode.cpp change looks wrong to me.  And this definitely needs api-review from Brendan.

Comment 5

[Igor Bukanov]

Attachment: fixing NewString while removing its usage

The new patch fixes JS_NewString while removing all its usage in FF code base. It fixes the embarrassing jsopcode.cpp changes via transferring the source bytes ownership to the caller.

The removal of JS_NewString itself can wait another patch.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 496114 [details] [diff] [review]
fixing NewString while removing its usage

I think this still needs someone like brendan to look at two issues:

1)  Is js_free the right thing to free the buffer with in JS_NewString?
2)  Is js_free the right thing to use to free the results of JS_sprintf_append?

r=me on the rest.

I guess JS_sprintf_append frees the input buffer on failure, which is why just assigning directly to |source| makes sense?

Comment 7

[Igor Bukanov]

(In reply to comment #6)
> 1)  Is js_free the right thing to free the buffer with in JS_NewString?

The difference between JS_free(cx, ptr) and js_free(ptr) is only relevant during the GC when the former schedules delayed allocation of the ptr.

> 2)  Is js_free the right thing to use to free the results of JS_sprintf_append?

The most rightful way is to call JS_smprintf_free here, but that is just a wrapper around js_free. But it would be interesting to know the reason behind JS_smprintf_free existence. Perhaps it was before js_free were introduced as a way to overwrite the system malloc/free?

> 
> r=me on the rest.
> 
> I guess JS_sprintf_append frees the input buffer on failure, which is why just
> assigning directly to |source| makes sense?

Yes, it releases the passed pointer on failures.

Comment 8

[LegNeato]

Why is this a beta 8 blocker?

Comment 9

[Andreas Gal :gal]

Because it leaks like a sieve. Patch is ready. Lets land it.

Comment 10

[Damon Sicore (:damons)]

If this is ready, land this immediately, please.  We're way too slow on getting beta 8 out the door.  If something is blocking you from making that happen (i.e., waiting for someone to land it for you, address review concerns, anything) please pick up the phone and call me and I'll remove any roadblocks for you.

Comment 11

[Brendan Eich [:brendan]]

Bug 607292 has the regressing patch.

Is this landing? We could do a super-safe spot fix to fix the leak if not.

/be

Comment 12

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/501c5ca70faa

Comment 13

[Brendan Eich [:brendan]]

Re: comment 7, {JS,PR}_smprintf_free is just veneer from NSPR days (JS originated with NSPR1, then forked its own copy of early NSPR2 files). Everything bottoms out in free. Memory pressure is done by cx->{malloc,calloc,realloc} and cx->free does freeLater if it can, but js_free is just inline veneer for free.

/be

Comment 14

[Robert Sayre]

This patch doesn't compile in debug builds.

Comment 15

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

In particular, the error was:

../../../js/src/jsopcode.cpp: In function 'bool ToDisassemblySource(JSContext*, jsval, JSAutoByteString*)':
../../../js/src/jsopcode.cpp:320: error: 'class JSAutoByteString' has no member named 'setBytes'

(The entirety of the jsopcode.cpp changes were inside an #ifdef DEBUG block.)

Comment 16

[Igor Bukanov]

http://hg.mozilla.org/tracemonkey/rev/010bd7365328

- I am traveling today so I May not monitor the tree. Please back out in case of any troubles.

(In reply to comment #15)
> In particular, the error was:

Sorry for adding a wrong version of the patch to the bug.

Comment 17

[LegNeato]

So does comment 16 mean this is completely fixed in m-c or the correct patch still needs to land on m-c? The status is hard to determine when looking at comment 16, comment 14, and comment 12.

Comment 18

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

It still needs to land on m-c; it landed on m-c (comment 12), was backed out (comment 14), and was then landed on tracemonkey.

Comment 19

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

I pushed it to m-c:
http://hg.mozilla.org/mozilla-central/rev/4c5f51f5e9a0