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

RESOLVED FIXED in Firefox 52

Status

()

defect
P1
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

unspecified
mozilla52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

(Whiteboard: [psm-assigned])

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

3 years ago
Posted 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)
(Assignee)

Updated

3 years ago
Blocks: 1283562
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.)
Attachment #8803336 - Flags: review?(dkeeler)

Updated

3 years ago
Component: Security → Security: PSM
Priority: -- → P1
Whiteboard: [psm-assigned]
(Assignee)

Comment 2

3 years ago
Posted 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]
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.
Attachment #8803818 - Flags: review?(dkeeler)
(Assignee)

Comment 4

3 years ago
Posted 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]
Patch

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

This is perfect - thank you!
Attachment #8804638 - Flags: review?(dkeeler) → review+

Comment 6

3 years ago
Pushed by jandemooij@gmail.com:
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

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a24793dce64c
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.