Stop using Scoped.h NSS types in transportlayerdtls.(cpp|h)

RESOLVED FIXED in Firefox 52

Status

()

Core
WebRTC: Networking
P1
normal
Rank:
15
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: Cykesiopka, Assigned: mt)

Tracking

unspecified
mozilla52
Points:
---

Firefox Tracking Flags

(firefox52 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

a year ago
Scoped.h is deprecated.
Comment hidden (mozreview-request)
(Reporter)

Comment 2

a year ago
mozreview-review
Comment on attachment 8802533 [details]
Bug 1311383 - Stop using Scoped.h NSS types in transportlayerdtls.(cpp|h).

https://reviewboard.mozilla.org/r/86920/#review85918

::: media/mtransport/transportlayerdtls.cpp:1233
(Diff revision 1)
>            rv = CheckDigest(digest, peer_cert);
>  
>            // Matches a digest, we are good to go
>            if (rv == SECSuccess) {
>              cert_ok_ = true;
> -            peer_cert = peer_cert.forget();
> +            peer_cert_ = Move(peer_cert);

I think I fixed a bug here?
`peer_cert = peer_cert.forget()` seems like a pointless operation (note lack of underscore for LHS), but confirmation of the change would be good.
Rank: 15
Priority: -- → P1
(Assignee)

Comment 3

a year ago
Created attachment 8802781 [details] [diff] [review]
0001-Bug-1311383-Move-to-UniquePtr-based-constructs-r-cyk.patch

This expands the scope of the patch slightly.  If you wanted to avoid touching other things, maybe you can just crib the removal of peer_cert_ from transportlayerdtls and add it to your patch.
Attachment #8802781 - Flags: review?(cykesiopka.bmo)
Comment hidden (mozreview-request)
(Assignee)

Comment 5

a year ago
mozreview-review
Comment on attachment 8802533 [details]
Bug 1311383 - Stop using Scoped.h NSS types in transportlayerdtls.(cpp|h).

https://reviewboard.mozilla.org/r/86922/#review86110

I think that we should remove `peer_cert_`, I'll leave whether you think the expanded scope of my patch is worthwhile.  Happy to land this, then mine if you prefer that (I can find another reviewer if necessary).

::: media/mtransport/transportlayerdtls.cpp:1159
(Diff revision 1)
>    size_t computed_digest_len;
>  
>    MOZ_MTLOG(ML_DEBUG, LAYER_INFO << "Checking digest, algorithm="
>              << digest->algorithm_);
>    nsresult res =
> -      DtlsIdentity::ComputeFingerprint(peer_cert,
> +      DtlsIdentity::ComputeFingerprint(peer_cert.get(),

This is one case where you didn't move to using a UniqueX reference.  I see that you probably avoided changing `DtlsIdentity::cert_` because of the comment about link errors.

I tried fixing this the obvious way and it worked.  I'll put my patch on the bug and you can try it.

::: media/mtransport/transportlayerdtls.cpp:1233
(Diff revision 1)
>            rv = CheckDigest(digest, peer_cert);
>  
>            // Matches a digest, we are good to go
>            if (rv == SECSuccess) {
>              cert_ok_ = true;
> -            peer_cert = peer_cert.forget();
> +            peer_cert_ = Move(peer_cert);

Yes, this was a bug, but we weren't actually using the value anywhere.  That's probably ALSO a bug, so after discussing with ekr, we concluded that it was best to remove the value entirely.  Again, see my patch.

Thanks for catching this.
Attachment #8802533 - Flags: review?(martin.thomson) → review+
(Reporter)

Updated

a year ago
Attachment #8802781 - Attachment is obsolete: true
Attachment #8802781 - Flags: review?(cykesiopka.bmo)
(Reporter)

Comment 6

a year ago
mozreview-review
Comment on attachment 8802782 [details]
Bug 1311383 - Use unique pointers for DTLS transport and related,

https://reviewboard.mozilla.org/r/87080/#review86484

Looks good, thanks!
(Though make sure you do a try push to ensure everything links fine on all platforms.)

::: dom/media/webrtc/RTCCertificate.h:63
(Diff revision 1)
>      return mExpires / PR_USEC_PER_MSEC;
>    }
>  
>    // Accessors for use by PeerConnectionImpl.
>    RefPtr<DtlsIdentity> CreateDtlsIdentity() const;
> -  CERTCertificate* Certificate() const { return mCertificate; }
> +  const UniqueCERTCertificate& Certificate() const { return mCertificate; }

> Bug 1311383 - Move to UniquePtr-based constructs, r?cykesiopka
Note that the UniqueX types are now actually based on std::unique_ptr.
(MozReview doesn't allow commit messages to be commented on yet, so I'm just abusing this line to make this note visible.)
Attachment #8802782 - Flags: review?(cykesiopka.bmo) → review+
(Reporter)

Updated

a year ago
Attachment #8802533 - Attachment is obsolete: true
(Reporter)

Updated

a year ago
Assignee: cykesiopka.bmo → martin.thomson
Comment hidden (mozreview-request)
(Assignee)

Comment 8

a year ago
Hmm, windows pgo builds report being busted.  However I can't actually see any reason for it in the log.  I'm going to ask around.
Comment hidden (mozreview-request)

Comment 10

a year ago
Pushed by martin.thomson@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/a2e55f7205f7
Use unique pointers for DTLS transport and related, r=Cykesiopka

Comment 11

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