Bug 1313715 - Avoid unnecessary uses of PR_SetError() under security/apps/ and security/certverifier/. r=keeler draft
authorCykesiopka <cykesiopka.bmo@gmail.com>
Wed, 14 Dec 2016 20:10:25 +0800
changeset 449559 aa07c0dfb5cc2a203e772b415b7a75b27d9bad3c
parent 449485 dac601a343ca5591740aa77922f77313d9ec5a51
child 539525 1170c857b0917105a192cd52cf123428f710cb9d
push id38606
push usercykesiopka.bmo@gmail.com
push dateWed, 14 Dec 2016 14:41:43 +0000
reviewerskeeler
bugs1313715, 1254403
milestone53.0a1
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()
 {