From fc43d11717e16dd427ac84fee614e5511e43cefd Mon Sep 17 00:00:00 2001 From: Qingsi Wang Date: Tue, 10 Apr 2018 15:50:46 -0700 Subject: [PATCH] Add thread checker to PortAllocator and its subclasses and fix a bug causing memory contention by threads. PortAllocator and its subclasses assume all of their methods except the constructor must be called on the same thread (the network thread in practice). This CL adds a thread checker to PortAllocator and its subclasses for thread safety, and fixes bugs of invoking some of their methods in PeerConnection on the signaling thread. Bug: webrtc:9112 Change-Id: I33ba9bae72ec09a45ec70435962f3f25cd31583c Reviewed-on: https://webrtc-review.googlesource.com/66945 Commit-Queue: Qingsi Wang Reviewed-by: Taylor Brandstetter Cr-Commit-Position: refs/heads/master@{#22814} --- p2p/base/fakeportallocator.h | 13 +-- p2p/base/p2ptransportchannel_unittest.cc | 1 + p2p/base/portallocator.cc | 21 +++- p2p/base/portallocator.h | 130 ++++++++++++++++++---- p2p/client/basicportallocator.cc | 4 + p2p/client/basicportallocator.h | 16 ++- p2p/client/basicportallocator_unittest.cc | 10 +- pc/peerconnection.cc | 80 +++++++------ pc/peerconnection.h | 3 + pc/peerconnection_integrationtest.cc | 10 +- pc/peerconnectioninterface_unittest.cc | 25 ----- 11 files changed, 215 insertions(+), 98 deletions(-) diff --git a/p2p/base/fakeportallocator.h b/p2p/base/fakeportallocator.h index 3f0a1d4f8c..76df3df693 100644 --- a/p2p/base/fakeportallocator.h +++ b/p2p/base/fakeportallocator.h @@ -18,11 +18,12 @@ #include "p2p/base/basicpacketsocketfactory.h" #include "p2p/base/portallocator.h" #include "p2p/base/udpport.h" +#include "rtc_base/bind.h" #include "rtc_base/nethelpers.h" +#include "rtc_base/thread.h" namespace rtc { class SocketFactory; -class Thread; } namespace cricket { @@ -219,12 +220,9 @@ class FakePortAllocator : public cricket::PortAllocator { owned_factory_.reset(new rtc::BasicPacketSocketFactory(network_thread_)); factory_ = owned_factory_.get(); } - } - - void Initialize() override { - // Port allocator should be initialized on the network thread. - RTC_CHECK(network_thread_->IsCurrent()); - initialized_ = true; + network_thread_->Invoke(RTC_FROM_HERE, + rtc::Bind(&PortAllocator::Initialize, + static_cast(this))); } void SetNetworkIgnoreMask(int network_ignore_mask) override {} @@ -245,7 +243,6 @@ class FakePortAllocator : public cricket::PortAllocator { rtc::Thread* network_thread_; rtc::PacketSocketFactory* factory_; std::unique_ptr owned_factory_; - bool initialized_ = false; }; } // namespace cricket diff --git a/p2p/base/p2ptransportchannel_unittest.cc b/p2p/base/p2ptransportchannel_unittest.cc index 2b0efacc2d..9d9df530eb 100644 --- a/p2p/base/p2ptransportchannel_unittest.cc +++ b/p2p/base/p2ptransportchannel_unittest.cc @@ -155,6 +155,7 @@ cricket::BasicPortAllocator* CreateBasicPortAllocator( cricket::BasicPortAllocator* allocator = new cricket::BasicPortAllocator(network_manager); + allocator->Initialize(); allocator->SetConfiguration(stun_servers, turn_servers, 0, false); return allocator; } diff --git a/p2p/base/portallocator.cc b/p2p/base/portallocator.cc index e766fa732c..d9383610d6 100644 --- a/p2p/base/portallocator.cc +++ b/p2p/base/portallocator.cc @@ -107,9 +107,19 @@ PortAllocator::PortAllocator() max_ipv6_networks_(kDefaultMaxIPv6Networks), step_delay_(kDefaultStepDelay), allow_tcp_listen_(true), - candidate_filter_(CF_ALL) {} + candidate_filter_(CF_ALL) { + // The allocator will be attached to a thread in Initialize. + thread_checker_.DetachFromThread(); +} -PortAllocator::~PortAllocator() = default; +void PortAllocator::Initialize() { + RTC_DCHECK(thread_checker_.CalledOnValidThread()); + initialized_ = true; +} + +PortAllocator::~PortAllocator() { + CheckRunOnValidThreadIfInitialized(); +} bool PortAllocator::SetConfiguration( const ServerAddresses& stun_servers, @@ -118,6 +128,7 @@ bool PortAllocator::SetConfiguration( bool prune_turn_ports, webrtc::TurnCustomizer* turn_customizer, const rtc::Optional& stun_candidate_keepalive_interval) { + CheckRunOnValidThreadIfInitialized(); bool ice_servers_changed = (stun_servers != stun_servers_ || turn_servers != turn_servers_); stun_servers_ = stun_servers; @@ -181,6 +192,7 @@ std::unique_ptr PortAllocator::CreateSession( int component, const std::string& ice_ufrag, const std::string& ice_pwd) { + CheckRunOnValidThreadAndInitialized(); auto session = std::unique_ptr( CreateSessionInternal(content_name, component, ice_ufrag, ice_pwd)); session->SetCandidateFilter(candidate_filter()); @@ -192,6 +204,7 @@ std::unique_ptr PortAllocator::TakePooledSession( int component, const std::string& ice_ufrag, const std::string& ice_pwd) { + CheckRunOnValidThreadAndInitialized(); RTC_DCHECK(!ice_ufrag.empty()); RTC_DCHECK(!ice_pwd.empty()); if (pooled_sessions_.empty()) { @@ -208,6 +221,7 @@ std::unique_ptr PortAllocator::TakePooledSession( } const PortAllocatorSession* PortAllocator::GetPooledSession() const { + CheckRunOnValidThreadAndInitialized(); if (pooled_sessions_.empty()) { return nullptr; } @@ -215,15 +229,18 @@ const PortAllocatorSession* PortAllocator::GetPooledSession() const { } void PortAllocator::FreezeCandidatePool() { + CheckRunOnValidThreadAndInitialized(); candidate_pool_frozen_ = true; } void PortAllocator::DiscardCandidatePool() { + CheckRunOnValidThreadIfInitialized(); pooled_sessions_.clear(); } void PortAllocator::GetCandidateStatsFromPooledSessions( CandidateStatsList* candidate_stats_list) { + CheckRunOnValidThreadAndInitialized(); for (const auto& session : pooled_sessions()) { session->GetCandidateStatsFromReadyPorts(candidate_stats_list); } diff --git a/p2p/base/portallocator.h b/p2p/base/portallocator.h index 95052c2db1..f985b2171b 100644 --- a/p2p/base/portallocator.h +++ b/p2p/base/portallocator.h @@ -22,6 +22,7 @@ #include "rtc_base/proxyinfo.h" #include "rtc_base/sigslot.h" #include "rtc_base/thread.h" +#include "rtc_base/thread_checker.h" namespace webrtc { class MetricsObserverInterface; @@ -326,19 +327,18 @@ class PortAllocatorSession : public sigslot::has_slots<> { }; // Every method of PortAllocator (including the destructor) must be called on -// the same thread, except for the constructor which may be called on any -// thread. +// the same thread after Initialize is called. // -// This allows constructing a PortAllocator subclass on one thread and -// passing it into an object that uses it on a different thread. +// This allows a PortAllocator subclass to be constructed and configured on one +// thread, and passed into an object that uses it on a different thread. class PortAllocator : public sigslot::has_slots<> { public: PortAllocator(); ~PortAllocator() override; - // This should be called on the PortAllocator's thread before the - // PortAllocator is used. Subclasses may override this if necessary. - virtual void Initialize() {} + // This MUST be called on the PortAllocator's thread after finishing + // constructing and configuring the PortAllocator subclasses. + virtual void Initialize(); // Set STUN and TURN servers to be used in future sessions, and set // candidate pool size, as described in JSEP. @@ -360,14 +360,23 @@ class PortAllocator : public sigslot::has_slots<> { const rtc::Optional& stun_candidate_keepalive_interval = rtc::nullopt); - const ServerAddresses& stun_servers() const { return stun_servers_; } + const ServerAddresses& stun_servers() const { + CheckRunOnValidThreadIfInitialized(); + return stun_servers_; + } const std::vector& turn_servers() const { + CheckRunOnValidThreadIfInitialized(); return turn_servers_; } - int candidate_pool_size() const { return candidate_pool_size_; } + int candidate_pool_size() const { + CheckRunOnValidThreadIfInitialized(); + return candidate_pool_size_; + } + const rtc::Optional& stun_candidate_keepalive_interval() const { + CheckRunOnValidThreadIfInitialized(); return stun_candidate_keepalive_interval_; } @@ -410,23 +419,48 @@ class PortAllocator : public sigslot::has_slots<> { // Discard any remaining pooled sessions. void DiscardCandidatePool(); - uint32_t flags() const { return flags_; } - void set_flags(uint32_t flags) { flags_ = flags; } + uint32_t flags() const { + CheckRunOnValidThreadIfInitialized(); + return flags_; + } + + void set_flags(uint32_t flags) { + CheckRunOnValidThreadIfInitialized(); + flags_ = flags; + } // These three methods are deprecated. If connections need to go through a // proxy, the application should create a BasicPortAllocator given a custom // PacketSocketFactory that creates proxy sockets. - const std::string& user_agent() const { return agent_; } - const rtc::ProxyInfo& proxy() const { return proxy_; } + const std::string& user_agent() const { + CheckRunOnValidThreadIfInitialized(); + return agent_; + } + + const rtc::ProxyInfo& proxy() const { + CheckRunOnValidThreadIfInitialized(); + return proxy_; + } + void set_proxy(const std::string& agent, const rtc::ProxyInfo& proxy) { + CheckRunOnValidThreadIfInitialized(); agent_ = agent; proxy_ = proxy; } // Gets/Sets the port range to use when choosing client ports. - int min_port() const { return min_port_; } - int max_port() const { return max_port_; } + int min_port() const { + CheckRunOnValidThreadIfInitialized(); + return min_port_; + } + + int max_port() const { + CheckRunOnValidThreadIfInitialized(); + return max_port_; + } + bool SetPortRange(int min_port, int max_port) { + CheckRunOnValidThreadIfInitialized(); if (min_port > max_port) { return false; } @@ -444,8 +478,15 @@ class PortAllocator : public sigslot::has_slots<> { // ICE down. We should work on making our ICE logic smarter (for example, // prioritizing pinging connections that are most likely to work) so that // every network interface can be used without impacting ICE's speed. - void set_max_ipv6_networks(int networks) { max_ipv6_networks_ = networks; } - int max_ipv6_networks() { return max_ipv6_networks_; } + void set_max_ipv6_networks(int networks) { + CheckRunOnValidThreadIfInitialized(); + max_ipv6_networks_ = networks; + } + + int max_ipv6_networks() { + CheckRunOnValidThreadIfInitialized(); + return max_ipv6_networks_; + } // Delay between different candidate gathering phases (UDP, TURN, TCP). // Defaults to 1 second, but PeerConnection sets it to 50ms. @@ -453,30 +494,59 @@ class PortAllocator : public sigslot::has_slots<> { // STUN transactions at once, but that's already happening if you configure // multiple STUN servers or have multiple network interfaces. We should // implement some global pacing logic instead if that's our goal. - uint32_t step_delay() const { return step_delay_; } - void set_step_delay(uint32_t delay) { step_delay_ = delay; } + uint32_t step_delay() const { + CheckRunOnValidThreadIfInitialized(); + return step_delay_; + } + + void set_step_delay(uint32_t delay) { + CheckRunOnValidThreadIfInitialized(); + step_delay_ = delay; + } + + bool allow_tcp_listen() const { + CheckRunOnValidThreadIfInitialized(); + return allow_tcp_listen_; + } - bool allow_tcp_listen() const { return allow_tcp_listen_; } void set_allow_tcp_listen(bool allow_tcp_listen) { + CheckRunOnValidThreadIfInitialized(); allow_tcp_listen_ = allow_tcp_listen; } - uint32_t candidate_filter() { return candidate_filter_; } + uint32_t candidate_filter() { + CheckRunOnValidThreadIfInitialized(); + return candidate_filter_; + } + void set_candidate_filter(uint32_t filter) { + CheckRunOnValidThreadIfInitialized(); candidate_filter_ = filter; } - bool prune_turn_ports() const { return prune_turn_ports_; } + bool prune_turn_ports() const { + CheckRunOnValidThreadIfInitialized(); + return prune_turn_ports_; + } // Gets/Sets the Origin value used for WebRTC STUN requests. - const std::string& origin() const { return origin_; } - void set_origin(const std::string& origin) { origin_ = origin; } + const std::string& origin() const { + CheckRunOnValidThreadIfInitialized(); + return origin_; + } + + void set_origin(const std::string& origin) { + CheckRunOnValidThreadIfInitialized(); + origin_ = origin; + } void SetMetricsObserver(webrtc::MetricsObserverInterface* observer) { + CheckRunOnValidThreadIfInitialized(); metrics_observer_ = observer; } webrtc::TurnCustomizer* turn_customizer() { + CheckRunOnValidThreadIfInitialized(); return turn_customizer_; } @@ -503,6 +573,17 @@ class PortAllocator : public sigslot::has_slots<> { return pooled_sessions_; } + // The following thread checks are only done in DCHECK for the consistency + // with the exsiting thread checks. + void CheckRunOnValidThreadIfInitialized() const { + RTC_DCHECK(!initialized_ || thread_checker_.CalledOnValidThread()); + } + + void CheckRunOnValidThreadAndInitialized() const { + RTC_DCHECK(initialized_ && thread_checker_.CalledOnValidThread()); + } + + bool initialized_ = false; uint32_t flags_; std::string agent_; rtc::ProxyInfo proxy_; @@ -513,6 +594,7 @@ class PortAllocator : public sigslot::has_slots<> { bool allow_tcp_listen_; uint32_t candidate_filter_; std::string origin_; + rtc::ThreadChecker thread_checker_; private: ServerAddresses stun_servers_; diff --git a/p2p/client/basicportallocator.cc b/p2p/client/basicportallocator.cc index 1b3947b6cf..a25a937ddb 100644 --- a/p2p/client/basicportallocator.cc +++ b/p2p/client/basicportallocator.cc @@ -186,6 +186,7 @@ void BasicPortAllocator::OnIceRegathering(PortAllocatorSession* session, } BasicPortAllocator::~BasicPortAllocator() { + CheckRunOnValidThreadIfInitialized(); // Our created port allocator sessions depend on us, so destroy our remaining // pooled sessions before anything else. DiscardCandidatePool(); @@ -195,12 +196,14 @@ void BasicPortAllocator::SetNetworkIgnoreMask(int network_ignore_mask) { // TODO(phoglund): implement support for other types than loopback. // See https://code.google.com/p/webrtc/issues/detail?id=4288. // Then remove set_network_ignore_list from NetworkManager. + CheckRunOnValidThreadIfInitialized(); network_ignore_mask_ = network_ignore_mask; } PortAllocatorSession* BasicPortAllocator::CreateSessionInternal( const std::string& content_name, int component, const std::string& ice_ufrag, const std::string& ice_pwd) { + CheckRunOnValidThreadAndInitialized(); PortAllocatorSession* session = new BasicPortAllocatorSession( this, content_name, component, ice_ufrag, ice_pwd); session->SignalIceRegathering.connect(this, @@ -209,6 +212,7 @@ PortAllocatorSession* BasicPortAllocator::CreateSessionInternal( } void BasicPortAllocator::AddTurnServer(const RelayServerConfig& turn_server) { + CheckRunOnValidThreadAndInitialized(); std::vector new_turn_servers = turn_servers(); new_turn_servers.push_back(turn_server); SetConfiguration(stun_servers(), new_turn_servers, candidate_pool_size(), diff --git a/p2p/client/basicportallocator.h b/p2p/client/basicportallocator.h index fb74ba5d74..1822b7da2f 100644 --- a/p2p/client/basicportallocator.h +++ b/p2p/client/basicportallocator.h @@ -47,13 +47,22 @@ class BasicPortAllocator : public PortAllocator { // Set to kDefaultNetworkIgnoreMask by default. void SetNetworkIgnoreMask(int network_ignore_mask) override; - int network_ignore_mask() const { return network_ignore_mask_; } + int network_ignore_mask() const { + CheckRunOnValidThreadIfInitialized(); + return network_ignore_mask_; + } - rtc::NetworkManager* network_manager() const { return network_manager_; } + rtc::NetworkManager* network_manager() const { + CheckRunOnValidThreadIfInitialized(); + return network_manager_; + } // If socket_factory() is set to NULL each PortAllocatorSession // creates its own socket factory. - rtc::PacketSocketFactory* socket_factory() { return socket_factory_; } + rtc::PacketSocketFactory* socket_factory() { + CheckRunOnValidThreadIfInitialized(); + return socket_factory_; + } PortAllocatorSession* CreateSessionInternal( const std::string& content_name, @@ -65,6 +74,7 @@ class BasicPortAllocator : public PortAllocator { void AddTurnServer(const RelayServerConfig& turn_server); RelayPortFactoryInterface* relay_port_factory() { + CheckRunOnValidThreadIfInitialized(); return relay_port_factory_; } diff --git a/p2p/client/basicportallocator_unittest.cc b/p2p/client/basicportallocator_unittest.cc index f39e61e3ee..6ed13b5798 100644 --- a/p2p/client/basicportallocator_unittest.cc +++ b/p2p/client/basicportallocator_unittest.cc @@ -166,6 +166,7 @@ class BasicPortAllocatorTestBase : public testing::Test, allocator_.reset(new BasicPortAllocator(&network_manager_, stun_servers, kRelayUdpIntAddr, kRelayTcpIntAddr, kRelaySslTcpIntAddr)); + allocator_->Initialize(); allocator_->set_step_delay(kMinimumStepDelay); } @@ -201,6 +202,7 @@ class BasicPortAllocatorTestBase : public testing::Test, // Endpoint is on the public network. No STUN or TURN. void ResetWithNoServersOrNat() { allocator_.reset(new BasicPortAllocator(&network_manager_)); + allocator_->Initialize(); allocator_->set_step_delay(kMinimumStepDelay); } // Endpoint is behind a NAT, with STUN specified. @@ -483,7 +485,8 @@ class BasicPortAllocatorTestBase : public testing::Test, } allocator_.reset(new BasicPortAllocator( &network_manager_, nat_socket_factory_.get(), stun_servers)); - allocator().set_step_delay(kMinimumStepDelay); + allocator_->Initialize(); + allocator_->set_step_delay(kMinimumStepDelay); } std::unique_ptr vss_; @@ -579,6 +582,7 @@ class BasicPortAllocatorTest : public FakeClockBase, AddInterface(kClientAddr, "net1"); AddInterface(kClientIPv6Addr, "net1"); allocator_.reset(new BasicPortAllocator(&network_manager_)); + allocator_->Initialize(); allocator_->SetConfiguration(allocator_->stun_servers(), allocator_->turn_servers(), 0, true); AddTurnServers(kTurnUdpIntIPv6Addr, rtc::SocketAddress()); @@ -616,6 +620,7 @@ class BasicPortAllocatorTest : public FakeClockBase, turn_server_.AddInternalSocket(kTurnTcpIntAddr, PROTO_TCP); AddInterface(kClientAddr); allocator_.reset(new BasicPortAllocator(&network_manager_)); + allocator_->Initialize(); allocator_->SetConfiguration(allocator_->stun_servers(), allocator_->turn_servers(), 0, true); AddTurnServers(kTurnUdpIntAddr, kTurnTcpIntAddr); @@ -662,6 +667,7 @@ class BasicPortAllocatorTest : public FakeClockBase, AddInterface(kClientAddr2, "net2", rtc::ADAPTER_TYPE_CELLULAR); AddInterface(kClientIPv6Addr2, "net2", rtc::ADAPTER_TYPE_CELLULAR); allocator_.reset(new BasicPortAllocator(&network_manager_)); + allocator_->Initialize(); allocator_->SetConfiguration(allocator_->stun_servers(), allocator_->turn_servers(), 0, true); // Have both UDP/TCP and IPv4/IPv6 TURN ports. @@ -1651,6 +1657,7 @@ TEST_F(BasicPortAllocatorTest, TestSharedSocketWithoutNatUsingTurn) { turn_server_.AddInternalSocket(kTurnTcpIntAddr, PROTO_TCP); AddInterface(kClientAddr); allocator_.reset(new BasicPortAllocator(&network_manager_)); + allocator_->Initialize(); AddTurnServers(kTurnUdpIntAddr, kTurnTcpIntAddr); @@ -1755,6 +1762,7 @@ TEST_F(BasicPortAllocatorTestWithRealClock, PROTO_UDP); AddInterface(kClientAddr); allocator_.reset(new BasicPortAllocator(&network_manager_)); + allocator_->Initialize(); RelayServerConfig turn_server(RELAY_TURN); RelayCredentials credentials(kTurnUsername, kTurnPassword); turn_server.credentials = credentials; diff --git a/pc/peerconnection.cc b/pc/peerconnection.cc index 1f63874e3b..90242a87a7 100644 --- a/pc/peerconnection.cc +++ b/pc/peerconnection.cc @@ -1980,11 +1980,6 @@ void PeerConnection::SetLocalDescription( PostSetSessionDescriptionSuccess(observer); - // According to JSEP, after setLocalDescription, changing the candidate pool - // size is not allowed, and changing the set of ICE servers will not result - // in new candidates being gathered. - port_allocator_->FreezeCandidatePool(); - // MaybeStartGathering needs to be called after posting // MSG_SET_SESSIONDESCRIPTION_SUCCESS, so that we don't signal any candidates // before signaling that SetLocalDescription completed. @@ -2821,6 +2816,9 @@ bool PeerConnection::SetConfiguration(const RTCConfiguration& configuration, RTCError* error) { TRACE_EVENT0("webrtc", "PeerConnection::SetConfiguration"); + // According to JSEP, after setLocalDescription, changing the candidate pool + // size is not allowed, and changing the set of ICE servers will not result + // in new candidates being gathered. if (local_description() && configuration.ice_candidate_pool_size != configuration_.ice_candidate_pool_size) { RTC_LOG(LS_ERROR) << "Can't change candidate pool size after calling " @@ -2978,23 +2976,12 @@ bool PeerConnection::RemoveIceCandidates( void PeerConnection::RegisterUMAObserver(UMAObserver* observer) { TRACE_EVENT0("webrtc", "PeerConnection::RegisterUmaObserver"); - uma_observer_ = observer; - - if (transport_controller()) { - transport_controller()->SetMetricsObserver(uma_observer_); - } - - for (auto transceiver : transceivers_) { - auto* channel = transceiver->internal()->channel(); - if (channel) { - channel->SetMetricsObserver(uma_observer_); - } - } - + network_thread()->Invoke( + RTC_FROM_HERE, + rtc::Bind(&PeerConnection::SetMetricObserver_n, this, observer)); // Send information about IPv4/IPv6 status. if (uma_observer_) { - port_allocator_->SetMetricsObserver(uma_observer_); - if (port_allocator_->flags() & cricket::PORTALLOCATOR_ENABLE_IPV6) { + if (port_allocator_flags_ & cricket::PORTALLOCATOR_ENABLE_IPV6) { uma_observer_->IncrementEnumCounter( kEnumCounterAddressFamily, kPeerConnection_IPv6, kPeerConnectionAddressFamilyCounter_Max); @@ -3027,6 +3014,25 @@ void PeerConnection::RegisterUMAObserver(UMAObserver* observer) { } } +void PeerConnection::SetMetricObserver_n(UMAObserver* observer) { + RTC_DCHECK(network_thread()->IsCurrent()); + uma_observer_ = observer; + if (transport_controller()) { + transport_controller()->SetMetricsObserver(uma_observer_); + } + + for (auto transceiver : transceivers_) { + auto* channel = transceiver->internal()->channel(); + if (channel) { + channel->SetMetricsObserver(uma_observer_); + } + } + + if (uma_observer_) { + port_allocator_->SetMetricsObserver(uma_observer_); + } +} + RTCError PeerConnection::SetBitrate(const BitrateParameters& bitrate) { if (!worker_thread()->IsCurrent()) { return worker_thread()->Invoke( @@ -4613,44 +4619,43 @@ bool PeerConnection::InitializePortAllocator_n( } port_allocator_->Initialize(); - // To handle both internal and externally created port allocator, we will // enable BUNDLE here. - int portallocator_flags = port_allocator_->flags(); - portallocator_flags |= cricket::PORTALLOCATOR_ENABLE_SHARED_SOCKET | - cricket::PORTALLOCATOR_ENABLE_IPV6 | - cricket::PORTALLOCATOR_ENABLE_IPV6_ON_WIFI; + port_allocator_flags_ = port_allocator_->flags(); + port_allocator_flags_ |= cricket::PORTALLOCATOR_ENABLE_SHARED_SOCKET | + cricket::PORTALLOCATOR_ENABLE_IPV6 | + cricket::PORTALLOCATOR_ENABLE_IPV6_ON_WIFI; // If the disable-IPv6 flag was specified, we'll not override it // by experiment. if (configuration.disable_ipv6) { - portallocator_flags &= ~(cricket::PORTALLOCATOR_ENABLE_IPV6); + port_allocator_flags_ &= ~(cricket::PORTALLOCATOR_ENABLE_IPV6); } else if (webrtc::field_trial::FindFullName("WebRTC-IPv6Default") .find("Disabled") == 0) { - portallocator_flags &= ~(cricket::PORTALLOCATOR_ENABLE_IPV6); + port_allocator_flags_ &= ~(cricket::PORTALLOCATOR_ENABLE_IPV6); } if (configuration.disable_ipv6_on_wifi) { - portallocator_flags &= ~(cricket::PORTALLOCATOR_ENABLE_IPV6_ON_WIFI); + port_allocator_flags_ &= ~(cricket::PORTALLOCATOR_ENABLE_IPV6_ON_WIFI); RTC_LOG(LS_INFO) << "IPv6 candidates on Wi-Fi are disabled."; } if (configuration.tcp_candidate_policy == kTcpCandidatePolicyDisabled) { - portallocator_flags |= cricket::PORTALLOCATOR_DISABLE_TCP; + port_allocator_flags_ |= cricket::PORTALLOCATOR_DISABLE_TCP; RTC_LOG(LS_INFO) << "TCP candidates are disabled."; } if (configuration.candidate_network_policy == kCandidateNetworkPolicyLowCost) { - portallocator_flags |= cricket::PORTALLOCATOR_DISABLE_COSTLY_NETWORKS; + port_allocator_flags_ |= cricket::PORTALLOCATOR_DISABLE_COSTLY_NETWORKS; RTC_LOG(LS_INFO) << "Do not gather candidates on high-cost networks"; } if (configuration.disable_link_local_networks) { - portallocator_flags |= cricket::PORTALLOCATOR_DISABLE_LINK_LOCAL_NETWORKS; + port_allocator_flags_ |= cricket::PORTALLOCATOR_DISABLE_LINK_LOCAL_NETWORKS; RTC_LOG(LS_INFO) << "Disable candidates on link-local network interfaces."; } - port_allocator_->set_flags(portallocator_flags); + port_allocator_->set_flags(port_allocator_flags_); // No step delay is used while allocating ports. port_allocator_->set_step_delay(cricket::kMinimumStepDelay); port_allocator_->set_candidate_filter( @@ -4676,6 +4681,12 @@ bool PeerConnection::ReconfigurePortAllocator_n( rtc::Optional stun_candidate_keepalive_interval) { port_allocator_->set_candidate_filter( ConvertIceTransportTypeToCandidateFilter(type)); + // According to JSEP, after setLocalDescription, changing the candidate pool + // size is not allowed, and changing the set of ICE servers will not result + // in new candidates being gathered. + if (local_description()) { + port_allocator_->FreezeCandidatePool(); + } // Call this last since it may create pooled allocator sessions using the // candidate filter set above. return port_allocator_->SetConfiguration( @@ -5077,7 +5088,10 @@ rtc::Optional PeerConnection::sctp_transport_name() const { cricket::CandidateStatsList PeerConnection::GetPooledCandidateStats() const { cricket::CandidateStatsList candidate_states_list; - port_allocator_->GetCandidateStatsFromPooledSessions(&candidate_states_list); + network_thread()->Invoke( + RTC_FROM_HERE, + rtc::Bind(&cricket::PortAllocator::GetCandidateStatsFromPooledSessions, + port_allocator_.get(), &candidate_states_list)); return candidate_states_list; } diff --git a/pc/peerconnection.h b/pc/peerconnection.h index fa46a4e151..401ffd49b4 100644 --- a/pc/peerconnection.h +++ b/pc/peerconnection.h @@ -654,6 +654,8 @@ class PeerConnection : public PeerConnectionInternal, webrtc::TurnCustomizer* turn_customizer, rtc::Optional stun_candidate_keepalive_interval); + void SetMetricObserver_n(UMAObserver* observer); + // Starts output of an RTC event log to the given output object. // This function should only be called from the worker thread. bool StartRtcEventLog_w(std::unique_ptr output, @@ -904,6 +906,7 @@ class PeerConnection : public PeerConnectionInternal, PeerConnectionInterface::RTCConfiguration configuration_; std::unique_ptr port_allocator_; + int port_allocator_flags_ = 0; // One PeerConnection has only one RTCP CNAME. // https://tools.ietf.org/html/draft-ietf-rtcweb-rtp-usage-26#section-4.9 diff --git a/pc/peerconnection_integrationtest.cc b/pc/peerconnection_integrationtest.cc index f7ac830a65..f05ece4efb 100644 --- a/pc/peerconnection_integrationtest.cc +++ b/pc/peerconnection_integrationtest.cc @@ -3182,8 +3182,14 @@ class PeerConnectionIntegrationIceStatesTest } void SetPortAllocatorFlags() { - caller()->port_allocator()->set_flags(port_allocator_flags_); - callee()->port_allocator()->set_flags(port_allocator_flags_); + network_thread()->Invoke( + RTC_FROM_HERE, + rtc::Bind(&cricket::PortAllocator::set_flags, + caller()->port_allocator(), port_allocator_flags_)); + network_thread()->Invoke( + RTC_FROM_HERE, + rtc::Bind(&cricket::PortAllocator::set_flags, + callee()->port_allocator(), port_allocator_flags_)); } std::vector CallerAddresses() { diff --git a/pc/peerconnectioninterface_unittest.cc b/pc/peerconnectioninterface_unittest.cc index 8ee39c945b..acb0dbdd04 100644 --- a/pc/peerconnectioninterface_unittest.cc +++ b/pc/peerconnectioninterface_unittest.cc @@ -1414,31 +1414,6 @@ TEST_P(PeerConnectionInterfaceTest, EXPECT_TRUE(raw_port_allocator->prune_turn_ports()); } -// Test that the PeerConnection initializes the port allocator passed into it, -// and on the correct thread. -TEST_P(PeerConnectionInterfaceTest, - CreatePeerConnectionInitializesPortAllocatorOnNetworkThread) { - std::unique_ptr network_thread( - rtc::Thread::CreateWithSocketServer()); - network_thread->Start(); - rtc::scoped_refptr pc_factory( - webrtc::CreatePeerConnectionFactory( - network_thread.get(), rtc::Thread::Current(), rtc::Thread::Current(), - fake_audio_capture_module_, - webrtc::CreateBuiltinAudioEncoderFactory(), - webrtc::CreateBuiltinAudioDecoderFactory(), nullptr, nullptr)); - std::unique_ptr port_allocator( - new cricket::FakePortAllocator(network_thread.get(), nullptr)); - cricket::FakePortAllocator* raw_port_allocator = port_allocator.get(); - PeerConnectionInterface::RTCConfiguration config; - rtc::scoped_refptr pc( - pc_factory->CreatePeerConnection( - config, nullptr, std::move(port_allocator), nullptr, &observer_)); - // FakePortAllocator RTC_CHECKs that it's initialized on the right thread, - // so all we have to do here is check that it's initialized. - EXPECT_TRUE(raw_port_allocator->initialized()); -} - // Check that GetConfiguration returns the configuration the PeerConnection was // constructed with, before SetConfiguration is called. TEST_P(PeerConnectionInterfaceTest, GetConfigurationAfterCreatePeerConnection) {