diff --git a/webrtc/p2p/base/p2ptransportchannel.cc b/webrtc/p2p/base/p2ptransportchannel.cc index 95696e1e2c..83bcf840ee 100644 --- a/webrtc/p2p/base/p2ptransportchannel.cc +++ b/webrtc/p2p/base/p2ptransportchannel.cc @@ -478,17 +478,38 @@ void P2PTransportChannel::OnUnknownAddress( remote_password = remote_ice_pwd_; } - Candidate new_remote_candidate; - if (candidate != NULL) { - new_remote_candidate = *candidate; + Candidate remote_candidate; + bool remote_candidate_is_new = (candidate == nullptr); + if (!remote_candidate_is_new) { + remote_candidate = *candidate; if (ufrag_per_port) { - new_remote_candidate.set_address(address); + remote_candidate.set_address(address); } } else { // Create a new candidate with this address. std::string type; + int remote_candidate_priority; if (port->IceProtocol() == ICEPROTO_RFC5245) { + // RFC 5245 + // If the source transport address of the request does not match any + // existing remote candidates, it represents a new peer reflexive remote + // candidate. type = PRFLX_PORT_TYPE; + + // The priority of the candidate is set to the PRIORITY attribute + // from the request. + const StunUInt32Attribute* priority_attr = + stun_msg->GetUInt32(STUN_ATTR_PRIORITY); + if (!priority_attr) { + LOG(LS_WARNING) << "P2PTransportChannel::OnUnknownAddress - " + << "No STUN_ATTR_PRIORITY found in the " + << "stun request message"; + port->SendBindingErrorResponse(stun_msg, address, + STUN_ERROR_BAD_REQUEST, + STUN_ERROR_REASON_BAD_REQUEST); + return; + } + remote_candidate_priority = priority_attr->value(); } else { // G-ICE doesn't support prflx candidate. // We set candidate type to STUN_PORT_TYPE if the binding request comes @@ -499,43 +520,24 @@ void P2PTransportChannel::OnUnknownAddress( } else { type = port->Type(); } + remote_candidate_priority = remote_candidate.GetPriority( + ICE_TYPE_PREFERENCE_PRFLX, port->Network()->preference(), 0); } - new_remote_candidate = + remote_candidate = Candidate(component(), ProtoToString(proto), address, 0, remote_username, remote_password, type, 0U, ""); // From RFC 5245, section-7.2.1.3: // The foundation of the candidate is set to an arbitrary value, different // from the foundation for all other remote candidates. - new_remote_candidate.set_foundation( - rtc::ToString(rtc::ComputeCrc32(new_remote_candidate.id()))); + remote_candidate.set_foundation( + rtc::ToString(rtc::ComputeCrc32(remote_candidate.id()))); - new_remote_candidate.set_priority(new_remote_candidate.GetPriority( - ICE_TYPE_PREFERENCE_PRFLX, port->Network()->preference(), 0)); + remote_candidate.set_priority(remote_candidate_priority); } if (port->IceProtocol() == ICEPROTO_RFC5245) { - // RFC 5245 - // If the source transport address of the request does not match any - // existing remote candidates, it represents a new peer reflexive remote - // candidate. - - // The priority of the candidate is set to the PRIORITY attribute - // from the request. - const StunUInt32Attribute* priority_attr = - stun_msg->GetUInt32(STUN_ATTR_PRIORITY); - if (!priority_attr) { - LOG(LS_WARNING) << "P2PTransportChannel::OnUnknownAddress - " - << "No STUN_ATTR_PRIORITY found in the " - << "stun request message"; - port->SendBindingErrorResponse(stun_msg, address, - STUN_ERROR_BAD_REQUEST, - STUN_ERROR_REASON_BAD_REQUEST); - return; - } - new_remote_candidate.set_priority(priority_attr->value()); - // RFC5245, the agent constructs a pair whose local candidate is equal to // the transport address on which the STUN request was received, and a // remote candidate equal to the source transport address where the @@ -545,10 +547,10 @@ void P2PTransportChannel::OnUnknownAddress( // When ports are muxed, this channel might get multiple unknown address // signals. In that case if the connection is already exists, we should // simply ignore the signal othewise send server error. - if (port->GetConnection(new_remote_candidate.address())) { + if (port->GetConnection(remote_candidate.address())) { if (port_muxed) { LOG(LS_INFO) << "Connection already exists for peer reflexive " - << "candidate: " << new_remote_candidate.ToString(); + << "candidate: " << remote_candidate.ToString(); return; } else { ASSERT(false); @@ -560,7 +562,7 @@ void P2PTransportChannel::OnUnknownAddress( } Connection* connection = port->CreateConnection( - new_remote_candidate, cricket::PortInterface::ORIGIN_THIS_PORT); + remote_candidate, cricket::PortInterface::ORIGIN_THIS_PORT); if (!connection) { ASSERT(false); port->SendBindingErrorResponse(stun_msg, address, @@ -569,8 +571,9 @@ void P2PTransportChannel::OnUnknownAddress( return; } - LOG(LS_INFO) << "Adding connection from peer reflexive candidate: " - << new_remote_candidate.ToString(); + LOG(LS_INFO) << "Adding connection from " + << (remote_candidate_is_new ? "peer reflexive" : "resurrected") + << " candidate: " << remote_candidate.ToString(); AddConnection(connection); connection->ReceivedPing(); @@ -585,7 +588,7 @@ void P2PTransportChannel::OnUnknownAddress( // Check for connectivity to this address. Create connections // to this address across all local ports. First, add this as a new remote // address - if (!CreateConnections(new_remote_candidate, port, true)) { + if (!CreateConnections(remote_candidate, port, true)) { // Hopefully this won't occur, because changing a destination address // shouldn't cause a new connection to fail ASSERT(false); diff --git a/webrtc/p2p/base/p2ptransportchannel_unittest.cc b/webrtc/p2p/base/p2ptransportchannel_unittest.cc index f6beee07bf..13f19ae0a5 100644 --- a/webrtc/p2p/base/p2ptransportchannel_unittest.cc +++ b/webrtc/p2p/base/p2ptransportchannel_unittest.cc @@ -1700,19 +1700,20 @@ TEST_F(P2PTransportChannelMultihomedTest, TestDrain) { DestroyChannels(); } -class P2PTransportChannelPingOrderTest : public testing::Test, - public sigslot::has_slots<> { +// A collection of tests which tests a single P2PTransportChannel by sending +// pings. +class P2PTransportChannelPingTest : public testing::Test, + public sigslot::has_slots<> { public: - P2PTransportChannelPingOrderTest() : - pss_(new rtc::PhysicalSocketServer), - vss_(new rtc::VirtualSocketServer(pss_.get())), - ss_scope_(vss_.get()) { - } + P2PTransportChannelPingTest() + : pss_(new rtc::PhysicalSocketServer), + vss_(new rtc::VirtualSocketServer(pss_.get())), + ss_scope_(vss_.get()) {} protected: void PrepareChannel(cricket::P2PTransportChannel* ch) { ch->SignalRequestSignaling.connect( - this, &P2PTransportChannelPingOrderTest::OnChannelRequestSignaling); + this, &P2PTransportChannelPingTest::OnChannelRequestSignaling); ch->SetIceProtocolType(cricket::ICEPROTO_RFC5245); ch->SetIceRole(cricket::ICEROLE_CONTROLLING); ch->SetIceCredentials(kIceUfrag[0], kIcePwd[0]); @@ -1741,13 +1742,17 @@ class P2PTransportChannelPingOrderTest : public testing::Test, return GetConnectionTo(ch, ip, port_num); } - cricket::Connection* GetConnectionTo(cricket::P2PTransportChannel* ch, - const std::string& ip, - int port_num) { + cricket::Port* GetPort(cricket::P2PTransportChannel* ch) { if (ch->ports().empty()) { return nullptr; } - cricket::Port* port = static_cast(ch->ports()[0]); + return static_cast(ch->ports()[0]); + } + + cricket::Connection* GetConnectionTo(cricket::P2PTransportChannel* ch, + const std::string& ip, + int port_num) { + cricket::Port* port = GetPort(ch); if (!port) { return nullptr; } @@ -1760,7 +1765,7 @@ class P2PTransportChannelPingOrderTest : public testing::Test, rtc::SocketServerScope ss_scope_; }; -TEST_F(P2PTransportChannelPingOrderTest, TestTriggeredChecks) { +TEST_F(P2PTransportChannelPingTest, TestTriggeredChecks) { cricket::FakePortAllocator pa(rtc::Thread::Current(), nullptr); cricket::P2PTransportChannel ch("trigger checks", 1, nullptr, &pa); PrepareChannel(&ch); @@ -1784,7 +1789,7 @@ TEST_F(P2PTransportChannelPingOrderTest, TestTriggeredChecks) { EXPECT_EQ(conn1, ch.FindNextPingableConnection()); } -TEST_F(P2PTransportChannelPingOrderTest, TestNoTriggeredChecksWhenWritable) { +TEST_F(P2PTransportChannelPingTest, TestNoTriggeredChecksWhenWritable) { cricket::FakePortAllocator pa(rtc::Thread::Current(), nullptr); cricket::P2PTransportChannel ch("trigger checks", 1, nullptr, &pa); PrepareChannel(&ch); @@ -1807,3 +1812,52 @@ TEST_F(P2PTransportChannelPingOrderTest, TestNoTriggeredChecksWhenWritable) { // a higher priority. EXPECT_EQ(conn2, ch.FindNextPingableConnection()); } + +TEST_F(P2PTransportChannelPingTest, ConnectionResurrection) { + cricket::FakePortAllocator pa(rtc::Thread::Current(), nullptr); + cricket::P2PTransportChannel ch("connection resurrection", 1, nullptr, &pa); + PrepareChannel(&ch); + ch.Connect(); + + // Create conn1 and keep track of original candidate priority. + ch.OnCandidate(CreateCandidate("1.1.1.1", 1, 1)); + cricket::Connection* conn1 = WaitForConnectionTo(&ch, "1.1.1.1", 1); + ASSERT_TRUE(conn1 != nullptr); + uint32 remote_priority = conn1->remote_candidate().priority(); + + // Create a higher priority candidate and make the connection + // readable/writable. This will prune conn1. + ch.OnCandidate(CreateCandidate("2.2.2.2", 2, 2)); + cricket::Connection* conn2 = WaitForConnectionTo(&ch, "2.2.2.2", 2); + ASSERT_TRUE(conn2 != nullptr); + conn2->ReceivedPing(); + conn2->ReceivedPingResponse(); + + // Wait for conn1 being destroyed. + EXPECT_TRUE_WAIT(GetConnectionTo(&ch, "1.1.1.1", 1) == nullptr, 3000); + cricket::Port* port = GetPort(&ch); + + // Create a minimal STUN message with prflx priority. + cricket::IceMessage request; + request.SetType(cricket::STUN_BINDING_REQUEST); + request.AddAttribute(new cricket::StunByteStringAttribute( + cricket::STUN_ATTR_USERNAME, kIceUfrag[1])); + uint32 prflx_priority = cricket::ICE_TYPE_PREFERENCE_PRFLX << 24; + request.AddAttribute(new cricket::StunUInt32Attribute( + cricket::STUN_ATTR_PRIORITY, prflx_priority)); + EXPECT_NE(prflx_priority, remote_priority); + + // conn1 should be resurrected with original priority. + port->SignalUnknownAddress(port, rtc::SocketAddress("1.1.1.1", 1), + cricket::PROTO_UDP, &request, kIceUfrag[1], false); + conn1 = WaitForConnectionTo(&ch, "1.1.1.1", 1); + ASSERT_TRUE(conn1 != nullptr); + EXPECT_EQ(conn1->remote_candidate().priority(), remote_priority); + + // conn3, a real prflx connection, should have prflx priority. + port->SignalUnknownAddress(port, rtc::SocketAddress("3.3.3.3", 1), + cricket::PROTO_UDP, &request, kIceUfrag[1], false); + cricket::Connection* conn3 = WaitForConnectionTo(&ch, "3.3.3.3", 1); + ASSERT_TRUE(conn3 != nullptr); + EXPECT_EQ(conn3->remote_candidate().priority(), prflx_priority); +}