zqz/platform-external-webrtc
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Fix bugs in collecting STUN candidate stats and configuring STUN

Browse Source 
candidate keepalive intervals.

StunStats for a STUN candidate cannot be updated after the initial report
in the stats collector. This is caused by the early return of cached
candidate reports for future queries after the initial report creation.

The STUN keepalive interval cannot be configured for UDPPort because of
incorrect type screening, where only StunPort was supported.

TBR=pthatcher@webrtc.org

Bug: webrtc:8951
Change-Id: I0c9c414f43e6327985be6e541e17b5d6f248a79d
Reviewed-on: https://webrtc-review.googlesource.com/58560
Commit-Queue: Qingsi Wang <qingsi@google.com>
Reviewed-by: Taylor Brandstetter <deadbeef@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#22278}
This commit is contained in:
Qingsi Wang
2018-03-01 18:25:20 -08:00
committed by Commit Bot
parent afe9eb33dd
commit 4ff5443e4e
10 changed files with 189 additions and 38 deletions
Download Patch File Download Diff File Expand all files Collapse all files

5
p2p/base/port.h
View File

@ -236,6 +236,11 @@ class Port : public PortInterface, public rtc::MessageHandler,
const std::string& password);
~Port() override;
// Note that the port type does NOT uniquely identify different subclasses of
// Port. Use the 2-tuple of the port type AND the protocol (GetProtocol()) to
// uniquely identify subclasses. Whenever a new subclass of Port introduces a
// conflit in the value of the 2-tuple, make sure that the implementation that
// relies on this 2-tuple for RTTI is properly changed.
const std::string& Type() const override;
rtc::Network* Network() const override;

6
p2p/base/port_unittest.cc
View File

@ -499,7 +499,8 @@ class PortTest : public testing::Test, public sigslot::has_slots<> {
UDPPort* CreateUdpPort(const SocketAddress& addr,
PacketSocketFactory* socket_factory) {
return UDPPort::Create(&main_, socket_factory, MakeNetwork(addr), 0, 0,
username_, password_, std::string(), true);
username_, password_, std::string(), true,
rtc::nullopt);
}
TCPPort* CreateTcpPort(const SocketAddress& addr) {
return CreateTcpPort(addr, &socket_factory_);
@ -514,7 +515,8 @@ class PortTest : public testing::Test, public sigslot::has_slots<> {
ServerAddresses stun_servers;
stun_servers.insert(kStunAddr);
return StunPort::Create(&main_, factory, MakeNetwork(addr), 0, 0, username_,
password_, stun_servers, std::string());
password_, stun_servers, std::string(),
rtc::nullopt);
}
Port* CreateRelayPort(const SocketAddress& addr, RelayType rtype,
ProtocolType int_proto, ProtocolType ext_proto) {

15
p2p/base/stunport.cc
View File

@ -457,17 +457,16 @@ void UDPPort::OnStunBindingRequestSucceeded(
int rtt_ms,
const rtc::SocketAddress& stun_server_addr,
const rtc::SocketAddress& stun_reflected_addr) {
if (bind_request_succeeded_servers_.find(stun_server_addr) !=
bind_request_succeeded_servers_.end()) {
return;
}
bind_request_succeeded_servers_.insert(stun_server_addr);
RTC_DCHECK(stats_.stun_binding_responses_received <
stats_.stun_binding_requests_sent);
stats_.stun_binding_responses_received++;
stats_.stun_binding_rtt_ms_total += rtt_ms;
stats_.stun_binding_rtt_ms_squared_total += rtt_ms * rtt_ms;
if (bind_request_succeeded_servers_.find(stun_server_addr) !=
bind_request_succeeded_servers_.end()) {
return;
}
bind_request_succeeded_servers_.insert(stun_server_addr);
// If socket is shared and |stun_reflected_addr| is equal to local socket
// address, or if the same address has been added by another STUN server,
// then discarding the stun address.
@ -554,9 +553,11 @@ StunPort* StunPort::Create(rtc::Thread* thread,
const std::string& username,
const std::string& password,
const ServerAddresses& servers,
const std::string& origin) {
const std::string& origin,
rtc::Optional<int> stun_keepalive_interval) {
StunPort* port = new StunPort(thread, factory, network, min_port, max_port,
username, password, servers, origin);
port->set_stun_keepalive_delay(stun_keepalive_interval);
if (!port->Init()) {
delete port;
port = NULL;

11
p2p/base/stunport.h
View File

@ -42,9 +42,11 @@ class UDPPort : public Port {
const std::string& username,
const std::string& password,
const std::string& origin,
bool emit_local_for_anyaddress) {
bool emit_local_for_anyaddress,
rtc::Optional<int> stun_keepalive_interval) {
UDPPort* port = new UDPPort(thread, factory, network, socket, username,
password, origin, emit_local_for_anyaddress);
port->set_stun_keepalive_delay(stun_keepalive_interval);
if (!port->Init()) {
delete port;
port = NULL;
@ -60,10 +62,12 @@ class UDPPort : public Port {
const std::string& username,
const std::string& password,
const std::string& origin,
bool emit_local_for_anyaddress) {
bool emit_local_for_anyaddress,
rtc::Optional<int> stun_keepalive_interval) {
UDPPort* port =
new UDPPort(thread, factory, network, min_port, max_port, username,
password, origin, emit_local_for_anyaddress);
port->set_stun_keepalive_delay(stun_keepalive_interval);
if (!port->Init()) {
delete port;
port = NULL;
@ -262,7 +266,8 @@ class StunPort : public UDPPort {
const std::string& username,
const std::string& password,
const ServerAddresses& servers,
const std::string& origin);
const std::string& origin,
rtc::Optional<int> stun_keepalive_interval);
void PrepareAddress() override;

9
p2p/base/stunport_unittest.cc
View File

@ -74,7 +74,7 @@ class StunPortTestBase : public testing::Test, public sigslot::has_slots<> {
stun_port_.reset(cricket::StunPort::Create(
rtc::Thread::Current(), &socket_factory_, &network_, 0, 0,
rtc::CreateRandomString(16), rtc::CreateRandomString(22), stun_servers,
std::string()));
std::string(), rtc::nullopt));
stun_port_->set_stun_keepalive_delay(stun_keepalive_delay_);
// If |stun_keepalive_lifetime_| is negative, let the stun port
// choose its lifetime from the network type.
@ -92,10 +92,9 @@ class StunPortTestBase : public testing::Test, public sigslot::has_slots<> {
ASSERT_TRUE(socket_ != NULL);
socket_->SignalReadPacket.connect(this, &StunPortTestBase::OnReadPacket);
stun_port_.reset(cricket::UDPPort::Create(
rtc::Thread::Current(), &socket_factory_,
&network_, socket_.get(),
rtc::CreateRandomString(16), rtc::CreateRandomString(22),
std::string(), false));
rtc::Thread::Current(), &socket_factory_, &network_, socket_.get(),
rtc::CreateRandomString(16), rtc::CreateRandomString(22), std::string(),
false, rtc::nullopt));
ASSERT_TRUE(stun_port_ != NULL);
ServerAddresses stun_servers;
stun_servers.insert(server_addr);

6
p2p/base/turnport_unittest.cc
View File

@ -321,9 +321,9 @@ class TurnPortTest : public testing::Test,
void CreateUdpPort() { CreateUdpPort(kLocalAddr2); }
void CreateUdpPort(const SocketAddress& address) {
udp_port_.reset(UDPPort::Create(&main_, &socket_factory_,
MakeNetwork(address), 0, 0, kIceUfrag2,
kIcePwd2, std::string(), false));
udp_port_.reset(UDPPort::Create(
&main_, &socket_factory_, MakeNetwork(address), 0, 0, kIceUfrag2,
kIcePwd2, std::string(), false, rtc::nullopt));
// UDP port will be controlled.
udp_port_->SetIceRole(ICEROLE_CONTROLLED);
udp_port_->SignalPortComplete.connect(

17
p2p/client/basicportallocator.cc
View File

@ -433,7 +433,11 @@ void BasicPortAllocatorSession::SetStunKeepaliveIntervalForReadyPorts(
const rtc::Optional<int>& stun_keepalive_interval) {
auto ports = ReadyPorts();
for (PortInterface* port : ports) {
if (port->Type() == STUN_PORT_TYPE) {
// The port type and protocol can be used to identify different subclasses
// of Port in the current implementation. Note that a TCPPort has the type
// LOCAL_PORT_TYPE but uses the protocol PROTO_TCP.
if (port->Type() == STUN_PORT_TYPE ||
(port->Type() == LOCAL_PORT_TYPE && port->GetProtocol() == PROTO_UDP)) {
static_cast<UDPPort*>(port)->set_stun_keepalive_delay(
stun_keepalive_interval);
}
@ -1297,13 +1301,15 @@ void AllocationSequence::CreateUDPPorts() {
port = UDPPort::Create(
session_->network_thread(), session_->socket_factory(), network_,
udp_socket_.get(), session_->username(), session_->password(),
session_->allocator()->origin(), emit_local_candidate_for_anyaddress);
session_->allocator()->origin(), emit_local_candidate_for_anyaddress,
session_->allocator()->stun_candidate_keepalive_interval());
} else {
port = UDPPort::Create(
session_->network_thread(), session_->socket_factory(), network_,
session_->allocator()->min_port(), session_->allocator()->max_port(),
session_->username(), session_->password(),
session_->allocator()->origin(), emit_local_candidate_for_anyaddress);
session_->allocator()->origin(), emit_local_candidate_for_anyaddress,
session_->allocator()->stun_candidate_keepalive_interval());
}
if (port) {
@ -1366,10 +1372,9 @@ void AllocationSequence::CreateStunPorts() {
session_->network_thread(), session_->socket_factory(), network_,
session_->allocator()->min_port(), session_->allocator()->max_port(),
session_->username(), session_->password(), config_->StunServers(),
session_->allocator()->origin());
session_->allocator()->origin(),
session_->allocator()->stun_candidate_keepalive_interval());
if (port) {
port->set_stun_keepalive_delay(
session_->allocator()->stun_candidate_keepalive_interval());
session_->AddAllocatedPort(port, this, true);
// Since StunPort is not created using shared socket, |port| will not be
// added to the dequeue.

90
p2p/client/basicportallocator_unittest.cc
View File

@ -14,6 +14,7 @@
#include "p2p/base/basicpacketsocketfactory.h"
#include "p2p/base/p2pconstants.h"
#include "p2p/base/p2ptransportchannel.h"
#include "p2p/base/stunport.h"
#include "p2p/base/testrelayserver.h"
#include "p2p/base/teststunserver.h"
#include "p2p/base/testturnserver.h"
@ -97,6 +98,25 @@ static const char kTurnPassword[] = "test";
// Add some margin of error for slow bots.
static const int kStunTimeoutMs = cricket::STUN_TOTAL_TIMEOUT;
namespace {
void CheckStunKeepaliveIntervalOfAllReadyPorts(
const cricket::PortAllocatorSession* allocator_session,
int expected) {
auto ready_ports = allocator_session->ReadyPorts();
for (const auto* port : ready_ports) {
if (port->Type() == cricket::STUN_PORT_TYPE ||
(port->Type() == cricket::LOCAL_PORT_TYPE &&
port->GetProtocol() == cricket::PROTO_UDP)) {
EXPECT_EQ(
static_cast<const cricket::UDPPort*>(port)->stun_keepalive_delay(),
expected);
}
}
}
} // namespace
namespace cricket {
// Helper for dumping candidates
@ -2100,4 +2120,74 @@ TEST_F(BasicPortAllocatorTest, TestSetCandidateFilterAfterCandidatesGathered) {
}
}
TEST_F(BasicPortAllocatorTest, SetStunKeepaliveIntervalForPorts) {
const int pool_size = 1;
const int expected_stun_keepalive_interval = 123;
AddInterface(kClientAddr);
allocator_->SetConfiguration(allocator_->stun_servers(),
allocator_->turn_servers(), pool_size, false,
nullptr, expected_stun_keepalive_interval);
auto* pooled_session = allocator_->GetPooledSession();
ASSERT_NE(nullptr, pooled_session);
EXPECT_EQ_SIMULATED_WAIT(true, pooled_session->CandidatesAllocationDone(),
kDefaultAllocationTimeout, fake_clock);
CheckStunKeepaliveIntervalOfAllReadyPorts(pooled_session,
expected_stun_keepalive_interval);
}
TEST_F(BasicPortAllocatorTest,
ChangeStunKeepaliveIntervalForPortsAfterInitialConfig) {
const int pool_size = 1;
AddInterface(kClientAddr);
allocator_->SetConfiguration(allocator_->stun_servers(),
allocator_->turn_servers(), pool_size, false,
nullptr, 123 /* stun keepalive interval */);
auto* pooled_session = allocator_->GetPooledSession();
ASSERT_NE(nullptr, pooled_session);
EXPECT_EQ_SIMULATED_WAIT(true, pooled_session->CandidatesAllocationDone(),
kDefaultAllocationTimeout, fake_clock);
const int expected_stun_keepalive_interval = 321;
allocator_->SetConfiguration(allocator_->stun_servers(),
allocator_->turn_servers(), pool_size, false,
nullptr, expected_stun_keepalive_interval);
CheckStunKeepaliveIntervalOfAllReadyPorts(pooled_session,
expected_stun_keepalive_interval);
}
TEST_F(BasicPortAllocatorTest,
SetStunKeepaliveIntervalForPortsWithSharedSocket) {
const int pool_size = 1;
const int expected_stun_keepalive_interval = 123;
AddInterface(kClientAddr);
allocator_->set_flags(allocator().flags() |
PORTALLOCATOR_ENABLE_SHARED_SOCKET);
allocator_->SetConfiguration(allocator_->stun_servers(),
allocator_->turn_servers(), pool_size, false,
nullptr, expected_stun_keepalive_interval);
EXPECT_TRUE(CreateSession(ICE_CANDIDATE_COMPONENT_RTP));
session_->StartGettingPorts();
EXPECT_TRUE_SIMULATED_WAIT(candidate_allocation_done_,
kDefaultAllocationTimeout, fake_clock);
CheckStunKeepaliveIntervalOfAllReadyPorts(session_.get(),
expected_stun_keepalive_interval);
}
TEST_F(BasicPortAllocatorTest,
SetStunKeepaliveIntervalForPortsWithoutSharedSocket) {
const int pool_size = 1;
const int expected_stun_keepalive_interval = 123;
AddInterface(kClientAddr);
allocator_->set_flags(allocator().flags() &
~(PORTALLOCATOR_ENABLE_SHARED_SOCKET));
allocator_->SetConfiguration(allocator_->stun_servers(),
allocator_->turn_servers(), pool_size, false,
nullptr, expected_stun_keepalive_interval);
EXPECT_TRUE(CreateSession(ICE_CANDIDATE_COMPONENT_RTP));
session_->StartGettingPorts();
EXPECT_TRUE_SIMULATED_WAIT(candidate_allocation_done_,
kDefaultAllocationTimeout, fake_clock);
CheckStunKeepaliveIntervalOfAllReadyPorts(session_.get(),
expected_stun_keepalive_interval);
}
} // namespace cricket