Closed Bug 138666 Opened 22 years ago Closed 22 years ago

Another Memory Leak in jsdtoa.c

Categories

(Core :: JavaScript Engine, defect)

x86
Windows NT
defect
Not set
normal

Tracking

()

VERIFIED FIXED

People

(Reporter: scole, Assigned: khanson)

References

Details

(Keywords: js1.5, memory-leak, Whiteboard: [adt2])

Attachments

(1 file)

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.
Attached patch Proposed fix.Splinter Review
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 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.
Keywords: mlk, patch
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 drivers@mozilla.org.

/be
Blocks: 138000
Keywords: js1.5, mozilla1.0+
Comment on attachment 80119 [details] [diff] [review]
Proposed fix.

bletcherous control flow, good fix. Thanks, sr=brendan@mozilla.org.

/be
Attachment #80119 - Flags: superreview+
No longer blocks: 138000
Blocks: 138000
Comment on attachment 80119 [details] [diff] [review]
Proposed fix.

r=khanson
Attachment #80119 - Flags: review+
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
Committed to trunk.
Is this needed for 1.0? Brendan, wanna shephard this one or pull it off the
list? Thanks.
Comment on attachment 80119 [details] [diff] [review]
Proposed fix.

a=chofmann for the 1.0 b branch
Attachment #80119 - Flags: approval+
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
Status: NEW → RESOLVED
Closed: 22 years ago
Keywords: adt1.0.0
Resolution: --- → FIXED
I am reopening this bug because I prematurely marked it FIXED.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Whiteboard: [adt2]
Jaime, this has been on the trunk for a week. It works. It's low risk. It should
land on the branch.
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?
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.
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
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!
Status: REOPENED → RESOLVED
Closed: 22 years ago22 years ago
Resolution: --- → FIXED
Whiteboard: [adt2] → [adt2] [Needs ADT approval]
> 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
Phil - Can you verify this has been fixed on the trunk, and caused no known
regressions.
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 -
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.
Keywords: adt1.0.0adt1.0.0+
Whiteboard: [adt2] [Needs ADT approval] → [adt2]
Checked into 1.0.0 branch.
Keywords: fixed1.0.0
Patch verified on 1.0.0 branch; adding verified1.0.0 keyword -
Status: RESOLVED → VERIFIED
Keywords: verified1.0.0
Flags: testcase-
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: