Implement the "needs-ice-restart" logic for SetConfiguration.
Changing the configuration will cause subsequently generated offers to change
the ufrag/pwd as necessary, so that a new round of gathering is started that
uses the new configuration.
This CL also makes some minor unrelated changes: changing the reference SDP in
the PC tests to more match what we generate, and relaxing the network thread
requirement for JsepTransport (since there's no reason the "needs-ice-restart"
flag can't be accessed from the signaling thread).
BUG=webrtc:6714
Review-Url: https://codereview.webrtc.org/2563153002
Cr-Commit-Position: refs/heads/master@{#15527}
diff --git a/webrtc/api/peerconnection.cc b/webrtc/api/peerconnection.cc
index e1f19ad..49f9030 100644
--- a/webrtc/api/peerconnection.cc
+++ b/webrtc/api/peerconnection.cc
@@ -1298,9 +1298,16 @@
}
}
- // 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 a655756..29badbd 100644
--- a/webrtc/api/peerconnectioninterface.h
+++ b/webrtc/api/peerconnectioninterface.h
@@ -214,6 +214,11 @@
std::vector<std::string> 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<IceServer> IceServers;
diff --git a/webrtc/api/peerconnectioninterface_unittest.cc b/webrtc/api/peerconnectioninterface_unittest.cc
index cfba3e6..1fb3a5f 100644
--- a/webrtc/api/peerconnectioninterface_unittest.cc
+++ b/webrtc/api/peerconnectioninterface_unittest.cc
@@ -73,11 +73,11 @@
"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 @@
"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 @@
"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 @@
"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 @@
"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 @@
"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 @@
"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 @@
"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 @@
"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 @@
"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 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 @@
return true;
}
+// Get the ufrags out of an SDP blob. Useful for testing ICE restart
+// behavior.
+std::vector<std::string> GetUfrags(
+ const webrtc::SessionDescriptionInterface* desc) {
+ std::vector<std::string> 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 @@
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<std::string> 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<std::string> 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<int>(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<std::string> 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<std::string> 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<std::string> initial_ufrags = GetUfrags(pc_->local_description());
+ ASSERT_EQ(2, initial_ufrags.size());
+
+ // Create offer and grab the new ufrags.
+ CreateOfferAsLocalDescription();
+ std::vector<std::string> 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 636184b..c5582d3 100644
--- a/webrtc/api/webrtcsession.cc
+++ b/webrtc/api/webrtcsession.cc
@@ -1330,6 +1330,14 @@
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<rtc::RTCCertificate>& certificate) {
transport_controller_->SetLocalCertificate(certificate);
diff --git a/webrtc/api/webrtcsession.h b/webrtc/api/webrtcsession.h
index 94f146f..1aa3e18 100644
--- a/webrtc/api/webrtcsession.h
+++ b/webrtc/api/webrtcsession.h
@@ -278,8 +278,19 @@
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 29da6f2..1f811f5 100644
--- a/webrtc/api/webrtcsessiondescriptionfactory.cc
+++ b/webrtc/api/webrtcsessiondescriptionfactory.cc
@@ -338,6 +338,18 @@
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 abfe044..bf4d86c 100644
--- a/webrtc/p2p/base/jseptransport.cc
+++ b/webrtc/p2p/base/jseptransport.cc
@@ -176,6 +176,11 @@
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 @@
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 @@
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 9684770..cc05ec0 100644
--- a/webrtc/p2p/base/jseptransport.h
+++ b/webrtc/p2p/base/jseptransport.h
@@ -288,6 +288,18 @@
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 @@
std::string* error_desc);
const std::string mid_;
+ // needs-ice-restart bit as described in JSEP.
+ bool needs_ice_restart_ = false;
rtc::scoped_refptr<rtc::RTCCertificate> certificate_;
rtc::SSLRole secure_role_ = rtc::SSL_CLIENT;
std::unique_ptr<rtc::SSLFingerprint> remote_fingerprint_;
diff --git a/webrtc/p2p/base/jseptransport_unittest.cc b/webrtc/p2p/base/jseptransport_unittest.cc
index 2f2510c..f882a5d 100644
--- a/webrtc/p2p/base/jseptransport_unittest.cc
+++ b/webrtc/p2p/base/jseptransport_unittest.cc
@@ -73,6 +73,41 @@
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 e4dc5af..4b24b19 100644
--- a/webrtc/p2p/base/transportcontroller.cc
+++ b/webrtc/p2p/base/transportcontroller.cc
@@ -80,6 +80,21 @@
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<bool>(
@@ -187,7 +202,7 @@
}
// 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 @@
}
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 @@
});
}
-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 @@
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 @@
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 @@
rtc::scoped_refptr<rtc::RTCCertificate>* 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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 cdac4b6..ea5ec80 100644
--- a/webrtc/p2p/base/transportcontroller.h
+++ b/webrtc/p2p/base/transportcontroller.h
@@ -64,6 +64,16 @@
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 @@
std::vector<RefCountedChannel>::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 9f30518..bafffa0 100644
--- a/webrtc/p2p/base/transportcontroller_unittest.cc
+++ b/webrtc/p2p/base/transportcontroller_unittest.cc
@@ -793,4 +793,45 @@
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 {
