Re-evalutes the ICE role on ICE restart.
Also unifies the logic of ICE restart.
BUG=1775
R=juberti@google.com, juberti@webrtc.org
Review URL: https://webrtc-codereview.appspot.com/20619004
git-svn-id: http://webrtc.googlecode.com/svn/trunk@6510 4adac7df-926f-26a2-2b94-8c16560cd09d
diff --git a/talk/app/webrtc/webrtcsession.cc b/talk/app/webrtc/webrtcsession.cc
index 1b1c287..9c27be5 100644
--- a/talk/app/webrtc/webrtcsession.cc
+++ b/talk/app/webrtc/webrtcsession.cc
@@ -430,8 +430,10 @@
// No transport description exist. This is not an ice restart.
continue;
}
- if (new_transport_desc->ice_pwd != old_transport_desc->ice_pwd &&
- new_transport_desc->ice_ufrag != old_transport_desc->ice_ufrag) {
+ if (cricket::IceCredentialsChanged(old_transport_desc->ice_ufrag,
+ old_transport_desc->ice_pwd,
+ new_transport_desc->ice_ufrag,
+ new_transport_desc->ice_pwd)) {
LOG(LS_INFO) << "Remote peer request ice restart.";
ice_restart_ = true;
break;
diff --git a/talk/p2p/base/p2ptransportchannel.cc b/talk/p2p/base/p2ptransportchannel.cc
index 887fc45..8a3ec7f 100644
--- a/talk/p2p/base/p2ptransportchannel.cc
+++ b/talk/p2p/base/p2ptransportchannel.cc
@@ -259,7 +259,8 @@
if (!ice_ufrag_.empty() && !ice_pwd_.empty()) {
// Restart candidate allocation if there is any change in either
// ice ufrag or password.
- ice_restart = (ice_ufrag_ != ice_ufrag) || (ice_pwd_!= ice_pwd);
+ ice_restart =
+ IceCredentialsChanged(ice_ufrag_, ice_pwd_, ice_ufrag, ice_pwd);
}
ice_ufrag_ = ice_ufrag;
diff --git a/talk/p2p/base/transport.cc b/talk/p2p/base/transport.cc
index 16087e3..2996487 100644
--- a/talk/p2p/base/transport.cc
+++ b/talk/p2p/base/transport.cc
@@ -118,6 +118,23 @@
return false;
}
+bool IceCredentialsChanged(const std::string& old_ufrag,
+ const std::string& old_pwd,
+ const std::string& new_ufrag,
+ const std::string& new_pwd) {
+ // TODO(jiayl): The standard (RFC 5245 Section 9.1.1.1) says that ICE should
+ // restart when both the ufrag and password are changed, but we do restart
+ // when either ufrag or passwrod is changed to keep compatible with GICE. We
+ // should clean this up when GICE is no longer used.
+ return (old_ufrag != new_ufrag) || (old_pwd != new_pwd);
+}
+
+static bool IceCredentialsChanged(const TransportDescription& old_desc,
+ const TransportDescription& new_desc) {
+ return IceCredentialsChanged(old_desc.ice_ufrag, old_desc.ice_pwd,
+ new_desc.ice_ufrag, new_desc.ice_pwd);
+}
+
Transport::Transport(talk_base::Thread* signaling_thread,
talk_base::Thread* worker_thread,
const std::string& content_name,
@@ -726,7 +743,17 @@
error_desc);
}
+ if (local_description_ && IceCredentialsChanged(*local_description_, desc)) {
+ IceRole new_ice_role = (action == CA_OFFER) ? ICEROLE_CONTROLLING
+ : ICEROLE_CONTROLLED;
+
+ // It must be called before ApplyLocalTransportDescription_w, which may
+ // trigger an ICE restart and depends on the new ICE role.
+ SetIceRole_w(new_ice_role);
+ }
+
local_description_.reset(new TransportDescription(desc));
+
for (ChannelMap::iterator iter = channels_.begin();
iter != channels_.end(); ++iter) {
ret &= ApplyLocalTransportDescription_w(iter->second.get(), error_desc);
diff --git a/talk/p2p/base/transport.h b/talk/p2p/base/transport.h
index 7f460d1..5a4b75f 100644
--- a/talk/p2p/base/transport.h
+++ b/talk/p2p/base/transport.h
@@ -189,6 +189,11 @@
bool BadTransportDescription(const std::string& desc, std::string* err_desc);
+bool IceCredentialsChanged(const std::string& old_ufrag,
+ const std::string& old_pwd,
+ const std::string& new_ufrag,
+ const std::string& new_pwd);
+
class Transport : public talk_base::MessageHandler,
public sigslot::has_slots<> {
public:
diff --git a/talk/p2p/base/transport_unittest.cc b/talk/p2p/base/transport_unittest.cc
index b91b1a0..a83d256 100644
--- a/talk/p2p/base/transport_unittest.cc
+++ b/talk/p2p/base/transport_unittest.cc
@@ -52,6 +52,9 @@
static const char kIceUfrag1[] = "TESTICEUFRAG0001";
static const char kIcePwd1[] = "TESTICEPWD00000000000001";
+static const char kIceUfrag2[] = "TESTICEUFRAG0002";
+static const char kIcePwd2[] = "TESTICEPWD00000000000002";
+
class TransportTest : public testing::Test,
public sigslot::has_slots<> {
public:
@@ -184,6 +187,96 @@
EXPECT_EQ(kIcePwd1, channel_->remote_ice_pwd());
}
+// Verifies that IceCredentialsChanged returns true when either ufrag or pwd
+// changed, and false in other cases.
+TEST_F(TransportTest, TestIceCredentialsChanged) {
+ EXPECT_TRUE(cricket::IceCredentialsChanged("u1", "p1", "u2", "p2"));
+ EXPECT_TRUE(cricket::IceCredentialsChanged("u1", "p1", "u2", "p1"));
+ EXPECT_TRUE(cricket::IceCredentialsChanged("u1", "p1", "u1", "p2"));
+ EXPECT_FALSE(cricket::IceCredentialsChanged("u1", "p1", "u1", "p1"));
+}
+
+// This test verifies that the callee's ICE role changes from controlled to
+// controlling when the callee triggers an ICE restart.
+TEST_F(TransportTest, TestIceControlledToControllingOnIceRestart) {
+ EXPECT_TRUE(SetupChannel());
+ transport_->SetIceRole(cricket::ICEROLE_CONTROLLED);
+
+ cricket::TransportDescription desc(
+ cricket::NS_JINGLE_ICE_UDP, kIceUfrag1, kIcePwd1);
+ ASSERT_TRUE(transport_->SetRemoteTransportDescription(desc,
+ cricket::CA_OFFER,
+ NULL));
+ ASSERT_TRUE(transport_->SetLocalTransportDescription(desc,
+ cricket::CA_ANSWER,
+ NULL));
+ EXPECT_EQ(cricket::ICEROLE_CONTROLLED, transport_->ice_role());
+
+ cricket::TransportDescription new_local_desc(
+ cricket::NS_JINGLE_ICE_UDP, kIceUfrag2, kIcePwd2);
+ ASSERT_TRUE(transport_->SetLocalTransportDescription(new_local_desc,
+ cricket::CA_OFFER,
+ NULL));
+ EXPECT_EQ(cricket::ICEROLE_CONTROLLING, transport_->ice_role());
+ EXPECT_EQ(cricket::ICEROLE_CONTROLLING, channel_->GetIceRole());
+}
+
+// This test verifies that the caller's ICE role changes from controlling to
+// controlled when the callee triggers an ICE restart.
+TEST_F(TransportTest, TestIceControllingToControlledOnIceRestart) {
+ EXPECT_TRUE(SetupChannel());
+ transport_->SetIceRole(cricket::ICEROLE_CONTROLLING);
+
+ cricket::TransportDescription desc(
+ cricket::NS_JINGLE_ICE_UDP, kIceUfrag1, kIcePwd1);
+ ASSERT_TRUE(transport_->SetLocalTransportDescription(desc,
+ cricket::CA_OFFER,
+ NULL));
+ ASSERT_TRUE(transport_->SetRemoteTransportDescription(desc,
+ cricket::CA_ANSWER,
+ NULL));
+ EXPECT_EQ(cricket::ICEROLE_CONTROLLING, transport_->ice_role());
+
+ cricket::TransportDescription new_local_desc(
+ cricket::NS_JINGLE_ICE_UDP, kIceUfrag2, kIcePwd2);
+ ASSERT_TRUE(transport_->SetLocalTransportDescription(new_local_desc,
+ cricket::CA_ANSWER,
+ NULL));
+ EXPECT_EQ(cricket::ICEROLE_CONTROLLED, transport_->ice_role());
+ EXPECT_EQ(cricket::ICEROLE_CONTROLLED, channel_->GetIceRole());
+}
+
+// This test verifies that the caller's ICE role is still controlling after the
+// callee triggers ICE restart if the callee's ICE mode is LITE.
+TEST_F(TransportTest, TestIceControllingOnIceRestartIfRemoteIsIceLite) {
+ EXPECT_TRUE(SetupChannel());
+ transport_->SetIceRole(cricket::ICEROLE_CONTROLLING);
+
+ cricket::TransportDescription desc(
+ cricket::NS_JINGLE_ICE_UDP, kIceUfrag1, kIcePwd1);
+ ASSERT_TRUE(transport_->SetLocalTransportDescription(desc,
+ cricket::CA_OFFER,
+ NULL));
+
+ cricket::TransportDescription remote_desc(
+ cricket::NS_JINGLE_ICE_UDP, std::vector<std::string>(),
+ kIceUfrag1, kIcePwd1, cricket::ICEMODE_LITE,
+ cricket::CONNECTIONROLE_NONE, NULL, cricket::Candidates());
+ ASSERT_TRUE(transport_->SetRemoteTransportDescription(remote_desc,
+ cricket::CA_ANSWER,
+ NULL));
+
+ EXPECT_EQ(cricket::ICEROLE_CONTROLLING, transport_->ice_role());
+
+ cricket::TransportDescription new_local_desc(
+ cricket::NS_JINGLE_ICE_UDP, kIceUfrag2, kIcePwd2);
+ ASSERT_TRUE(transport_->SetLocalTransportDescription(new_local_desc,
+ cricket::CA_ANSWER,
+ NULL));
+ EXPECT_EQ(cricket::ICEROLE_CONTROLLING, transport_->ice_role());
+ EXPECT_EQ(cricket::ICEROLE_CONTROLLING, channel_->GetIceRole());
+}
+
// This test verifies that the Completed and Failed states can be reached.
TEST_F(TransportTest, TestChannelCompletedAndFailed) {
transport_->SetIceRole(cricket::ICEROLE_CONTROLLING);
