From 370544594e18deb7f560f961295c8cf3f0a679f1 Mon Sep 17 00:00:00 2001 From: deadbeef Date: Mon, 20 Jun 2016 12:55:52 -0700 Subject: [PATCH] Update ICE role on all ports, not just ones used for new connections. 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} --- webrtc/p2p/base/p2ptransportchannel.cc | 29 ++++++----- webrtc/p2p/base/p2ptransportchannel.h | 9 +++- .../p2p/base/p2ptransportchannel_unittest.cc | 52 +++++++++++++++++++ 3 files changed, 77 insertions(+), 13 deletions(-) diff --git a/webrtc/p2p/base/p2ptransportchannel.cc b/webrtc/p2p/base/p2ptransportchannel.cc index d83726eab5..332d951a18 100644 --- a/webrtc/p2p/base/p2ptransportchannel.cc +++ b/webrtc/p2p/base/p2ptransportchannel.cc @@ -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::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::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(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"; diff --git a/webrtc/p2p/base/p2ptransportchannel.h b/webrtc/p2p/base/p2ptransportchannel.h index ada3ecfb8b..83f809b48a 100644 --- a/webrtc/p2p/base/p2ptransportchannel.h +++ b/webrtc/p2p/base/p2ptransportchannel.h @@ -297,7 +297,14 @@ class P2PTransportChannel : public TransportChannelImpl, bool incoming_only_; int error_; std::vector> allocator_sessions_; - std::vector ports_; + // |ports_| contains ports that are used to form new connections when + // new remote candidates are added. + std::vector 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 removed_ports_; // |connections_| is a sorted list with the first one always be the // |best_connection_| when it's not nullptr. The combination of diff --git a/webrtc/p2p/base/p2ptransportchannel_unittest.cc b/webrtc/p2p/base/p2ptransportchannel_unittest.cc index cbf5e76a7a..45add0c733 100644 --- a/webrtc/p2p/base/p2ptransportchannel_unittest.cc +++ b/webrtc/p2p/base/p2ptransportchannel_unittest.cc @@ -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: