Fixing issue with DTLS channel when using "presume writable" flag.

Fixing issue with DTLS channel when using "presumed writable" flag.

The DtlsTransportChannel wasn't properly handling the
P2PTransportChannel becoming writable before receiving a remote
fingerprint; it wasn't starting the DTLS handshake whenever this
happened.

Committed: https://crrev.com/367efdcec1d73c807f9522b4502cbc1e59fb29b1
Cr-Commit-Position: refs/heads/master@{#13469}

[Taylor Brandstetter, 2016-07-12 19:08:47]

deadbeef@webrtc.org changed reviewers:
+ pthatcher@webrtc.org, skvlad@webrtc.org

[Taylor Brandstetter, 2016-07-12 19:08:48]

PTAL Peter and Vlad.

[skvlad, 2016-07-12 21:53:52]

lgtm

https://codereview.webrtc.org/2140283002/diff/1/webrtc/p2p/base/dtlstransport...
File webrtc/p2p/base/dtlstransportchannel.cc (right):

https://codereview.webrtc.org/2140283002/diff/1/webrtc/p2p/base/dtlstransport...
webrtc/p2p/base/dtlstransportchannel.cc:253: MaybeStartDtls();
The other two calls wrap this in VERIFY() - do you think this should also be
done here?

[pthatcher1, 2016-07-13 00:04:20]

lgtm

All just style stuff, but this is pretty urgent to fix, so I won't block
submission on the style.

https://codereview.webrtc.org/2140283002/diff/1/webrtc/p2p/base/dtlstransport...
File webrtc/p2p/base/dtlstransportchannel.cc (right):

https://codereview.webrtc.org/2140283002/diff/1/webrtc/p2p/base/dtlstransport...
webrtc/p2p/base/dtlstransportchannel.cc:253: MaybeStartDtls();
On 2016/07/12 21:53:52, skvlad wrote:
> The other two calls wrap this in VERIFY() - do you think this should also be
> done here?

If the other call is inlined, then there would just be one call to
MaybeStartDtls here, outside of the if statement.

https://codereview.webrtc.org/2140283002/diff/1/webrtc/p2p/base/dtlstransport...
webrtc/p2p/base/dtlstransportchannel.cc:680: set_dtls_state(DTLS_TRANSPORT_NEW);
Maybe have a DCHECK(dtls_active_);

https://codereview.webrtc.org/2140283002/diff/1/webrtc/p2p/base/dtlstransport...
webrtc/p2p/base/dtlstransportchannel.cc:682: VERIFY(MaybeStartDtls());
Can we inline this?

https://codereview.webrtc.org/2140283002/diff/1/webrtc/p2p/base/dtlstransport...
File webrtc/p2p/base/dtlstransportchannel_unittest.cc (right):

https://codereview.webrtc.org/2140283002/diff/1/webrtc/p2p/base/dtlstransport...
webrtc/p2p/base/dtlstransportchannel_unittest.cc:130: if (cert) {
Maybe have it be early return?

if (!cert) {
  return fingerprint;
}
...

https://codereview.webrtc.org/2140283002/diff/1/webrtc/p2p/base/dtlstransport...
webrtc/p2p/base/dtlstransportchannel_unittest.cc:158: it != channels_.end();
++it) {
c++11 style for loop?

https://codereview.webrtc.org/2140283002/diff/1/webrtc/p2p/base/dtlstransport...
webrtc/p2p/base/dtlstransportchannel_unittest.cc:174: set_local_cert_ = (cert !=
nullptr);
Would has_local_cert_ be a better name?

https://codereview.webrtc.org/2140283002/diff/1/webrtc/p2p/base/dtlstransport...
webrtc/p2p/base/dtlstransportchannel_unittest.cc:197: }
It seems like a MakeTransportDescription(action, cert, role) would be useful
here, since this code is duplicated between SetRemoteTranportDescription and
SetLocalTransportDescription.

cricket::TransportDescription remote_desc = MakeTransportDescription(action,
cert, role);
bool expect_failure = ((action == CA_ANSWER) && (flags & NF_EXPECT_FAILURE));
EXPECT_EQ(!expect_failure, transport_->SetRemoteDescription(remote_desc, action,
nullptr));

[Taylor Brandstetter, 2016-07-13 16:17:41]

https://codereview.webrtc.org/2140283002/diff/1/webrtc/p2p/base/dtlstransport...
File webrtc/p2p/base/dtlstransportchannel.cc (right):

https://codereview.webrtc.org/2140283002/diff/1/webrtc/p2p/base/dtlstransport...
webrtc/p2p/base/dtlstransportchannel.cc:253: MaybeStartDtls();
On 2016/07/12 21:53:52, skvlad wrote:
> The other two calls wrap this in VERIFY() - do you think this should also be
> done here?

If we always want to DCHECK that this succeeds, I'll just move the DCHECK into
the function itself and have it return void.

https://codereview.webrtc.org/2140283002/diff/1/webrtc/p2p/base/dtlstransport...
webrtc/p2p/base/dtlstransportchannel.cc:680: set_dtls_state(DTLS_TRANSPORT_NEW);
On 2016/07/13 00:04:19, pthatcher1 wrote:
> Maybe have a DCHECK(dtls_active_);

No longer necessary. This is inlined in SetRemoteFingerprint which explicitly
checks dtls_active_.

https://codereview.webrtc.org/2140283002/diff/1/webrtc/p2p/base/dtlstransport...
webrtc/p2p/base/dtlstransportchannel.cc:682: VERIFY(MaybeStartDtls());
On 2016/07/13 00:04:19, pthatcher1 wrote:
> Can we inline this?

Done.

https://codereview.webrtc.org/2140283002/diff/1/webrtc/p2p/base/dtlstransport...
File webrtc/p2p/base/dtlstransportchannel_unittest.cc (right):

https://codereview.webrtc.org/2140283002/diff/1/webrtc/p2p/base/dtlstransport...
webrtc/p2p/base/dtlstransportchannel_unittest.cc:130: if (cert) {
On 2016/07/13 00:04:19, pthatcher1 wrote:
> Maybe have it be early return?
> 
> if (!cert) {
>   return fingerprint;
> }
> ...

Done.

https://codereview.webrtc.org/2140283002/diff/1/webrtc/p2p/base/dtlstransport...
webrtc/p2p/base/dtlstransportchannel_unittest.cc:158: it != channels_.end();
++it) {
On 2016/07/13 00:04:19, pthatcher1 wrote:
> c++11 style for loop?

Done.

https://codereview.webrtc.org/2140283002/diff/1/webrtc/p2p/base/dtlstransport...
webrtc/p2p/base/dtlstransportchannel_unittest.cc:174: set_local_cert_ = (cert !=
nullptr);
On 2016/07/13 00:04:20, pthatcher1 wrote:
> Would has_local_cert_ be a better name?

The method is called SetLocalTransportDescription so that seemed redundant.

https://codereview.webrtc.org/2140283002/diff/1/webrtc/p2p/base/dtlstransport...
webrtc/p2p/base/dtlstransportchannel_unittest.cc:197: }
On 2016/07/13 00:04:19, pthatcher1 wrote:
> It seems like a MakeTransportDescription(action, cert, role) would be useful
> here, since this code is duplicated between SetRemoteTranportDescription and
> SetLocalTransportDescription.
> 
> cricket::TransportDescription remote_desc = MakeTransportDescription(action,
> cert, role);
> bool expect_failure = ((action == CA_ANSWER) && (flags & NF_EXPECT_FAILURE));
> EXPECT_EQ(!expect_failure, transport_->SetRemoteDescription(remote_desc,
action,
> nullptr));
> 

Setting the ciphers you mean? I'll make a separate method for that. But it
doesn't make sense to be in "MakeTransportDescription". Making the transport
description is only two statements, it doesn't need its own method.

Also, "EXPECT_EQ(!expect_failure" is a double negative; I'd prefer to keep it
how it is.

[Taylor Brandstetter, 2016-07-13 16:20:16]

https://codereview.webrtc.org/2140283002/diff/1/webrtc/p2p/base/dtlstransport...
File webrtc/p2p/base/dtlstransportchannel_unittest.cc (right):

https://codereview.webrtc.org/2140283002/diff/1/webrtc/p2p/base/dtlstransport...
webrtc/p2p/base/dtlstransportchannel_unittest.cc:174: set_local_cert_ = (cert !=
nullptr);
On 2016/07/13 16:17:41, Taylor Brandstetter wrote:
> On 2016/07/13 00:04:20, pthatcher1 wrote:
> > Would has_local_cert_ be a better name?
> 
> The method is called SetLocalTransportDescription so that seemed redundant.

Oh I misread your comment. I thought you were asking for cert to be renamed to
local_cert.

Still, I think "set_local_cert" is more clear than "has_local_cert". The test
client class can have a cert without setting it.

[pthatcher1, 2016-07-13 17:54:00]

lgtm

https://codereview.webrtc.org/2140283002/diff/40001/webrtc/p2p/base/dtlstrans...
File webrtc/p2p/base/dtlstransportchannel.cc (right):

https://codereview.webrtc.org/2140283002/diff/40001/webrtc/p2p/base/dtlstrans...
webrtc/p2p/base/dtlstransportchannel.cc:256: MaybeStartDtls();
No need to do it in this CL, but would it make sense to always MaybeStartDtls
inside of SetupDtls?

[Taylor Brandstetter, 2016-07-13 18:33:18]

https://codereview.webrtc.org/2140283002/diff/40001/webrtc/p2p/base/dtlstrans...
File webrtc/p2p/base/dtlstransportchannel.cc (right):

https://codereview.webrtc.org/2140283002/diff/40001/webrtc/p2p/base/dtlstrans...
webrtc/p2p/base/dtlstransportchannel.cc:256: MaybeStartDtls();
On 2016/07/13 17:54:00, pthatcher1 wrote:
> No need to do it in this CL, but would it make sense to always MaybeStartDtls
> inside of SetupDtls?

Sure, that's what I was already doing in a follow-up CL.

[Taylor Brandstetter, 2016-07-13 18:35:22]

The CQ bit was checked by deadbeef@webrtc.org

[Taylor Brandstetter, 2016-07-13 18:35:23]

The patchset sent to the CQ was uploaded after l-g-t-m from skvlad@webrtc.org,
pthatcher@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2140283002/#ps60001 (title:
"Minor code shuffling.")

[commit-bot: I haz the power, 2016-07-13 18:35:32]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-07-13 19:10:22]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-07-13 19:10:24]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-13 19:10:31]

Description was changed from

==========
Fixing issue with DTLS channel when using "presumed writable" flag.

The DtlsTransportChannel wasn't properly handling the
P2PTransportChannel becoming writable before receiving a remote
fingerprint; it wasn't starting the DTLS handshake whenever this
happened.
==========

to

==========
Fixing issue with DTLS channel when using "presumed writable" flag.

The DtlsTransportChannel wasn't properly handling the
P2PTransportChannel becoming writable before receiving a remote
fingerprint; it wasn't starting the DTLS handshake whenever this
happened.

Committed: https://crrev.com/367efdcec1d73c807f9522b4502cbc1e59fb29b1
Cr-Commit-Position: refs/heads/master@{#13469}
==========

[commit-bot: I haz the power, 2016-07-13 19:10:32]

Patchset 4 (id:??) landed as
https://crrev.com/367efdcec1d73c807f9522b4502cbc1e59fb29b1
Cr-Commit-Position: refs/heads/master@{#13469}