Closed Bug 1311996 Opened 5 years ago Closed 5 years ago

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


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




Tracking Status
firefox52 --- fixed


(Reporter: jandem, Assigned: jandem)



(Whiteboard: [psm-assigned])


(1 file, 2 obsolete files)

Attached patch Patch (obsolete) — Splinter Review
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.
Attachment #8803336 - Flags: review?(dkeeler)
Blocks: 1283562
Comment on attachment 8803336 [details] [diff] [review]

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.)
Attachment #8803336 - Flags: review?(dkeeler)
Component: Security → Security: PSM
Priority: -- → P1
Whiteboard: [psm-assigned]
Attached patch Patch (obsolete) — Splinter Review
Thanks for the quick review. I think this addresses your feedback.
Attachment #8803336 - Attachment is obsolete: true
Attachment #8803818 - Flags: review?(dkeeler)
Comment on attachment 8803818 [details] [diff] [review]

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.
Attachment #8803818 - Flags: review?(dkeeler)
Attached patch PatchSplinter Review
Third try. If this still isn't ready, I'd appreciate if you could take this bug.
Attachment #8803818 - Attachment is obsolete: true
Attachment #8804638 - Flags: review?(dkeeler)
Comment on attachment 8804638 [details] [diff] [review]

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

This is perfect - thank you!
Attachment #8804638 - Flags: review?(dkeeler) → review+
Pushed by
Fix code using pkix::Result to not conflict with the new mozilla::Result type. r=keeler
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.