85267 - memory leak in jsdtoa.c

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Mike Epstein]

I'm seeing a few 28-byte and 36-byte memory leaks, all allocated from Balloc, 
called by various functions (d2b, i2b, mult, lshift, diff) in jsbtoa.c.  A 
typical stack trace from dbmalloc looks like this:

0997EFD4 unknown                      malloc(143047)      36 4C0FA109020000
00000023 /home/epstein/tm/dist/SunOS5.7_x86_gcc_NATIVE_DBG.OBJ/bin/tmmain malloc
00000089 /home/epstein/run/bin/libjs.so Balloc
00000092 /home/epstein/run/bin/libjs.so diff
00000f3b /home/epstein/run/bin/libjs.so JS_dtoa
000000e1 /home/epstein/run/bin/libjs.so JS_dtostr
0000009b /home/epstein/run/bin/libjs.so js_NumberToString
000000f4 /home/epstein/run/bin/libjs.so js_ValueToString
00005aee /home/epstein/run/bin/libjs.so js_Interpret
000002d5 /home/epstein/run/bin/libjs.so js_Execute
00000022 /home/epstein/run/bin/libjs.so JS_ExecuteScript

From a cursory inspection, I think js_FinishDtoa() should be freeing the 
Bigints on the freelist.

Comment 1

[Phil Schwartau]

Formally confirming bug for consideration -

Comment 2

[Brendan Eich [:brendan]]

Hoping [e-mail] will come up with the patch -- I'm still hacking
hard on FASTLOAD_20010529_BRANCH.

/be

Comment 3

[Steve Taylor]

Attachment: CVS diff for potential patch

The memory leaks are due to the freelist array and p5s list not being cleared
down when the JavaScript engine is shutdown. You also need to call
js_FinishDtoa from within JS_ShutDown in jsapi.c.

Comment 4

[Brendan Eich [:brendan]]

Comment on attachment 59323 [details] [diff] [review]
CVS diff for potential patch

Dunno how khanson's rewrite of this code is coming, but the patch looks ok to
me, except for these nits:

0.  Please don't add tabs, even though jsdtoa.c has some already.  Separate
cleanup pass should not have to fix newly added tabs.

1.  Don't waste cycles initializing twice (the optimizer may take the redundant
initialization out, but it might not):

    int count = 0;
 . . .
    for (count = 0; ...

e.g. (similarly for temp).

2.  Use double indirection to avoid recomputing addresses.  E.g.,

    for (count = 0; count <= Kmax; count++)
    {
	Bigint **listp = &freelist[count];
	while ((temp = *listp) != NULL)
	{
	    *listp = temp->next;
	    FREE(temp);
	}
    }

3.  Use a keyword-paren style consistent with yourself, and with the rest of
the file: don't mix 'for (' with 'while('.

4.  Use diff -u for patches.

/be

Comment 5

[Kenton Hanson (gone)]

I am currently reviewing this patch.  I had previously written some code to
address thsis problem but the proposed patch appears to be a good solution.

khanson

Comment 6

[Kenton Hanson (gone)]

The proposed patch from item 3 deals only with returning the freelist memory on
exit from the Javascript engine at shutdown.  I don't know how necessary that
is, I presume all memory allocated to the engine would be returned at that time.  

The memory leak is elsewhere in jsdtoa.c.  By setting global counters for calls
to Balloc and Bfree I can see that a leak exists.  I will track it down and
provide a patch.

Comment 7

[Kenton Hanson (gone)]

sr=khanson

The proposed patch appears correct.  The memory leak I imagined was accounted
for by the p5s list.  

Comment 8

[Kenton Hanson (gone)]

Comment on attachment 59323 [details] [diff] [review]
CVS diff for potential patch

Excuse my mistaken sr=, I meant r =.

Comment 9

[Kenton Hanson (gone)]

Retargeting for 9.8.

Comment 10

[Morten Brix Pedersen]

Attachment: New corrected patch

Same patch, but with Brendan Eich's suggestions

Comment 11

[Kenton Hanson (gone)]

targeting 9.9.  I will re review the updated patch.

Comment 12

[Kenton Hanson (gone)]

Comment on attachment 63225 [details] [diff] [review]
New corrected patch

r=khanson

Is related to bug #120992.

Comment 13

[Daniel Bratell]

Comment on attachment 63225 [details] [diff] [review]
New corrected patch

> #ifdef MALLOC
> extern void *MALLOC(size_t);
>+extern void *FREE(size_t);
> #else
> #define MALLOC malloc
>+#define FREE free
> #endif

FREE should be guarded by an ifdef FREE and added to the documentation 
at the top of the file, where the MALLOC macro is described.

Also, FREE doesn't return a pointer so drop the '*' from the prototype.

>-static Bigint *p5s;
>+static Bigint *p5s = NULL;

statics are null from the beginning. No need to explicitly put NULL in it.

>+    Bigint *b1 = NULL, *p5 = NULL, *p51 = NULL;

All these are given values later in the function. It only costs extra cycles
nulling them before giving them their real value, and you stop compilers from 
detecting if they can be used without being initialized.

The rest looks fine.

Comment 14

[Daniel Bratell]

Comment on attachment 63225 [details] [diff] [review]
New corrected patch

Oh, and the argument to a free function is void*, not size_t. 

Just to clearify with what I meant by compilers not detecting ... Compilers
normally do flow control analysis and some warn you if you in some way can use
a variable that has not been given a value yet. This typically is a bug in the
code, but if the variable is given a dummy value (NULL for instance), when it's
declared the compiler can't detect this bug.

Comment 15

[Daniel Bratell]

Comment on attachment 63225 [details] [diff] [review]
New corrected patch

+    Bigint *temp = NULL;
too

Comment 16

[Steve Adamski (gone)]

Moved to Mozilla 1.2 as a temporary holding place, as bugs remaining on 0.9.9
are being scrutinized, now that the 
milestone has passed.

If this must be fixed for MachV, then it needs to be assigned the "nsbeta1"
keyword.

Comment 17

[Brendan Eich [:brendan]]

It's not good practice to move bugs with patches out to some remote milestone
pending some netscape.com-specific bug triage -- Mozilla is open source, the
module owner and peers must strive to keep patch-providers happy, or patch
providers will go away.

In this case, the patch apparently needs only sr= (no one asked me and I lost
track of this bug in my bugmail folder), and it should go in for 1.0, if not for
0.9.9.  If it had received sr= already, and if the patch provider had been cc'd,
it would have gone into 0.9.9 -- and there's still plenty of time for that
release, there's no reason to chop this off at the knees.

steve.taylor, morten, bratell: I'm cc'ing you all (I wonder if you lost track of
this bug too) -- is any of you willing to get sr= and ask [e-mail]
for approval for 0.9.9?

/be

Comment 18

[Brendan Eich [:brendan]]

Comment on attachment 63225 [details] [diff] [review]
New corrected patch

This isn't strictly necessary, and I hope it doesn't move four bytes out of BSS
and into idata:

-static Bigint *p5s;
+static Bigint *p5s = NULL;

Minimize change, or tell me why this is necessary, and sr=[e-mail].

/be

Comment 19

[Daniel Bratell]

I have this patch as a part of the patch to bug 120992 and can not (easily) 
check it in before that patch is through the review process. In my copy there is 
no p5s = NULL, but the rest looks the same. Since it's a static I wouldn't 
expect it to make any difference though. 

Comment 20

[Steven Cole]

Daniel,

Any hope that this can go in now that bug 120992 has been checked in?

--scole

Comment 21

[Daniel Bratell]

Good that you reminded me. When bug 120992 changed strategy I forgot about this
one. Patch checked in without the irrelevant parts as requested by reviewers.

Comment 22

[Phil Schwartau]

Patch check-in verified.

Mike, or anyone else; if this did not fix the leaks reported here,
please reopen this bug; thanks -