Last Comment Bug 391774 - PKIX_Shutdown is not called by nssinit.c
: PKIX_Shutdown is not called by nssinit.c
: mlk
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: trunk
: All All
: P1 trivial (vote)
: 3.12
Assigned To: Alexei Volkov
Depends on: 391815
  Show dependency treegraph
Reported: 2007-08-10 16:44 PDT by David Baron :dbaron: ⌚️UTC-10
Modified: 2007-10-04 12:07 PDT (History)
2 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---

remove ifdef for PKIX_Shutdown (1.04 KB, patch)
2007-09-20 16:25 PDT, Alexei Volkov
nelson: review-
Details | Diff | Splinter Review
ignore file patch (3.13 KB, patch)
2007-10-01 13:27 PDT, Alexei Volkov
nelson: review-
Details | Diff | Splinter Review
Patch v1. (6.35 KB, patch)
2007-10-03 07:06 PDT, Slavomir Katuscak
nelson: review+
Details | Diff | Splinter Review

Description David Baron :dbaron: ⌚️UTC-10 2007-08-10 16:44:11 PDT
This bug is filed based on the NSS_3_12_ALPHA1B tag, which is what is currently used by the trunk of Firefox, etc.

security/nss/lib/nss/nssinit.c currently calls PKIX_Initialize but not PKIX_Shutdown.  It seems that someplace in NSS_Shutdown should have the following code:

+    if (plContext) {
+        PKIX_Shutdown(plContext);
+        plContext = NULL;
+    }

This alone doesn't fix any memory leaks, but with improvements to PKIX_Shutdown I think it would fix some memory leaks.
Comment 1 Nelson Bolyard (seldom reads bugmail) 2007-08-11 01:56:28 PDT
The fix for this bug should not be committed while it remains blocked by 
bug 391815, and bug 391815 remains unresolved.
Comment 2 Alexei Volkov 2007-09-19 12:37:16 PDT
nss shutdown function now calls PKIX_Shutdown. The fix was committed with the patch for bug 395427. But still, PKIX_Shutdown is ifdef-ed awaiting a fix for bug  391815. So, will keep this bug open until ifdef for PKIX_Shutdown call is removed.   
Comment 3 Alexei Volkov 2007-09-20 16:25:37 PDT
Created attachment 281745 [details] [diff] [review]
remove ifdef for PKIX_Shutdown
Comment 4 Nelson Bolyard (seldom reads bugmail) 2007-09-27 16:01:04 PDT
Comment on attachment 281745 [details] [diff] [review]
remove ifdef for PKIX_Shutdown

This patch is obviously correct, as far as it goes, but it also needs to remove from the file of ignored leak stacks any stacks that will be fixed by the removal of the ifdef.  It may or may not be possible to do that now, depending on whether all the leak stacks caused by PKIX initialization have been put into the ignored file already, or not.  They can't be removed until they are put in, of course.  
The next patch should have two parts:
1) the part for nssinit.c that removes the ifdefs (the present patch), and 
2) the patch to the "ignored" file.
Comment 5 Slavomir Katuscak 2007-10-01 06:21:39 PDT
This leak is already detected by our tools, dependency on bug 391815 can be
Comment 6 Nelson Bolyard (seldom reads bugmail) 2007-10-01 12:10:15 PDT
Comment on attachment 281745 [details] [diff] [review]
remove ifdef for PKIX_Shutdown

Please attach a patch that will remove the leak stacks that will be fixed by this patch.  I will give this patch and that one r+ at the same time.
Comment 7 Alexei Volkov 2007-10-01 13:27:49 PDT
Created attachment 283055 [details] [diff] [review]
ignore file patch

PKIX_Shutdown will be called from NSS_Shutdown with patch in attachment 281745 [details] [diff] [review] applied to the trunk.

This patch removes all one time initialization leaks from ignore file in the assumption that PKIX_Shutdown will free these memories blocks. But memory checking tinderbox should go orange, since PKIX_Shitdown currently does not take care of at least one real leak: 

Comment 8 Nelson Bolyard (seldom reads bugmail) 2007-10-01 16:52:12 PDT
Comment on attachment 283055 [details] [diff] [review]
ignore file patch

Please do not remove any stacks that you know are not actually fixed by the first patch for this bug.
Comment 9 Slavomir Katuscak 2007-10-03 07:06:50 PDT
Created attachment 283353 [details] [diff] [review]
Patch v1.

I tested memory leaks with PKIX shutdown uncommented to find leaks which stays there. There was only one PKIX leak found (Alexei already mentioned it in comment 7), so I opened new bug for this (bug 398401) and reassigned stacks related to this bug to it in ignore list. 

Now we can uncomment PKIX shutdown + remove all stacks assigned to it from ignore list and close this bug. I prepared patch for this (replacing previous patches).
Comment 10 Nelson Bolyard (seldom reads bugmail) 2007-10-04 11:50:49 PDT
Comment on attachment 283353 [details] [diff] [review]
Patch v1.

Alexei or Slavo, please commit this patch ASAP.
Comment 11 Alexei Volkov 2007-10-04 12:07:47 PDT
/cvsroot/mozilla/security/nss/tests/memleak/ignored,v  <--  ignored
new revision: 1.27; previous revision: 1.26
/cvsroot/mozilla/security/nss/lib/nss/nssinit.c,v  <--  nssinit.c
new revision: 1.84; previous revision: 1.83

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