diff --git a/webrtc/api/peerconnection.cc b/webrtc/api/peerconnection.cc index e1f19adf3e..49f90307d0 100644 --- a/webrtc/api/peerconnection.cc +++ b/webrtc/api/peerconnection.cc @@ -1298,9 +1298,16 @@ bool PeerConnection::SetConfiguration(const RTCConfiguration& configuration) { } } - // TODO(deadbeef): Shouldn't have to hop to the worker thread twice... + // TODO(deadbeef): Shouldn't have to hop to the network thread twice... session_->SetIceConfig(session_->ParseIceConfig(configuration)); + // As described in JSEP, calling setConfiguration with new ICE servers or + // candidate policy must set a "needs-ice-restart" bit so that the next offer + // triggers an ICE restart which will pick up the changes. + if (configuration.servers != configuration_.servers || + configuration.type != configuration_.type) { + session_->SetNeedsIceRestartFlag(); + } configuration_ = configuration; return true; } diff --git a/webrtc/api/peerconnectioninterface.h b/webrtc/api/peerconnectioninterface.h index a6557562a4..29badbd8bb 100644 --- a/webrtc/api/peerconnectioninterface.h +++ b/webrtc/api/peerconnectioninterface.h @@ -214,6 +214,11 @@ class PeerConnectionInterface : public rtc::RefCountInterface { std::vector urls; std::string username; std::string password; + bool operator==(const IceServer& o) const { + return uri == o.uri && urls == o.urls && username == o.username && + password == o.password; + } + bool operator!=(const IceServer& o) const { return !(*this == o); } }; typedef std::vector IceServers; diff --git a/webrtc/api/peerconnectioninterface_unittest.cc b/webrtc/api/peerconnectioninterface_unittest.cc index cfba3e67ec..1fb3a5f14c 100644 --- a/webrtc/api/peerconnectioninterface_unittest.cc +++ b/webrtc/api/peerconnectioninterface_unittest.cc @@ -73,11 +73,11 @@ static const char kSdpStringWithStream1[] = "o=- 0 0 IN IP4 127.0.0.1\r\n" "s=-\r\n" "t=0 0\r\n" + "m=audio 1 RTP/AVPF 103\r\n" "a=ice-ufrag:e5785931\r\n" "a=ice-pwd:36fb7878390db89481c1d46daa4278d8\r\n" "a=fingerprint:sha-256 58:AB:6E:F5:F1:E4:57:B7:E9:46:F4:86:04:28:F9:A7:ED:" "BD:AB:AE:40:EF:CE:9A:51:2C:2A:B1:9B:8B:78:84\r\n" - "m=audio 1 RTP/AVPF 103\r\n" "a=mid:audio\r\n" "a=sendrecv\r\n" "a=rtcp-mux\r\n" @@ -86,6 +86,10 @@ static const char kSdpStringWithStream1[] = "a=ssrc:1 mslabel:stream1\r\n" "a=ssrc:1 label:audiotrack0\r\n" "m=video 1 RTP/AVPF 120\r\n" + "a=ice-ufrag:e5785931\r\n" + "a=ice-pwd:36fb7878390db89481c1d46daa4278d8\r\n" + "a=fingerprint:sha-256 58:AB:6E:F5:F1:E4:57:B7:E9:46:F4:86:04:28:F9:A7:ED:" + "BD:AB:AE:40:EF:CE:9A:51:2C:2A:B1:9B:8B:78:84\r\n" "a=mid:video\r\n" "a=sendrecv\r\n" "a=rtcp-mux\r\n" @@ -101,11 +105,11 @@ static const char kSdpStringWithStream1AudioTrackOnly[] = "o=- 0 0 IN IP4 127.0.0.1\r\n" "s=-\r\n" "t=0 0\r\n" + "m=audio 1 RTP/AVPF 103\r\n" "a=ice-ufrag:e5785931\r\n" "a=ice-pwd:36fb7878390db89481c1d46daa4278d8\r\n" "a=fingerprint:sha-256 58:AB:6E:F5:F1:E4:57:B7:E9:46:F4:86:04:28:F9:A7:ED:" "BD:AB:AE:40:EF:CE:9A:51:2C:2A:B1:9B:8B:78:84\r\n" - "m=audio 1 RTP/AVPF 103\r\n" "a=mid:audio\r\n" "a=sendrecv\r\n" "a=rtpmap:103 ISAC/16000\r\n" @@ -122,12 +126,12 @@ static const char kSdpStringWithStream1And2[] = "o=- 0 0 IN IP4 127.0.0.1\r\n" "s=-\r\n" "t=0 0\r\n" + "a=msid-semantic: WMS stream1 stream2\r\n" + "m=audio 1 RTP/AVPF 103\r\n" "a=ice-ufrag:e5785931\r\n" "a=ice-pwd:36fb7878390db89481c1d46daa4278d8\r\n" "a=fingerprint:sha-256 58:AB:6E:F5:F1:E4:57:B7:E9:46:F4:86:04:28:F9:A7:ED:" "BD:AB:AE:40:EF:CE:9A:51:2C:2A:B1:9B:8B:78:84\r\n" - "a=msid-semantic: WMS stream1 stream2\r\n" - "m=audio 1 RTP/AVPF 103\r\n" "a=mid:audio\r\n" "a=sendrecv\r\n" "a=rtcp-mux\r\n" @@ -137,6 +141,10 @@ static const char kSdpStringWithStream1And2[] = "a=ssrc:3 cname:stream2\r\n" "a=ssrc:3 msid:stream2 audiotrack1\r\n" "m=video 1 RTP/AVPF 120\r\n" + "a=ice-ufrag:e5785931\r\n" + "a=ice-pwd:36fb7878390db89481c1d46daa4278d8\r\n" + "a=fingerprint:sha-256 58:AB:6E:F5:F1:E4:57:B7:E9:46:F4:86:04:28:F9:A7:ED:" + "BD:AB:AE:40:EF:CE:9A:51:2C:2A:B1:9B:8B:78:84\r\n" "a=mid:video\r\n" "a=sendrecv\r\n" "a=rtcp-mux\r\n" @@ -152,16 +160,20 @@ static const char kSdpStringWithoutStreams[] = "o=- 0 0 IN IP4 127.0.0.1\r\n" "s=-\r\n" "t=0 0\r\n" + "m=audio 1 RTP/AVPF 103\r\n" "a=ice-ufrag:e5785931\r\n" "a=ice-pwd:36fb7878390db89481c1d46daa4278d8\r\n" "a=fingerprint:sha-256 58:AB:6E:F5:F1:E4:57:B7:E9:46:F4:86:04:28:F9:A7:ED:" "BD:AB:AE:40:EF:CE:9A:51:2C:2A:B1:9B:8B:78:84\r\n" - "m=audio 1 RTP/AVPF 103\r\n" "a=mid:audio\r\n" "a=sendrecv\r\n" "a=rtcp-mux\r\n" "a=rtpmap:103 ISAC/16000\r\n" "m=video 1 RTP/AVPF 120\r\n" + "a=ice-ufrag:e5785931\r\n" + "a=ice-pwd:36fb7878390db89481c1d46daa4278d8\r\n" + "a=fingerprint:sha-256 58:AB:6E:F5:F1:E4:57:B7:E9:46:F4:86:04:28:F9:A7:ED:" + "BD:AB:AE:40:EF:CE:9A:51:2C:2A:B1:9B:8B:78:84\r\n" "a=mid:video\r\n" "a=sendrecv\r\n" "a=rtcp-mux\r\n" @@ -173,17 +185,21 @@ static const char kSdpStringWithMsidWithoutStreams[] = "o=- 0 0 IN IP4 127.0.0.1\r\n" "s=-\r\n" "t=0 0\r\n" + "a=msid-semantic: WMS\r\n" + "m=audio 1 RTP/AVPF 103\r\n" "a=ice-ufrag:e5785931\r\n" "a=ice-pwd:36fb7878390db89481c1d46daa4278d8\r\n" "a=fingerprint:sha-256 58:AB:6E:F5:F1:E4:57:B7:E9:46:F4:86:04:28:F9:A7:ED:" "BD:AB:AE:40:EF:CE:9A:51:2C:2A:B1:9B:8B:78:84\r\n" - "a=msid-semantic: WMS\r\n" - "m=audio 1 RTP/AVPF 103\r\n" "a=mid:audio\r\n" "a=sendrecv\r\n" "a=rtcp-mux\r\n" "a=rtpmap:103 ISAC/16000\r\n" "m=video 1 RTP/AVPF 120\r\n" + "a=ice-ufrag:e5785931\r\n" + "a=ice-pwd:36fb7878390db89481c1d46daa4278d8\r\n" + "a=fingerprint:sha-256 58:AB:6E:F5:F1:E4:57:B7:E9:46:F4:86:04:28:F9:A7:ED:" + "BD:AB:AE:40:EF:CE:9A:51:2C:2A:B1:9B:8B:78:84\r\n" "a=mid:video\r\n" "a=sendrecv\r\n" "a=rtcp-mux\r\n" @@ -195,11 +211,11 @@ static const char kSdpStringWithoutStreamsAudioOnly[] = "o=- 0 0 IN IP4 127.0.0.1\r\n" "s=-\r\n" "t=0 0\r\n" + "m=audio 1 RTP/AVPF 103\r\n" "a=ice-ufrag:e5785931\r\n" "a=ice-pwd:36fb7878390db89481c1d46daa4278d8\r\n" "a=fingerprint:sha-256 58:AB:6E:F5:F1:E4:57:B7:E9:46:F4:86:04:28:F9:A7:ED:" "BD:AB:AE:40:EF:CE:9A:51:2C:2A:B1:9B:8B:78:84\r\n" - "m=audio 1 RTP/AVPF 103\r\n" "a=mid:audio\r\n" "a=sendrecv\r\n" "a=rtcp-mux\r\n" @@ -211,17 +227,21 @@ static const char kSdpStringSendOnlyWithoutStreams[] = "o=- 0 0 IN IP4 127.0.0.1\r\n" "s=-\r\n" "t=0 0\r\n" + "m=audio 1 RTP/AVPF 103\r\n" "a=ice-ufrag:e5785931\r\n" "a=ice-pwd:36fb7878390db89481c1d46daa4278d8\r\n" "a=fingerprint:sha-256 58:AB:6E:F5:F1:E4:57:B7:E9:46:F4:86:04:28:F9:A7:ED:" "BD:AB:AE:40:EF:CE:9A:51:2C:2A:B1:9B:8B:78:84\r\n" - "m=audio 1 RTP/AVPF 103\r\n" "a=mid:audio\r\n" "a=sendrecv\r\n" "a=sendonly\r\n" "a=rtcp-mux\r\n" "a=rtpmap:103 ISAC/16000\r\n" "m=video 1 RTP/AVPF 120\r\n" + "a=ice-ufrag:e5785931\r\n" + "a=ice-pwd:36fb7878390db89481c1d46daa4278d8\r\n" + "a=fingerprint:sha-256 58:AB:6E:F5:F1:E4:57:B7:E9:46:F4:86:04:28:F9:A7:ED:" + "BD:AB:AE:40:EF:CE:9A:51:2C:2A:B1:9B:8B:78:84\r\n" "a=mid:video\r\n" "a=sendrecv\r\n" "a=sendonly\r\n" @@ -233,14 +253,14 @@ static const char kSdpStringInit[] = "o=- 0 0 IN IP4 127.0.0.1\r\n" "s=-\r\n" "t=0 0\r\n" - "a=ice-ufrag:e5785931\r\n" - "a=ice-pwd:36fb7878390db89481c1d46daa4278d8\r\n" - "a=fingerprint:sha-256 58:AB:6E:F5:F1:E4:57:B7:E9:46:F4:86:04:28:F9:A7:ED:" - "BD:AB:AE:40:EF:CE:9A:51:2C:2A:B1:9B:8B:78:84\r\n" "a=msid-semantic: WMS\r\n"; static const char kSdpStringAudio[] = "m=audio 1 RTP/AVPF 103\r\n" + "a=ice-ufrag:e5785931\r\n" + "a=ice-pwd:36fb7878390db89481c1d46daa4278d8\r\n" + "a=fingerprint:sha-256 58:AB:6E:F5:F1:E4:57:B7:E9:46:F4:86:04:28:F9:A7:ED:" + "BD:AB:AE:40:EF:CE:9A:51:2C:2A:B1:9B:8B:78:84\r\n" "a=mid:audio\r\n" "a=sendrecv\r\n" "a=rtcp-mux\r\n" @@ -248,6 +268,10 @@ static const char kSdpStringAudio[] = static const char kSdpStringVideo[] = "m=video 1 RTP/AVPF 120\r\n" + "a=ice-ufrag:e5785931\r\n" + "a=ice-pwd:36fb7878390db89481c1d46daa4278d8\r\n" + "a=fingerprint:sha-256 58:AB:6E:F5:F1:E4:57:B7:E9:46:F4:86:04:28:F9:A7:ED:" + "BD:AB:AE:40:EF:CE:9A:51:2C:2A:B1:9B:8B:78:84\r\n" "a=mid:video\r\n" "a=sendrecv\r\n" "a=rtcp-mux\r\n" @@ -326,6 +350,18 @@ bool GetFirstSsrc(const cricket::ContentInfo* content_info, int* ssrc) { return true; } +// Get the ufrags out of an SDP blob. Useful for testing ICE restart +// behavior. +std::vector GetUfrags( + const webrtc::SessionDescriptionInterface* desc) { + std::vector ufrags; + for (const cricket::TransportInfo& info : + desc->description()->transport_infos()) { + ufrags.push_back(info.description.ice_ufrag); + } + return ufrags; +} + void SetSsrcToZero(std::string* sdp) { const char kSdpSsrcAtribute[] = "a=ssrc:"; const char kSdpSsrcAtributeZero[] = "a=ssrc:0"; @@ -2727,6 +2763,116 @@ TEST_F(PeerConnectionInterfaceTest, OnAddTrackCallback) { EXPECT_EQ(observer_.last_added_track_label_, kVideoTracks[0]); } +// Test that when SetConfiguration is called and the configuration is +// changing, the next offer causes an ICE restart. +TEST_F(PeerConnectionInterfaceTest, SetConfigurationCausingIceRetart) { + PeerConnectionInterface::RTCConfiguration config; + config.type = PeerConnectionInterface::kRelay; + // Need to pass default constraints to prevent disabling of DTLS... + FakeConstraints default_constraints; + CreatePeerConnection(config, &default_constraints); + AddAudioVideoStream(kStreamLabel1, "audio_label", "video_label"); + + // Do initial offer/answer so there's something to restart. + CreateOfferAsLocalDescription(); + CreateAnswerAsRemoteDescription(kSdpStringWithStream1); + + // Grab the ufrags. + std::vector initial_ufrags = GetUfrags(pc_->local_description()); + + // Change ICE policy, which should trigger an ICE restart on the next offer. + config.type = PeerConnectionInterface::kAll; + EXPECT_TRUE(pc_->SetConfiguration(config)); + CreateOfferAsLocalDescription(); + + // Grab the new ufrags. + std::vector subsequent_ufrags = + GetUfrags(pc_->local_description()); + + // Sanity check. + EXPECT_EQ(initial_ufrags.size(), subsequent_ufrags.size()); + // Check that each ufrag is different. + for (int i = 0; i < static_cast(initial_ufrags.size()); ++i) { + EXPECT_NE(initial_ufrags[i], subsequent_ufrags[i]); + } +} + +// Test that when SetConfiguration is called and the configuration *isn't* +// changing, the next offer does *not* cause an ICE restart. +TEST_F(PeerConnectionInterfaceTest, SetConfigurationNotCausingIceRetart) { + PeerConnectionInterface::RTCConfiguration config; + config.type = PeerConnectionInterface::kRelay; + // Need to pass default constraints to prevent disabling of DTLS... + FakeConstraints default_constraints; + CreatePeerConnection(config, &default_constraints); + AddAudioVideoStream(kStreamLabel1, "audio_label", "video_label"); + + // Do initial offer/answer so there's something to restart. + CreateOfferAsLocalDescription(); + CreateAnswerAsRemoteDescription(kSdpStringWithStream1); + + // Grab the ufrags. + std::vector initial_ufrags = GetUfrags(pc_->local_description()); + + // Call SetConfiguration with a config identical to what the PC was + // constructed with. + EXPECT_TRUE(pc_->SetConfiguration(config)); + CreateOfferAsLocalDescription(); + + // Grab the new ufrags. + std::vector subsequent_ufrags = + GetUfrags(pc_->local_description()); + + EXPECT_EQ(initial_ufrags, subsequent_ufrags); +} + +// Test for a weird corner case scenario: +// 1. Audio/video session established. +// 2. SetConfiguration changes ICE config; ICE restart needed. +// 3. ICE restart initiated by remote peer, but only for one m= section. +// 4. Next createOffer should initiate an ICE restart, but only for the other +// m= section; it would be pointless to do an ICE restart for the m= section +// that was already restarted. +TEST_F(PeerConnectionInterfaceTest, SetConfigurationCausingPartialIceRestart) { + PeerConnectionInterface::RTCConfiguration config; + config.type = PeerConnectionInterface::kRelay; + // Need to pass default constraints to prevent disabling of DTLS... + FakeConstraints default_constraints; + CreatePeerConnection(config, &default_constraints); + AddAudioVideoStream(kStreamLabel1, "audio_label", "video_label"); + + // Do initial offer/answer so there's something to restart. + CreateOfferAsLocalDescription(); + CreateAnswerAsRemoteDescription(kSdpStringWithStream1); + + // Change ICE policy, which should set the "needs-ice-restart" flag. + config.type = PeerConnectionInterface::kAll; + EXPECT_TRUE(pc_->SetConfiguration(config)); + + // Do ICE restart for the first m= section, initiated by remote peer. + webrtc::JsepSessionDescription* remote_offer = + new webrtc::JsepSessionDescription(SessionDescriptionInterface::kOffer); + EXPECT_TRUE(remote_offer->Initialize(kSdpStringWithStream1, nullptr)); + remote_offer->description()->transport_infos()[0].description.ice_ufrag = + "modified"; + EXPECT_TRUE(DoSetRemoteDescription(remote_offer)); + CreateAnswerAsLocalDescription(); + + // Grab the ufrags. + std::vector initial_ufrags = GetUfrags(pc_->local_description()); + ASSERT_EQ(2, initial_ufrags.size()); + + // Create offer and grab the new ufrags. + CreateOfferAsLocalDescription(); + std::vector subsequent_ufrags = + GetUfrags(pc_->local_description()); + ASSERT_EQ(2, subsequent_ufrags.size()); + + // Ensure that only the ufrag for the second m= section changed. + EXPECT_EQ(initial_ufrags[0], subsequent_ufrags[0]); + EXPECT_NE(initial_ufrags[1], subsequent_ufrags[1]); +} + class PeerConnectionMediaConfigTest : public testing::Test { protected: void SetUp() override { diff --git a/webrtc/api/webrtcsession.cc b/webrtc/api/webrtcsession.cc index 636184b852..c5582d3630 100644 --- a/webrtc/api/webrtcsession.cc +++ b/webrtc/api/webrtcsession.cc @@ -1330,6 +1330,14 @@ bool WebRtcSession::IceRestartPending(const std::string& content_name) const { pending_ice_restarts_.end(); } +void WebRtcSession::SetNeedsIceRestartFlag() { + transport_controller_->SetNeedsIceRestartFlag(); +} + +bool WebRtcSession::NeedsIceRestart(const std::string& content_name) const { + return transport_controller_->NeedsIceRestart(content_name); +} + void WebRtcSession::OnCertificateReady( const rtc::scoped_refptr& certificate) { transport_controller_->SetLocalCertificate(certificate); diff --git a/webrtc/api/webrtcsession.h b/webrtc/api/webrtcsession.h index 94f146ff57..1aa3e187ae 100644 --- a/webrtc/api/webrtcsession.h +++ b/webrtc/api/webrtcsession.h @@ -278,8 +278,19 @@ class WebRtcSession : cricket::DataChannelType data_channel_type() const; + // Returns true if there was an ICE restart initiated by the remote offer. bool IceRestartPending(const std::string& content_name) const; + // Set the "needs-ice-restart" flag as described in JSEP. After the flag is + // set, offers should generate new ufrags/passwords until an ICE restart + // occurs. + void SetNeedsIceRestartFlag(); + // Returns true if the ICE restart flag above was set, and no ICE restart has + // occurred yet for this transport (by applying a local description with + // changed ufrag/password). If the transport has been deleted as a result of + // bundling, returns false. + bool NeedsIceRestart(const std::string& content_name) const; + // Called when an RTCCertificate is generated or retrieved by // WebRTCSessionDescriptionFactory. Should happen before setLocalDescription. void OnCertificateReady( diff --git a/webrtc/api/webrtcsessiondescriptionfactory.cc b/webrtc/api/webrtcsessiondescriptionfactory.cc index 29da6f2c02..1f811f5911 100644 --- a/webrtc/api/webrtcsessiondescriptionfactory.cc +++ b/webrtc/api/webrtcsessiondescriptionfactory.cc @@ -338,6 +338,18 @@ void WebRtcSessionDescriptionFactory::OnMessage(rtc::Message* msg) { void WebRtcSessionDescriptionFactory::InternalCreateOffer( CreateSessionDescriptionRequest request) { + if (session_->local_description()) { + for (const cricket::TransportInfo& transport : + session_->local_description()->description()->transport_infos()) { + // If the needs-ice-restart flag is set as described by JSEP, we should + // generate an offer with a new ufrag/password to trigger an ICE restart. + if (session_->NeedsIceRestart(transport.content_name)) { + request.options.transport_options[transport.content_name].ice_restart = + true; + } + } + } + cricket::SessionDescription* desc(session_desc_factory_.CreateOffer( request.options, session_->local_description() ? session_->local_description()->description() diff --git a/webrtc/p2p/base/jseptransport.cc b/webrtc/p2p/base/jseptransport.cc index abfe0449a4..bf4d86c3af 100644 --- a/webrtc/p2p/base/jseptransport.cc +++ b/webrtc/p2p/base/jseptransport.cc @@ -176,6 +176,11 @@ bool JsepTransport::SetLocalTransportDescription( error_desc); } + bool ice_restarting = + local_description_set_ && + IceCredentialsChanged(local_description_->ice_ufrag, + local_description_->ice_pwd, description.ice_ufrag, + description.ice_pwd); local_description_.reset(new TransportDescription(description)); rtc::SSLFingerprint* local_fp = @@ -199,11 +204,17 @@ bool JsepTransport::SetLocalTransportDescription( if (action == CA_PRANSWER || action == CA_ANSWER) { ret &= NegotiateTransportDescription(action, error_desc); } - if (ret) { - local_description_set_ = true; + if (!ret) { + return false; } - return ret; + if (needs_ice_restart_ && ice_restarting) { + needs_ice_restart_ = false; + LOG(LS_VERBOSE) << "needs-ice-restart flag cleared for transport " << mid(); + } + + local_description_set_ = true; + return true; } bool JsepTransport::SetRemoteTransportDescription( @@ -233,6 +244,17 @@ bool JsepTransport::SetRemoteTransportDescription( return ret; } +void JsepTransport::SetNeedsIceRestartFlag() { + if (!needs_ice_restart_) { + needs_ice_restart_ = true; + LOG(LS_VERBOSE) << "needs-ice-restart flag set for transport " << mid(); + } +} + +bool JsepTransport::NeedsIceRestart() const { + return needs_ice_restart_; +} + void JsepTransport::GetSslRole(rtc::SSLRole* ssl_role) const { RTC_DCHECK(ssl_role); *ssl_role = secure_role_; diff --git a/webrtc/p2p/base/jseptransport.h b/webrtc/p2p/base/jseptransport.h index 9684770bf7..cc05ec097f 100644 --- a/webrtc/p2p/base/jseptransport.h +++ b/webrtc/p2p/base/jseptransport.h @@ -288,6 +288,18 @@ class JsepTransport : public sigslot::has_slots<> { ContentAction action, std::string* error_desc); + // Set the "needs-ice-restart" flag as described in JSEP. After the flag is + // set, offers should generate new ufrags/passwords until an ICE restart + // occurs. + // + // This and the below method can be called safely from any thread as long as + // SetXTransportDescription is not in progress. + void SetNeedsIceRestartFlag(); + // Returns true if the ICE restart flag above was set, and no ICE restart has + // occurred yet for this transport (by applying a local description with + // changed ufrag/password). + bool NeedsIceRestart() const; + void GetSslRole(rtc::SSLRole* ssl_role) const; // TODO(deadbeef): Make this const. See comment in transportcontroller.h. @@ -348,6 +360,8 @@ class JsepTransport : public sigslot::has_slots<> { std::string* error_desc); const std::string mid_; + // needs-ice-restart bit as described in JSEP. + bool needs_ice_restart_ = false; rtc::scoped_refptr certificate_; rtc::SSLRole secure_role_ = rtc::SSL_CLIENT; std::unique_ptr remote_fingerprint_; diff --git a/webrtc/p2p/base/jseptransport_unittest.cc b/webrtc/p2p/base/jseptransport_unittest.cc index 2f2510c476..f882a5de6d 100644 --- a/webrtc/p2p/base/jseptransport_unittest.cc +++ b/webrtc/p2p/base/jseptransport_unittest.cc @@ -73,6 +73,41 @@ TEST_F(JsepTransportTest, TestIceCredentialsChanged) { EXPECT_FALSE(cricket::IceCredentialsChanged("u1", "p1", "u1", "p1")); } +// Tests SetNeedsIceRestartFlag and NeedsIceRestart, ensuring NeedsIceRestart +// only starts returning "false" once an ICE restart has been initiated. +TEST_F(JsepTransportTest, NeedsIceRestart) { + // Do initial offer/answer so there's something to restart. + cricket::TransportDescription local_desc(kIceUfrag1, kIcePwd1); + cricket::TransportDescription remote_desc(kIceUfrag1, kIcePwd1); + ASSERT_TRUE(transport_->SetLocalTransportDescription( + local_desc, cricket::CA_OFFER, nullptr)); + ASSERT_TRUE(transport_->SetRemoteTransportDescription( + remote_desc, cricket::CA_ANSWER, nullptr)); + + // Flag initially should be false. + EXPECT_FALSE(transport_->NeedsIceRestart()); + + // After setting flag, it should be true. + transport_->SetNeedsIceRestartFlag(); + EXPECT_TRUE(transport_->NeedsIceRestart()); + + // Doing an identical offer/answer shouldn't clear the flag. + ASSERT_TRUE(transport_->SetLocalTransportDescription( + local_desc, cricket::CA_OFFER, nullptr)); + ASSERT_TRUE(transport_->SetRemoteTransportDescription( + remote_desc, cricket::CA_ANSWER, nullptr)); + EXPECT_TRUE(transport_->NeedsIceRestart()); + + // Doing an offer/answer that restarts ICE should clear the flag. + cricket::TransportDescription ice_restart_local_desc(kIceUfrag2, kIcePwd2); + cricket::TransportDescription ice_restart_remote_desc(kIceUfrag2, kIcePwd2); + ASSERT_TRUE(transport_->SetLocalTransportDescription( + ice_restart_local_desc, cricket::CA_OFFER, nullptr)); + ASSERT_TRUE(transport_->SetRemoteTransportDescription( + ice_restart_remote_desc, cricket::CA_ANSWER, nullptr)); + EXPECT_FALSE(transport_->NeedsIceRestart()); +} + TEST_F(JsepTransportTest, TestGetStats) { EXPECT_TRUE(SetupChannel()); cricket::TransportStats stats; diff --git a/webrtc/p2p/base/transportcontroller.cc b/webrtc/p2p/base/transportcontroller.cc index e4dc5afa21..4b24b1990d 100644 --- a/webrtc/p2p/base/transportcontroller.cc +++ b/webrtc/p2p/base/transportcontroller.cc @@ -80,6 +80,21 @@ void TransportController::SetIceRole(IceRole ice_role) { rtc::Bind(&TransportController::SetIceRole_n, this, ice_role)); } +void TransportController::SetNeedsIceRestartFlag() { + for (auto& kv : transports_) { + kv.second->SetNeedsIceRestartFlag(); + } +} + +bool TransportController::NeedsIceRestart( + const std::string& transport_name) const { + const JsepTransport* transport = GetJsepTransport(transport_name); + if (!transport) { + return false; + } + return transport->NeedsIceRestart(); +} + bool TransportController::GetSslRole(const std::string& transport_name, rtc::SSLRole* role) const { return network_thread_->Invoke( @@ -187,7 +202,7 @@ TransportChannel* TransportController::CreateTransportChannel_n( } // Need to create a new channel. - JsepTransport* transport = GetOrCreateJsepTransport_n(transport_name); + JsepTransport* transport = GetOrCreateJsepTransport(transport_name); // Create DTLS channel wrapping ICE channel, and configure it. TransportChannelImpl* ice = @@ -251,7 +266,7 @@ void TransportController::DestroyTransportChannel_n( } channels_.erase(it); - JsepTransport* t = GetJsepTransport_n(transport_name); + JsepTransport* t = GetJsepTransport(transport_name); bool channel_removed = t->RemoveChannel(component); RTC_DCHECK(channel_removed); // Just as we create a Transport when its first channel is created, @@ -362,16 +377,14 @@ TransportController::GetChannelIterator_n(const std::string& transport_name, }); } -const JsepTransport* TransportController::GetJsepTransport_n( +const JsepTransport* TransportController::GetJsepTransport( const std::string& transport_name) const { - RTC_DCHECK(network_thread_->IsCurrent()); auto it = transports_.find(transport_name); return (it == transports_.end()) ? nullptr : it->second.get(); } -JsepTransport* TransportController::GetJsepTransport_n( +JsepTransport* TransportController::GetJsepTransport( const std::string& transport_name) { - RTC_DCHECK(network_thread_->IsCurrent()); auto it = transports_.find(transport_name); return (it == transports_.end()) ? nullptr : it->second.get(); } @@ -392,11 +405,11 @@ TransportController::RefCountedChannel* TransportController::GetChannel_n( return (it == channels_.end()) ? nullptr : &(*it); } -JsepTransport* TransportController::GetOrCreateJsepTransport_n( +JsepTransport* TransportController::GetOrCreateJsepTransport( const std::string& transport_name) { RTC_DCHECK(network_thread_->IsCurrent()); - JsepTransport* transport = GetJsepTransport_n(transport_name); + JsepTransport* transport = GetJsepTransport(transport_name); if (transport) { return transport; } @@ -447,7 +460,7 @@ bool TransportController::GetSslRole_n(const std::string& transport_name, rtc::SSLRole* role) const { RTC_DCHECK(network_thread_->IsCurrent()); - const JsepTransport* t = GetJsepTransport_n(transport_name); + const JsepTransport* t = GetJsepTransport(transport_name); if (!t) { return false; } @@ -483,7 +496,7 @@ bool TransportController::GetLocalCertificate_n( rtc::scoped_refptr* certificate) const { RTC_DCHECK(network_thread_->IsCurrent()); - const JsepTransport* t = GetJsepTransport_n(transport_name); + const JsepTransport* t = GetJsepTransport(transport_name); if (!t) { return false; } @@ -512,7 +525,7 @@ bool TransportController::SetLocalTransportDescription_n( std::string* err) { RTC_DCHECK(network_thread_->IsCurrent()); - JsepTransport* transport = GetJsepTransport_n(transport_name); + JsepTransport* transport = GetJsepTransport(transport_name); if (!transport) { // If we didn't find a transport, that's not an error; // it could have been deleted as a result of bundling. @@ -556,7 +569,7 @@ bool TransportController::SetRemoteTransportDescription_n( SetIceRole_n(ICEROLE_CONTROLLING); } - JsepTransport* transport = GetJsepTransport_n(transport_name); + JsepTransport* transport = GetJsepTransport(transport_name); if (!transport) { // If we didn't find a transport, that's not an error; // it could have been deleted as a result of bundling. @@ -586,7 +599,7 @@ bool TransportController::AddRemoteCandidates_n( return false; } - JsepTransport* transport = GetJsepTransport_n(transport_name); + JsepTransport* transport = GetJsepTransport(transport_name); if (!transport) { // If we didn't find a transport, that's not an error; // it could have been deleted as a result of bundling. @@ -625,7 +638,7 @@ bool TransportController::RemoveRemoteCandidates_n(const Candidates& candidates, for (const auto& kv : candidates_by_transport_name) { const std::string& transport_name = kv.first; const Candidates& candidates = kv.second; - JsepTransport* transport = GetJsepTransport_n(transport_name); + JsepTransport* transport = GetJsepTransport(transport_name); if (!transport) { // If we didn't find a transport, that's not an error; // it could have been deleted as a result of bundling. @@ -646,7 +659,7 @@ bool TransportController::ReadyForRemoteCandidates_n( const std::string& transport_name) const { RTC_DCHECK(network_thread_->IsCurrent()); - const JsepTransport* transport = GetJsepTransport_n(transport_name); + const JsepTransport* transport = GetJsepTransport(transport_name); if (!transport) { return false; } @@ -657,7 +670,7 @@ bool TransportController::GetStats_n(const std::string& transport_name, TransportStats* stats) { RTC_DCHECK(network_thread_->IsCurrent()); - JsepTransport* transport = GetJsepTransport_n(transport_name); + JsepTransport* transport = GetJsepTransport(transport_name); if (!transport) { return false; } diff --git a/webrtc/p2p/base/transportcontroller.h b/webrtc/p2p/base/transportcontroller.h index cdac4b6dcb..ea5ec80a90 100644 --- a/webrtc/p2p/base/transportcontroller.h +++ b/webrtc/p2p/base/transportcontroller.h @@ -64,6 +64,16 @@ class TransportController : public sigslot::has_slots<>, void SetIceConfig(const IceConfig& config); void SetIceRole(IceRole ice_role); + // Set the "needs-ice-restart" flag as described in JSEP. After the flag is + // set, offers should generate new ufrags/passwords until an ICE restart + // occurs. + void SetNeedsIceRestartFlag(); + // Returns true if the ICE restart flag above was set, and no ICE restart has + // occurred yet for this transport (by applying a local description with + // changed ufrag/password). If the transport has been deleted as a result of + // bundling, returns false. + bool NeedsIceRestart(const std::string& transport_name) const; + bool GetSslRole(const std::string& transport_name, rtc::SSLRole* role) const; // Specifies the identity to use in this session. @@ -203,15 +213,15 @@ class TransportController : public sigslot::has_slots<>, std::vector::const_iterator GetChannelIterator_n( const std::string& transport_name, int component) const; - const JsepTransport* GetJsepTransport_n( + const JsepTransport* GetJsepTransport( const std::string& transport_name) const; - JsepTransport* GetJsepTransport_n(const std::string& transport_name); + JsepTransport* GetJsepTransport(const std::string& transport_name); const RefCountedChannel* GetChannel_n(const std::string& transport_name, int component) const; RefCountedChannel* GetChannel_n(const std::string& transport_name, int component); - JsepTransport* GetOrCreateJsepTransport_n(const std::string& transport_name); + JsepTransport* GetOrCreateJsepTransport(const std::string& transport_name); void DestroyAllChannels_n(); bool SetSslMaxProtocolVersion_n(rtc::SSLProtocolVersion version); diff --git a/webrtc/p2p/base/transportcontroller_unittest.cc b/webrtc/p2p/base/transportcontroller_unittest.cc index 9f30518af7..bafffa031e 100644 --- a/webrtc/p2p/base/transportcontroller_unittest.cc +++ b/webrtc/p2p/base/transportcontroller_unittest.cc @@ -793,4 +793,45 @@ TEST_F(TransportControllerTest, TestSetRemoteIceLiteInAnswer) { EXPECT_EQ(ICEMODE_LITE, channel->remote_ice_mode()); } +// Tests SetNeedsIceRestartFlag and NeedsIceRestart, setting the flag and then +// initiating an ICE restart for one of the transports. +TEST_F(TransportControllerTest, NeedsIceRestart) { + CreateChannel("audio", 1); + CreateChannel("video", 1); + + // Do initial offer/answer so there's something to restart. + TransportDescription local_desc(kIceUfrag1, kIcePwd1); + TransportDescription remote_desc(kIceUfrag1, kIcePwd1); + ASSERT_TRUE(transport_controller_->SetLocalTransportDescription( + "audio", local_desc, CA_OFFER, nullptr)); + ASSERT_TRUE(transport_controller_->SetLocalTransportDescription( + "video", local_desc, CA_OFFER, nullptr)); + ASSERT_TRUE(transport_controller_->SetRemoteTransportDescription( + "audio", remote_desc, CA_ANSWER, nullptr)); + ASSERT_TRUE(transport_controller_->SetRemoteTransportDescription( + "video", remote_desc, CA_ANSWER, nullptr)); + + // Initially NeedsIceRestart should return false. + EXPECT_FALSE(transport_controller_->NeedsIceRestart("audio")); + EXPECT_FALSE(transport_controller_->NeedsIceRestart("video")); + + // Set the needs-ice-restart flag and verify NeedsIceRestart starts returning + // true. + transport_controller_->SetNeedsIceRestartFlag(); + EXPECT_TRUE(transport_controller_->NeedsIceRestart("audio")); + EXPECT_TRUE(transport_controller_->NeedsIceRestart("video")); + // For a nonexistent transport, false should be returned. + EXPECT_FALSE(transport_controller_->NeedsIceRestart("deadbeef")); + + // Do ICE restart but only for audio. + TransportDescription ice_restart_local_desc(kIceUfrag2, kIcePwd2); + ASSERT_TRUE(transport_controller_->SetLocalTransportDescription( + "audio", ice_restart_local_desc, CA_OFFER, nullptr)); + ASSERT_TRUE(transport_controller_->SetLocalTransportDescription( + "video", local_desc, CA_OFFER, nullptr)); + // NeedsIceRestart should still be true for video. + EXPECT_FALSE(transport_controller_->NeedsIceRestart("audio")); + EXPECT_TRUE(transport_controller_->NeedsIceRestart("video")); +} + } // namespace cricket {