diff --git a/webrtc/p2p/base/fakeportallocator.h b/webrtc/p2p/base/fakeportallocator.h index 58e1d5fac6..0bb9c6e66c 100644 --- a/webrtc/p2p/base/fakeportallocator.h +++ b/webrtc/p2p/base/fakeportallocator.h @@ -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_; diff --git a/webrtc/p2p/base/p2ptransportchannel.cc b/webrtc/p2p/base/p2ptransportchannel.cc index 1e565ececd..f506126cef 100644 --- a/webrtc/p2p/base/p2ptransportchannel.cc +++ b/webrtc/p2p/base/p2ptransportchannel.cc @@ -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::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::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(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"; diff --git a/webrtc/p2p/base/p2ptransportchannel.h b/webrtc/p2p/base/p2ptransportchannel.h index bac7093f4c..3f0191e83d 100644 --- a/webrtc/p2p/base/p2ptransportchannel.h +++ b/webrtc/p2p/base/p2ptransportchannel.h @@ -301,7 +301,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 b13fac29b5..6c26704388 100644 --- a/webrtc/p2p/base/p2ptransportchannel_unittest.cc +++ b/webrtc/p2p/base/p2ptransportchannel_unittest.cc @@ -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: