Bug 1313715 - Avoid unnecessary uses of PR_SetError() under security/apps/ and security/certverifier/. r=keeler
authorCykesiopka <cykesiopka.bmo@gmail.com>
Wed, 14 Dec 2016 20:10:25 +0800
changeset 325982 676ca54f13dbfbab36e40b1bbc0e42416c6a3ea8
parent 325981 801e9e6efb1742fd3e086685cec6698c8d05a995
child 325983 71b55fc1f44266c17c2b7e3b70d3a73ceec1319e
push id84847
push usercbook@mozilla.com
push dateThu, 15 Dec 2016 13:25:18 +0000
treeherdermozilla-inbound@85a362b51c44 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerskeeler
bugs1313715, 1254403
milestone53.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1313715 - Avoid unnecessary uses of PR_SetError() under security/apps/ and security/certverifier/. r=keeler The PR_SetError() + PR_GetError() pattern is error prone and unnecessary. Also fixes Bug 1254403. MozReview-Commit-ID: DRI69xY4vxC
security/apps/AppSignatureVerification.cpp
security/apps/AppTrustDomain.cpp
security/apps/AppTrustDomain.h
security/certverifier/CertVerifier.cpp
security/certverifier/ExtendedValidation.cpp
security/certverifier/ExtendedValidation.h
security/certverifier/NSSCertDBTrustDomain.cpp
security/certverifier/NSSCertDBTrustDomain.h
security/manager/ssl/ScopedNSSTypes.h
security/manager/ssl/nsNSSComponent.cpp
--- a/security/apps/AppSignatureVerification.cpp
+++ b/security/apps/AppSignatureVerification.cpp
@@ -634,33 +634,34 @@ VerifyCertificate(CERTCertificate* signe
   // TODO: null pinArg is tolerated.
   if (NS_WARN_IF(!signerCert) || NS_WARN_IF(!voidContext)) {
     return NS_ERROR_INVALID_ARG;
   }
   const VerifyCertificateContext& context =
     *static_cast<const VerifyCertificateContext*>(voidContext);
 
   AppTrustDomain trustDomain(context.builtChain, pinArg);
-  if (trustDomain.SetTrustedRoot(context.trustedRoot) != SECSuccess) {
-    return MapSECStatus(SECFailure);
+  nsresult rv = trustDomain.SetTrustedRoot(context.trustedRoot);
+  if (NS_FAILED(rv)) {
+    return rv;
   }
   Input certDER;
-  mozilla::pkix::Result rv = certDER.Init(signerCert->derCert.data,
-                                          signerCert->derCert.len);
-  if (rv != Success) {
-    return mozilla::psm::GetXPCOMFromNSSError(MapResultToPRErrorCode(rv));
+  mozilla::pkix::Result result = certDER.Init(signerCert->derCert.data,
+                                              signerCert->derCert.len);
+  if (result != Success) {
+    return mozilla::psm::GetXPCOMFromNSSError(MapResultToPRErrorCode(result));
   }
 
-  rv = BuildCertChain(trustDomain, certDER, Now(),
-                      EndEntityOrCA::MustBeEndEntity,
-                      KeyUsage::digitalSignature,
-                      KeyPurposeId::id_kp_codeSigning,
-                      CertPolicyId::anyPolicy,
-                      nullptr/*stapledOCSPResponse*/);
-  if (rv == mozilla::pkix::Result::ERROR_EXPIRED_CERTIFICATE) {
+  result = BuildCertChain(trustDomain, certDER, Now(),
+                          EndEntityOrCA::MustBeEndEntity,
+                          KeyUsage::digitalSignature,
+                          KeyPurposeId::id_kp_codeSigning,
+                          CertPolicyId::anyPolicy,
+                          nullptr /*stapledOCSPResponse*/);
+  if (result == mozilla::pkix::Result::ERROR_EXPIRED_CERTIFICATE) {
     // For code-signing you normally need trusted 3rd-party timestamps to
     // handle expiration properly. The signer could always mess with their
     // system clock so you can't trust the certificate was un-expired when
     // the signing took place. The choice is either to ignore expiration
     // or to enforce expiration at time of use. The latter leads to the
     // user-hostile result that perfectly good code stops working.
     //
     // Our package format doesn't support timestamps (nor do we have a
@@ -669,20 +670,20 @@ VerifyCertificate(CERTCertificate* signe
     // on the signing systems. We also have a revocation mechanism if we
     // need it. It's OK to ignore cert expiration under these conditions.
     //
     // This is an invalid approach if
     //  * we issue certs to let others sign their own packages
     //  * mozilla::pkix returns "expired" when there are "worse" problems
     //    with the certificate or chain.
     // (see bug 1267318)
-    rv = Success;
+    result = Success;
   }
-  if (rv != Success) {
-    return mozilla::psm::GetXPCOMFromNSSError(MapResultToPRErrorCode(rv));
+  if (result != Success) {
+    return mozilla::psm::GetXPCOMFromNSSError(MapResultToPRErrorCode(result));
   }
 
   return NS_OK;
 }
 
 nsresult
 VerifySignature(AppTrustedRoot trustedRoot, const SECItem& buffer,
                 const SECItem& detachedDigest,
--- a/security/apps/AppTrustDomain.cpp
+++ b/security/apps/AppTrustDomain.cpp
@@ -13,17 +13,16 @@
 #include "nsComponentManagerUtils.h"
 #include "nsIFile.h"
 #include "nsIFileStreams.h"
 #include "nsIX509CertDB.h"
 #include "nsNSSCertificate.h"
 #include "nsNetUtil.h"
 #include "pkix/pkixnss.h"
 #include "prerror.h"
-#include "secerr.h"
 
 // Generated in Makefile.in
 #include "marketplace-prod-public.inc"
 #include "marketplace-prod-reviewers.inc"
 #include "marketplace-dev-public.inc"
 #include "marketplace-dev-reviewers.inc"
 #include "marketplace-stage.inc"
 #include "xpcshell.inc"
@@ -52,17 +51,17 @@ unsigned int AppTrustDomain::sDevImporte
 
 AppTrustDomain::AppTrustDomain(UniqueCERTCertList& certChain, void* pinArg)
   : mCertChain(certChain)
   , mPinArg(pinArg)
   , mMinRSABits(DEFAULT_MIN_RSA_BITS)
 {
 }
 
-SECStatus
+nsresult
 AppTrustDomain::SetTrustedRoot(AppTrustedRoot trustedRoot)
 {
   SECItem trustedDER;
 
   // Load the trusted certificate into the in-memory NSS database so that
   // CERT_CreateSubjectCertList can find it.
 
   switch (trustedRoot)
@@ -115,70 +114,64 @@ AppTrustDomain::SetTrustedRoot(AppTruste
       break;
 
     case nsIX509CertDB::DeveloperImportedRoot: {
       StaticMutexAutoLock lock(sMutex);
       if (!sDevImportedDERData) {
         MOZ_ASSERT(!NS_IsMainThread());
         nsCOMPtr<nsIFile> file(do_CreateInstance("@mozilla.org/file/local;1"));
         if (!file) {
-          PR_SetError(SEC_ERROR_IO, 0);
-          return SECFailure;
+          return NS_ERROR_FAILURE;
         }
         nsresult rv = file->InitWithNativePath(
-            Preferences::GetCString(kDevImportedDER));
+          Preferences::GetCString(kDevImportedDER));
         if (NS_FAILED(rv)) {
-          PR_SetError(SEC_ERROR_IO, 0);
-          return SECFailure;
+          return rv;
         }
 
         nsCOMPtr<nsIInputStream> inputStream;
-        NS_NewLocalFileInputStream(getter_AddRefs(inputStream), file, -1, -1,
-                                   nsIFileInputStream::CLOSE_ON_EOF);
-        if (!inputStream) {
-          PR_SetError(SEC_ERROR_IO, 0);
-          return SECFailure;
+        rv = NS_NewLocalFileInputStream(getter_AddRefs(inputStream), file, -1,
+                                        -1, nsIFileInputStream::CLOSE_ON_EOF);
+        if (NS_FAILED(rv)) {
+          return rv;
         }
 
         uint64_t length;
         rv = inputStream->Available(&length);
         if (NS_FAILED(rv)) {
-          PR_SetError(SEC_ERROR_IO, 0);
-          return SECFailure;
+          return rv;
         }
 
         auto data = MakeUnique<char[]>(length);
         rv = inputStream->Read(data.get(), length, &sDevImportedDERLen);
         if (NS_FAILED(rv)) {
-          PR_SetError(SEC_ERROR_IO, 0);
-          return SECFailure;
+          return rv;
         }
 
         MOZ_ASSERT(length == sDevImportedDERLen);
         sDevImportedDERData.reset(
           BitwiseCast<unsigned char*, char*>(data.release()));
       }
 
       trustedDER.data = sDevImportedDERData.get();
       trustedDER.len = sDevImportedDERLen;
       break;
     }
 
     default:
-      PR_SetError(SEC_ERROR_INVALID_ARGS, 0);
-      return SECFailure;
+      return NS_ERROR_INVALID_ARG;
   }
 
   mTrustedRoot.reset(CERT_NewTempCertificate(CERT_GetDefaultCertDB(),
                                              &trustedDER, nullptr, false, true));
   if (!mTrustedRoot) {
-    return SECFailure;
+    return mozilla::psm::GetXPCOMFromNSSError(PR_GetError());
   }
 
-  return SECSuccess;
+  return NS_OK;
 }
 
 Result
 AppTrustDomain::FindIssuer(Input encodedIssuerName, IssuerChecker& checker,
                            Time)
 
 {
   MOZ_ASSERT(mTrustedRoot);
--- a/security/apps/AppTrustDomain.h
+++ b/security/apps/AppTrustDomain.h
@@ -18,17 +18,17 @@ namespace mozilla { namespace psm {
 
 class AppTrustDomain final : public mozilla::pkix::TrustDomain
 {
 public:
   typedef mozilla::pkix::Result Result;
 
   AppTrustDomain(UniqueCERTCertList& certChain, void* pinArg);
 
-  SECStatus SetTrustedRoot(AppTrustedRoot trustedRoot);
+  nsresult SetTrustedRoot(AppTrustedRoot trustedRoot);
 
   virtual Result GetCertTrust(mozilla::pkix::EndEntityOrCA endEntityOrCA,
                               const mozilla::pkix::CertPolicyId& policy,
                               mozilla::pkix::Input candidateCertDER,
                               /*out*/ mozilla::pkix::TrustLevel& trustLevel)
                               override;
   virtual Result FindIssuer(mozilla::pkix::Input encodedIssuerName,
                             IssuerChecker& checker,
--- a/security/certverifier/CertVerifier.cpp
+++ b/security/certverifier/CertVerifier.cpp
@@ -17,20 +17,17 @@
 #include "cert.h"
 #include "mozilla/Assertions.h"
 #include "mozilla/Casting.h"
 #include "nsNSSComponent.h"
 #include "nsServiceManagerUtils.h"
 #include "pk11pub.h"
 #include "pkix/pkix.h"
 #include "pkix/pkixnss.h"
-#include "prerror.h"
-#include "secerr.h"
 #include "secmod.h"
-#include "sslerr.h"
 
 using namespace mozilla::ct;
 using namespace mozilla::pkix;
 using namespace mozilla::psm;
 
 mozilla::LazyLogModule gCertVerifierLog("certverifier");
 
 namespace mozilla { namespace psm {
@@ -481,19 +478,19 @@ CertVerifier::VerifyCert(CERTCertificate
       // Try to validate for EV first.
       NSSCertDBTrustDomain::OCSPFetching evOCSPFetching
         = (mOCSPDownloadConfig == ocspOff) ||
           (flags & FLAG_LOCAL_ONLY) ? NSSCertDBTrustDomain::LocalOnlyOCSPForEV
                                     : NSSCertDBTrustDomain::FetchOCSPForEV;
 
       CertPolicyId evPolicy;
       SECOidTag evPolicyOidTag;
-      SECStatus srv = GetFirstEVPolicy(cert, evPolicy, evPolicyOidTag);
+      bool foundEVPolicy = GetFirstEVPolicy(*cert, evPolicy, evPolicyOidTag);
       for (size_t i = 0;
-           i < sha1ModeConfigurationsCount && rv != Success && srv == SECSuccess;
+           i < sha1ModeConfigurationsCount && rv != Success && foundEVPolicy;
            i++) {
         // Don't attempt verification if the SHA1 mode set by preferences
         // (mSHA1Mode) is more restrictive than the SHA1 mode option we're on.
         // (To put it another way, only attempt verification if the SHA1 mode
         // option we're on is as restrictive or more restrictive than
         // mSHA1Mode.) This allows us to gather telemetry information while
         // still enforcing the mode set by preferences.
         if (SHA1ModeMoreRestrictiveThanGivenMode(sha1ModeConfigurations[i])) {
--- a/security/certverifier/ExtendedValidation.cpp
+++ b/security/certverifier/ExtendedValidation.cpp
@@ -15,18 +15,16 @@
 #include "mozilla/Casting.h"
 #include "mozilla/PodOperations.h"
 #ifdef ANDROID
 #include "nsPrintfCString.h"
 #endif
 #include "pk11pub.h"
 #include "pkix/pkixtypes.h"
 #include "prerror.h"
-#include "prinit.h"
-#include "secerr.h"
 
 extern mozilla::LazyLogModule gPIPNSSLog;
 
 #define CONST_OID static const unsigned char
 #define OI(x) { siDEROID, (unsigned char*) x, sizeof x }
 
 struct nsMyTrustedEVInfo
 {
@@ -1368,71 +1366,71 @@ LoadExtendedValidationInfo()
 #endif
       return NS_ERROR_FAILURE;
     }
   }
 
   return NS_OK;
 }
 
-// Find the first policy OID that is known to be an EV policy OID.
-SECStatus
-GetFirstEVPolicy(CERTCertificate* cert,
+// Helper function for GetFirstEVPolicy(): returns the first suitable policy
+// from the given list of policies.
+bool
+GetFirstEVPolicyFromPolicyList(const UniqueCERTCertificatePolicies& policies,
+                       /*out*/ mozilla::pkix::CertPolicyId& policy,
+                       /*out*/ SECOidTag& policyOidTag)
+{
+  for (size_t i = 0; policies->policyInfos[i]; i++) {
+    const CERTPolicyInfo* policyInfo = policies->policyInfos[i];
+    SECOidTag policyInfoOID = policyInfo->oid;
+    if (policyInfoOID == SEC_OID_UNKNOWN || !isEVPolicy(policyInfoOID)) {
+      continue;
+    }
+
+    const SECOidData* oidData = SECOID_FindOIDByTag(policyInfoOID);
+    MOZ_ASSERT(oidData);
+    MOZ_ASSERT(oidData->oid.data);
+    MOZ_ASSERT(oidData->oid.len > 0);
+    MOZ_ASSERT(oidData->oid.len <= mozilla::pkix::CertPolicyId::MAX_BYTES);
+    if (!oidData || !oidData->oid.data || oidData->oid.len == 0 ||
+        oidData->oid.len > mozilla::pkix::CertPolicyId::MAX_BYTES) {
+      continue;
+    }
+
+    policy.numBytes = AssertedCast<uint16_t>(oidData->oid.len);
+    PodCopy(policy.bytes, oidData->oid.data, policy.numBytes);
+    policyOidTag = policyInfoOID;
+    return true;
+  }
+
+  return false;
+}
+
+bool
+GetFirstEVPolicy(CERTCertificate& cert,
                  /*out*/ mozilla::pkix::CertPolicyId& policy,
                  /*out*/ SECOidTag& policyOidTag)
 {
-  if (!cert) {
-    PR_SetError(SEC_ERROR_INVALID_ARGS, 0);
-    return SECFailure;
+  if (!cert.extensions) {
+    return false;
   }
 
-  if (cert->extensions) {
-    for (int i=0; cert->extensions[i]; i++) {
-      const SECItem* oid = &cert->extensions[i]->id;
-
-      SECOidTag oidTag = SECOID_FindOIDTag(oid);
-      if (oidTag != SEC_OID_X509_CERTIFICATE_POLICIES)
-        continue;
-
-      SECItem* value = &cert->extensions[i]->value;
-
-      CERTCertificatePolicies* policies;
-      CERTPolicyInfo** policyInfos;
-
-      policies = CERT_DecodeCertificatePoliciesExtension(value);
-      if (!policies)
-        continue;
-
-      policyInfos = policies->policyInfos;
+  for (size_t i = 0; cert.extensions[i]; i++) {
+    const CERTCertExtension* extension = cert.extensions[i];
+    if (SECOID_FindOIDTag(&extension->id) != SEC_OID_X509_CERTIFICATE_POLICIES) {
+      continue;
+    }
 
-      bool found = false;
-      while (*policyInfos) {
-        const CERTPolicyInfo* policyInfo = *policyInfos++;
+    UniqueCERTCertificatePolicies policies(
+      CERT_DecodeCertificatePoliciesExtension(&extension->value));
+    if (!policies) {
+      continue;
+    }
 
-        SECOidTag oid_tag = policyInfo->oid;
-        if (oid_tag != SEC_OID_UNKNOWN && isEVPolicy(oid_tag)) {
-          const SECOidData* oidData = SECOID_FindOIDByTag(oid_tag);
-          PR_ASSERT(oidData);
-          PR_ASSERT(oidData->oid.data);
-          PR_ASSERT(oidData->oid.len > 0);
-          PR_ASSERT(oidData->oid.len <= mozilla::pkix::CertPolicyId::MAX_BYTES);
-          if (oidData && oidData->oid.data && oidData->oid.len > 0 &&
-              oidData->oid.len <= mozilla::pkix::CertPolicyId::MAX_BYTES) {
-            policy.numBytes = static_cast<uint16_t>(oidData->oid.len);
-            memcpy(policy.bytes, oidData->oid.data, policy.numBytes);
-            policyOidTag = oid_tag;
-            found = true;
-          }
-          break;
-        }
-      }
-      CERT_DestroyCertificatePoliciesExtension(policies);
-      if (found) {
-        return SECSuccess;
-      }
+    if (GetFirstEVPolicyFromPolicyList(policies, policy, policyOidTag)) {
+      return true;
     }
   }
 
-  PR_SetError(SEC_ERROR_POLICY_VALIDATION_FAILED, 0);
-  return SECFailure;
+  return false;
 }
 
 } } // namespace mozilla::psm
--- a/security/certverifier/ExtendedValidation.h
+++ b/security/certverifier/ExtendedValidation.h
@@ -3,26 +3,37 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #ifndef ExtendedValidation_h
 #define ExtendedValidation_h
 
 #include "ScopedNSSTypes.h"
 #include "certt.h"
-#include "prtypes.h"
 
 namespace mozilla { namespace pkix { struct CertPolicyId; } }
 
 namespace mozilla { namespace psm {
 
 nsresult LoadExtendedValidationInfo();
-SECStatus GetFirstEVPolicy(CERTCertificate* cert,
-                           /*out*/ mozilla::pkix::CertPolicyId& policy,
-                           /*out*/ SECOidTag& policyOidTag);
+/**
+ * Finds the first policy OID in the given cert that is known to be an EV policy
+ * OID.
+ *
+ * @param cert
+ *        The cert to find the first EV policy of.
+ * @param policy
+ *        The found policy.
+ * @param policyOidTag
+ *        The OID tag of the found policy.
+ * @return true if a suitable policy was found, false otherwise.
+ */
+bool GetFirstEVPolicy(CERTCertificate& cert,
+                      /*out*/ mozilla::pkix::CertPolicyId& policy,
+                      /*out*/ SECOidTag& policyOidTag);
 
 // CertIsAuthoritativeForEVPolicy does NOT evaluate whether the cert is trusted
 // or distrusted.
 bool CertIsAuthoritativeForEVPolicy(const UniqueCERTCertificate& cert,
                                     const mozilla::pkix::CertPolicyId& policy);
 
 } } // namespace mozilla::psm
 
--- a/security/certverifier/NSSCertDBTrustDomain.cpp
+++ b/security/certverifier/NSSCertDBTrustDomain.cpp
@@ -12,18 +12,18 @@
 #include "NSSErrorsService.h"
 #include "OCSPRequestor.h"
 #include "OCSPVerificationTrustDomain.h"
 #include "PublicKeyPinningService.h"
 #include "cert.h"
 #include "certdb.h"
 #include "mozilla/Assertions.h"
 #include "mozilla/Casting.h"
+#include "mozilla/Move.h"
 #include "mozilla/PodOperations.h"
-#include "mozilla/UniquePtr.h"
 #include "mozilla/Unused.h"
 #include "nsNSSCertificate.h"
 #include "nsServiceManagerUtils.h"
 #include "nss.h"
 #include "pk11pub.h"
 #include "pkix/Result.h"
 #include "pkix/pkix.h"
 #include "pkix/pkixnss.h"
@@ -39,18 +39,16 @@ using namespace mozilla;
 using namespace mozilla::pkix;
 
 extern LazyLogModule gCertVerifierLog;
 
 static const uint64_t ServerFailureDelaySeconds = 5 * 60;
 
 namespace mozilla { namespace psm {
 
-const char BUILTIN_ROOTS_MODULE_DEFAULT_NAME[] = "Builtin Roots Module";
-
 NSSCertDBTrustDomain::NSSCertDBTrustDomain(SECTrustType certDBTrustType,
                                            OCSPFetching ocspFetching,
                                            OCSPCache& ocspCache,
              /*optional but shouldn't be*/ void* pinArg,
                                            CertVerifier::OcspGetConfig ocspGETConfig,
                                            uint32_t certShortLifetimeInDays,
                                            CertVerifier::PinningMode pinningMode,
                                            unsigned int minRSABits,
@@ -1102,64 +1100,58 @@ DisableMD5()
   NSS_SetAlgorithmPolicy(SEC_OID_MD5,
     0, NSS_USE_ALG_IN_CERT_SIGNATURE | NSS_USE_ALG_IN_CMS_SIGNATURE);
   NSS_SetAlgorithmPolicy(SEC_OID_PKCS1_MD5_WITH_RSA_ENCRYPTION,
     0, NSS_USE_ALG_IN_CERT_SIGNATURE | NSS_USE_ALG_IN_CMS_SIGNATURE);
   NSS_SetAlgorithmPolicy(SEC_OID_PKCS5_PBE_WITH_MD5_AND_DES_CBC,
     0, NSS_USE_ALG_IN_CERT_SIGNATURE | NSS_USE_ALG_IN_CMS_SIGNATURE);
 }
 
-SECStatus
-LoadLoadableRoots(/*optional*/ const char* dir, const char* modNameUTF8)
+bool
+LoadLoadableRoots(const nsCString& dir, const nsCString& modNameUTF8)
 {
-  PR_ASSERT(modNameUTF8);
-
-  if (!modNameUTF8) {
-    PR_SetError(SEC_ERROR_INVALID_ARGS, 0);
-    return SECFailure;
-  }
-
-  UniquePtr<char, void(&)(char*)>
-    fullLibraryPath(PR_GetLibraryName(dir, "nssckbi"), PR_FreeLibraryName);
+  UniquePRLibraryName fullLibraryPath(
+    PR_GetLibraryName(dir.IsEmpty() ? nullptr : dir.get(), "nssckbi"));
   if (!fullLibraryPath) {
-    return SECFailure;
+    return false;
   }
 
   // Escape the \ and " characters.
   nsAutoCString escapedFullLibraryPath(fullLibraryPath.get());
   escapedFullLibraryPath.ReplaceSubstring("\\", "\\\\");
   escapedFullLibraryPath.ReplaceSubstring("\"", "\\\"");
   if (escapedFullLibraryPath.IsEmpty()) {
-    return SECFailure;
+    return false;
   }
 
-  // If a module exists with the same name, delete it.
-  int modType;
-  SECMOD_DeleteModule(modNameUTF8, &modType);
+  // If a module exists with the same name, make a best effort attempt to delete
+  // it. Note that it isn't possible to delete the internal module, so checking
+  // the return value would be detrimental in that case.
+  int unusedModType;
+  Unused << SECMOD_DeleteModule(modNameUTF8.get(), &unusedModType);
 
   nsAutoCString pkcs11ModuleSpec;
-  pkcs11ModuleSpec.AppendPrintf("name=\"%s\" library=\"%s\"", modNameUTF8,
+  pkcs11ModuleSpec.AppendPrintf("name=\"%s\" library=\"%s\"", modNameUTF8.get(),
                                 escapedFullLibraryPath.get());
   if (pkcs11ModuleSpec.IsEmpty()) {
-    return SECFailure;
+    return false;
   }
 
   UniqueSECMODModule rootsModule(
     SECMOD_LoadUserModule(const_cast<char*>(pkcs11ModuleSpec.get()), nullptr,
                           false));
   if (!rootsModule) {
-    return SECFailure;
+    return false;
   }
 
   if (!rootsModule->loaded) {
-    PR_SetError(PR_INVALID_STATE_ERROR, 0);
-    return SECFailure;
+    return false;
   }
 
-  return SECSuccess;
+  return true;
 }
 
 void
 UnloadLoadableRoots(const char* modNameUTF8)
 {
   PR_ASSERT(modNameUTF8);
   UniqueSECMODModule rootsModule(SECMOD_FindModule(modNameUTF8));
 
--- a/security/certverifier/NSSCertDBTrustDomain.h
+++ b/security/certverifier/NSSCertDBTrustDomain.h
@@ -35,26 +35,28 @@ enum class NetscapeStepUpPolicy : uint32
   MatchBefore23August2015 = 2,
   NeverMatch = 3,
 };
 
 SECStatus InitializeNSS(const char* dir, bool readOnly, bool loadPKCS11Modules);
 
 void DisableMD5();
 
-extern const char BUILTIN_ROOTS_MODULE_DEFAULT_NAME[];
-
-// The dir parameter is the path to the directory containing the NSS builtin
-// roots module. Usually this is the same as the path to the other NSS shared
-// libraries. If it is null then the (library) path will be searched.
-//
-// The modNameUTF8 parameter should usually be
-// BUILTIN_ROOTS_MODULE_DEFAULT_NAME.
-SECStatus LoadLoadableRoots(/*optional*/ const char* dir,
-                            const char* modNameUTF8);
+/**
+ * Loads root certificates from a module.
+ *
+ * @param dir
+ *        The path to the directory containing the NSS builtin roots module.
+ *        Usually the same as the path to the other NSS shared libraries.
+ *        If empty, the (library) path will be searched.
+ * @param modNameUTF8
+ *        The UTF-8 name to give the module for display purposes.
+ * @return true if the roots were successfully loaded, false otherwise.
+ */
+bool LoadLoadableRoots(const nsCString& dir, const nsCString& modNameUTF8);
 
 void UnloadLoadableRoots(const char* modNameUTF8);
 
 nsresult DefaultServerNicknameForCert(const CERTCertificate* cert,
                               /*out*/ nsCString& nickname);
 
 void SaveIntermediateCerts(const UniqueCERTCertList& certList);
 
--- a/security/manager/ssl/ScopedNSSTypes.h
+++ b/security/manager/ssl/ScopedNSSTypes.h
@@ -197,19 +197,16 @@ MOZ_TYPE_SPECIFIC_SCOPED_POINTER_TEMPLAT
                                           PK11SlotInfo,
                                           PK11_FreeSlot)
 MOZ_TYPE_SPECIFIC_SCOPED_POINTER_TEMPLATE(ScopedPK11SymKey,
                                           PK11SymKey,
                                           PK11_FreeSymKey)
 MOZ_TYPE_SPECIFIC_SCOPED_POINTER_TEMPLATE(ScopedPK11GenericObject,
                                           PK11GenericObject,
                                           PK11_DestroyGenericObject)
-MOZ_TYPE_SPECIFIC_SCOPED_POINTER_TEMPLATE(ScopedSEC_PKCS12DecoderContext,
-                                          SEC_PKCS12DecoderContext,
-                                          SEC_PKCS12DecoderFinish)
 namespace internal {
 
 inline void
 PORT_FreeArena_false(PLArenaPool* arena)
 {
   // PL_FreeArenaPool can't be used because it doesn't actually free the
   // memory, which doesn't work well with memory analysis tools.
   return PORT_FreeArena(arena, false);
@@ -366,20 +363,23 @@ MOZ_TYPE_SPECIFIC_UNIQUE_PTR_TEMPLATE(Un
                                       PK11SymKey,
                                       PK11_FreeSymKey)
 
 MOZ_TYPE_SPECIFIC_UNIQUE_PTR_TEMPLATE(UniquePLArenaPool,
                                       PLArenaPool,
                                       internal::PORT_FreeArena_false)
 MOZ_TYPE_SPECIFIC_UNIQUE_PTR_TEMPLATE(UniquePORTString,
                                       char,
-                                      PORT_Free);
+                                      PORT_Free)
 MOZ_TYPE_SPECIFIC_UNIQUE_PTR_TEMPLATE(UniquePRFileDesc,
                                       PRFileDesc,
                                       PR_Close)
+MOZ_TYPE_SPECIFIC_UNIQUE_PTR_TEMPLATE(UniquePRLibraryName,
+                                      char,
+                                      PR_FreeLibraryName)
 
 MOZ_TYPE_SPECIFIC_UNIQUE_PTR_TEMPLATE(UniqueSECAlgorithmID,
                                       SECAlgorithmID,
                                       internal::SECOID_DestroyAlgorithmID_true)
 MOZ_TYPE_SPECIFIC_UNIQUE_PTR_TEMPLATE(UniqueSECItem,
                                       SECItem,
                                       internal::SECITEM_FreeItem_true)
 MOZ_TYPE_SPECIFIC_UNIQUE_PTR_TEMPLATE(UniqueSECKEYPrivateKey,
--- a/security/manager/ssl/nsNSSComponent.cpp
+++ b/security/manager/ssl/nsNSSComponent.cpp
@@ -1200,19 +1200,17 @@ nsNSSComponent::LoadLoadableRoots()
       }
 
       if (NS_FAILED(mozFile->GetNativePath(libDir))) {
         continue;
       }
     }
 
     NS_ConvertUTF16toUTF8 modNameUTF8(modName);
-    if (mozilla::psm::LoadLoadableRoots(
-            libDir.Length() > 0 ? libDir.get() : nullptr,
-            modNameUTF8.get()) == SECSuccess) {
+    if (mozilla::psm::LoadLoadableRoots(libDir, modNameUTF8)) {
       break;
     }
   }
 }
 
 void
 nsNSSComponent::UnloadLoadableRoots()
 {