1311996 - pkix::Result conflicts with the new mozilla::Result type

Status: RESOLVED FIXED

(Core :: Security: PSM, defect, P1)

Description

[Jan de Mooij [:jandem]]

Attachment: Patch

Bug 1277368 is adding a mozilla::Result type, but that's causing build failures because it conflicts with the pkix::Result enum. This patch changes the security/ code to explicitly use the pkix namespace.

I can also rename it to PkixResult or something. Let me know if you have other ideas.

Comment 1

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8803336 [details] [diff] [review]
Patch

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

Thanks for taking care of this. Stylistically, it looks like using the full "mozilla::pkix::Result" is more common than just "pkix::Result" (some files even use "typedef mozilla::pkix::Result Result;") (except for the CT code, which it looks like does use just "pkix::Result"). So, to be consistent, let's either use the full namespace or the typedef (again, except for the CT code, which is fine as-is (so that's CTSerialization.cpp and MultiLogCTVerifier.cpp)). (Also, we currently don't ever use "using mozilla::pkix::Result;", so I'd rather not go that route either.)

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Patch

Thanks for the quick review. I think this addresses your feedback.

Comment 3

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8803818 [details] [diff] [review]
Patch

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

This looks good, except I was a bit hasty with an earlier comment (see below). Sorry about that. Also, I think we should handle OCSPVerificationTrustDomain.cpp slightly differently.

::: security/certverifier/MultiLogCTVerifier.cpp
@@ +15,5 @@
>  
>  using namespace mozilla::pkix;
>  
>  // Note: this moves |sct| to the target list in |result|, invalidating |sct|.
> +static pkix::Result

Shoot - it actually looks like I was wrong (the use of "pkix::Result" is really only in the CT header files). Let's just use the typedef here and keep these as Result.

::: security/certverifier/OCSPVerificationTrustDomain.cpp
@@ +14,5 @@
>    : mCertDBTrustDomain(certDBTrustDomain)
>  {
>  }
>  
> +mozilla::pkix::Result

Actually, I think what might be more correct here is to wrap the implementation in this file  in "namespace mozilla { namespace psm {" (and a corresponding "} } // namespace mozilla::psm" at the end), since that's how it's declared in the header file (I believe this only compiles currently due to unified compilation and that some other file says "using namespace mozilla::psm;").

Anyway, at that point I think we can just rely on the typedef in OCSPVerificationTrustDomain.h.

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Patch

Third try. If this still isn't ready, I'd appreciate if you could take this bug.

Comment 5

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8804638 [details] [diff] [review]
Patch

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

This is perfect - thank you!

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a24793dce64c
Fix code using pkix::Result to not conflict with the new mozilla::Result type. r=keeler

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a24793dce64c