Closed Bug 1147085 Opened 5 years ago Closed 5 years ago

remove nsINSSCertCache

Categories

(Core :: Security: PSM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: keeler, Assigned: keeler)

Details

(Keywords: addon-compat)

Attachments

(1 file, 1 obsolete file)

nsINSSCertCache is only used to get a list of every certificate NSS knows about (it can also "cache" a specified nsIX509CertList, but that makes no sense and we don't do it anyway). nsIX509CertDB.getCerts() implements the desired functionality in an easier-to-use/understand API. We should just replace any uses of nsINSSCertCache with nsIX509CertDB.getCerts().
Hmm, it looks like the cache was added in Bug 124037 to decrease cert manager load times.

Has NSS improved to such a point that this is no longer necessary (I vaguely recall NSS having an internal cache now), or would we still have to do something to keep load times down in the cert manager implementation?
I think we can use nsIX509CertDB.getCerts() in the same way the cert manager currently uses nsINSSCertCache, so there shouldn't be a performance regression there.
Attached patch patch (obsolete) — Splinter Review
I deliberately didn't change any style issues in nsCertTree. This patch only touches a few lines there, so I don't think it's worth changing those files to the current style at this time.
Assignee: nobody → dkeeler
Status: NEW → ASSIGNED
Attachment #8588743 - Flags: review?(cykesiopka.bmo)
Comment on attachment 8588743 [details] [diff] [review]
patch

Review of attachment 8588743 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM, assuming the GetRawCertList() nssShutdownPreventionLock thing is a non-issue.

::: security/manager/ssl/public/nsICertTree.idl
@@ +24,5 @@
>  
>    nsIX509Cert getCert(in unsigned long index);
>    nsICertTreeItem getTreeItem(in unsigned long index);
>    boolean isHostPortOverride(in unsigned long index);
>    

Nit: trailing whitespace

::: security/manager/ssl/src/nsCertTree.cpp
@@ -14,5 @@
>  #include "nsReadableUtils.h"
>  #include "nsUnicharUtils.h"
>  #include "nsNSSCertificate.h"
>  #include "nsNSSCertHelper.h"
> -#include "nsINSSCertCache.h"

Does nsIX509CertList.h need to be included instead?

@@ +20,5 @@
>  #include "nsISupportsPrimitives.h"
>  #include "nsXPCOMCID.h"
>  #include "nsTHashtable.h"
>  #include "nsHashKeys.h"
>   

Nit: trailing whitespace

@@ +636,5 @@
>                                      nsCertCompareFunc  aCertCmpFn,
>                                      void              *aCertCmpFnArg)
>  {
>    NS_ENSURE_ARG_POINTER(aCache);
> +  CERTCertList *certList = reinterpret_cast<CERTCertList*>(aCache->GetRawCertList());

The documentation for GetRawCertList() says:
/* getRawCertList MUST be called only from functions where
 * the nssShutdownPreventionLock has been adquired.
 */

Is this a non-issue here?
Attachment #8588743 - Flags: review?(cykesiopka.bmo) → review+
Attached patch patch v2Splinter Review
Thank you for the review!
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=803e9df88218

(In reply to Cykesiopka from comment #4)

...

> ::: security/manager/ssl/src/nsCertTree.cpp
> @@ -14,5 @@
> >  #include "nsReadableUtils.h"
> >  #include "nsUnicharUtils.h"
> >  #include "nsNSSCertificate.h"
> >  #include "nsNSSCertHelper.h"
> > -#include "nsINSSCertCache.h"
> 
> Does nsIX509CertList.h need to be included instead?

I don't think so - nsNSSCertificate.h includes it (nsCertTree.cpp includes nsNSSCertificate.h). If there's build bustage, hopefully we'll find out with the try push :)

...

> @@ +636,5 @@
> >                                      nsCertCompareFunc  aCertCmpFn,
> >                                      void              *aCertCmpFnArg)
> >  {
> >    NS_ENSURE_ARG_POINTER(aCache);
> > +  CERTCertList *certList = reinterpret_cast<CERTCertList*>(aCache->GetRawCertList());
> 
> The documentation for GetRawCertList() says:
> /* getRawCertList MUST be called only from functions where
>  * the nssShutdownPreventionLock has been adquired.
>  */
> 
> Is this a non-issue here?

Good catch - I added the appropriate checks and some comments explaining the issue.
Attachment #8588743 - Attachment is obsolete: true
Attachment #8589226 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/1dfe22ca4abe
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.