Closed Bug 1260643 Opened 4 years ago Closed 4 years ago

Convert most uses of ScopedCERTCertificate in PSM to UniqueCERTCertificate

Categories

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

defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: Cykesiopka, Assigned: Cykesiopka)

References

Details

(Whiteboard: [psm-assigned])

Attachments

(1 file)

ScopedCERTCertificate is based on Scoped.h, which is deprecated. We should use the UniquePtr equivalent.
s/Convert uses/Convert most uses/ because there's been new code landed recently using Scoped[...] that's being actively worked upon, and it seems best to avoid conflicts for now.
Summary: Convert uses of ScopedCERTCertificate in PSM to UniqueCERTCertificate → Convert most uses of ScopedCERTCertificate in PSM to UniqueCERTCertificate
Comment on attachment 8742096 [details]
MozReview Request: Bug 1260643 - Convert most uses of ScopedCERTCertificate in PSM to UniqueCERTCertificate. r=keeler

https://reviewboard.mozilla.org/r/46939/#review44293

LGTM.

::: security/certverifier/CertVerifier.cpp:662
(Diff revision 1)
>                   /*optional out*/ OCSPStaplingStatus* ocspStaplingStatus,
>                   /*optional out*/ KeySizeStatus* keySizeStatus,
>                   /*optional out*/ SHA1ModeResult* sha1ModeResult,
>                   /*optional out*/ PinningTelemetryInfo* pinningTelemetryInfo)
>  {
> -  PR_ASSERT(peerCert);
> +  PR_ASSERT(peerCert.get());

Does PR_ASSERT not cause operator bool() to be used on the UniqueCERTCertificate? (As in, why does .get() need to be added here?) (I guess I'm a little unsure when .get() needs to be used vs. not used.)
Attachment #8742096 - Flags: review?(dkeeler) → review+
https://reviewboard.mozilla.org/r/46939/#review44293

Thanks!

> Does PR_ASSERT not cause operator bool() to be used on the UniqueCERTCertificate? (As in, why does .get() need to be added here?) (I guess I'm a little unsure when .get() needs to be used vs. not used.)

I originally changed these because I wasn't 100% sure operator bool() would work correctly, and I wanted to play it safe.
I did some testing, and it looks like all the ASSERT and NS_ENSURE macros use operator bool() correctly, so I will revert these changes.
Comment on attachment 8742096 [details]
MozReview Request: Bug 1260643 - Convert most uses of ScopedCERTCertificate in PSM to UniqueCERTCertificate. r=keeler

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46939/diff/1-2/
Attachment #8742096 - Attachment description: MozReview Request: Bug 1260643 - Convert most uses of ScopedCERTCertificate in PSM to UniqueCERTCertificate. → MozReview Request: Bug 1260643 - Convert most uses of ScopedCERTCertificate in PSM to UniqueCERTCertificate. r=keeler
https://hg.mozilla.org/mozilla-central/rev/9c98c0300a89
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.