Allow remote fingerprint update during a call

webrtc/p2p/base/dtlstransportchannel.cc

Index: webrtc/p2p/base/dtlstransportchannel.cc
diff --git a/webrtc/p2p/base/dtlstransportchannel.cc b/webrtc/p2p/base/dtlstransportchannel.cc
index fff24f30b81617e39475f5e804902f6782d979c2..3a6a38d1ed196d4a0922ded647db08557a6bf3e0 100644
--- a/webrtc/p2p/base/dtlstransportchannel.cc
+++ b/webrtc/p2p/base/dtlstransportchannel.cc
@@ -199,6 +199,8 @@ bool DtlsTransportChannelWrapper::SetRemoteFingerprint(
     size_t digest_len) {
   rtc::Buffer remote_fingerprint_value(digest, digest_len);
 
+  // Once we have the local certificate, the same remote fingerprint can be set
+  // multiple times.
   if (dtls_active_ && remote_fingerprint_value_ == remote_fingerprint_value &&
       !digest_alg.empty()) {
     // This may happen during renegotiation.
@@ -206,28 +208,36 @@ bool DtlsTransportChannelWrapper::SetRemoteFingerprint(
     return true;
   }
 
-  // Allow SetRemoteFingerprint with a NULL digest even if SetLocalCertificate
-  // hasn't been called.
-  if (dtls_ || (!dtls_active_ && !digest_alg.empty())) {
-    LOG_J(LS_ERROR, this) << "Can't set DTLS remote settings in this state.";
-    return false;
-  }
-
+  // If the other side doesn't support DTLS, turn off |dtls_active_|.
   if (digest_alg.empty()) {
+    RTC_DCHECK(!digest_len);
     LOG_J(LS_INFO, this) << "Other side didn't support DTLS.";
     dtls_active_ = false;
     return true;
   }
 
+  // Otherwise, we must have a local certificate before setting remote
+  // fingerprint.
+  if (!dtls_active_) {
+    LOG_J(LS_ERROR, this) << "Can't set DTLS remote settings in this state.";
+    return false;
+  }
+
   // At this point we know we are doing DTLS
   remote_fingerprint_value_ = remote_fingerprint_value.Pass();
   remote_fingerprint_algorithm_ = digest_alg;
 
+  bool reconnect = dtls_;
+
   if (!SetupDtls()) {
     set_dtls_state(DTLS_TRANSPORT_FAILED);
     return false;
   }
 
+  if (reconnect) {
+    Reconnect();
+  }
+
   return true;
 }
 
@@ -530,8 +540,13 @@ void DtlsTransportChannelWrapper::OnDtlsEvent(rtc::StreamInterface* dtls,
   if (sig & rtc::SE_READ) {
     char buf[kMaxDtlsPacketLen];
     size_t read;
-    if (dtls_->Read(buf, sizeof(buf), &read, NULL) == rtc::SR_SUCCESS) {
+    rtc::StreamResult result = dtls_->Read(buf, sizeof(buf), &read, NULL);
+    if (result == rtc::SR_SUCCESS) {
       SignalReadPacket(this, buf, read, rtc::CreatePacketTime(0), 0);
+    } else if (result == rtc::SR_EOS) {
+      // If the SSL stream has closed remotely, reset the |sig| to be SE_CLOSE

[juberti, 2015/12/02 00:29:43]

Won't we get a SE_CLOSE anyway after the SE_READ?

[guoweis_webrtc, 2015/12/02 18:44:15]

This crashes randomly after many repeated runs so it seems a timing issue. My
plan was to land this first and then write a test case to prevent the transfer
test cases become flaky. I don't have full investigation why this crashes at
this point.

Let me know if you want me to back this one out though.

[guoweis_webrtc, 2015/12/03 22:59:41]

I'm backing this out. This still crashes but with the change of when we reset
DTLS, it seems less likely now. I'll make the change later with a test case.

+      // so it could be handled below.
+      sig = rtc::SE_CLOSE;
     }
   }
   if (sig & rtc::SE_CLOSE) {
@@ -616,4 +631,14 @@ void DtlsTransportChannelWrapper::OnConnectionRemoved(
   SignalConnectionRemoved(this);
 }
 
+void DtlsTransportChannelWrapper::Reconnect() {
+  set_dtls_state(DTLS_TRANSPORT_NEW);
+  set_writable(false);
+  if (channel_->writable()) {
+    OnWritableState(channel_);
+  } else {
+    channel_->Connect();

[juberti, 2015/12/02 00:29:44]

Why do we need to call Connect() here? We didn't do this before.

[guoweis_webrtc, 2015/12/02 18:44:15]

Indeed, the tranfered endpoint has always maintained writable for its
|channel_|. However, I'm wondering if we should also handle the case when it
becomes unwritable? 

[guoweis_webrtc, 2015/12/03 22:59:41]

Right, we don't need this part. Removing it.

+  }
+}
+
 }  // namespace cricket