Closed
Bug 1147085
Opened 10 years ago
Closed 10 years ago
remove nsINSSCertCache
Categories
(Core :: Security: PSM, defect)
Core
Security: PSM
Tracking
()
RESOLVED
FIXED
mozilla40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: keeler, Assigned: keeler)
Details
(Keywords: addon-compat)
Attachments
(1 file, 1 obsolete file)
25.35 KB,
patch
|
keeler
:
review+
|
Details | Diff | Splinter Review |
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•10 years ago
|
||
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?
Assignee | ||
Comment 2•10 years ago
|
||
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.
Assignee | ||
Comment 3•10 years ago
|
||
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 4•10 years ago
|
||
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+
Assignee | ||
Comment 5•10 years ago
|
||
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+
Assignee | ||
Comment 6•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/1dfe22ca4abe
https://hg.mozilla.org/mozilla-central/rev/1dfe22ca4abe
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Assignee | ||
Comment 8•10 years ago
|
||
Posted to dev.platform: https://groups.google.com/forum/#!topic/mozilla.dev.platform/_FlE9mib6r8
Keywords: addon-compat
You need to log in
before you can comment on or make changes to this bug.
Description
•