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.

Review-Url: https://codereview.webrtc.org/2053043003
Cr-Commit-Position: refs/heads/master@{#13226}
This commit is contained in:
deadbeef 2016-06-20 12:55:52 -07:00 committed by Commit bot
parent ce89cbd04a
commit 370544594e
3 changed files with 77 additions and 13 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

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

@ -279,6 +279,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));
@ -305,16 +306,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;
@ -967,13 +972,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;
@ -1513,10 +1518,9 @@ 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);
ports_.erase(std::remove(ports_.begin(), ports_.end(), port));
removed_ports_.erase(
std::remove(removed_ports_.begin(), removed_ports_.end(), port));
LOG(INFO) << "Removed port from p2p socket: "
<< static_cast<int>(ports_.size()) << " remaining";
@ -1533,6 +1537,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

@ -297,7 +297,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

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

@ -2686,6 +2686,58 @@ 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());
}
class P2PTransportChannelMostLikelyToWorkFirstTest
: public P2PTransportChannelPingTest {
public: