363827 - NSS memory leak checking.

Status: RESOLVED FIXED

(NSS :: Test, defect, P2)

Description

[Slavomir Katuscak]

Need to implement memory leak checking and integrate into nightly testing suite.

Comment 1

[Slavomir Katuscak]

Attachment: Memory leak checking implementation.

This is the current version of memory leak checking implementation. I'm still testing it and I would like analyze all found leaks before I check ti into CVS. It's possible that there will be some changes. 

There are some special details of usage - script is not added to all.sh by default, needs to be specified over TESTS variable (bug 335752), because we don't expect everyone running test suite to run also memory leak tests. Also there should be defined paths to correct versions of DBX (on Solaris), because current paths contains incorrect versions. Also Valgrind should be in path (on Linux).

I was testing on these DBX version:
Sparc:
/usr/dist/pkgs/sunstudio_sparc,v11.0/SUNWspro/bin/dbx
X86-64:
/usr/dist/pkgs/sunstudio_i386,v11.0/SUNWspro/prod/bin/amd64/dbx 
X86-32:
/usr/dist/pkgs/sunstudio_i386,v11.0/SUNWspro/prod/bin/dbx 

Also I plan to test it on Tinderbox before check in (bug 363197).

Comment 2

[Slavomir Katuscak]

Attachment: Memory leak checking implementation.

Some small fixes. Contains also list of all known leaks in ignore list. Ignored leaks are also logged, but test isn't marked as failed. It's good for leaks which we don't plan to fix in short time, otherwise we will have always red status.

Comment 3

[Christophe Ravel]

Could you add some comments in your code to describe what each function is doing (at least) ?

Comment 4

[Slavomir Katuscak]

Attachment: Memory leak checking implementation + comments.

Comment 5

[Christophe Ravel]

Nelson is currently not available for the review.
It is very important that we have the results from the memory leak tests before the code freeze for NSS 3.11.6.
I suggest that we seek for a second review from Glen.

Nelson will still be able to comment on this patch when he comes back. We will keep this bug opened until we have the final review from Nelson.

Comment 6

[Slavomir Katuscak]

We already have results from these tests, I have runned them on most of our testing machines and for every found leak I reported bug (or found bug ID if already reported). You can see in patch, that there is list of all known leaks with their bug IDs.

Comment 7

[glen beasley]

Comment on attachment 253371 [details] [diff] [review]
Memory leak checking implementation + comments.

patch appears to be quite logical. I did not test though.

Comment 8

[Slavomir Katuscak]

Commited to trunk:

RCS file: /cvsroot/mozilla/security/nss/tests/memleak/ignored,v
done
Checking in memleak/ignored;
/cvsroot/mozilla/security/nss/tests/memleak/ignored,v  <--  ignored
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/tests/memleak/memleak.sh,v
done
Checking in memleak/memleak.sh;
/cvsroot/mozilla/security/nss/tests/memleak/memleak.sh,v  <--  memleak.sh
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/tests/memleak/sslreq.dat,v
done
Checking in memleak/sslreq.dat;
/cvsroot/mozilla/security/nss/tests/memleak/sslreq.dat,v  <--  sslreq.dat
initial revision: 1.1
done

Comment 9

[Slavomir Katuscak]

Commited to branch:

Checking in memleak/ignored;
/cvsroot/mozilla/security/nss/tests/memleak/ignored,v  <--  ignored
new revision: 1.1.2.1; previous revision: 1.1
done
Checking in memleak/memleak.sh;
/cvsroot/mozilla/security/nss/tests/memleak/memleak.sh,v  <--  memleak.sh
new revision: 1.1.2.1; previous revision: 1.1
done
Checking in memleak/sslreq.dat;
/cvsroot/mozilla/security/nss/tests/memleak/sslreq.dat,v  <--  sslreq.dat
new revision: 1.1.2.1; previous revision: 1.1
done

Comment 10

[Slavomir Katuscak]

Attachment: Update of ignored stacks list.

I started 4 Tinderboxes with memory leak checking on NSS_3_11_BRANCH. 
I found there 3 problems:

1. Some small implementation details like strdup and _strdup are different, needed to add new stacks.

2. DBX has limit of 16 lines in stacks. When there are more lines, first ones (main(),...) are skipped, it needs to add there more variants of the same stack (onese where first line is not 'main').

3. Fix for bug 367384 was checked only into the trunk, so bug is still in the branch. As it's known leak, I would like to have it in ignored list.

Comment 11

[Slavomir Katuscak]

I'm thinking about detection of bug number for each found stack, it's possible to parse it from ignored stacks list and it can make analyse of resuls much easier.

Also I want to ask you for permission to update this list without patch reviews, especially for reasons described in previous comment, sometimes it takes too much time to wait for 2 reviews when Tinderbox is failing and this list doesn't affect functionality. 

Comment 12

[Christophe Ravel]

There is a risk that we would ignore new stacks with this new patch. Could you try to increase the number of lines in the stack DBX can display ?
I see that we use the following parameters for DBX:

check -leaks -match 16 -frames 16

Is it related to the number of lines displayed ?
Can we increase this limit ?

Comment 13

[Slavomir Katuscak]

Yes, it's related to these parameters. There is limitation to 16 frames, it's already set to maximum. Another option is to hack DBX code and increase this limit there.

Comment 14

[Alexei Volkov]

Comment on attachment 254583 [details] [diff] [review]
Update of ignored stacks list.

I'm not sure about sftk_fipsPowerUpSelfTest. The rest looks ok to my. Glen, could you please review.

Comment 15

[Slavomir Katuscak]

Alexei, this sftk_fipsPowerUpSelfTest is from bug 361089 (sorry, I wrote wrong bug id in comment 10). This bug is already fixed, but patch was checked only into the trunk, no to the branch. 

Comment 16

[Slavomir Katuscak]

Attachment: Leak counter fix + bug id feature.

1. fixed problem with leak counter (need to use temporary file, because pipe causes forking and counter values were lost)
2. added feature to report bug id of found leak (if found in ignore list)

Comment 17

[Slavomir Katuscak]

Attachment: Fix status reporting bug in previous patch.

Comment 18

[Slavomir Katuscak]

Attachment: Update of ignored stacks list v2.

Added new found leak from 3.11.6 release build.

I would like to keep all known leaks in this list. 
I prepared reporting script for nighlty testing which will detect and print bug IDs of all found leaks from this list.

Comment 19

[Slavomir Katuscak]

Trunk:
Checking in ignored;
/cvsroot/mozilla/security/nss/tests/memleak/ignored,v  <--  ignored
new revision: 1.2; previous revision: 1.1
done
Checking in memleak.sh;
/cvsroot/mozilla/security/nss/tests/memleak/memleak.sh,v  <--  memleak.sh
new revision: 1.3; previous revision: 1.2
done

Branch:
Checking in ignored;
/cvsroot/mozilla/security/nss/tests/memleak/ignored,v  <--  ignored
new revision: 1.1.2.2; previous revision: 1.1.2.1
done
Checking in memleak.sh;
/cvsroot/mozilla/security/nss/tests/memleak/memleak.sh,v  <--  memleak.sh
new revision: 1.1.2.2; previous revision: 1.1.2.1
done