892148 - GetFingerprint in PeerConnectionImpl.cpp is broken

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Description

[Ryan Seys]

Currently PeerConnectionImpl::GetFingerprint in PeerConnectionImpl.cpp is commented out, and uncommenting won't build.

Comment 1

[Max Jonas Werner [:Makkes]]

Attachment: 0001-Make-DTLS-fingerprint-accessible-from-JS.patch

Comment 2

[Martin Thomson [:mt:]]

Comment on attachment 819865 [details] [diff] [review]
0001-Make-DTLS-fingerprint-accessible-from-JS.patch

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

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +752,4 @@
>    // DTLS identity
>    unsigned char fingerprint[DTLS_FINGERPRINT_LENGTH];
>    size_t fingerprint_length;
> +  res = mIdentity->ComputeFingerprint("sha-256",

I always get a little worried when you have code that looks like this:
x = thing->doSomething(...);
y = thing->doNextThing(x, ...);
z = thing->keepGoing(y, ...);

PeerConnectionImpl.cpp shouldn't know that sha-256 is the thing to use.  That's an identity thing.  Have the identity class deal with all that stuff.

Comment 3

[Adam Roach [:abr]]

Comment on attachment 819865 [details] [diff] [review]
0001-Make-DTLS-fingerprint-accessible-from-JS.patch

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

r- for the new[]/delete mismatch. Otherwise, this looks ready to go with one nit.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +752,4 @@
>    // DTLS identity
>    unsigned char fingerprint[DTLS_FINGERPRINT_LENGTH];
>    size_t fingerprint_length;
> +  res = mIdentity->ComputeFingerprint("sha-256",

Martin has volunteered to fix this in a follow-up; see Bug 946348.

@@ +1264,3 @@
>  
>  NS_IMETHODIMP
>  PeerConnectionImpl::GetFingerprint(char** fingerprint)

Just as a minor nit, since we're uncommenting this code: I think the use of NS_ERROR_FAILURE when mIdentity is missing is too generic, and that we should instead be returning NS_ERROR_NOT_INITIALIZED.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h
@@ +332,5 @@
> +  {
> +    char *tmp;
> +    GetFingerprint(&tmp);
> +    fingerprint.AssignASCII(tmp);
> +    delete tmp;

delete[] tmp;

I've filed Bug 946377 to fix the other errant deletes in this header file.

Comment 4

[Martin Thomson [:mt:]]

Attachment: Makes DTLS fingerprint accessible from JS

I've just added brackets to the delete to address Adam's concern.  All the identity patches I've got loaded work with the change, so I think that we're good to go.  (Not sure about BMO protocol at this stage, but I'm going to checkin? :abr and let him tell me that I'm wrong.)

Comment 5

[Adam Roach [:abr]]

(In reply to Martin Thomson [:mt] from comment #4)
> (Not sure about BMO protocol at this stage, but I'm going to
> checkin? :abr and let him tell me that I'm wrong.)

Generally, you want to get an r+ before you do a checkin?, since other people (RyanVM, in particular) will sometimes search b.m.o for patches marked "checkin?" and check them in. You should treat setting this flag with pretty much the same reverence you would a repo push.

Comment 6

[Adam Roach [:abr]]

(In reply to Martin Thomson [:mt] from comment #4)
> I've just added brackets to the delete to address Adam's concern.

Thanks. One note for future reference (and this is me personally -- others may feel differently): if I call out a nit in a review, I expect some action; either a code change, or an explanation as to why it is preferable not to fix the nit. Pretty much any explanation will do, but I don't like screaming into the void.

In this case, given that the timeframe on this is critical, the nit is exceedingly minor, and the tree is in a rare open state, I'll land the patch as-is.

Comment 7

[Adam Roach [:abr]]

I'm running a local build for sanity-checking purposes, and plan to push onto m-i before retiring for the evening (assuming the local build doesn't turn up anything fishy).

Comment 8

[Adam Roach [:abr]]

Comment on attachment 8342705 [details] [diff] [review]
Makes DTLS fingerprint accessible from JS

https://hg.mozilla.org/integration/mozilla-inbound/rev/4dd987eef431

Comment 9

[Martin Thomson [:mt:]]

(In reply to Adam Roach [:abr] from comment #6)
> In this case, given that the timeframe on this is critical, the nit is
> exceedingly minor, and the tree is in a rare open state, I'll land the patch
> as-is.

I'm really sorry, I didn't understand the comment from context and missed it on my second pass.

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4dd987eef431