1019814 - Remove CERTCertificate dependencies from TrustDomain::GetCertTrust

Status: RESOLVED FIXED

(Core :: Security: PSM, enhancement)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: remove-CERTCertificate-dependency-from-TrustDomain-GetCertTrust.patch

See bug 1019771 for backgronud.

This patch just shifts the CERTCertificate dependency into the TrustDomain. The conversion from SECItem* to CERTCertificate should be a pretty quick hashtable lookup + reference count increment because NSS maintains a cache from SECItem* -> CERTCertificate. Thus, the change shouldn't have a (significant) performance impact.

Comment 1

[Dana Keeler (she/her) (use needinfo) (:keeler for reviews)]

Comment on attachment 8433521 [details] [diff] [review]
remove-CERTCertificate-dependency-from-TrustDomain-GetCertTrust.patch

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

LGTM. Did you want to include in the TrustDomain implementations a comment explaining the expectation that NSS will maintain a hash of the SECItem -> CERTCertificate conversion so it won't be a performance concern?

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to David Keeler (:keeler) [use needinfo?] from comment #1)
> LGTM. Did you want to include in the TrustDomain implementations a comment
> explaining the expectation that NSS will maintain a hash of the SECItem ->
> CERTCertificate conversion so it won't be a performance concern?

I added a note to that effect in NSSCertDBTrustDomain.cpp. I didn't bother doing so for AppTrustDomain because we don't care (as much) about tiny performance issues in that class.

Thanks for the quick review!

https://hg.mozilla.org/integration/mozilla-inbound/rev/44be87ea2e1b

Comment 3

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/44be87ea2e1b