351510 - Remove USE_MOZ_THREAD code from mozilla/security/lib/jar

Status: RESOLVED FIXED

(NSS :: Tools, defect, P3)

Description

[(no longer active)]

As per bug #337361 comment #3, the usage of USE_MOZ_THREAD preprocessor macro should be removed from mozilla/security/lib/jar.  The code must be tested to make sure the new code works the same as the old code, except for using the "mozilla" thread to run code using the NSS cert DB (see also bug #337361 comment #6.)

Comment 1

[(no longer active)]

Attachment: Remove usage of USE_MOZ_THREAD

This patch removes usage of USE_MOZ_THREAD from jarver.c and jarsign.c.  It seems that with this patch, the jarevil.h and jarevil.c files won't be necessary any more.  Can someone confirm this, please?  Look at the following (after applying this patch):

akhgari@AKHGARI /cygdrive/e/mozilla
$ grep -rn jarevil *
security/nss/lib/jar/CVS/Entries:7:/jarevil.c/1.8/Sun Apr 25 15:03:11 2004//TNSS_3_11_20060831_TAG
security/nss/lib/jar/CVS/Entries:8:/jarevil.h/1.3/Sun Apr 25 15:03:11 2004//TNSS_3_11_20060831_TAG
security/nss/lib/jar/jarevil.c:52:#include "jarevil.h"
security/nss/lib/jar/jarevil.h:38: *  jarevil.h
security/nss/lib/jar/manifest.mn:49:    jarevil.c \

Comment 2

[(no longer active)]

Accepting this bug (shouldn't assigning it to myself have changed its status to ASSIGNED?)

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 236924 [details] [diff] [review]
Remove usage of USE_MOZ_THREAD

r=nelsonb

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Ehsan,  In answer to your question, yes, this patch obviates jarevil.*
and The patch should also remove any mention of jarevil.h or jarevil.c
from the manifest.mn file.  When this patch is committed, we can "cvs 
remove" the two jarevil files, making libJAR less evil.  :)

I think the next step is to run a memory leak checking tool on signtool
to find any leaks in libJAR.  I expect there are lots.

Thanks for your patch.

Comment 5

[(no longer active)]

(In reply to comment #4)
> Ehsan,  In answer to your question, yes, this patch obviates jarevil.*
> and The patch should also remove any mention of jarevil.h or jarevil.c
> from the manifest.mn file.  When this patch is committed, we can "cvs 
> remove" the two jarevil files, making libJAR less evil.  :)

OK, I'll post a new patch with the corrected manifest.mn file in a moment.

> I think the next step is to run a memory leak checking tool on signtool
> to find any leaks in libJAR.  I expect there are lots.

What tools are used in the Mozilla community for this purpose?

Comment 6

[(no longer active)]

Attachment: Remove usage of USE_MOZ_THREAD and correct manifest.mn

This patch removed jarevil.c reference from manifest.mn.  Other than that, it's identical to the previous one (attachment #236924 [details] [diff] [review]).

Comment 7

[(no longer active)]

Attachment: Remove usage of USE_MOZ_THREAD and correct manifest.mn (revised)

This patch fixes a number of compilation errors in the previous one (attachment #237303 [details] [diff] [review]).

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 237308 [details] [diff] [review]
Remove usage of USE_MOZ_THREAD and correct manifest.mn (revised)

Please tell me how you've tested this.  
Have you used signtool?
If so, what signtool commands have you tested?

As for leak tools, use any you have.  There are many out there.  Some known to me include:
dbx checkleaks command
purify
valgrind
Some malloc libraries have a debug feature which is called at exit and prints a report of leaked blocks.
When leak testing, be sure to set the environment variable NSS_DISABLE_ARENA_FREE_LIST to 1.

Comment 9

[(no longer active)]

(In reply to comment #8)
> (From update of attachment 237308 [details] [diff] [review] [edit])
> Please tell me how you've tested this.  

Well, I've only got Firefox to compile and run successfully.  I'm not sure what I should do in Firefox to test the libJAR code in the NSS module.

> Have you used signtool?
> If so, what signtool commands have you tested?

Nope.  I am not familiar with signtool, and I definitely don't know what commands it accepts and how it should be tested.  I need help in this regard.

> As for leak tools, use any you have.  There are many out there.  Some known to
> me include:
> dbx checkleaks command
> purify
> valgrind
> Some malloc libraries have a debug feature which is called at exit and prints a
> report of leaked blocks.

I have always stuck with VC's memory allocation tracking in debug builds, but I'm not too sure how to control the VC compiler's parameters in a Firefox tree to make sure those are active.  :-(

And I have got used to using smart pointers (from the Boost library) in place of raw pointers, and also use classes which implement the RAII (Resource Allocation Is Initialization) paradigm, so I don't expect a lot of leaks in the codes that I myself write (although from time to time I get some leaks left out.)

> When leak testing, be sure to set the environment variable
> NSS_DISABLE_ARENA_FREE_LIST to 1.  

OK, thanks for the tip.

One of the areas in Firefox that I'm willing to invest some time in is cutting down some memory usage, and fixing leaks can be a big step at this.  I appreciate if you give me some hints in this regard.  (As you've probably figured out already, I'm new to the Mozilla code tree.)

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Unifdef USE_MOZ_THREAD (bug 351510) and remove dead code (bug 351443).
Patch contributed by [e-mail]. r=nelson, alexei.volkov
Modified Files: jarint.h jarsign.c jarver.c manifest.mn
Removed Files: jarevil.c jarevil.h

Removing jarevil.c;      new revision: delete; previous revision: 1.8
Removing jarevil.h;      new revision: delete; previous revision: 1.3
Checking in jarint.h;    new revision: 1.5; previous revision: 1.4
Checking in jarsign.c;   new revision: 1.6; previous revision: 1.5
Checking in jarver.c;    new revision: 1.13; previous revision: 1.12
Checking in manifest.mn; new revision: 1.5; previous revision: 1.4