diff --git a/talk/app/webrtc/webrtcsession.cc b/talk/app/webrtc/webrtcsession.cc index 1b1c287817..9c27be52a9 100644 --- a/talk/app/webrtc/webrtcsession.cc +++ b/talk/app/webrtc/webrtcsession.cc @@ -430,8 +430,10 @@ class IceRestartAnswerLatch { // 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 887fc45efa..8a3ec7ff70 100644 --- a/talk/p2p/base/p2ptransportchannel.cc +++ b/talk/p2p/base/p2ptransportchannel.cc @@ -259,7 +259,8 @@ void P2PTransportChannel::SetIceCredentials(const std::string& ice_ufrag, 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 16087e3f06..2996487c41 100644 --- a/talk/p2p/base/transport.cc +++ b/talk/p2p/base/transport.cc @@ -118,6 +118,23 @@ bool BadTransportDescription(const std::string& desc, std::string* err_desc) { 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 @@ bool Transport::SetLocalTransportDescription_w( 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 7f460d1127..5a4b75feb3 100644 --- a/talk/p2p/base/transport.h +++ b/talk/p2p/base/transport.h @@ -189,6 +189,11 @@ struct TransportStats { 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 b91b1a0d0a..a83d2566d4 100644 --- a/talk/p2p/base/transport_unittest.cc +++ b/talk/p2p/base/transport_unittest.cc @@ -52,6 +52,9 @@ using talk_base::SocketAddress; 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 @@ TEST_F(TransportTest, TestChannelIceParameters) { 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(), + 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);