nsIX509Cert.issuer performs synchronous certificate verification and isn't even guaranteed to return a verified result. Fortunately we should be able to use nsISSLStatus.succeededCertChain instead, which contains the already-verified certificate chain of the connection we're interested in. This should be a performance as well as architectural improvement.

Comment on attachment 8968350 [details] bug 1454504 - use a more performant API to find a root certificate in CertUtils.checkCert https://reviewboard.mozilla.org/r/237028/#review242852 ::: toolkit/modules/CertUtils.jsm:146 (Diff revision 1) > } > return; > } > > - var cert = > - aChannel.securityInfo.QueryInterface(Ci.nsISSLStatusProvider). > + let sslStatus = aChannel.securityInfo.QueryInterface(Ci.nsISSLStatusProvider) > + .SSLStatus.QueryInterface(Ci.nsISSLStatus); Nit: QI(nsISSLStatus) is completely unnecessary. ::: toolkit/modules/CertUtils.jsm:155 (Diff revision 1) > > - if (aAllowNonBuiltInCerts === true) > + if (aAllowNonBuiltInCerts === true) { > return; > + } > > - var issuerCert = cert; > + let certEnumerator = sslStatus.succeededCertChain.getEnumerator(); Random fact: nsISSLStatus.idl is super well-documented. ::: toolkit/modules/CertUtils.jsm:156 (Diff revision 1) > - while (issuerCert.issuer && !issuerCert.issuer.equals(issuerCert)) > - issuerCert = issuerCert.issuer; > + let issuerCert = null; > + while (certEnumerator.hasMoreElements()) { Nit: `for (issuerCert of XPCOMUtils.IterSimpleEnumerator(certEnumerator, Ci.nsIX509Cert))`

Comment on attachment 8968350 [details] bug 1454504 - use a more performant API to find a root certificate in CertUtils.checkCert https://reviewboard.mozilla.org/r/237028/#review243144 r+ with Kris's comments addressed.

Comment on attachment 8968350 [details] bug 1454504 - use a more performant API to find a root certificate in CertUtils.checkCert https://reviewboard.mozilla.org/r/237028/#review242852 Thanks for the reviews! > Nit: QI(nsISSLStatus) is completely unnecessary. Heh - whoops. Fixed. > Random fact: nsISSLStatus.idl is super well-documented. Indeed :( > Nit: `for (issuerCert of XPCOMUtils.IterSimpleEnumerator(certEnumerator, Ci.nsIX509Cert))` Cool - fixed.

Try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=389e57f8a8090aa6a7eaadedf09343dfc67546f5

Pushed by dkeeler@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a231670d9c66 use a more performant API to find a root certificate in CertUtils.checkCert r=kmag,mossop

Please let us know if any testing is needed here or mark the bug as "qe-verify-"

If there are existing smoke tests that ensure that updating add-ons and the browser still works, it would be good to confirm that this change doesn't break that. Thanks!

We made a smoke testing around updates and everything worked as expected. Marking bug as verified.