Status

()

Core
JavaScript Engine
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: bz, Assigned: Igor Bukanov)

Tracking

({mlk, regression})

Trunk
x86
Mac OS X
mlk, regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 beta8+)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment, 1 obsolete attachment)

In particular, since we no longer have a deflated string cache we're not actually taking ownership if the passed-in buffer anywhere.
blocking2.0: --- → ?
Blocks: 617097
Keywords: mlk, regression
(Assignee)

Comment 1

7 years ago
Thanks for catching this. JS_NewString should definitely free the passed buffer.
Assignee: general → igor
(Assignee)

Comment 2

7 years ago
Created attachment 495901 [details] [diff] [review]
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.
Attachment #495901 - Flags: review?(bzbarsky)
(Assignee)

Comment 3

7 years ago
Brendan: is the above JS_NewString removal OK?

Updated

7 years ago
blocking2.0: ? → beta8+
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.
Attachment #495901 - Flags: review?(bzbarsky) → review-
(Assignee)

Comment 5

7 years ago
Created attachment 496114 [details] [diff] [review]
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.
Attachment #495901 - Attachment is obsolete: true
Attachment #496114 - Flags: review?(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?
Attachment #496114 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 7

7 years ago
(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

7 years ago
Why is this a beta 8 blocker?

Comment 9

7 years ago
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
Blocks: 607292

Comment 12

7 years ago
http://hg.mozilla.org/mozilla-central/rev/501c5ca70faa
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Attachment #496114 - Attachment description: fixing NeString while removing its usage → fixing NewString while removing its usage
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

7 years ago
This patch doesn't compile in debug builds.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.)
(Assignee)

Comment 16

7 years ago
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.
Whiteboard: fixed-in-tracemonkey

Comment 17

7 years ago
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
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago7 years ago
Resolution: --- → FIXED
(Assignee)

Updated

7 years ago
Blocks: 618262
You need to log in before you can comment on or make changes to this bug.