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: