Closed Bug 1039064 Opened 6 years ago Closed 6 years ago

Use a strongly-typed enum for mozilla::pkix::Result instead of NSPR-style error handling

Categories

(Core :: Security: PSM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34
Tracking Status
firefox33 + fixed
firefox34 --- fixed

People

(Reporter: briansmith, Assigned: briansmith)

References

Details

Attachments

(3 files, 10 obsolete files)

323.55 KB, patch
briansmith
: review+
Details | Diff | Splinter Review
19.06 KB, patch
keeler
: review+
Details | Diff | Splinter Review
322.55 KB, patch
keeler
: review+
Details | Diff | Splinter Review
Attached patch Part 1: mozilla::pkix parts (obsolete) — Splinter Review
In an attempt to mislead you into thinking that this patch isn't that big, I have split the patch up into four parts. However, the four parts do not build independently of each other and they will need to be folded together before being pushed to inbound.

Forgetting to call PR_SetError and calling PR_GetError at the wrong time are the two most common bugs I've seen in my time hacking on Gecko. Also, in general, it is hard to reason about code that has this hidden mutable shared state affecting almost ever line of code. Additionally, I am trying to remove as many NSPR/NSS dependencies are possible. Thus, these patches.
Attachment #8456572 - Flags: review?(dkeeler)
Attached patch Part 2: CertVerifier parts (obsolete) — Splinter Review
Attachment #8456574 - Flags: review?(dkeeler)
Attached patch Part 4: Update tests (obsolete) — Splinter Review
Attachment #8456576 - Flags: review?(dkeeler)
Comment on attachment 8456574 [details] [diff] [review]
Part 2: CertVerifier parts

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

::: security/certverifier/NSSCertDBTrustDomain.cpp
@@ +454,3 @@
>    }
>  
> +  Result rv = Success; // TODO: Fixing a bug in using PR_GetError uninitialized?

Please pay special attention to the changes to this file, especially this method, and especially this line and the other line I will mark below...

@@ +505,4 @@
>    }
>  
>    if (!response) {
> +    Result error = rv;

The way we read rv here, assuming it has been set, seems tricky. If I don't initialize rv = Success when I declare rv above, then I get a warning that rv may be used uninitialized, and I think the compiler is right. I am having trouble following the original code and I think I may have found a bug. In particular, in the original code to the left, it isn't clear how we know that anything has called PR_SetError() at the time when PR_GetError is called.
Here is the try push, though it seems trysever is acting particularly poorly today:
https://tbpl.mozilla.org/?tree=Try&rev=83cd36a2c886

I did verify locally that "./mach build && ./mach gtest "pkix*" && ./mach xpcshell-test security/manager/ssl/tests/unit" succeeded, as I always do.
Comment on attachment 8456572 [details] [diff] [review]
Part 1: mozilla::pkix parts

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

::: security/pkix/lib/pkixocsp.cpp
@@ +857,5 @@
>  
> +Result
> +CreateEncodedOCSPRequest(TrustDomain& trustDomain, const struct CertID& certID,
> +                         /*out*/ uint8_t (&out)[OCSP_REQUEST_MAX_LENGTH],
> +                         /*out*/ size_t& outLen)

Previously, callers would write:

SECItem* result = CreateEncodedOCSPRequest(...);
if (!result) {
  PRErrorCode rv = PR_GetError();
}

Thus, result acted both as the returned value and the indicator of an error. I had to change this function so that it could return both an error code and the returned value as separate values. Since I also wanted to remove the memory allocation and also remove the PLArenaPool usage, I went ahead and refactored this into what I think will be its final form.
Attached patch Part 4: Update tests [v2] (obsolete) — Splinter Review
Attachment #8456576 - Attachment is obsolete: true
Attachment #8456576 - Flags: review?(dkeeler)
Attachment #8456649 - Flags: review?(dkeeler)
Attachment #8456582 - Attachment is obsolete: true
Attachment #8456582 - Flags: review?(dkeeler)
Attachment #8456650 - Flags: review?(dkeeler)
Here's another go at tryserver, since the previous run was super borked by releng problems. It seems like there is a big backlog of tests but everything looks good so far:
https://tbpl.mozilla.org/?tree=Try&rev=718b9d1d7834
Comment on attachment 8456572 [details] [diff] [review]
Part 1: mozilla::pkix parts

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

This looks good, but I'm concerned about how the PSM-registered key pinning error is supposed to work with this (see the third comment).

::: security/pkix/include/pkix/pkix.h
@@ +103,4 @@
>  
>  // The out parameter expired will be true if the response has expired. If the
>  // response also indicates a revoked or unknown certificate, that error
> +// will be returned. Otherwise, SEC_ERROR_OCSP_OLD_RESPONSE will be returned

Wouldn't this be Result::ERROR_OCSP_OLD_RESPONSE now?

::: security/pkix/include/pkix/pkixnss.h
@@ +46,5 @@
>  // TODO(bug 966856): Add SHA-2 support
>  // TODO: Taking the output buffer as (uint8_t*, size_t) is counter to our
>  // other, extensive, memory safety efforts in mozilla::pkix, and we should find
>  // a way to provide a more-obviously-safe interface.
> +Result DigestBuf(const SECItem& item, /*out*/ uint8_t *digestBuf,

nit: uint8_t*

@@ +67,5 @@
> +static const PRErrorCode ERROR_BASE = -0x4000;
> +static const PRErrorCode ERROR_LIMIT = ERROR_BASE + 1000;
> +
> +enum ErrorCode {
> +  SEC_ERROR_KEY_PINNING_FAILURE = ERROR_BASE + 0

This is now out of sync with the string in PSMErrorTableText. I think we're going to have a hard time keeping these two data structures in sync if they aren't in the same area of code. If it's a requirement that all TrustDomain implementations use only Results known to mozilla::pkix, we need to come up with another solution for how this is going to work. Maybe all of the PSM error registration infrastructure should be moved to the nss-specific parts of mozilla::pkix?

::: security/pkix/include/pkix/pkixtypes.h
@@ +315,5 @@
>    // TODO: Taking the output buffer as (uint8_t*, size_t) is counter to our
>    // other, extensive, memory safety efforts in mozilla::pkix, and we should
>    // find a way to provide a more-obviously-safe interface.
>    static const size_t DIGEST_LENGTH = 20; // length of SHA-1 digest
> +  virtual Result DigestBuf(const SECItem& item, /*out*/ uint8_t *digestBuf,

nit: uint8_t*

::: security/pkix/lib/pkixcheck.cpp
@@ +371,3 @@
>        // used as end-entity" error code since it doesn't have such a
>        // prohibition. We should add such an error code and stop abusing
>        // SEC_ERROR_CA_CERT_INVALID this way.

This is probably a good time to update this comment and/or address the underlying issue (add another error code?)

::: security/pkix/lib/pkixnss.cpp
@@ +37,5 @@
>  
>  namespace mozilla { namespace pkix {
>  
> +//Result
> +//MapSECStatus(SECStatus srv)

Can we just remove this? Or is this informational?

@@ +161,5 @@
> +  }
> +  return Success;
> +}
> +
> +#define MAP_LIST \

Clever. It would be nice to be able to programmatically check that every value in the Result enum is handled here - do you know of a way of doing that?

::: security/pkix/lib/pkixocsp.cpp
@@ +334,2 @@
>      case CertStatus::Revoked:
> +      return Result::ERROR_REVOKED_CERTIFICATE;

Since mozilla::pkix is going to have its own error values, we don't have to follow how NSS names/uses the errors (unless that's the whole idea?). Maybe we could be a bit more consistent about how these are named: "Result::ERROR_OCSP_REVOKED_CERT".
Attachment #8456572 - Flags: review?(dkeeler) → review-
Comment on attachment 8456574 [details] [diff] [review]
Part 2: CertVerifier parts

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

This looks good. In particular, I believe you are correct about there being a bug where this code uses PR_GetError when it hasn't been deterministically set. There's another bug I noticed, as well. r=me with comments addressed.

::: security/certverifier/NSSCertDBTrustDomain.cpp
@@ +338,4 @@
>    if (stapledOCSPResponse) {
>      PR_ASSERT(endEntityOrCA == EndEntityOrCA::MustBeEndEntity);
>      bool expired;
> +    stapledOCSPResponseResult = 

nit: trailing space

@@ +377,3 @@
>                                                cachedResponseValidThrough);
>    if (cachedResponsePresent) {
> +    if (cachedResponseResult  == Success && cachedResponseValidThrough >= time) {

nit: unnecessary extra space before '=='

@@ +411,5 @@
>    } else {
>      PR_LOG(gCertVerifierLog, PR_LOG_DEBUG,
>             ("NSSCertDBTrustDomain: no cached OCSP response"));
>    }
> +  // At this point, if and only if cachedErrorResult  is Success, there was no

nit: unnecessary extra space before "is"

@@ +505,4 @@
>    }
>  
>    if (!response) {
> +    Result error = rv;

Looks like you're right - if cachedResponseErrorCode is something like SEC_ERROR_OCSP_SERVER_ERROR, PR_GetError() would be unrelated to that error and could be more or less anything.

@@ +511,3 @@
>      }
>      PRTime timeout = time + ServerFailureDelay;
> +    rv = mOCSPCache.Put(certID, error, time, timeout);

Looks like another bug with the original implementation - are we putting back into the cache the cached error? Again, if this is something like SEC_ERROR_OCSP_SERVER_ERROR, that value will never expire and we'll never try to contact the server again :(
I think the right thing to do here is only call Put if error != Success (before setting it to cachedResponseResult)

::: security/certverifier/NSSCertDBTrustDomain.h
@@ +55,5 @@
>                         CertVerifier::ocsp_get_config ocspGETConfig,
>            /*optional*/ CERTChainVerifyCallback* checkChainCallback = nullptr,
>            /*optional*/ ScopedCERTCertList* builtChain = nullptr);
>  
> +  Result FindIssuer(const SECItem& encodedIssuerName, IssuerChecker& checker,

Is the presence/absence of the "virtual" keyword important?

::: security/certverifier/OCSPCache.h
@@ +27,5 @@
>  
>  #include "hasht.h"
>  #include "mozilla/Mutex.h"
>  #include "mozilla/Vector.h"
> +#include "pkix/Input.h"

Shouldn't this be pkix/Result.h?
Attachment #8456574 - Flags: review?(dkeeler) → review+
Comment on attachment 8456575 [details] [diff] [review]
Part 3: Changes to the rest of PSM

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

This looks fine modulo the issues already mentioned in part 1.

::: netwerk/base/public/nsINSSErrorsService.idl
@@ +51,5 @@
>      const long NSS_SSL_ERROR_BASE  = -(0x3000);
>      const long NSS_SSL_ERROR_LIMIT = (NSS_SSL_ERROR_BASE + 1000);
>  
>      /**
> +     * Keep in sync with pkixnss.h

Can we add some static asserts somewhere that ensures this?

::: security/manager/ssl/src/NSSErrorsService.h
@@ -15,5 @@
>  namespace mozilla {
>  namespace psm {
>  
> -enum PSMErrorCodes {
> -  PSM_ERROR_KEY_PINNING_FAILURE = (nsINSSErrorsService::PSM_ERROR_BASE + 0)

So, again, we moved/renamed this but didn't address the error table strings.
Attachment #8456575 - Flags: review?(dkeeler) → review+
Comment on attachment 8456572 [details] [diff] [review]
Part 1: mozilla::pkix parts

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

::: security/pkix/lib/pkixocsp.cpp
@@ +903,5 @@
> +  static_assert(totalLenWithoutSerialNumberData < OCSP_REQUEST_MAX_LENGTH,
> +                "totalLenWithoutSerialNumberData too big");
> +  if (certID.serialNumber.len >
> +        OCSP_REQUEST_MAX_LENGTH - totalLenWithoutSerialNumberData) {
> +    return Result::ERROR_BAD_DER;

Oh, I didn't notice this until I got to a later patch - any particular reason we don't want to have a Result::ERROR_BAD_DATA to differentiate this case here? (I just want to confirm that this is a deliberate choice rather than an oversight.)
Comment on attachment 8456649 [details] [diff] [review]
Part 4: Update tests [v2]

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

LGTM with nits addressed.

::: security/manager/ssl/tests/gtest/OCSPCacheTest.cpp
@@ +244,2 @@
>                       timeIn + 1026 - 50, timeIn + 1026);
> +  ASSERT_EQ(result, Result::ERROR_REVOKED_CERTIFICATE);

It would probably be best if we were consistent on which value goes first for ASSERT_EQ statements (the expected value or the actual value). I realize the style of this test isn't great in the first place, but as long as you're changing these, this seems like a good opportunity to make it right.

::: security/pkix/test/gtest/pkixbuild_tests.cpp
@@ +170,5 @@
>      return ::mozilla::pkix::VerifySignedData(signedData, subjectPublicKeyInfo,
>                                               nullptr);
>    }
>  
> +  Result DigestBuf(const SECItem& item, /*out*/ uint8_t *digestBuf,

nit: uint8_t*

::: security/pkix/test/gtest/pkixcert_extension_tests.cpp
@@ +108,5 @@
>      return ::mozilla::pkix::VerifySignedData(signedData, subjectPublicKeyInfo,
>                                               nullptr);
>    }
>  
> +  Result DigestBuf(const SECItem&, /*out*/ uint8_t *, size_t)

nit: uint8_t*

::: security/pkix/test/gtest/pkixder_input_tests.cpp
@@ +124,5 @@
>  
>    ASSERT_EQ(Success,
>              input.Init(DER_SEQUENCE_OF_INT8, sizeof DER_SEQUENCE_OF_INT8));
>  
> +  ASSERT_EQ(Result::FATAL_ERROR_INVALID_ARGS, 

nit: trailing space

@@ +804,5 @@
>    ASSERT_EQ(Success, input.Init(DER_TRUNCATED_SEQUENCE_OF_INT8,
>                                  sizeof DER_TRUNCATED_SEQUENCE_OF_INT8));
>  
>    std::vector<uint8_t> readValues;
> +  ASSERT_EQ(Result::ERROR_BAD_DER, 

nit: trailing space

::: security/pkix/test/gtest/pkixocsp_CreateEncodedOCSPRequest_tests.cpp
@@ +62,3 @@
>    }
>  
> +  Result VerifySignedData(const SignedDataWithSignature&, const SECItem&)

So, again, the presence/absence of "virtual". In particular, this class isn't consistent (we should at least be consistent within each TrustDomain we define).

@@ +68,3 @@
>    }
>  
> +  Result DigestBuf(const SECItem& item, /*out*/ uint8_t *digestBuf,

nit: uint8_t*

::: security/pkix/test/gtest/pkixocsp_VerifyEncodedOCSPResponse.cpp
@@ +82,5 @@
>      return ::mozilla::pkix::VerifySignedData(signedData, subjectPublicKeyInfo,
>                                               nullptr);
>    }
>  
> +  Result DigestBuf(const SECItem& item, /*out*/ uint8_t *digestBuf,

nit: uint8_t*
Attachment #8456649 - Flags: review?(dkeeler) → review+
Comment on attachment 8456572 [details] [diff] [review]
Part 1: mozilla::pkix parts

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

::: security/pkix/include/pkix/pkix.h
@@ +103,4 @@
>  
>  // The out parameter expired will be true if the response has expired. If the
>  // response also indicates a revoked or unknown certificate, that error
> +// will be returned. Otherwise, SEC_ERROR_OCSP_OLD_RESPONSE will be returned

I went through every mention of SEC_ERROR_* and changed them to Result::ERROR_* as appropriate.

::: security/pkix/include/pkix/pkixnss.h
@@ +46,5 @@
>  // TODO(bug 966856): Add SHA-2 support
>  // TODO: Taking the output buffer as (uint8_t*, size_t) is counter to our
>  // other, extensive, memory safety efforts in mozilla::pkix, and we should find
>  // a way to provide a more-obviously-safe interface.
> +Result DigestBuf(const SECItem& item, /*out*/ uint8_t *digestBuf,

I made this change for all of security/apps, security/certverifier, and security/pkix.

@@ +67,5 @@
> +static const PRErrorCode ERROR_BASE = -0x4000;
> +static const PRErrorCode ERROR_LIMIT = ERROR_BASE + 1000;
> +
> +enum ErrorCode {
> +  SEC_ERROR_KEY_PINNING_FAILURE = ERROR_BASE + 0

I moved the error table stuff into pkixnss.cpp.

::: security/pkix/lib/pkixcheck.cpp
@@ +371,3 @@
>        // used as end-entity" error code since it doesn't have such a
>        // prohibition. We should add such an error code and stop abusing
>        // SEC_ERROR_CA_CERT_INVALID this way.

I filed bug 1040446 for this improvement and I updated the comment to say TODO(bug 1040446) instead of XXX.

::: security/pkix/lib/pkixnss.cpp
@@ +37,5 @@
>  
>  namespace mozilla { namespace pkix {
>  
> +//Result
> +//MapSECStatus(SECStatus srv)

I removed this commented-out code.

@@ +161,5 @@
> +  }
> +  return Success;
> +}
> +
> +#define MAP_LIST \

I am going to investigate this in a separate patch (Part 6 or whatever) in this bug.

::: security/pkix/lib/pkixocsp.cpp
@@ +334,2 @@
>      case CertStatus::Revoked:
> +      return Result::ERROR_REVOKED_CERTIFICATE;

The issue here is that a certificate may be revoked using a mechanism other than OCSP, such as CRLSets.

@@ +903,5 @@
> +  static_assert(totalLenWithoutSerialNumberData < OCSP_REQUEST_MAX_LENGTH,
> +                "totalLenWithoutSerialNumberData too big");
> +  if (certID.serialNumber.len >
> +        OCSP_REQUEST_MAX_LENGTH - totalLenWithoutSerialNumberData) {
> +    return Result::ERROR_BAD_DER;

This was intentional. The problem is that the certificate is bad, but SEC_ERROR_BAD_DATA is more general and has more vague error messages. This is something that we could improve upon in the future, although I imagine that this is an extremely rare occurrence so we probably shouldn't bother.
Comment on attachment 8456574 [details] [diff] [review]
Part 2: CertVerifier parts

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

::: security/certverifier/NSSCertDBTrustDomain.cpp
@@ +338,4 @@
>    if (stapledOCSPResponse) {
>      PR_ASSERT(endEntityOrCA == EndEntityOrCA::MustBeEndEntity);
>      bool expired;
> +    stapledOCSPResponseResult = 

nixed.

@@ +377,3 @@
>                                                cachedResponseValidThrough);
>    if (cachedResponsePresent) {
> +    if (cachedResponseResult  == Success && cachedResponseValidThrough >= time) {

nixed.

@@ +411,5 @@
>    } else {
>      PR_LOG(gCertVerifierLog, PR_LOG_DEBUG,
>             ("NSSCertDBTrustDomain: no cached OCSP response"));
>    }
> +  // At this point, if and only if cachedErrorResult  is Success, there was no

nixed.

@@ +511,3 @@
>      }
>      PRTime timeout = time + ServerFailureDelay;
> +    rv = mOCSPCache.Put(certID, error, time, timeout);

I tried doing what you suggest, but the tests are not passing. Is my variant of this worse than the existing code? If not, let's file a new bug for fixing this, especially since it may need to be uplifted to Aurora and/or Beta.
Comment on attachment 8456575 [details] [diff] [review]
Part 3: Changes to the rest of PSM

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

::: netwerk/base/public/nsINSSErrorsService.idl
@@ +51,5 @@
>      const long NSS_SSL_ERROR_BASE  = -(0x3000);
>      const long NSS_SSL_ERROR_LIMIT = (NSS_SSL_ERROR_BASE + 1000);
>  
>      /**
> +     * Keep in sync with pkixnss.h

I added some static_asserts.

::: security/manager/ssl/src/NSSErrorsService.h
@@ -15,5 @@
>  namespace mozilla {
>  namespace psm {
>  
> -enum PSMErrorCodes {
> -  PSM_ERROR_KEY_PINNING_FAILURE = (nsINSSErrorsService::PSM_ERROR_BASE + 0)

Fixed.
Comment on attachment 8456649 [details] [diff] [review]
Part 4: Update tests [v2]

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

::: security/manager/ssl/tests/gtest/OCSPCacheTest.cpp
@@ +244,2 @@
>                       timeIn + 1026 - 50, timeIn + 1026);
> +  ASSERT_EQ(result, Result::ERROR_REVOKED_CERTIFICATE);

Fixed.

::: security/pkix/test/gtest/pkixder_input_tests.cpp
@@ +124,5 @@
>  
>    ASSERT_EQ(Success,
>              input.Init(DER_SEQUENCE_OF_INT8, sizeof DER_SEQUENCE_OF_INT8));
>  
> +  ASSERT_EQ(Result::FATAL_ERROR_INVALID_ARGS, 

nixed.

@@ +804,5 @@
>    ASSERT_EQ(Success, input.Init(DER_TRUNCATED_SEQUENCE_OF_INT8,
>                                  sizeof DER_TRUNCATED_SEQUENCE_OF_INT8));
>  
>    std::vector<uint8_t> readValues;
> +  ASSERT_EQ(Result::ERROR_BAD_DER, 

nixed.

::: security/pkix/test/gtest/pkixocsp_CreateEncodedOCSPRequest_tests.cpp
@@ +62,3 @@
>    }
>  
> +  Result VerifySignedData(const SignedDataWithSignature&, const SECItem&)

I added "virtual" to the methods that were missing it.
(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #17)
> @@ +511,3 @@
> >      }
> >      PRTime timeout = time + ServerFailureDelay;
> > +    rv = mOCSPCache.Put(certID, error, time, timeout);
> 
> I tried doing what you suggest, but the tests are not passing. Is my variant
> of this worse than the existing code? If not, let's file a new bug for
> fixing this, especially since it may need to be uplifted to Aurora and/or
> Beta.

This patch doesn't make this bug worse. I filed bug 1040889.
Here is the interdiff on top of part 5, which also spome bits of the rebasing on top of mozilla-inbound (bug 360126 in particular).
Attachment #8458942 - Flags: review?(dkeeler)
Attached patch Part 6: address review comments (obsolete) — Splinter Review
This Note that some of the minor nits were addressed, but not in this patch.

Also, I'm still working on the static checking of the completeness of the MAP_LIST macro; I will post a separate patch for that.
Attachment #8458942 - Attachment is obsolete: true
Attachment #8458942 - Flags: review?(dkeeler)
Attachment #8458947 - Flags: review?(dkeeler)
Comment on attachment 8458947 [details] [diff] [review]
Part 6: address review comments

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

Great - r=me as long as we don't re-use the prefix SEC_ERROR for mozilla::pkix errors.

::: netwerk/base/public/nsINSSErrorsService.idl
@@ +62,5 @@
> +     * mozilla::pkix errors will start at a negative value that both doesn't
> +     * overlap with the current value ranges for NSS errors and that will fit
> +     * in 16 bits when negated.
> +     *
> +     * Keep these in sync with pkixnss.h.

We probably only need one "Keep these in sync with pkixnss.h." comment :)

::: security/pkix/include/pkix/pkixnss.h
@@ +70,5 @@
>  static const PRErrorCode ERROR_BASE = -0x4000;
>  static const PRErrorCode ERROR_LIMIT = ERROR_BASE + 1000;
>  
>  enum ErrorCode {
>    SEC_ERROR_KEY_PINNING_FAILURE = ERROR_BASE + 0

"SEC_ERROR_" is going to confuse people. Maybe "MOZILLA_PKIX_ERROR_" or "MOZPKIX_ERROR_" to be shorter?
Attachment #8458947 - Flags: review?(dkeeler) → review+
Comment on attachment 8456572 [details] [diff] [review]
Part 1: mozilla::pkix parts

Part 6 addresses my concerns with this patch.
Attachment #8456572 - Flags: review- → review+
Attachment #8456572 - Attachment is obsolete: true
Attachment #8456574 - Attachment is obsolete: true
Attachment #8456575 - Attachment is obsolete: true
Attachment #8456649 - Attachment is obsolete: true
Attachment #8456650 - Attachment is obsolete: true
Attachment #8458947 - Attachment is obsolete: true
Attachment #8460705 - Flags: review+
David, please review the parts of this that have to deal with bug 1040889?
Attachment #8460706 - Flags: review?(dkeeler)
Comment on attachment 8460706 [details] [diff] [review]
NSSCertDBTrustDomain::CheckRevocation part of patch

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

This looks good. I'll adapt the patch in bug 1040889 to be more like this solution, since we'll have to uplift that patch.
Attachment #8460706 - Flags: review?(dkeeler) → review+
(In reply to David Keeler (:keeler) [use needinfo?] from comment #27)
> This looks good. I'll adapt the patch in bug 1040889 to be more like this
> solution, since we'll have to uplift that patch.

Thanks. Just so we don't deadlock: I will wait to land this until after bug 1040889 has landed, to make your uplifting easier.
Target Milestone: mozilla33 → mozilla34
https://hg.mozilla.org/mozilla-central/rev/5f7dc391e861
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Attached patch patch for beta (obsolete) — Splinter Review
Approval Request Comment
[Feature/regressing bug #]: mozilla::pkix (this is needed for bug 1034124, which will fix a significant compatibility issue)
[User impact if declined]: see bug 1034124
[Describe test coverage new/current, TBPL]: we have significant test coverage
[Risks and why]: some risk - this is a large patch. However, it's already in aurora, and we've had no problems yet.
[String/UUID change made/needed]: none
Attachment #8486074 - Flags: review+
Attachment #8486074 - Flags: approval-mozilla-beta?
[Tracking Requested - why for this release]: see comment 32
(In reply to David Keeler (:keeler) [use needinfo?] from comment #32)
> Created attachment 8486074 [details] [diff] [review]
> patch for beta
> 
> Approval Request Comment
> [Feature/regressing bug #]: mozilla::pkix (this is needed for bug 1034124,
> which will fix a significant compatibility issue)
> [User impact if declined]: see bug 1034124
> [Describe test coverage new/current, TBPL]: we have significant test coverage
> [Risks and why]: some risk - this is a large patch. However, it's already in
> aurora, and we've had no problems yet.

This requires a substantial code change. I'm reluctant to take a change of this size due to the potential regression risk. I asked questions in bug 1034124 that I think we need to address before really considering this for beta.

> [String/UUID change made/needed]: none

This answer is incorrect as there is a UUID bump in the patch. The general rule is no UUID bumps on beta unless it is the lessor of two evils (the other being leaving a serious issue in the release). I don't yet know that this feature qualifies.
(In reply to Lawrence Mandel [:lmandel] from comment #34)
> (In reply to David Keeler (:keeler) [use needinfo?] from comment #32)
> > [String/UUID change made/needed]: none
> 
> This answer is incorrect as there is a UUID bump in the patch.

Sorry - that was careless of me. I'll be more diligent in the future.
(In reply to David Keeler (:keeler) [use needinfo?] from comment #35)
> Sorry - that was careless of me. I'll be more diligent in the future.

np. We all make mistakes. This is why I review patches along with the questionnaire. 

Given that we don't want to take UUID changes on beta, if we are to accept this feature, is there a way to implement it that doesn't require an UUID bump?
Flags: needinfo?(dkeeler)
Yes - the UUID change was just for some constant renamings, which aren't essential - I could prepare a patch that uses the old names.
Flags: needinfo?(dkeeler)
Approval Request Comment
(see previous approval request comment)
[String/UUID change made/needed]: now with actually no idl changes
Attachment #8486074 - Attachment is obsolete: true
Attachment #8486074 - Flags: approval-mozilla-beta?
Attachment #8488911 - Flags: review+
Attachment #8488911 - Flags: approval-mozilla-beta?
Why a refactoring of code is necessary for the uplift? Can this be avoided for bug 1034124?
Flags: needinfo?(dkeeler)
Comment on attachment 8488911 [details] [diff] [review]
patch for beta without idl change

Taking this prereq of bug 1034124. While it is a large patch, a good chunk is tests and the patch is mainly refactoring, which looks pretty safe. keeler is on call in case any follow-ups are needed to respond to breakage. beta+
Attachment #8488911 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
I think this will be the path of least breakage, but yes, I'm on call for any fallout.
Flags: needinfo?(dkeeler)
Backed out from beta for ASAN xpcshell hangs in the addon manager tests. Looks like tests were randomly hanging until they hit the 5min test timeout, at which point the harness tried to kill it, failed, and eventually hit the 1000s harness timeout instead.
https://hg.mozilla.org/releases/mozilla-beta/rev/b8c9b76b6585

https://tbpl.mozilla.org/php/getParsedLog.php?id=48113327&tree=Mozilla-Beta
https://tbpl.mozilla.org/php/getParsedLog.php?id=48117384&tree=Mozilla-Beta
The patches in that push weren't at fault. Sorry for the churn.
https://hg.mozilla.org/releases/mozilla-beta/rev/1f599d357743
According to previous comments, these patches seem to already have significant test coverage, so I'm marking this qe-verify-.

Brian, if you think there is something manual QA can look at here, please flip the flag.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.