Last Comment Bug 391457 - libpkix does not check for object ref leak at shutdown
: libpkix does not check for object ref leak at shutdown
Status: RESOLVED FIXED
PKIX
:
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: trunk
: All All
: P1 enhancement (vote)
: 3.12
Assigned To: Alexei Volkov
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2007-08-08 16:16 PDT by Alexei Volkov
Modified: 2007-11-12 13:44 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch v1 (85.42 KB, patch)
2007-09-26 13:22 PDT, Alexei Volkov
nelson: review-
Details | Diff | Splinter Review
Patch v2 (105.65 KB, patch)
2007-10-03 14:37 PDT, Alexei Volkov
nelson: review-
Details | Diff | Splinter Review
Patch v3 (30.76 KB, patch)
2007-10-10 15:33 PDT, Alexei Volkov
nelson: review+
Details | Diff | Splinter Review
Ifdef user defined object type code. (64.13 KB, patch)
2007-10-26 12:20 PDT, Alexei Volkov
nelson: review-
Details | Diff | Splinter Review

Description Alexei Volkov 2007-08-08 16:16:10 PDT
almost every type of objects X in libpkix has associated with it information located in a block of memory equals to the size of PKIX_PL_Object that precedes object X. References to an object X are controlled by PKIX_INCRED and PKIX_DECRED macros and object ref counter which is the member of PKIX_PL_Object.

I didn't find a code that would check for ref leaks which seems to me quite logical thing to do.
Comment 1 Nelson Bolyard (seldom reads bugmail) 2007-08-08 17:44:41 PDT
Question for all recipients of this comment:

Does anyone know why PKIX object structures aren't all defined as 
containing the basic PKIX_PL_Object structure as the first member?

Does anyone know why we have to play pointer arithmetic games to covert
a PKIX_PL_Object into one of its subclasses, or to convert a subclass
object into the superclass?  

This seems like an insanely bad idea (having to do pointer arithmetic),
and I expect it will be the cause of many bugs in years to come.

Consider a project proposal to change libPKIX to make all subclass object
structures have the superclass object structure as the first member, so 
that the SAME pointer value that points to a subclass object also points 
to the superclass object, and only casting (no arithmetic) is needed to 
covert from one to the other.  

Is such a proposal too much work to contemplate for 3.12?  
Would it be a day, a week, a month?  months? 
If we don't do it before we release 3.12, will we ever have a chance to do 
it in a later release?  
Or will backward compatibility compel us to keep it that way for years?
Comment 2 Alexei Volkov 2007-09-26 13:22:47 PDT
Created attachment 282453 [details] [diff] [review]
Patch v1

Initial code update with feature that will allow to identify libpkix object leaks.

Test result with current patch shows that some libpkix tests have memory leaks, but all.sh(with libpkix tests excluded) tests passed without any leaks.

I also notice, that libpkix has race conditions: libpkix_buildthreads (multi threaded libpkix test) periodically fails with positive object counts.
Comment 3 Nelson Bolyard (seldom reads bugmail) 2007-09-27 16:20:29 PDT
Comment on attachment 282453 [details] [diff] [review]
Patch v1

1. This patch contains a one-line fix for a different bug.
I am referring to the one line addition to the macro PKIX_ERROR_FATAL,
in lib/libpkix/pkix/util/pkix_tools.h
Please attach that tiny patch to that bug, and request review.
I'd like that fix to get checked in separately from this huge patch.
Please do NOT commit that change as part of this huge patch.

2. The patch adds a single line to many files.  The single line is:

+        entry.objCounter = 0;

and the patch adds a single line to struct pkix_ClassTable_EntryStruct
in nss/lib/libpkix/pkix/util/pkix_tools.h

+	pkixErrorCode = descNum; \

r+=nelson for those changes (but be careful to separate the one line
addition to struct pkix_ClassTable_EntryStruct from the one line addition
to macro PKIX_ERROR_FATAL in lib/libpkix/pkix/util/pkix_tools.h).

3. r+=nelson for the added declaration of FUNCTION: PKIX_Error_GetErrorCode
to file nss/lib/libpkix/include/pkix_util.h

The changes described in items 2 and 3 above can be checked in now.

In the next comment, I will review the parts of this patch that change
these files:

nss/lib/libpkix/include/pkix_errorstrings.h
nss/lib/libpkix/pkix_pl_nss/system/pkix_pl_lifecycle.c
nss/lib/libpkix/pkix_pl_nss/system/pkix_pl_object.c
Comment 4 Nelson Bolyard (seldom reads bugmail) 2007-09-27 16:45:59 PDT
Comment on attachment 282453 [details] [diff] [review]
Patch v1

In comment 3, I gave r+ for about 80% of this patch.  
This comment applies to the parts in 

nss/lib/libpkix/include/pkix_errorstrings.h
nss/lib/libpkix/pkix_pl_nss/system/pkix_pl_lifecycle.c
nss/lib/libpkix/pkix_pl_nss/system/pkix_pl_object.c

1. in function pkix_pl_lifecycle_ObjectLeakCheck, in DEBUG builds only,
I'd like to see this function output (to stderr, or to a file named in 
an environment variable, if present) one line for each class of object
that had leaked instances.  It should say something like

Class NAMENAMENAMENAME leaked NNN objects of size DDDDD bytes, total = DDDDD bytes

I realize that this will require having access to an array of class names. 
I think we should have one already, but if not, it should be possible to 
create one trivially using the existing macros. 

It would also be good to print out a total number of leaked objects of 
all known types. 

2. ifdef out all the "user defined class" stuff.

3. This patch gets rid of SOME (but not all) references to a variable
named refCountTotal.  It should eliminate all references and the definition
of that variable.

4. Function PKIX_PL_Object_DecRef has the following erroneous code:

>         refCountError = (objectHeader->references == 0);
> 
>         if (!refCountError) {
>                 objectHeader->references--;

This is errneous for several reasons, including:
a) the decrement is not atomic
b) the value tested for zero is not guaranteed to be the same value 
either immediately before or immediately after the decrement
c) the possibility of a negative reference count value is not considered.

This code should ONLY use PR_Atomic functions to manipulate the value
and the test should be done on a value that is guaranteed to be the
same as the value of the ref counter immediately before, or immediately
after the decrement.  The function PR_AtomicDecrement will perform an
atomic decrement, and will return a number that is guaranteed to be 
the decremented value (the result of the decrement).  The code should
use that function, and test the result for being less than zero to 
detect an error.

The code to increment the reference count should likewise use 
PR_AtomicIncrement and should test the result, rather than testing
the value of the variable non-atomically. 

5. Function PKIX_PL_Object_DecRef locks the object,

819         PKIX_CHECK(pkix_LockObject(object, plContext),
820                     PKIX_ERRORLOCKINGOBJECT);

then calls pkix_pl_Object_Destroy, which unlocks the object

190         PKIX_CHECK(pkix_UnlockObject(object, plContext),
191                     PKIX_ERRORUNLOCKINGOBJECT);

then destroys the lock and frees it.  That locking and unlocking 
shown above is unnecessary and arguably wrong.  Please remove it.
Comment 5 Alexei Volkov 2007-09-28 10:46:35 PDT
> 1. in function pkix_pl_lifecycle_ObjectLeakCheck, in DEBUG builds only,
> I'd like to see this function output (to stderr, or to a file named in 
> an environment variable, if present) one line for each class of object
> that had leaked instances.  It should say something like
> 
> Class NAMENAMENAMENAME leaked NNN objects of size DDDDD bytes, total = DDDDD
> bytes
> 
> I realize that this will require having access to an array of class names. 
> I think we should have one already, but if not, it should be possible to 
> create one trivially using the existing macros. 
> 
> It would also be good to print out a total number of leaked objects of 
> all known types.
libpkix already has a structure one member of which is a string(a char*) that has class name. I'm going to use it to cast from class type number to class type description.
  

> 
> 2. ifdef out all the "user defined class" stuff.

There are quite a few places where ifdef should occurred. I prefer to do it in a separate patch. For that I've filed a bug 397825 for that matter. 


> 4. Function PKIX_PL_Object_DecRef has the following erroneous code:
> 
> >         refCountError = (objectHeader->references == 0);
> > 
> >         if (!refCountError) {
> >                 objectHeader->references--;
> 
> This is errneous for several reasons, including:
> a) the decrement is not atomic
> b) the value tested for zero is not guaranteed to be the same value 
> either immediately before or immediately after the decrement
> c) the possibility of a negative reference count value is not considered.
> 
> This code should ONLY use PR_Atomic functions to manipulate the value
> and the test should be done on a value that is guaranteed to be the
> same as the value of the ref counter immediately before, or immediately
> after the decrement.  The function PR_AtomicDecrement will perform an
> atomic decrement, and will return a number that is guaranteed to be 
> the decremented value (the result of the decrement).  The code should
> use that function, and test the result for being less than zero to 
> detect an error.
> 
> The code to increment the reference count should likewise use 
> PR_AtomicIncrement and should test the result, rather than testing
> the value of the variable non-atomically. 
> 
> 5. Function PKIX_PL_Object_DecRef locks the object,
> 
> 819         PKIX_CHECK(pkix_LockObject(object, plContext),
> 820                     PKIX_ERRORLOCKINGOBJECT);
> 
> then calls pkix_pl_Object_Destroy, which unlocks the object
> 
> 190         PKIX_CHECK(pkix_UnlockObject(object, plContext),
> 191                     PKIX_ERRORUNLOCKINGOBJECT);
> 
> then destroys the lock and frees it.  That locking and unlocking 
> shown above is unnecessary and arguably wrong.  Please remove it.
> 

Bug 397830 is filed to take care of issues described as items 4 and 5.
The fix will be in a separate patch. 
Comment 6 Alexei Volkov 2007-09-28 13:50:44 PDT
> Class NAMENAMENAMENAME leaked NNN objects of size DDDDD bytes, total = DDDDD
> bytes

Although size of leaks is nice to have, it is not an absolute MUST have. 

At this time we would no be able to tell the size of objects since we do not have static object size information. The size of object that libpkix allocated gets defined only at the time PKIX_PL_Object_Alloc is called.

PKIX_PL_Object_Alloc(
        PKIX_TYPENUM type,
        PKIX_UInt32 size,
        PKIX_PL_Object **pObject,
        void *plContext)

Comment 7 Nelson Bolyard (seldom reads bugmail) 2007-09-28 14:27:39 PDT
I disagree that the size information is merely nice-to-have.
There are several simple ways it can be done, including (not limited to)
a) make PKIX_PL_Object_Alloc store the size in the object header, or
b) store the size in the array of information known about object types
(the same array in which the object type name string is).
Comment 8 Alexei Volkov 2007-09-28 14:53:43 PDT
This (In reply to comment #7)
> I disagree that the size information is merely nice-to-have.

Well, IMO the count of the objects is the primary objectives, that
directly corresponds to the amount of memory, that was leaked. From the code that saw, I've concluded, that size is directly correlates with objects types
(I didn't find any tricky memory allocations). And so knowing the size of the structure will provide an info about the number of byte that been leaked.

> There are several simple ways it can be done, including (not limited to)
> a) make PKIX_PL_Object_Alloc store the size in the object header, or
It is simple to do, but since we don't have connections between system class array and the allocated objects, the saved information would be useless for
leak detector function from pkix_pl_lifecycle.c .

> b) store the size in the array of information known about object types
> (the same array in which the object type name string is).
This is better solution, but will require synchronization - a compromise. We will need to have looks for it.

I suggest that in the long run we get rid of size parameter in Object_Alloc and  use size info stored in systemClass info array. I believe it is doable, because of two reasons: * size of object and its type have 1:1 correlation ship; * Object_Alloc is not use for any other operations.  

So, IMO the way to go is to create a static table of sizes and use it for object allocation.
Comment 9 Alexei Volkov 2007-09-28 14:55:16 PDT
> So, IMO the way to go is to create a static table of sizes and use it for
                                       ^^^^^^ constant global table

Comment 10 Nelson Bolyard (seldom reads bugmail) 2007-09-28 15:17:27 PDT
libPKIX is designed so that that information about the different classes of 
objects is dynamically registered at startup time.  I think we should preserve
that design.  So, I'd say that the size of the object should be part of the
object class registration.  It can be added to the table of registered object
classes along with their names.  Object_Alloc can (initially) ASSERT that the
size it receives as an argument matches the registered size.  Once we know 
that that assertion never fails, then we can eliminate the size argument from 
Object_Alloc.  

I think that, today, we developers are mostly unfamiliar with the different
libPKIX objects.  We don't know all their names, much less their sizes.  
And I suspect that their sizes differ a lot.  Some are small and others 
may be big.  So knowing that we leaked (say) 50 objects does not tell us 
much about the sizes.  A leak of 50 big objects needs to be fixed more 
urgently than a leak of 50 tiny ones.  I expect there will be some big 
surprises about object sizes when they are reported by this facility.
Comment 11 Alexei Volkov 2007-10-03 14:37:30 PDT
Created attachment 283440 [details] [diff] [review]
Patch v2

More code cleanup.
Added assertion to check if requested size for allocation is equal to size of the object. This is done to remove size from the list of argument of PKIX_PL_Object_Alloc.
Added logging(through PR_LOG) number of leaked object, and total leaked size of memory per object type.
Comment 12 Nelson Bolyard (seldom reads bugmail) 2007-10-04 21:58:22 PDT
Lexei, Your monster patch, "Patch v2", is really a number of separate, 
independent, patches all combined into one.  I don't want you to check them 
all in in one big monster checkin.  I want you to check them in as if they 
were separate patches, with separate checkin comments.  

In this review comment, I'm going to give r+ to a several SUBSETs of that 
big patch that I want you to commit separately.  PLEASE don't combine 
separate independent patches into one monster patch.  

These should be separate checkins:

1. lib/libpkix/include/pkix_util.h 
You created a public declaration for a formerly undeclared function.
Nothing depends on it, and it depends on nothing.  Please check it in all
by itself.   Please don't ask me to review that part of this patch again.
If you need to file a separate bug about that, do so.  

2. Your patch to lib/libpkix/pkix/util/pkix_tools.h includes a one-line
change to 
    #define PKIX_ERROR_FATAL(descNum)

I'm sure I gave that r+ before.  There is a separate bug for that problem.
Please check that one line patch in separately, and the checkin comment 
should say it is a patch for the bug about that problem.  Please do NOT 
check it in as part of the big patch for THIS bug.

3. The patch for lib/libpkix/pkix/util/pkix_tools.h also adds two members
to struct pkix_ClassTable_EntryStruct.  That is clearly tied to the MANY
files where the only change was to add two lines to them, lines like this:

         entry.description = "CertSelector";
+        entry.objCounter = 0;
+        entry.typeObjectSize = sizeof(PKIX_CertSelector);

Please checkin that change to struct pkix_ClassTable_EntryStruct, and the
changes to all the files where the ONLY change was the addition of those
two lines.  Check those in as a single checkin.  Do not check in any 
other changes together with those.  

r+ for those 3 separate checkin patch sets above.

When you have checked in the above 3 separate patch sets, you will 
still have the following files from this patch not yet checked in:

lib/nss/nssinit.c                     (This is  already checked in)
lib/libpkix/config.mk 
lib/libpkix/include/pkix_pl_system.h
lib/libpkix/include/pkixt.h  
lib/libpkix/pkix_pl_nss/system/pkix_pl_lifecycle.c
lib/libpkix/pkix_pl_nss/system/pkix_pl_object.c
lib/libpkix/pkix_pl_nss/system/pkix_pl_object.h

I'll review the patch to those ohter 6 files separately, next.
Comment 13 Nelson Bolyard (seldom reads bugmail) 2007-10-04 22:29:31 PDT
OK, now before I review the patches to those 6 other files, I need to make a
comment about all the patches that add 2 lines to the "RegisterSelf" 
functions for all the classes.  Every one of those functions is inefficient
and bloated.  Every one of those does these steps:

a) allocate a local, "automatic" uninitialized, struct on the stack.
b) initialize it with run time code (which is slower and bigger than using
a pre-initialized static constant struct.)
c) copy it to an identical struct that is in an array.  

It would be much more efficient if each one of those had a static const 
structure, initialized by the compiler, and copied that.  

The MOST efficient thing might be for each RegisterSelf function to have a 
static non-const structure, intialized by the compiler (not at run time),
and have the array be an array of pointers to those structs, not an array
of those structs, then have each function set the address of its static 
function into the array of pointers.  But that might not do the right thing
if NSS was Initiallized, Shutdown, and reinitialized several times.  

The static const struct that is copied would be a good compromise.  But it's
purely a bloat-reduction + efficiency thing, so it can wait.
Comment 14 Nelson Bolyard (seldom reads bugmail) 2007-10-04 22:41:48 PDT
Sorry, in comment 12, I mean Alexei, not Lexei.  
Comment 15 Nelson Bolyard (seldom reads bugmail) 2007-10-04 23:19:07 PDT
The patch to nss/lib/libpkix/include/pkixt.h removes 
    TYPEMACRO(HTTPCLIENT), \
from the PKIX_TYPES macro, but leaves 
    ERRMACRO(HTTPCLIENT), \
in the PKIX_ERRORCLASSES macro.  Why? 

Should it be removed from both?
Is there any reason to keep it in the ERRORCLASSES if it's not a PKIX_TYPE?

Should we merge the two macros PKIX_TYPES and PKIX_ERRORCLASSES into one?
We should sort both of them into alphabetical order, so that we can spot
names in one list but not in the other more quickly.  
Comment 16 Nelson Bolyard (seldom reads bugmail) 2007-10-05 00:13:01 PDT
Comment on attachment 283440 [details] [diff] [review]
Patch v2

In comment 12 above, I gave r+ to several parts of this patch.
They can be, and should be checked in now.
Here I am continuing the review from comment 12...

4. Changing the type of the first argument of PKIX_PL_Object_Alloc
nss/lib/libpkix/include/pkix_pl_system.h             - the only change 
nss/lib/libpkix/pkix_pl_nss/system/pkix_pl_object.c  - one line change 
 
If those changes were in a separate patch, I'd give it r+

5. Removing all the copies of this declaration 
        extern pkix_ClassTable_Entry systemClasses[PKIX_NUMTYPES];
from nss/lib/libpkix/pkix_pl_nss/system/pkix_pl_object.c and putting one copy 
of that declaration into nss/lib/libpkix/pkix_pl_nss/system/pkix_pl_object.h

If this was a separate patch, I'd give it r+

6. Adding function pkix_pl_Object_RegisterSelf(void *plContext) to 
nss/lib/libpkix/pkix_pl_nss/system/pkix_pl_object.c and adding a declaration 
of it into nss/lib/libpkix/pkix_pl_nss/system/pkix_pl_object.h 

If this was a separate patch, I'd give it r+

7. New PRLogModuleInfo *pkixLog is leaked.  It is never destroyed.  r-
Also since it's global, it doesn't need to be explicitly initialized to NULL.

8. All operations on objectHeader->references are still done non-atomically. 
Please do all operations on objectHeader->references atomically, as discussed 
in comment 4, item 4. We may yet get rid of the object lock.

9. pkix_pl_lifecycle_ObjectLeakCheck()
This function is defined unconditionally, not inside an ifdef.  
It has one caller, that is inside #ifdef DEBUG.  When DEBUG is not defined,
this static function is an orphan, and will generate warnings.  r-

I suggest that you call it unconditionally, and only ASSERT that the result 
is zero if NSS_STRICT_SHUTDOWN is in the environment.  

10. instead of #ifdef DEBUG_PKIX, I suggest that you test an environment 
variable and only call PR_NewLogModule if that variable is defined.  Then, 
after that, use a run-time check of pkixLog, rather than a compile time 
check of DEBUG_PKIX to decide whether or not to take actions that only are 
useful if we're logging.  This is OK even for non-DEBUG builds.
Comment 17 Alexei Volkov 2007-10-08 16:58:25 PDT
files approved in comment #12 have been integrated.
Comment 18 Alexei Volkov 2007-10-10 15:33:56 PDT
Created attachment 284379 [details] [diff] [review]
Patch v3

Implementation of suggested changes.
Comment 19 Nelson Bolyard (seldom reads bugmail) 2007-10-19 18:26:09 PDT
Comment on attachment 284379 [details] [diff] [review]
Patch v3

r=nelson.  I request some additional changes.

1. In new function pkix_pl_lifecycle_ObjectLeakCheck()
the expression systemClasses[typeCounter] appears 7 times in
a single loop, indexing the array 7 times with the same index
value.  Please use a pointer, and compute its value once at 
the beginning of the loop.  Better yet, initialize the pointer
before the loop, and just increment the pointer during the loop.

2. In function PKIX_PL_Object_IncRef, instead of 

    PKIX_Boolean refCountError = PKIX_FALSE;
and
    refCountError = (PR_AtomicIncrement(&objectHeader->references) <= 1);
    if (refCountError) {

Please use something like

    PKIX_Int32 refCount;

    refCount = PR_AtomicIncrement(&objectHeader->references);
    if (refCount <= 1) {

3. I'm not happy about the code for the so-called "user-defined types",
but I believe you're planning to remove all the code for those types 
soon, so I'll just ignore it for now.

> PKIX_Error *
> PKIX_PL_Object_Alloc(
>-        PKIX_UInt32 type,
>+        PKIX_TYPENUM type,
>         PKIX_UInt32 size,
>         PKIX_PL_Object **pObject,
>         void *plContext);
> 
> /*
>  * FUNCTION: PKIX_PL_Object_IsTypeRegistered
>  * DESCRIPTION:
>  *
>  *  Checks whether "type" has been registered by a previous call to
>  *  PKIX_PL_Object_RegisterType() and stores the Boolean result at "pBool".
>Index: lib/libpkix/include/pkixt.h
>===================================================================
>RCS file: /cvsroot/mozilla/security/nss/lib/libpkix/include/pkixt.h,v
>retrieving revision 1.7
>diff -U 10 -p -r1.7 pkixt.h
>--- lib/libpkix/include/pkixt.h	25 Sep 2007 00:48:08 -0000	1.7
>+++ lib/libpkix/include/pkixt.h	10 Oct 2007 22:31:43 -0000
>@@ -201,21 +201,20 @@ typedef int PKIX_Boolean;
>     TYPEMACRO(ERROR), \
>     TYPEMACRO(HASHTABLE), \
>     TYPEMACRO(LIST), \
>     TYPEMACRO(LOGGER), \
>     TYPEMACRO(MUTEX), \
>     TYPEMACRO(OID), \
>     TYPEMACRO(RWLOCK), \
>     TYPEMACRO(STRING), \
>     TYPEMACRO(CERTBASICCONSTRAINTS), \
>     TYPEMACRO(CERT), \
>-    TYPEMACRO(HTTPCLIENT), \
>     TYPEMACRO(CRL), \
>     TYPEMACRO(CRLENTRY), \
>     TYPEMACRO(DATE), \
>     TYPEMACRO(GENERALNAME), \
>     TYPEMACRO(CERTNAMECONSTRAINTS), \
>     TYPEMACRO(PUBLICKEY), \
>     TYPEMACRO(TRUSTANCHOR), \
>     TYPEMACRO(X500NAME), \
>     TYPEMACRO(HTTPCERTSTORECONTEXT), \
>     TYPEMACRO(BUILDRESULT), \
>@@ -292,21 +291,20 @@ typedef enum {     /* Now invoke all tho
>    ERRMACRO(BYTEARRAY), \
>    ERRMACRO(BIGINT), \
>    ERRMACRO(HASHTABLE), \
>    ERRMACRO(CERT), \
>    ERRMACRO(X500NAME), \
>    ERRMACRO(GENERALNAME), \
>    ERRMACRO(PUBLICKEY), \
>    ERRMACRO(DATE), \
>    ERRMACRO(TRUSTANCHOR), \
>    ERRMACRO(PROCESSINGPARAMS), \
>-   ERRMACRO(HTTPCLIENT), \
>    ERRMACRO(VALIDATEPARAMS), \
>    ERRMACRO(VALIDATE), \
>    ERRMACRO(VALIDATERESULT), \
>    ERRMACRO(CERTCHAINCHECKER), \
>    ERRMACRO(CERTSELECTOR), \
>    ERRMACRO(COMCERTSELPARAMS), \
>    ERRMACRO(TARGETCERTCHECKERSTATE), \
>    ERRMACRO(CERTBASICCONSTRAINTS), \
>    ERRMACRO(CERTPOLICYQUALIFIER), \
>    ERRMACRO(CERTPOLICYINFO), \
>Index: lib/libpkix/pkix_pl_nss/system/pkix_pl_lifecycle.c
>===================================================================
>RCS file: /cvsroot/mozilla/security/nss/lib/libpkix/pkix_pl_nss/system/pkix_pl_lifecycle.c,v
>retrieving revision 1.10
>diff -U 10 -p -r1.10 pkix_pl_lifecycle.c
>--- lib/libpkix/pkix_pl_nss/system/pkix_pl_lifecycle.c	9 Oct 2007 16:51:35 -0000	1.10
>+++ lib/libpkix/pkix_pl_nss/system/pkix_pl_lifecycle.c	10 Oct 2007 22:31:44 -0000
>@@ -39,20 +39,21 @@
>  *
>  * Lifecycle Functions for the PKIX PL library.
>  *
>  */
> 
> #include "pkix_pl_lifecycle.h"
> 
> PKIX_Boolean pkix_pl_initialized = PKIX_FALSE;
> pkix_ClassTable_Entry systemClasses[PKIX_NUMTYPES];
> PRLock *classTableLock;
>+PRLogModuleInfo *pkixLog = NULL;
> 
> /*
>  * PKIX_ALLOC_ERROR is a special error object hard-coded into the
>  * pkix_error.o object file. It is thrown if system memory cannot be
>  * allocated. PKIX_ALLOC_ERROR is immutable.
>  * IncRef, DecRef, and Settor functions cannot be called.
>  */
> 
> /* Keep this structure definition here for its is used only once here */
> struct PKIX_Alloc_Error_ObjectStruct {
>@@ -77,81 +78,113 @@ static PKIX_Alloc_Error_Object pkix_Allo
>         (PKIX_Error *)0,                /* PKIX_Error *cause */
>         (PKIX_PL_Object *)0,            /* PKIX_PL_Object *info */
>    }
> };
> 
> PKIX_Error* PKIX_ALLOC_ERROR(void)
> {
>     return &pkix_Alloc_Error_Data.error;
> }
> 
>+static PKIX_UInt32
>+pkix_pl_lifecycle_ObjectLeakCheck()
>+{
>+        int   typeCounter = 0;
>+        PKIX_UInt32 numObjects = 0;
>+        char  classNameBuff[128];
>+        char *className = NULL;
>+
>+        for (; typeCounter < PKIX_NUMTYPES; typeCounter++) {
>+
>+                numObjects += systemClasses[typeCounter].objCounter;
>+                
>+                if (!pkixLog || !systemClasses[typeCounter].objCounter) {
>+                    continue;
>+                }
>+                className = systemClasses[typeCounter].description;
>+                if (!className) {
>+                    className = classNameBuff;
>+                    sprintf(className, "Unknown(ref %d)", 
>+                            systemClasses[typeCounter].objCounter);
>+                }
>+
>+                PR_LOG(pkixLog, 1, ("Class %s leaked %d objects of "
>+                        "size %d bytes, total = %d bytes\n", className, 
>+                        systemClasses[typeCounter].objCounter,
>+                        systemClasses[typeCounter].typeObjectSize,
>+                        systemClasses[typeCounter].objCounter *
>+                        systemClasses[typeCounter].typeObjectSize));
>+        }
>+ 
>+        return numObjects;
>+}
>+
> /*
>  * PKIX_PL_Initialize (see comments in pkix_pl_system.h)
>  */
> PKIX_Error *
> PKIX_PL_Initialize(
>         PKIX_Boolean platformInitNeeded,
>         PKIX_Boolean useArenas,
>         void **pPlContext)
> {
>         void *plContext = NULL;
> 
>-        pkix_ClassTable_Entry nullEntry = {NULL};
>-        /* XXX currently using canned value for config dir; add to plContext */
>-
>         PKIX_ENTER(OBJECT, "PKIX_PL_Initialize");
> 
>         /*
>          * This function can only be called once. If it has already been
>          * called, we return a positive status.
>          */
>         if (pkix_pl_initialized) {
>             PKIX_RETURN(OBJECT);
>         }
> 
>         classTableLock = PR_NewLock();
>         if (classTableLock == NULL) {
>             return PKIX_ALLOC_ERROR();
>         }
> 
>-        /* we don't need to register OBJECT */
>-        systemClasses[PKIX_OBJECT_TYPE] = nullEntry;
>+        if (PR_GetEnv("NSS_STRICT_SHUTDOWN")) {
>+            pkixLog = PR_NewLogModule("pkix");
>+        }
>+        /*
>+         * Register Object, it is the base object of all other objects.
>+         */
>+        pkix_pl_Object_RegisterSelf(plContext);
> 
>         /*
>          * Register Error and String, since they will be needed if
>          * there is a problem in registering any other type.
>          */
>         pkix_Error_RegisterSelf(plContext);
>-        (void) pkix_pl_String_RegisterSelf(plContext);
>+        pkix_pl_String_RegisterSelf(plContext);
> 
> 
>         /*
>          * We register all other system types
>          * (They don't need to be in order, but it's
>          * easier to keep track of what types are registered
>          * if we register them in the same order as their
>          * numbers, defined in pkixt.h.
>          */
>-        (void) pkix_pl_BigInt_RegisterSelf(plContext);   /* 1-10 */
>-        (void) pkix_pl_ByteArray_RegisterSelf(plContext);
>-        /* already registered! pkix_Error_RegisterSelf(plContext); */
>-        (void) pkix_pl_HashTable_RegisterSelf(plContext);
>+        pkix_pl_BigInt_RegisterSelf(plContext);   /* 1-10 */
>+        pkix_pl_ByteArray_RegisterSelf(plContext);
>+        pkix_pl_HashTable_RegisterSelf(plContext);
>         pkix_List_RegisterSelf(plContext);
>         pkix_Logger_RegisterSelf(plContext);
>-        (void) pkix_pl_Mutex_RegisterSelf(plContext);
>-        (void) pkix_pl_OID_RegisterSelf(plContext);
>-        (void) pkix_pl_RWLock_RegisterSelf(plContext);
>-        /* already registered! pkix_pl_String_RegisterSelf(plContext); */
>+        pkix_pl_Mutex_RegisterSelf(plContext);
>+        pkix_pl_OID_RegisterSelf(plContext);
>+        pkix_pl_RWLock_RegisterSelf(plContext);
> 
>         pkix_pl_CertBasicConstraints_RegisterSelf(plContext); /* 11-20 */
>         pkix_pl_Cert_RegisterSelf(plContext);
>-        /* pkix_HttpClient_RegisterSelf(plContext); */
>         pkix_pl_CRL_RegisterSelf(plContext);
>         pkix_pl_CRLEntry_RegisterSelf(plContext);
>         pkix_pl_Date_RegisterSelf(plContext);
>         pkix_pl_GeneralName_RegisterSelf(plContext);
>         pkix_pl_CertNameConstraints_RegisterSelf(plContext);
>         pkix_pl_PublicKey_RegisterSelf(plContext);
>         pkix_TrustAnchor_RegisterSelf(plContext);
> 
>         pkix_pl_X500Name_RegisterSelf(plContext);   /* 21-30 */
>         pkix_pl_HttpCertStoreContext_RegisterSelf(plContext);
>@@ -180,21 +213,21 @@ PKIX_PL_Initialize(
>         pkix_ForwardBuilderState_RegisterSelf(plContext);
>         pkix_SignatureCheckerState_RegisterSelf(plContext);
>         pkix_NameConstraintsCheckerState_RegisterSelf(plContext);
>         pkix_DefaultRevocationChecker_RegisterSelf(plContext);
>         pkix_pl_LdapRequest_RegisterSelf(plContext);
>         pkix_pl_LdapResponse_RegisterSelf(plContext);
>         pkix_pl_LdapDefaultClient_RegisterSelf(plContext);
>         pkix_pl_Socket_RegisterSelf(plContext);
> 
>         pkix_ResourceLimits_RegisterSelf(plContext); /* 51-59 */
>-        (void) pkix_pl_MonitorLock_RegisterSelf(plContext);
>+        pkix_pl_MonitorLock_RegisterSelf(plContext);
>         pkix_pl_InfoAccess_RegisterSelf(plContext);
>         pkix_pl_AIAMgr_RegisterSelf(plContext);
>         pkix_OcspChecker_RegisterSelf(plContext);
>         pkix_pl_OcspRequest_RegisterSelf(plContext);
>         pkix_pl_OcspResponse_RegisterSelf(plContext);
>         pkix_pl_HttpDefaultClient_RegisterSelf(plContext);
>         pkix_VerifyNode_RegisterSelf(plContext);
> 
>         if (pPlContext) {
>             PKIX_CHECK(PKIX_PL_NssContext_Create
>@@ -210,28 +243,35 @@ cleanup:
> 
>         PKIX_RETURN(OBJECT);
> }
> 
> /*
>  * PKIX_PL_Shutdown (see comments in pkix_pl_system.h)
>  */
> PKIX_Error *
> PKIX_PL_Shutdown(void *plContext)
> {
>+        PKIX_UInt32 numLeakedObjects = 0;
>+
>         PKIX_ENTER(OBJECT, "PKIX_PL_Shutdown");
> 
>         if (!pkix_pl_initialized) {
>             /* The library was not initilized */
>             PKIX_RETURN(OBJECT);
>         }
> 
>         PR_DestroyLock(classTableLock);
> 
>+        numLeakedObjects = pkix_pl_lifecycle_ObjectLeakCheck();
>+
>+        if (PR_GetEnv("NSS_STRICT_SHUTDOWN")) {
>+            PORT_Assert(numLeakedObjects == 0);
>+        }
>+
>         if (plContext != NULL) {
>                 PKIX_PL_NssContext_Destroy(plContext);
>         }
> 
>         pkix_pl_initialized = PKIX_FALSE;
> 
>         PKIX_RETURN(OBJECT);
>-
> }
>Index: lib/libpkix/pkix_pl_nss/system/pkix_pl_object.c
>===================================================================
>RCS file: /cvsroot/mozilla/security/nss/lib/libpkix/pkix_pl_nss/system/pkix_pl_object.c,v
>retrieving revision 1.4
>diff -U 10 -p -r1.4 pkix_pl_object.c
>--- lib/libpkix/pkix_pl_nss/system/pkix_pl_object.c	6 Sep 2007 20:48:58 -0000	1.4
>+++ lib/libpkix/pkix_pl_nss/system/pkix_pl_object.c	10 Oct 2007 22:31:44 -0000
>@@ -36,25 +36,20 @@
>  * ***** END LICENSE BLOCK ***** */
> /*
>  * pkix_pl_object.c
>  *
>  * Object Construction, Destruction and Callback Functions
>  *
>  */
> 
> #include "pkix_pl_object.h"
> 
>-/* --Debugging---------------------------------------------------- */
>-
>-static PKIX_UInt32 refCountTotal = 0;
>-
>-                              
> /* --Class-Table-Initializers------------------------------------ */
> 
> /*
>  * Create storage space for 20 Class Table buckets.
>  * These are only for user-defined types. System types are registered
>  * separately by PKIX_PL_Initialize.
>  */
> 
> static pkix_pl_HT_Elem*
> pkix_Raw_ClassTable_Buckets[] = {
>@@ -180,23 +175,20 @@ pkix_pl_Object_Destroy(
>                     PKIX_RECEIVEDCORRUPTEDOBJECTARGUMENT);
> 
>         /* Attempt to delete an object still being used */
>         if (objectHeader->references != 0) {
>                 PKIX_ERROR_FATAL(PKIX_OBJECTSTILLREFERENCED);
>         }
> 
>         objectHeader->magicHeader = 0;
>         PKIX_DECREF(objectHeader->stringRep);
> 
>-        PKIX_CHECK(pkix_UnlockObject(object, plContext),
>-                    PKIX_ERRORUNLOCKINGOBJECT);
>-
>         /* Destroy this object's lock */
>         PKIX_OBJECT_DEBUG("\tCalling PR_DestroyLock).\n");
>         PR_DestroyLock(objectHeader->lock);
>         objectHeader->lock = NULL;
>         object = NULL;
> 
>         PKIX_FREE(objectHeader);
> 
> cleanup:
> 
>@@ -266,21 +258,20 @@ pkix_pl_Object_Equals_Default(
>  *  Returns NULL if the function succeeds.
>  *  Returns an Object Error if the function fails in a non-fatal way.
>  *  Returns a Fatal Error if the function fails in an unrecoverable way.
>  */
> static PKIX_Error *
> pkix_pl_Object_ToString_Default(
>         PKIX_PL_Object *object,
>         PKIX_PL_String **pString,
>         void *plContext)
> {
>-        extern pkix_ClassTable_Entry systemClasses[PKIX_NUMTYPES];
>         pkix_ClassTable_Entry *ctEntry = NULL;
>         PKIX_PL_String *formatString = NULL;
>         PKIX_PL_String *descString = NULL;
>         char *format = "%s@Address: %x";
>         char *description = NULL;
>         PKIX_UInt32 type;
> 
>         PKIX_ENTER(OBJECT, "pkix_pl_Object_ToString_Default");
>         PKIX_NULLCHECK_TWO(object, pString);
> 
>@@ -425,21 +416,20 @@ pkix_pl_Object_Hashcode_Default(
>  *  Returns NULL if the function succeeds.
>  *  Returns an Object Error if the function fails in a non-fatal way.
>  *  Returns a Fatal Error if the function fails in an unrecoverable way.
>  */
> PKIX_Error *
> pkix_pl_Object_RetrieveEqualsCallback(
>         PKIX_PL_Object *object,
>         PKIX_PL_EqualsCallback *pEqualsCallback,
>         void *plContext)
> {
>-        extern pkix_ClassTable_Entry systemClasses[PKIX_NUMTYPES];
>         PKIX_PL_Object *objectHeader = NULL;
>         pkix_ClassTable_Entry *ctEntry = NULL;
>         PKIX_PL_EqualsCallback func = NULL;
>         pkix_ClassTable_Entry entry;
> 
>         PKIX_ENTER(OBJECT, "pkix_pl_Object_RetrieveEqualsCallback");
>         PKIX_NULLCHECK_TWO(object, pEqualsCallback);
> 
>         PKIX_CHECK(pkix_pl_Object_GetHeader
>                     (object, &objectHeader, plContext),
>@@ -474,28 +464,66 @@ pkix_pl_Object_RetrieveEqualsCallback(
>                 } else {
>                         *pEqualsCallback = ctEntry->equalsFunction;
>                 }
>         }
> 
> cleanup:
> 
>         PKIX_RETURN(OBJECT);
> }
> 
>+/*
>+ * FUNCTION: pkix_pl_Object_RegisterSelf
>+ * DESCRIPTION:
>+ *  Registers PKIX_OBJECT_TYPE and its related functions with systemClasses[]
>+ * THREAD SAFETY:
>+ *  Not Thread Safe - for performance and complexity reasons
>+ *
>+ *  Since this function is only called by PKIX_PL_Initialize, which should
>+ *  only be called once, it is acceptable that this function is not
>+ *  thread-safe.
>+ *
>+ *  PKIX_PL_Object should have all function pointes to be to NULL: they
>+ *  work as proxy function to a real objects.
>+ *  
>+ */
>+PKIX_Error *
>+pkix_pl_Object_RegisterSelf(void *plContext)
>+{
>+        extern pkix_ClassTable_Entry systemClasses[PKIX_NUMTYPES];
>+        pkix_ClassTable_Entry entry;
>+
>+        PKIX_ENTER(ERROR, "pkix_pl_Object_RegisterSelf");
>+
>+        entry.description = "Object";
>+        entry.objCounter = 0;
>+        entry.typeObjectSize = sizeof(PKIX_PL_Object);
>+        entry.destructor = NULL;
>+        entry.equalsFunction = NULL;
>+        entry.hashcodeFunction = NULL;
>+        entry.toStringFunction = NULL;
>+        entry.comparator = NULL;
>+        entry.duplicateFunction = NULL;
>+
>+        systemClasses[PKIX_OBJECT_TYPE] = entry;
>+
>+        PKIX_RETURN(ERROR);
>+}
>+
> /* --Public-Functions------------------------------------------------------- */
> 
> /*
>  * FUNCTION: PKIX_PL_Object_Alloc (see comments in pkix_pl_system.h)
>  */
> PKIX_Error *
> PKIX_PL_Object_Alloc(
>-        PKIX_UInt32 type,
>+        PKIX_TYPENUM type,
>         PKIX_UInt32 size,
>         PKIX_PL_Object **pObject,
>         void *plContext)
> {
>         PKIX_PL_Object *object = NULL;
>         pkix_ClassTable_Entry *ctEntry = NULL;
>         PKIX_Boolean typeRegistered;
> 
>         PKIX_ENTER(OBJECT, "PKIX_PL_Object_Alloc");
>         PKIX_NULLCHECK_ONE(pObject);
>@@ -519,42 +547,43 @@ PKIX_PL_Object_Alloc(
>                 PR_Unlock(classTableLock);
>                 if (pkixErrorResult){
>                         PKIX_ERROR_FATAL(PKIX_COULDNOTLOOKUPINHASHTABLE);
>                 }
> 
>                 typeRegistered = (ctEntry != NULL);
> 
>                 if (!typeRegistered) {
>                         PKIX_ERROR_FATAL(PKIX_UNKNOWNTYPEARGUMENT);
>                 }
>+        } else {
>+                ctEntry = &systemClasses[type];
>         }
>+        
>+        PORT_Assert(size == ctEntry->typeObjectSize);
> 
>         /* Allocate space for the object header and the requested size */
>         PKIX_CHECK(PKIX_PL_Malloc
>                     (((PKIX_UInt32)sizeof (PKIX_PL_Object))+size,
>                     (void **)&object,
>                     plContext),
>                     PKIX_MALLOCFAILED);
> 
>         /* Initialize all object fields */
>         object->magicHeader = PKIX_MAGIC_HEADER;
>         object->type = type;
>         object->references = 1; /* Default to a single reference */
>         object->stringRep = NULL;
>         object->hashcode = 0;
>         object->hashcodeCached = 0;
> 
>-        /* XXX Debugging - not thread safe */
>-        refCountTotal++;
>-
>-        PKIX_REF_COUNT_DEBUG_ARG("PKIX_PL_Object_Alloc: refCountTotal == %d \n",
>-                            refCountTotal);
>+        /* Atomically increment object counter */
>+        PR_AtomicIncrement(&ctEntry->objCounter);
> 
>         /* Cannot use PKIX_PL_Mutex because it depends on Object */
>         /* Using NSPR Locks instead */
>         PKIX_OBJECT_DEBUG("\tCalling PR_NewLock).\n");
>         object->lock = PR_NewLock();
>         if (object->lock == NULL) {
>                 PKIX_FREE(pObject);
>                 return (PKIX_ALLOC_ERROR());
>         }
> 
>@@ -740,63 +769,43 @@ PKIX_PL_Object_IncRef(
>         }
> 
>         if (object == (PKIX_PL_Object*)PKIX_ALLOC_ERROR()) {
>                 PKIX_ERROR_FATAL(PKIX_ATTEMPTTOINCREFALLOCERROR);
>         }
> 
>         /* Shift pointer from user data to object header */
>         PKIX_CHECK(pkix_pl_Object_GetHeader(object, &objectHeader, plContext),
>                     PKIX_RECEIVEDCORRUPTEDOBJECTARGUMENT);
> 
>-        /* Lock the test and increment */
>-        PKIX_OBJECT_DEBUG("\tAcquiring object lock).\n");
>-        PKIX_CHECK(pkix_LockObject(object, plContext),
>-                    PKIX_ERRORLOCKINGOBJECT);
>-
>         /* This object should never have zero references */
>-        refCountError = (objectHeader->references == 0);
>-
>-        if (!refCountError) {
>-                objectHeader->references++;
>-                refCountTotal++;
>-                PKIX_REF_COUNT_DEBUG_ARG("PKIX_PL_Object_IncRef: "
>-                                    "refCountTotal == %d \n", refCountTotal);
>-        }
>-
>-        PKIX_CHECK(pkix_UnlockObject(object, plContext),
>-                    PKIX_ERRORUNLOCKINGOBJECT);
>+        refCountError = (PR_AtomicIncrement(&objectHeader->references) <= 1);
> 
>         if (refCountError) {
>                 PKIX_THROW(FATAL, PKIX_OBJECTWITHNONPOSITIVEREFERENCES);
>         }
> 
> cleanup:
> 
>         PKIX_RETURN(OBJECT);
> }
> 
> /*
>  * FUNCTION: PKIX_PL_Object_DecRef (see comments in pkix_pl_system.h)
>  */
> PKIX_Error *
> PKIX_PL_Object_DecRef(
>         PKIX_PL_Object *object,
>         void *plContext)
> {
>-        extern pkix_ClassTable_Entry systemClasses[PKIX_NUMTYPES];
>-        PKIX_PL_DestructorCallback destructor = NULL;
>-        pkix_ClassTable_Entry *ctEntry = NULL;
>-        PKIX_PL_DestructorCallback func = NULL;
>+        PKIX_Int32 refCount = 0;
>         PKIX_PL_Object *objectHeader = NULL;
>-        pkix_ClassTable_Entry entry;
>         PKIX_PL_NssContext *context = NULL;
>-        PKIX_Boolean refCountError = PKIX_FALSE;
> 
>         PKIX_ENTER(OBJECT, "PKIX_PL_Object_DecRef");
>         PKIX_NULLCHECK_ONE(object);
> 
>         if (plContext){
>                 /* 
>                  * PKIX_PL_NssContext is not a complete PKIX Type, it doesn't
>                  * have a header therefore we cannot verify its type before
>                  * casting.
>                  */  
>@@ -807,108 +816,88 @@ PKIX_PL_Object_DecRef(
>         }
> 
>         if (object == (PKIX_PL_Object*)PKIX_ALLOC_ERROR()) {
>                 PKIX_ERROR_FATAL(PKIX_ATTEMPTTODECREFALLOCERROR);
>         }
> 
>         /* Shift pointer from user data to object header */
>         PKIX_CHECK(pkix_pl_Object_GetHeader(object, &objectHeader, plContext),
>                     PKIX_RECEIVEDCORRUPTEDOBJECTARGUMENT);
> 
>-        /* Lock the test for the reference count and the decrement */
>-        PKIX_OBJECT_DEBUG("\tAcquiring object lock).\n");
>-        PKIX_CHECK(pkix_LockObject(object, plContext),
>-                    PKIX_ERRORLOCKINGOBJECT);
>-
>-        refCountError = (objectHeader->references == 0);
>-
>-        if (!refCountError) {
>-                objectHeader->references--;
>-                refCountTotal--;
>-
>-                PKIX_REF_COUNT_DEBUG_ARG("PKIX_PL_Object_DecRef: "
>-                                    "refCountTotal == %d \n", refCountTotal);
>-
>-                if (objectHeader->references == 0) {
>-                        /* first, special handling for system types */
>-                        if (objectHeader->type < PKIX_NUMTYPES){
>-                                entry = systemClasses[objectHeader->type];
>-                                func = entry.destructor;
>-                                if (func != NULL){
>-                                /* Call destructor on user data if necessary */
>-                                    PKIX_CHECK_FATAL(func(object, plContext),
>-                                             PKIX_ERRORINOBJECTDEFINEDESTROY);
>-                                }
>-                        } else {
>-                                PKIX_OBJECT_DEBUG("\tCalling PR_Lock).\n");
>-                                PR_Lock(classTableLock);
>-                                pkixErrorResult = pkix_pl_PrimHashTable_Lookup
>-                                        (classTable,
>-                                        (void *)&objectHeader->type,
>-                                        objectHeader->type,
>-                                        NULL,
>-                                        (void **)&ctEntry,
>-                                        plContext);
>-                                PKIX_OBJECT_DEBUG
>-                                        ("\tCalling PR_Unlock).\n");
>-                                PR_Unlock(classTableLock);
>-                                if (pkixErrorResult){
>-                                    PKIX_ERROR_FATAL
>-                                            (PKIX_ERRORINGETTINGDESTRUCTOR);
>-                                }
>-
>-                                if (ctEntry != NULL){
>-                                        destructor = ctEntry->destructor;
>-                                }
>-
>-                                if (destructor != NULL) {
>-                                /* Call destructor on user data if necessary */
>-                                        PKIX_CHECK_FATAL(destructor
>-                                                (object, plContext),
>-                                                PKIX_ERRORINOBJECTDEFINEDESTROY);
>-                                }
>-                        }
>-
>-                        /* pkix_pl_Object_Destroy assumes the lock is held */
>-                        /* It will call unlock and destroy the object */
>-                        return (pkix_pl_Object_Destroy(object, plContext));
>+        refCount = PR_AtomicDecrement(&objectHeader->references);
> 
>+        if (refCount == 0) {
>+            PKIX_PL_DestructorCallback destructor = NULL;
>+            pkix_ClassTable_Entry *ctEntry = NULL;
>+            
>+            /* first, special handling for system types */
>+            if (objectHeader->type >= PKIX_NUMTYPES){
>+                PKIX_OBJECT_DEBUG("\tCalling PR_Lock).\n");
>+                PR_Lock(classTableLock);
>+                pkixErrorResult = pkix_pl_PrimHashTable_Lookup
>+                    (classTable,
>+                     (void *)&objectHeader->type,
>+                     objectHeader->type,
>+                     NULL,
>+                     (void **)&ctEntry,
>+                     plContext);
>+                PKIX_OBJECT_DEBUG
>+                    ("\tCalling PR_Unlock).\n");
>+                PR_Unlock(classTableLock);
>+                if (pkixErrorResult){
>+                    PKIX_ERROR_FATAL
>+                        (PKIX_ERRORINGETTINGDESTRUCTOR);
>                 }
>+                
>+                if (ctEntry != NULL){
>+                    destructor = ctEntry->destructor;
>+                }
>+            } else {
>+                ctEntry = &systemClasses[objectHeader->type];
>+                destructor = ctEntry->destructor;
>+            }
>+            
>+            if (destructor != NULL){
>+                /* Call destructor on user data if necessary */
>+                PKIX_CHECK_FATAL(destructor(object, plContext),
>+                                 PKIX_ERRORINOBJECTDEFINEDESTROY);
>+            }
>+            
>+            /* Atomically decrement object counter */
>+            PR_AtomicDecrement(&ctEntry->objCounter);
>+            
>+            /* pkix_pl_Object_Destroy assumes the lock is held */
>+            /* It will call unlock and destroy the object */
>+            return (pkix_pl_Object_Destroy(object, plContext));
>         }
> 
>-        PKIX_OBJECT_DEBUG("\tReleasing object lock).\n");
>-        PKIX_CHECK(pkix_UnlockObject(object, plContext),
>-                    PKIX_ERRORUNLOCKINGOBJECT);
>-
>-        /* if a reference count was already zero, throw an error */
>-        if (refCountError) {
>+        if (refCount < 0) {
>                 return (PKIX_ALLOC_ERROR());
>         }
> 
> cleanup:
> 
>         PKIX_RETURN(OBJECT);
> }
> 
> 
> 
> /*
>  * FUNCTION: PKIX_PL_Object_Equals (see comments in pkix_pl_system.h)
>  */
> PKIX_Error *
> PKIX_PL_Object_Equals(
>         PKIX_PL_Object *firstObject,
>         PKIX_PL_Object *secondObject,
>         PKIX_Boolean *pResult,
>         void *plContext)
> {
>-        extern pkix_ClassTable_Entry systemClasses[PKIX_NUMTYPES];
>         PKIX_PL_Object *firstObjectHeader = NULL;
>         PKIX_PL_Object *secondObjectHeader = NULL;
>         pkix_ClassTable_Entry *ctEntry = NULL;
>         PKIX_PL_EqualsCallback func = NULL;
>         pkix_ClassTable_Entry entry;
> 
>         PKIX_ENTER(OBJECT, "PKIX_PL_Object_Equals");
>         PKIX_NULLCHECK_THREE(firstObject, secondObject, pResult);
> 
>         PKIX_CHECK(pkix_pl_Object_GetHeader
>@@ -970,21 +959,20 @@ cleanup:
> 
> /*
>  * FUNCTION: PKIX_PL_Object_Duplicate (see comments in pkix_pl_system.h)
>  */
> PKIX_Error *
> PKIX_PL_Object_Duplicate(
>         PKIX_PL_Object *firstObject,
>         PKIX_PL_Object **pNewObject,
>         void *plContext)
> {
>-        extern pkix_ClassTable_Entry systemClasses[PKIX_NUMTYPES];
>         PKIX_PL_Object *firstObjectHeader = NULL;
>         pkix_ClassTable_Entry *ctEntry = NULL;
>         PKIX_PL_DuplicateCallback func = NULL;
>         pkix_ClassTable_Entry entry;
> 
>         PKIX_ENTER(OBJECT, "PKIX_PL_Object_Duplicate");
>         PKIX_NULLCHECK_TWO(firstObject, pNewObject);
> 
>         PKIX_CHECK(pkix_pl_Object_GetHeader
>                     (firstObject, &firstObjectHeader, plContext),
>@@ -1031,21 +1019,20 @@ cleanup:
> 
> /*
>  * FUNCTION: PKIX_PL_Object_Hashcode (see comments in pkix_pl_system.h)
>  */
> PKIX_Error *
> PKIX_PL_Object_Hashcode(
>         PKIX_PL_Object *object,
>         PKIX_UInt32 *pValue,
>         void *plContext)
> {
>-        extern pkix_ClassTable_Entry systemClasses[PKIX_NUMTYPES];
>         PKIX_PL_Object *objectHeader = NULL;
>         pkix_ClassTable_Entry *ctEntry = NULL;
>         PKIX_PL_HashcodeCallback func = NULL;
>         pkix_ClassTable_Entry entry;
>         PKIX_Boolean objectLocked = PKIX_FALSE;
>         PKIX_UInt32 objectHash;
> 
>         PKIX_ENTER(OBJECT, "PKIX_PL_Object_Hashcode");
>         PKIX_NULLCHECK_TWO(object, pValue);
> 
>@@ -1124,21 +1111,20 @@ cleanup:
> 
> /*
>  * FUNCTION: PKIX_PL_Object_ToString (see comments in pkix_pl_system.h)
>  */
> PKIX_Error *
> PKIX_PL_Object_ToString(
>         PKIX_PL_Object *object,
>         PKIX_PL_String **pString,
>         void *plContext)
> {
>-        extern pkix_ClassTable_Entry systemClasses[PKIX_NUMTYPES];
>         PKIX_PL_Object *objectHeader = NULL;
>         pkix_ClassTable_Entry *ctEntry = NULL;
>         PKIX_PL_ToStringCallback func = NULL;
>         pkix_ClassTable_Entry entry;
>         PKIX_Boolean objectLocked = PKIX_FALSE;
>         PKIX_PL_String *objectString;
> 
>         PKIX_ENTER(OBJECT, "PKIX_PL_Object_ToString");
>         PKIX_NULLCHECK_TWO(object, pString);
> 
>@@ -1250,21 +1236,20 @@ cleanup:
> /*
>  * FUNCTION: PKIX_PL_Object_Compare (see comments in pkix_pl_system.h)
>  */
> PKIX_Error *
> PKIX_PL_Object_Compare(
>         PKIX_PL_Object *firstObject,
>         PKIX_PL_Object *secondObject,
>         PKIX_Int32 *pResult,
>         void *plContext)
> {
>-        extern pkix_ClassTable_Entry systemClasses[PKIX_NUMTYPES];
>         PKIX_PL_Object *firstObjectHeader = NULL;
>         PKIX_PL_Object *secondObjectHeader = NULL;
>         pkix_ClassTable_Entry *ctEntry = NULL;
>         PKIX_PL_ComparatorCallback func = NULL;
>         pkix_ClassTable_Entry entry;
> 
>         PKIX_ENTER(OBJECT, "PKIX_PL_Object_Compare");
>         PKIX_NULLCHECK_THREE(firstObject, secondObject, pResult);
> 
>         /* Shift pointer from user data to object header */
>Index: lib/libpkix/pkix_pl_nss/system/pkix_pl_object.h
>===================================================================
>RCS file: /cvsroot/mozilla/security/nss/lib/libpkix/pkix_pl_nss/system/pkix_pl_object.h,v
>retrieving revision 1.3
>diff -U 10 -p -r1.3 pkix_pl_object.h
>--- lib/libpkix/pkix_pl_nss/system/pkix_pl_object.h	31 Aug 2007 03:21:41 -0000	1.3
>+++ lib/libpkix/pkix_pl_nss/system/pkix_pl_object.h	10 Oct 2007 22:31:44 -0000
>@@ -66,21 +66,21 @@ extern "C" {
>  *
>  * Object operations receive a pointer to raw user data as an argument.
>  * The macro HEADER(object) returns a pointer to the object header.
>  * An assertion then verifies that the first field is the MAGIC_HEADER.
>  */
> 
> /* PKIX_PL_Object Structure Definition */
> struct PKIX_PL_ObjectStruct {
>         PKIX_UInt32 magicHeader;
>         PKIX_UInt32 type;
>-        PKIX_UInt32 references;
>+        PKIX_Int32 references;
>         PRLock *lock;
>         PKIX_PL_String *stringRep;
>         PKIX_UInt32 hashcode;
>         PKIX_Boolean hashcodeCached;
> };
> 
> /* see source file for function documentation */
> 
> PKIX_Error *
> pkix_pl_Object_RetrieveEqualsCallback(
>@@ -95,16 +95,20 @@ PKIX_PL_Object_RegisterSystemType(
>         PKIX_PL_EqualsCallback equalsFunction,
>         PKIX_PL_HashcodeCallback hashcodeFunction,
>         PKIX_PL_ToStringCallback toStringFunction,
>         PKIX_PL_ComparatorCallback comparator,
>         PKIX_PL_DuplicateCallback duplicateFunction,
>         void *plContext);
> 
> extern PKIX_Boolean initializing;
> extern PKIX_Boolean initialized;
> extern PRLock *classTableLock;
>+extern pkix_ClassTable_Entry systemClasses[PKIX_NUMTYPES];
>+
>+PKIX_Error *
>+pkix_pl_Object_RegisterSelf(void *plContext);
> 
> #ifdef __cplusplus
> }
> #endif
> 
> #endif /* _PKIX_PL_OBJECT_H */
Comment 20 Alexei Volkov 2007-10-22 13:54:13 PDT
Code related to "user-defined-types" will be removed in the patch for bug 397825.
Comment 21 Alexei Volkov 2007-10-22 13:59:02 PDT
Patch V3 is integrated.
Comment 22 Nelson Bolyard (seldom reads bugmail) 2007-10-22 20:36:10 PDT
This checkin caused all tinderboxes to go orange. 
The reason was nto esy to spot.  It is assertion failures.  

Assertion failure: numLeakedObjects == 0, at pkix_pl_lifecycle.c:266
./all.sh: line 912: 11292 Aborted                 
${PROFTOOL} selfserv -D -p ${PORT} -d ${P_R_SERVERDIR} -n ${HOSTADDR} \
${SERVER_OPTIONS} ${ECC_OPTIONS} -w nss ${sparam} -i ${R_SERVERPID} $verbose


Assertion failure: numLeakedObjects == 0, at pkix_pl_lifecycle.c:266
./all.sh: line 1423: 20363 Aborted                 
tstclnt -d $trgDir -S -h $host -p $IOPR_DOWNLOAD_PORT -w ${R_PWFILE} \
-o <$req >$file

As a stopgap measure, I propose that we remove that assertion failure 
until the known leaks are fixed.
Comment 23 Nelson Bolyard (seldom reads bugmail) 2007-10-22 20:37:05 PDT
Where is the log output that reports which objects are leaked, and how
many of each kind?
Comment 24 Alexei Volkov 2007-10-22 20:45:34 PDT
We decided to enable printing of object only when PR log is enabled. The log is called "pkix".

I've commented out the assertion that makes tests to fail. Will uncomment the code after bug 397832 is fixed. 
Comment 25 Nelson Bolyard (seldom reads bugmail) 2007-10-22 22:16:25 PDT
Alexei, Please write a patch to enable the PKIX object leak logging, and 
cause the log output to be copied into the build log so we can see it in
the Tinderbox logs.
Comment 26 Alexei Volkov 2007-10-26 12:20:14 PDT
Created attachment 286327 [details] [diff] [review]
Ifdef user defined object type code.
Comment 27 Nelson Bolyard (seldom reads bugmail) 2007-10-30 13:14:24 PDT
Comment on attachment 286327 [details] [diff] [review]
Ifdef user defined object type code.

r-
I emailed review comments to Alexei
Comment 28 Alexei Volkov 2007-11-12 13:44:20 PST
fixed. The last attachment will be added into bug 403538

Note You need to log in before you can comment on or make changes to this bug.