130711 - Memory leak in JS_dtoa

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Steven Cole]

Calling the function JS_dtoa to try and make a fixed point string can cause
memory leaks.  For instance, trying to change the double 0.0001 into a 2-digit
fixed point number (0.00) can cause the leak.  The Javascript equivalent is
(0.0001).toFixed(2)

Patch coming up.

Comment 1

[Steven Cole]

Attachment: Plugs up the "no_digits" memory leak

Comment 2

[Phil Schwartau]

cc'ing Brendan, shaver on this one -

Comment 3

[Kenton Hanson (gone)]

Steven,  JS_dtoa is large complicated routine.  I am familiar with it and
willing to review the patch.  Do you have a test exhibiting the memory leak?

A different bug with the same summary is bug #85267.

Comment 4

[Steven Cole]

Kenton, please review.  

The patch is rather simple:  The problem is that the "b" allocated in the early
portion of JS_dtoa can be leaked by the return statement three lines above the
allocation, thanks to "goto" statements further down in the code.

The patch for bug 85267 may eventually make this problem go away, but I need
leaks gone today, and this is a relatively simple patch.

A simple JS testcase that exhibits this problem is simply:

a = (0.00001).toFixed(2);

--steve

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 73957 [details] [diff] [review]
Plugs up the "no_digits" memory leak

Why set b = NULL there, half-way between its decl and the no_digits: label? 
Just wondering if it wouldn't be best to do whta this file does elsewhere, and
initialize at declaration -- or do what most js*.c does, and set just before
any (potential) use, right before the no_digits: label?

sr=[e-mail] contingent on khanson's r= and on some better
initialization location.

This looks complementary to the patch in bug 85267 -- both should go in for
mozilla1.0.  I'll go sr= over in 85267 now.

/be

Comment 6

[Steven Cole]

Brendan,  The truth about initialization of b: My eyes mis-parsed the enclosing
"{", and that looked like the latest initialization step.  It should, in fact,
be initialized two lines before the no_digits label.

[Sigh.  Hallelujah for code review, eh?  One of these days I'll actually submit
a patch that *doesn't* need changes.]

--steve

Comment 7

[Steven Cole]

Attachment: Fix locations for initialization and free

Shoot, I had the blasted Bfree() in the wrong location too.  Too much cutting
and pasting!

Comment 8

[Brendan Eich [:brendan]]

Comment on attachment 73998 [details] [diff] [review]
Fix locations for initialization and free

Carry sr= forward.

Steve, I gave your bugzilla account canconfirm and editbugs privileges, so you
should be able to obsolete patches when attaching new ones now.

/be

Comment 9

[Kenton Hanson (gone)]

Comment on attachment 73998 [details] [diff] [review]
Fix locations for initialization and free

r=khanson

OK I'll bite.

Comment 10

[Brendan Eich [:brendan]]

Ok, scole, can you mail [e-mail] citing this bug's low risk, its
benefits, and r=khanson/sr=brendan, seeking approval for mozilla1.0?

/be

Comment 11

[Steven Cole]

Approval request has been made.  (Thanks for the editbugs preference, Brendan;
I've wanted to be able to obsolete patches for a while now...)

--scole

Comment 12

[Asa Dotzler [:asa]]

Comment on attachment 73998 [details] [diff] [review]
Fix locations for initialization and free

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 13

[Brendan Eich [:brendan]]

Kenton, can you check this in?  Thanks,

/be

Comment 14

[Steven Cole]

Hey, Phil:

I'm thinking of trying to run the JS test suite through Purify to try and find
any more of these leaks... (provided it runs quickly enough).   And so it'd be
nice if this test was actually *in* the test suite.  

Could you please add a test to ecma_3/Number/15.7.4.5-1.js for something like
(0.00001).toFixed(2) ? 

(When the result is all zeros it goes through a different code path anyway, so
it ought to be in the suite just for that reason...)

Thanks,
--scole

Comment 15

[Phil Schwartau]

Steve, thanks for the suggestion. I have added a couple cases
just like that to the test:

[//d/JS_TRUNK/mozilla/js/tests/ecma_3/Number] cvs ci 15.7.4.5-1.js
Checking in 15.7.4.5-1.js;
/cvsroot/mozilla/js/tests/ecma_3/Number/15.7.4.5-1.js,v  <--  15.7.4.5-1.js
new revision: 1.3; previous revision: 1.2
done

Comment 16

[Steven Cole]

It looks like Kenton checked this in yesterday.  Marking FIXED.

Comment 17

[Phil Schwartau]

Marking Verified -