Allow remote fingerprint update during a call

talk/session/media/channel.cc

Index: talk/session/media/channel.cc
diff --git a/talk/session/media/channel.cc b/talk/session/media/channel.cc
index ef26704f1a4f0058d895e7fb3d539030c7a300e3..1cad9dc871bbc5726497061b40f26a5a01ce844e 100644
--- a/talk/session/media/channel.cc
+++ b/talk/session/media/channel.cc
@@ -177,7 +177,7 @@ BaseChannel::BaseChannel(rtc::Thread* thread,
       writable_(false),
       rtp_ready_to_send_(false),
       rtcp_ready_to_send_(false),
-      was_ever_writable_(false),
+      dtls_srtp_setup_pending_(false),
       local_content_direction_(MD_INACTIVE),
       remote_content_direction_(MD_INACTIVE),
       has_received_packet_(false),
@@ -278,6 +278,10 @@ void BaseChannel::set_transport_channel(TransportChannel* new_tc) {
   }
   ASSERT(old_tc != new_tc);
 
+  // When a TransportChannel is attached, we always assume there is something to
+  // do for DTLS/SRTP to prevent sending anything without encrypted.
+  dtls_srtp_setup_pending_ = true;

[pthatcher1, 2015/11/30 20:23:11]

This is kind of hard to read and understand.  In the case where we are writable
already, UpdateWritableState_w() below will cause SRTP to be setup.   In the
case where it's not, it won't be.  But that's not very clear.   It doesn't help
that this line is far away from that line.  

This is already on the worker thread, so no packets will be sent while we're in
this code block.  I think it would be safer to:

1.  Move this line down to write above UpdateWritableState_w() below.
2.  Rename it to "srtp_ready_ = false".  
3.  srtp_filter_.ResetParams(); here as well. 
4.  Put a comment indicating that UpdateWritableState_w will get SRTP ready if
it's writable, or that it will get it ready once it is writable.

[guoweis_webrtc, 2015/12/01 22:05:12]

Done.

+
   if (old_tc) {
     DisconnectFromTransportChannel(old_tc);
     transport_controller_->DestroyTransportChannel_w(
@@ -318,6 +322,10 @@ void BaseChannel::set_rtcp_transport_channel(TransportChannel* new_tc) {
   rtcp_transport_channel_ = new_tc;
 
   if (new_tc) {
+    // When a TransportChannel is attached, we always assume there is something
+    // to do for DTLS/SRTP to prevent sending anything without encrypted.
+    dtls_srtp_setup_pending_ = true;

[pthatcher1, 2015/11/30 20:23:11]

Same here as with set_transport_channel()

[guoweis_webrtc, 2015/12/01 22:05:12]

Done.

+
     ConnectToTransportChannel(new_tc);
     for (const auto& pair : rtcp_socket_options_) {
       new_tc->SetOption(pair.first, pair.second);
@@ -336,6 +344,7 @@ void BaseChannel::ConnectToTransportChannel(TransportChannel* tc) {
   tc->SignalWritableState.connect(this, &BaseChannel::OnWritableState);
   tc->SignalReadPacket.connect(this, &BaseChannel::OnChannelRead);
   tc->SignalReadyToSend.connect(this, &BaseChannel::OnReadyToSend);
+  tc->SignalDtlsState.connect(this, &BaseChannel::OnDtlsState);
 }
 
 void BaseChannel::DisconnectFromTransportChannel(TransportChannel* tc) {
@@ -344,6 +353,7 @@ void BaseChannel::DisconnectFromTransportChannel(TransportChannel* tc) {
   tc->SignalWritableState.disconnect(this);
   tc->SignalReadPacket.disconnect(this);
   tc->SignalReadyToSend.disconnect(this);
+  tc->SignalDtlsState.disconnect(this);
 }
 
 bool BaseChannel::Enable(bool enable) {
@@ -416,10 +426,9 @@ bool BaseChannel::IsReadyToReceive() const {
 bool BaseChannel::IsReadyToSend() const {
   // Send outgoing data if we are enabled, have local and remote content,
   // and we have had some form of connectivity.
-  return enabled() &&
-         IsReceiveContentDirection(remote_content_direction_) &&
+  return enabled() && IsReceiveContentDirection(remote_content_direction_) &&
          IsSendContentDirection(local_content_direction_) &&
-         was_ever_writable();
+         !dtls_srtp_setup_pending();

[pthatcher1, 2015/11/30 20:23:11]

I think we need this to be something like:

was_ever_writable() && (srtp_ready() || !NeedSrtp())

to work correctly with SCTP data channels.  

[guoweis_webrtc, 2015/12/01 22:05:12]

Done.

 }
 
 bool BaseChannel::SendPacket(rtc::Buffer* packet,
@@ -474,6 +483,24 @@ void BaseChannel::OnReadyToSend(TransportChannel* channel) {
   SetReadyToSend(channel == rtcp_transport_channel_, true);
 }
 
+void BaseChannel::OnDtlsState(TransportChannel* channel,
+                              DtlsTransportState state) {
+  if (!ShouldSetupDtlsSrtp()) {

[pthatcher1, 2015/11/30 20:23:11]

I think it would make it more readable to rename ShouldSetupDtlsSrtp to 
NeedSrtp().

[guoweis_webrtc, 2015/12/01 22:05:12]

NeedSrtp doesn't capture the Dtls concept.

+    dtls_srtp_setup_pending_ = false;

[pthatcher1, 2015/11/30 20:23:11]

I think a combination of (srtp_ready() || !NeedSrtp()) would make this line
unecessary.

+    return;
+  }
+
+  // Resetting the srtp filter as long as it's not the CONNECTED signal. For

[pthatcher1, 2015/11/30 20:23:11]

Resetting => Reset
as long as => if 

[pthatcher1, 2015/11/30 20:23:11]

CONNECTED signal => CONNECTED state

[guoweis_webrtc, 2015/12/01 22:05:12]

Done.

+  // CONNECTED signal, setting up DTLS-SRTP context is deferred to

[pthatcher1, 2015/11/30 20:23:11]

For CONNECTED signal => For the connected state.

[guoweis_webrtc, 2015/12/01 22:05:12]

Done.

+  // ChannelWritable_w to cover other scenarios like the whole channel is
+  // writable (not just this TransportChannel) or when TransportChannel is
+  // attached after DTLS is negotiated.
+  if (state != DTLS_TRANSPORT_CONNECTED) {
+    srtp_filter_.ResetParams();
+    dtls_srtp_setup_pending_ = true;
+  }
+}
+
 void BaseChannel::SetReadyToSend(bool rtcp, bool ready) {
   if (rtcp) {
     rtcp_ready_to_send_ = ready;
@@ -761,11 +788,12 @@ void BaseChannel::UpdateWritableState_w() {
 
 void BaseChannel::ChannelWritable_w() {
   ASSERT(worker_thread_ == rtc::Thread::Current());
-  if (writable_)
+  if (writable_) {
     return;
+  }
 
   LOG(LS_INFO) << "Channel writable (" << content_name_ << ")"
-               << (was_ever_writable_ ? "" : " for the first time");
+               << (dtls_srtp_setup_pending_ ? "" : " for the first time");

[pthatcher1, 2015/11/30 20:23:11]

Now this isn't accurate any more.  Perhaps it would be easier to just use
was_ever_writable_ still.

 
   std::vector<ConnectionInfo> infos;
   transport_channel_->GetStats(&infos);
@@ -778,22 +806,7 @@ void BaseChannel::ChannelWritable_w() {
     }
   }
 
-  // If we're doing DTLS-SRTP, now is the time.
-  if (!was_ever_writable_ && ShouldSetupDtlsSrtp()) {
-    if (!SetupDtlsSrtp(false)) {
-      SignalDtlsSetupFailure_w(false);
-      return;
-    }
-
-    if (rtcp_transport_channel_) {
-      if (!SetupDtlsSrtp(true)) {
-        SignalDtlsSetupFailure_w(true);
-        return;
-      }
-    }
-  }
-
-  was_ever_writable_ = true;
+  TrySetupDtlsSrtp_w();

[pthatcher1, 2015/11/30 20:23:11]

Why is this TrySetupDtlsSrtp_w() instead of SetupDtlsSrtp_w() or
MaybeSetupDtlsSrtp_w()?

[guoweis_webrtc, 2015/12/01 22:05:12]

Done.

   writable_ = true;
   ChangeState();
 }
@@ -915,6 +928,30 @@ bool BaseChannel::SetupDtlsSrtp(bool rtcp_channel) {
   return ret;
 }
 
+void BaseChannel::TrySetupDtlsSrtp_w() {
+  if (!dtls_srtp_setup_pending_) {
+    return;
+  }
+
+  if (!ShouldSetupDtlsSrtp()) {
+    dtls_srtp_setup_pending_ = false;
+    return;
+  }
+
+  if (!SetupDtlsSrtp(false)) {
+    SignalDtlsSetupFailure_w(false);
+    return;
+  }
+
+  if (rtcp_transport_channel_) {
+    if (!SetupDtlsSrtp(true)) {
+      SignalDtlsSetupFailure_w(true);
+      return;
+    }
+  }
+  dtls_srtp_setup_pending_ = false;
+}
+
 void BaseChannel::ChannelNotWritable_w() {
   ASSERT(worker_thread_ == rtc::Thread::Current());
   if (!writable_)