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

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect, P1)

Description

[:Cykesiopka]

Scoped.h is deprecated.

Comment 1

[:Cykesiopka]

Attachment: Bug 1311383 - Stop using Scoped.h NSS types in transportlayerdtls.(cpp|h).

Scoped.h is deprecated.

Review commit: https://reviewboard.mozilla.org/r/86922/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/86922/

Comment 2

[:Cykesiopka]

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.

Comment 3

[Martin Thomson [:mt:]]

Attachment: 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.

Comment 4

[Martin Thomson [:mt:]]

Attachment: Bug 1311383 - Use unique pointers for DTLS transport and related,

Review commit: https://reviewboard.mozilla.org/r/87080/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/87080/

Comment 5

[Martin Thomson [:mt:]]

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.

Comment 6

[:Cykesiopka]

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.)

Comment 7

[Martin Thomson [:mt:]]

Comment on attachment 8802782 [details]
Bug 1311383 - Use unique pointers for DTLS transport and related,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/87080/diff/1-2/

Comment 8

[Martin Thomson [:mt:]]

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 9

[Martin Thomson [:mt:]]

Comment on attachment 8802782 [details]
Bug 1311383 - Use unique pointers for DTLS transport and related,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/87080/diff/2-3/

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a2e55f7205f7
Use unique pointers for DTLS transport and related, r=Cykesiopka

Comment 11

[Ryan VanderMeulen [:RyanVM]]

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