DTLS-SRTP set up is bypassed when the channel has been writable.

This regression was introduced by CL 1505573002 to support remote fingerprint update. What happened is that during PrAnswer, we incorrectly do not apply bundle. However, the channel has become writable at that time. When Answer comes, we still reset the srtp_filter but since the channel has been writable, the new SRTP context has never been applied.

We're making sure that we could always apply SRTP context even when channel has been writable. We'll address the issue that bundle should apply even in PrAnswer in a different CL.

BUG=568734

Review URL: https://codereview.webrtc.org/1532543003

Cr-Commit-Position: refs/heads/master@{#11075}

talk/app/webrtc/peerconnection_unittest.cc

@@ -1267,6 +1267,26 @@ TEST_F(P2PTestConductor, LocalP2PTestDtlsTransferCallee) {
   VerifyRenderedSize(640, 480);
 }
 
+// This test sets up a non-bundle call and apply bundle during ICE restart. When
+// bundle is in effect in the restart, the channel can successfully reset its
+// DTLS-SRTP context.
+TEST_F(P2PTestConductor, LocalP2PTestDtlsBundleInIceRestart) {
+  MAYBE_SKIP_TEST(rtc::SSLStreamAdapter::HaveDtlsSrtp);
+  FakeConstraints setup_constraints;
+  setup_constraints.AddMandatory(MediaConstraintsInterface::kEnableDtlsSrtp,
+                                 true);
+  ASSERT_TRUE(CreateTestClients(&setup_constraints, &setup_constraints));
+  receiving_client()->RemoveBundleFromReceivedSdp(true);
+  LocalP2PTest();
+  VerifyRenderedSize(640, 480);
+
+  initializing_client()->IceRestart();
+  receiving_client()->SetExpectIceRestart(true);
+  receiving_client()->RemoveBundleFromReceivedSdp(false);
+  LocalP2PTest();
+  VerifyRenderedSize(640, 480);
+}
+
 // This test sets up a call transfer to a new callee with a different DTLS
 // fingerprint.
 TEST_F(P2PTestConductor, LocalP2PTestDtlsTransferCaller) {

talk/session/media/channel.cc

@@ -267,24 +267,39 @@ bool BaseChannel::SetTransport_w(const std::string& transport_name) {
   // changes and wait until the DTLS handshake is complete to set the newly
   // negotiated parameters.
   if (ShouldSetupDtlsSrtp()) {
+    // Set |writable_| to false such that UpdateWritableState_w can set up
+    // DTLS-SRTP when the writable_ becomes true again.
+    writable_ = false;
     srtp_filter_.ResetParams();
   }
 
-  set_transport_channel(transport_controller_->CreateTransportChannel_w(
-      transport_name, cricket::ICE_CANDIDATE_COMPONENT_RTP));
-  if (!transport_channel()) {
-    return false;
-  }
+  // TODO(guoweis): Remove this grossness when we remove non-muxed RTCP.
   if (rtcp_transport_enabled()) {
     LOG(LS_INFO) << "Create RTCP TransportChannel for " << content_name()
                  << " on " << transport_name << " transport ";
-    set_rtcp_transport_channel(transport_controller_->CreateTransportChannel_w(
-        transport_name, cricket::ICE_CANDIDATE_COMPONENT_RTCP));
+    set_rtcp_transport_channel(
+        transport_controller_->CreateTransportChannel_w(
+            transport_name, cricket::ICE_CANDIDATE_COMPONENT_RTCP),
+        false /* update_writablity */);
     if (!rtcp_transport_channel()) {
       return false;
     }
   }
 
+  // We're not updating the writablity during the transition state.
+  set_transport_channel(transport_controller_->CreateTransportChannel_w(
+      transport_name, cricket::ICE_CANDIDATE_COMPONENT_RTP));
+  if (!transport_channel()) {
+    return false;
+  }
+
+  // TODO(guoweis): Remove this grossness when we remove non-muxed RTCP.
+  if (rtcp_transport_enabled()) {
+    // We can only update the RTCP ready to send after set_transport_channel has
+    // handled channel writability.
+    SetReadyToSend(
+        true, rtcp_transport_channel() && rtcp_transport_channel()->writable());
+  }
   transport_name_ = transport_name;
   return true;
 }
@@ -320,7 +335,8 @@ void BaseChannel::set_transport_channel(TransportChannel* new_tc) {
   SetReadyToSend(false, new_tc && new_tc->writable());
 }
 
-void BaseChannel::set_rtcp_transport_channel(TransportChannel* new_tc) {
+void BaseChannel::set_rtcp_transport_channel(TransportChannel* new_tc,
+                                             bool update_writablity) {
   ASSERT(worker_thread_ == rtc::Thread::Current());
 
   TransportChannel* old_tc = rtcp_transport_channel_;
@@ -348,10 +364,12 @@ void BaseChannel::set_rtcp_transport_channel(TransportChannel* new_tc) {
     }
   }
 
-  // Update aggregate writable/ready-to-send state between RTP and RTCP upon
-  // setting new channel
-  UpdateWritableState_w();
-  SetReadyToSend(true, new_tc && new_tc->writable());
+  if (update_writablity) {
+    // Update aggregate writable/ready-to-send state between RTP and RTCP upon
+    // setting new channel
+    UpdateWritableState_w();
+    SetReadyToSend(true, new_tc && new_tc->writable());
+  }
 }
 
 void BaseChannel::ConnectToTransportChannel(TransportChannel* tc) {
@@ -1072,7 +1090,7 @@ void BaseChannel::ActivateRtcpMux() {
 void BaseChannel::ActivateRtcpMux_w() {
   if (!rtcp_mux_filter_.IsActive()) {
     rtcp_mux_filter_.SetActive();
-    set_rtcp_transport_channel(nullptr);
+    set_rtcp_transport_channel(nullptr, true);
     rtcp_transport_enabled_ = false;
   }
 }
@@ -1095,7 +1113,7 @@ bool BaseChannel::SetRtcpMux_w(bool enable, ContentAction action,
         LOG(LS_INFO) << "Enabling rtcp-mux for " << content_name()
                      << " by destroying RTCP transport channel for "
                      << transport_name();
-        set_rtcp_transport_channel(nullptr);
+        set_rtcp_transport_channel(nullptr, true);
         rtcp_transport_enabled_ = false;
       }
       break;

talk/session/media/channel.h

@@ -179,8 +179,11 @@ class BaseChannel
   // Sets the |transport_channel_| (and |rtcp_transport_channel_|, if |rtcp_| is
   // true). Gets the transport channels from |transport_controller_|.
   bool SetTransport_w(const std::string& transport_name);
+
   void set_transport_channel(TransportChannel* transport);
-  void set_rtcp_transport_channel(TransportChannel* transport);
+  void set_rtcp_transport_channel(TransportChannel* transport,
+                                  bool update_writablity);
+
   bool was_ever_writable() const { return was_ever_writable_; }
   void set_local_content_direction(MediaContentDirection direction) {
     local_content_direction_ = direction;

talk/session/media/srtpfilter.cc

@@ -450,6 +450,10 @@ bool SrtpFilter::ApplyParams(const CryptoParams& send_params,
 bool SrtpFilter::ResetParams() {
   offer_params_.clear();
   state_ = ST_INIT;
+  (void)send_session_.release();
+  (void)recv_session_.release();
+  (void)send_rtcp_session_.release();
+  (void)recv_rtcp_session_.release();
   LOG(LS_INFO) << "SRTP reset to init state";
   return true;
 }