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

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

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.

Brendan: is the above JS_NewString removal OK?

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.

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 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?

(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.

Why is this a beta 8 blocker?

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

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.

Bug 607292 has the regressing patch. Is this landing? We could do a super-safe spot fix to fix the leak if not. /be

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

This patch doesn't compile in debug builds.

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.)

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.

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.

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.

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