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}
diff --git a/talk/app/webrtc/peerconnection_unittest.cc b/talk/app/webrtc/peerconnection_unittest.cc
index 49f4500..a127869 100644
--- a/talk/app/webrtc/peerconnection_unittest.cc
+++ b/talk/app/webrtc/peerconnection_unittest.cc
@@ -1267,6 +1267,26 @@
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) {
diff --git a/talk/session/media/channel.cc b/talk/session/media/channel.cc
index 818a659..5f4c275 100644
--- a/talk/session/media/channel.cc
+++ b/talk/session/media/channel.cc
@@ -267,24 +267,39 @@
// 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 @@
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 @@
}
}
- // 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_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 @@
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;
diff --git a/talk/session/media/channel.h b/talk/session/media/channel.h
index 8faefe6..3a8550c 100644
--- a/talk/session/media/channel.h
+++ b/talk/session/media/channel.h
@@ -179,8 +179,11 @@
// 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;
diff --git a/talk/session/media/srtpfilter.cc b/talk/session/media/srtpfilter.cc
index 4a54740..7961a7b 100644
--- a/talk/session/media/srtpfilter.cc
+++ b/talk/session/media/srtpfilter.cc
@@ -450,6 +450,10 @@
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;
}
