138666 - Another Memory Leak in jsdtoa.c

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Steven Cole]

It looks like my fix in bug 130711 wasn't quite complete.  I unearthed another
memory leak in the JS_dtoa routine which happens when the JS code
   5.6e-05 .toFixed(2)
is run.

It doesn't look like the guys working on bug 120992 have found this either; I'll
add a comment there to point them here.

Patch coming momentarily.

Comment 1

[Steven Cole]

Attachment: Proposed fix.

The issue was simply that there are more bigints that could be allocated than
I'd counted on.  The testcase above is fixed by the "Bfree(S)" line, but after
looking at the code, I don't trust mlo and mhi either, so the patch frees them
too.

--scole

Comment 2

[Daniel Bratell]

Comment on attachment 80119 [details] [diff] [review]
Proposed fix.

Good catch!

I don't know if I'm qualified to review js bugs but I have spent too much time
in this file and this fix looks fine to me. NULL and 0 are already mixed so I'm
fine with using NULL. 

I do wish though, that the code in the file could be easier to maintain. goto
backwards. Bah! 

Anyway, r=bratell for whatever that's worth.

Comment 3

[Brendan Eich [:brendan]]

Kenton, let's get this into the trunk (I'm about to sr= the patch, you can r= it
too and check it in there) and propose it for 1.0 to [e-mail].

/be

Comment 4

[Brendan Eich [:brendan]]

Comment on attachment 80119 [details] [diff] [review]
Proposed fix.

bletcherous control flow, good fix. Thanks, sr=[e-mail].

/be

Comment 5

[Kenton Hanson (gone)]

Comment on attachment 80119 [details] [diff] [review]
Proposed fix.

r=khanson

Comment 6

[Steven Cole]

Kenton, can you check this in, please?  I don't have write access to mozilla's
cvs server.  (And I don't want it either...)

Also, should I email drivers about getting this into RC2, or are you?

--scole

Comment 7

[Kenton Hanson (gone)]

Committed to trunk.

Comment 8

[Asa Dotzler [:asa]]

Is this needed for 1.0? Brendan, wanna shephard this one or pull it off the
list? Thanks.

Comment 9

[chris hofmann]

Comment on attachment 80119 [details] [diff] [review]
Proposed fix.

a=chofmann for the 1.0 b branch

Comment 10

[Kenton Hanson (gone)]

Not a crasher, but low risk.  Less than ten lines change.  Initializes pointers
to  NULL and checks, to free them on exit from the routine.  Memory leak occurs
for rare occurrances of binary<->decimal conversions, i.e., most conversions do
not cause this leak. Has baked on the trunk for a week. Has sr=brendan,
r=khanson, a=choffman

Comment 11

[Kenton Hanson (gone)]

I am reopening this bug because I prematurely marked it FIXED.

Comment 12

[Jaime Rodriguez, Jr.]

Do we know how much is this leaking? This should defintely land on the trunk
first, and bake before we take it to the branch.

Comment 13

[Asa Dotzler [:asa]]

Jaime, this has been on the trunk for a week. It works. It's low risk. It should
land on the branch.

Comment 14

[Jaime Rodriguez, Jr.]

If its been on the trunk for a week, why is it marked "Reopened"? 

Also if the following statement is true, "Memory leak occurs for rare
occurrances of binary<->decimal conversions, i.e., most conversions do not cause
this leak", than why should we roll the dice and take the risk on this one?
Again, how large is this leak on the "rare occurrances" when it occurs?

Comment 15

[Kenton Hanson (gone)]

I REOPENED it because I was told that it should not be marked FIXED until it
lands on the branch.  The patch has been in the trunk since 4/24.  The occurence
of the leak is rare.  How rare?  I will have to do a detailed analysis of
jsdtoa.c.  Give me 24 hrs.

Comment 16

[Steven Cole]

The typical leak is 24 to 32 bytes; it happens only during the execution of the
"toFixed" method of Number objects within JavaScript, and only when the
execution of that routine results in digits truncated at the right with only
zeros remaining.

(In other words, something like (0.0000003423).toFixed(2), which results in "0.00".)

How often does this happen in real web sites?  Not known.  It's certainly
trivial to write a JS fragment which consumes all of a system's ram via this
leak, but any old infinite loop will bring mozilla to its knees anyway, so this
argument doesn't have much strength.

We were encountering this leak during the execution of one of our small
applications written in JS; we run in an extremely ram-limited environment and
any leak at all is deadly (more for its impact on heap fragmentation, though). 
But we're not mozilla, either.

--scole

Comment 17

[Jaime Rodriguez, Jr.]

Thanks for the information Steven and Kenton.

Kenton - For bugs checked into the trunk, pls mark as Resolved/Fixed. If you'd
like to get them on the 1.0 branch, pls submit for ADT approval (in addition to
Drivers), by adding adt1.0.0 keyword to the bug. This will get it on our radar.
thanks!

Comment 18

[Phil Schwartau]

> Kenton - For bugs checked into the trunk, pls mark as Resolved/Fixed.
> If you'd like to get them on the 1.0 branch, pls submit for ADT approval
> (in addition to Drivers), by adding adt1.0.0 keyword to the bug.
> This will get it on our radar.

Yes, but to stay on the component's radar the bug should not be 
marked "Fixed" until all work on it is finished. I was the one 
who advised Kenton not to mark this bug "Fixed" yet, since this
is the pattern everyone else follows:

1) Check into trunk and add a comment indicating this
2) Leave the bug open and petition ADT
3) Wait for ADT approval
4) Check into branch and add a comment indicating this
5) Now mark the bug "Fixed" and add "fixed1.0.0" keyword


EXAMPLES

bug 111576: on trunk 4/11, on branch 4/16, marked "Fixed" on 4/16
http://bugzilla.mozilla.org/show_activity.cgi?id=111576

bug 140974: on trunk 4/30, on branch 5/1, marked "Fixed" on 5/1  
http://bugzilla.mozilla.org/show_activity.cgi?id=140974

Comment 19

[Jaime Rodriguez, Jr.]

Phil - Can you verify this has been fixed on the trunk, and caused no known
regressions.

Comment 20

[Phil Schwartau]

Yes, it looks good: I've tested both the debug and optimized JS shell
with this fix, and got no test regressions. I'm only getting the 
known failures we have, from other bugs -

Comment 21

[Jaime Rodriguez, Jr.]

adt1.0.0+ (on ADT's behalf) approval for checkin to the 1.0 branch. Pls check
this in to 1.0 branch today, then add the fixed1.0.0 keyword.

Comment 22

[Kenton Hanson (gone)]

Checked into 1.0.0 branch.

Comment 23

[Phil Schwartau]

Patch verified on 1.0.0 branch; adding verified1.0.0 keyword -