1147085 - remove nsINSSCertCache

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

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().

Comment 1

[:Cykesiopka]

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?

Comment 2

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

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.

Comment 3

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: patch

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.

Comment 4

[:Cykesiopka]

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?

Comment 5

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: patch v2

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.

Comment 6

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1dfe22ca4abe

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1dfe22ca4abe

Comment 8

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Posted to dev.platform: https://groups.google.com/forum/#!topic/mozilla.dev.platform/_FlE9mib6r8