admin/webrtc_m130
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Update ICE role on all ports, not just ones used for new connections.

Browse Source 
Previously, if the ICE role changed, SetIceRole was only called on
the ports from the most recent ICE generation. However, STUN pings
may still be sent and received by older generation ports, so they
should receive an updated role as well.

This was previously triggering an ASSERT, because a P2PTransportChannel
expects the ICE role of each of its ports to match its own role.

Committed: https://crrev.com/370544594e18deb7f560f961295c8cf3f0a679f1
Review-Url: https://codereview.webrtc.org/2053043003
Cr-Original-Commit-Position: refs/heads/master@{#13226}
Cr-Commit-Position: refs/heads/master@{#13247}
This commit is contained in:
deadbeef 2016-06-21 14:19:48 -07:00 committed by Commit bot
parent 123f33cd00
commit dfc4244d9e
4 changed files with 113 additions and 14 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

6
webrtc/p2p/base/fakeportallocator.h
View File

@ -132,6 +132,8 @@ class FakePortAllocatorSession : public PortAllocatorSession {
port_.reset(TestUDPPort::Create(worker_thread_, factory_, &network,
network.GetBestIP(), 0, 0, username(),
password(), std::string(), false));
port_->SignalDestroyed.connect(
this, &FakePortAllocatorSession::OnPortDestroyed);
AddPort(port_.get());
}
++port_config_count_;
@ -188,6 +190,10 @@ class FakePortAllocatorSession : public PortAllocatorSession {
allocation_done_ = true;
SignalCandidatesAllocationDone(this);
}
void OnPortDestroyed(cricket::PortInterface* port) {
// Don't want to double-delete port if it deletes itself.
port_.release();
}
rtc::Thread* worker_thread_;
rtc::PacketSocketFactory* factory_;

32
webrtc/p2p/base/p2ptransportchannel.cc
View File

@ -280,6 +280,7 @@ void P2PTransportChannel::AddAllocatorSession(
// We now only want to apply new candidates that we receive to the ports
// created by this new session because these are replacing those of the
// previous sessions.
removed_ports_.insert(removed_ports_.end(), ports_.begin(), ports_.end());
ports_.clear();
allocator_sessions_.push_back(std::move(session));
@ -306,16 +307,20 @@ void P2PTransportChannel::SetIceRole(IceRole ice_role) {
ASSERT(worker_thread_ == rtc::Thread::Current());
if (ice_role_ != ice_role) {
ice_role_ = ice_role;
for (std::vector<PortInterface *>::iterator it = ports_.begin();
it != ports_.end(); ++it) {
(*it)->SetIceRole(ice_role);
for (PortInterface* port : ports_) {
port->SetIceRole(ice_role);
}
// Update role on removed ports as well, because they may still have
// connections alive that should be using the correct role.
for (PortInterface* port : removed_ports_) {
port->SetIceRole(ice_role);
}
}
}
void P2PTransportChannel::SetIceTiebreaker(uint64_t tiebreaker) {
ASSERT(worker_thread_ == rtc::Thread::Current());
if (!ports_.empty()) {
if (!ports_.empty() || !removed_ports_.empty()) {
LOG(LS_ERROR)
<< "Attempt to change tiebreaker after Port has been allocated.";
return;
@ -973,13 +978,13 @@ int P2PTransportChannel::SetOption(rtc::Socket::Option opt, int value) {
it->second = value;
}
for (size_t i = 0; i < ports_.size(); ++i) {
int val = ports_[i]->SetOption(opt, value);
for (PortInterface* port : ports_) {
int val = port->SetOption(opt, value);
if (val < 0) {
// Because this also occurs deferred, probably no point in reporting an
// error
LOG(WARNING) << "SetOption(" << opt << ", " << value << ") failed: "
<< ports_[i]->GetError();
LOG(WARNING) << "SetOption(" << opt << ", " << value
<< ") failed: " << port->GetError();
}
}
return 0;
@ -1553,11 +1558,11 @@ void P2PTransportChannel::OnConnectionDestroyed(Connection* connection) {
void P2PTransportChannel::OnPortDestroyed(PortInterface* port) {
ASSERT(worker_thread_ == rtc::Thread::Current());
// Remove this port from the list (if we didn't drop it already).
std::vector<PortInterface*>::iterator iter =
std::find(ports_.begin(), ports_.end(), port);
if (iter != ports_.end())
ports_.erase(iter);
// Remove this port from the lists (if we didn't drop it already).
ports_.erase(std::remove(ports_.begin(), ports_.end(), port), ports_.end());
removed_ports_.erase(
std::remove(removed_ports_.begin(), removed_ports_.end(), port),
removed_ports_.end());
LOG(INFO) << "Removed port from p2p socket: "
<< static_cast<int>(ports_.size()) << " remaining";
@ -1574,6 +1579,7 @@ void P2PTransportChannel::OnPortNetworkInactive(PortInterface* port) {
if (it == ports_.end()) {
return;
}
removed_ports_.push_back(*it);
ports_.erase(it);
LOG(INFO) << "Removed port due to inactive networks: " << ports_.size()
<< " remaining";

9
webrtc/p2p/base/p2ptransportchannel.h
View File

@ -301,7 +301,14 @@ class P2PTransportChannel : public TransportChannelImpl,
bool incoming_only_;
int error_;
std::vector<std::unique_ptr<PortAllocatorSession>> allocator_sessions_;
std::vector<PortInterface *> ports_;
// |ports_| contains ports that are used to form new connections when
// new remote candidates are added.
std::vector<PortInterface*> ports_;
// |removed_ports_| contains ports that have been removed from |ports_| and
// are not being used to form new connections, but that aren't yet destroyed.
// They may have existing connections, and they still fire signals such as
// SignalUnknownAddress.
std::vector<PortInterface*> removed_ports_;
// |connections_| is a sorted list with the first one always be the
// |best_connection_| when it's not nullptr. The combination of

80
webrtc/p2p/base/p2ptransportchannel_unittest.cc
View File

@ -18,6 +18,7 @@
#include "webrtc/p2p/base/testturnserver.h"
#include "webrtc/p2p/client/basicportallocator.h"
#include "webrtc/base/dscp.h"
#include "webrtc/base/fakeclock.h"
#include "webrtc/base/fakenetwork.h"
#include "webrtc/base/firewallsocketserver.h"
#include "webrtc/base/gunit.h"
@ -2844,6 +2845,85 @@ TEST_F(P2PTransportChannelPingTest, TestStopPortAllocatorSessions) {
EXPECT_TRUE(!ch.allocator_session()->IsGettingPorts());
}
// Test that the ICE role is updated even on ports with inactive networks when
// doing continual gathering. These ports may still have connections that need
// a correct role, in case the network becomes active before the connection is
// destroyed.
TEST_F(P2PTransportChannelPingTest,
TestIceRoleUpdatedOnPortAfterSignalNetworkInactive) {
cricket::FakePortAllocator pa(rtc::Thread::Current(), nullptr);
cricket::P2PTransportChannel ch(
"test channel", cricket::ICE_CANDIDATE_COMPONENT_DEFAULT, &pa);
// Starts with ICEROLE_CONTROLLING.
PrepareChannel(&ch);
cricket::IceConfig config = CreateIceConfig(1000, true);
ch.SetIceConfig(config);
ch.Connect();
ch.MaybeStartGathering();
ch.AddRemoteCandidate(CreateHostCandidate("1.1.1.1", 1, 1));
cricket::Connection* conn = WaitForConnectionTo(&ch, "1.1.1.1", 1);
ASSERT_TRUE(conn != nullptr);
// Make the fake port signal that its network is inactive, then change the
// ICE role and expect it to be updated.
conn->port()->SignalNetworkInactive(conn->port());
ch.SetIceRole(cricket::ICEROLE_CONTROLLED);
EXPECT_EQ(cricket::ICEROLE_CONTROLLED, conn->port()->GetIceRole());
}
// Test that the ICE role is updated even on ports with inactive networks.
// These ports may still have connections that need a correct role, for the
// pings sent by those connections until they're replaced by newer-generation
// connections.
TEST_F(P2PTransportChannelPingTest, TestIceRoleUpdatedOnPortAfterIceRestart) {
cricket::FakePortAllocator pa(rtc::Thread::Current(), nullptr);
cricket::P2PTransportChannel ch(
"test channel", cricket::ICE_CANDIDATE_COMPONENT_DEFAULT, &pa);
// Starts with ICEROLE_CONTROLLING.
PrepareChannel(&ch);
ch.Connect();
ch.MaybeStartGathering();
ch.AddRemoteCandidate(CreateHostCandidate("1.1.1.1", 1, 1));
cricket::Connection* conn = WaitForConnectionTo(&ch, "1.1.1.1", 1);
ASSERT_TRUE(conn != nullptr);
// Do an ICE restart, change the role, and expect the old port to have its
// role updated.
ch.SetIceCredentials(kIceUfrag[1], kIcePwd[1]);
ch.MaybeStartGathering();
ch.SetIceRole(cricket::ICEROLE_CONTROLLED);
EXPECT_EQ(cricket::ICEROLE_CONTROLLED, conn->port()->GetIceRole());
}
// Test that after some amount of time without receiving data, the connection
// and port are destroyed.
TEST_F(P2PTransportChannelPingTest, TestPortDestroyedAfterTimeout) {
rtc::ScopedFakeClock fake_clock;
cricket::FakePortAllocator pa(rtc::Thread::Current(), nullptr);
cricket::P2PTransportChannel ch(
"test channel", cricket::ICE_CANDIDATE_COMPONENT_DEFAULT, &pa);
PrepareChannel(&ch);
// Only a controlled channel should expect its ports to be destroyed.
ch.SetIceRole(cricket::ICEROLE_CONTROLLED);
ch.Connect();
ch.MaybeStartGathering();
ch.AddRemoteCandidate(CreateHostCandidate("1.1.1.1", 1, 1));
cricket::Connection* conn = WaitForConnectionTo(&ch, "1.1.1.1", 1);
ASSERT_TRUE(conn != nullptr);
// Simulate 2 minutes going by. This should be enough time for the port to
// time out.
for (int second = 0; second < 120; ++second) {
fake_clock.AdvanceTime(rtc::TimeDelta::FromSeconds(1));
}
EXPECT_EQ(nullptr, GetConnectionTo(&ch, "1.1.1.1", 1));
EXPECT_EQ(nullptr, GetPort(&ch));
}
class P2PTransportChannelMostLikelyToWorkFirstTest
: public P2PTransportChannelPingTest {
public: