Replace a broken assumption in candidate gathering for mDNS candidates. The gathering of host candidates with mDNS names is asynchronous and its completion can happen after a srflx candidate is gathered by the same underlying socket. We have a broken check in UDPPort::CreateConnection() that assumes the gathering of host and srflx candidates is sequential. This CL also does minor refactoring and clean-up. Bug: chromium:944577 Change-Id: Ic28136a9515081f40b232a22fcbf4209814ed33a Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/138043 Reviewed-by: Steve Anton <steveanton@webrtc.org> Reviewed-by: Jeroen de Borst <jeroendb@webrtc.org> Reviewed-by: Amit Hilbuch <amithi@webrtc.org> Commit-Queue: Qingsi Wang <qingsi@webrtc.org> Cr-Commit-Position: refs/heads/master@{#28030}
diff --git a/p2p/base/p2p_transport_channel_unittest.cc b/p2p/base/p2p_transport_channel_unittest.cc index bab75de..623bb7c 100644 --- a/p2p/base/p2p_transport_channel_unittest.cc +++ b/p2p/base/p2p_transport_channel_unittest.cc
@@ -10,6 +10,7 @@ #include <list> #include <memory> +#include <utility> #include "absl/memory/memory.h" #include "p2p/base/fake_port_allocator.h" @@ -24,11 +25,13 @@ #include "rtc_base/checks.h" #include "rtc_base/dscp.h" #include "rtc_base/fake_clock.h" +#include "rtc_base/fake_mdns_responder.h" #include "rtc_base/fake_network.h" #include "rtc_base/firewall_socket_server.h" #include "rtc_base/gunit.h" #include "rtc_base/helpers.h" #include "rtc_base/logging.h" +#include "rtc_base/mdns_responder_interface.h" #include "rtc_base/nat_server.h" #include "rtc_base/nat_socket_factory.h" #include "rtc_base/proxy_server.h" @@ -4619,7 +4622,8 @@ ConfigureEndpoints(OPEN, OPEN, kOnlyLocalPorts, kOnlyLocalPorts); // ICE parameter will be set up when creating the channels. set_remote_ice_parameter_source(FROM_SETICEPARAMETERS); - GetEndpoint(0)->network_manager_.CreateMdnsResponder(rtc::Thread::Current()); + GetEndpoint(0)->network_manager_.set_mdns_responder( + absl::make_unique<webrtc::FakeMdnsResponder>(rtc::Thread::Current())); GetEndpoint(1)->async_resolver_factory_ = &mock_async_resolver_factory; CreateChannels(); // Pause sending candidates from both endpoints until we find out what port @@ -4690,7 +4694,8 @@ ConfigureEndpoints(OPEN, OPEN, kOnlyLocalPorts, kOnlyLocalPorts); // ICE parameter will be set up when creating the channels. set_remote_ice_parameter_source(FROM_SETICEPARAMETERS); - GetEndpoint(0)->network_manager_.CreateMdnsResponder(rtc::Thread::Current()); + GetEndpoint(0)->network_manager_.set_mdns_responder( + absl::make_unique<webrtc::FakeMdnsResponder>(rtc::Thread::Current())); GetEndpoint(1)->async_resolver_factory_ = &mock_async_resolver_factory; CreateChannels(); // Pause sending candidates from both endpoints until we find out what port @@ -4757,7 +4762,8 @@ ConfigureEndpoints(OPEN, OPEN, kOnlyLocalPorts, kOnlyLocalPorts); // ICE parameter will be set up when creating the channels. set_remote_ice_parameter_source(FROM_SETICEPARAMETERS); - GetEndpoint(0)->network_manager_.CreateMdnsResponder(rtc::Thread::Current()); + GetEndpoint(0)->network_manager_.set_mdns_responder( + absl::make_unique<webrtc::FakeMdnsResponder>(rtc::Thread::Current())); GetEndpoint(1)->async_resolver_factory_ = &mock_async_resolver_factory; CreateChannels(); // Pause sending candidates from both endpoints until we find out what port @@ -4813,7 +4819,8 @@ kOnlyLocalPorts); // ICE parameter will be set up when creating the channels. set_remote_ice_parameter_source(FROM_SETICEPARAMETERS); - GetEndpoint(0)->network_manager_.CreateMdnsResponder(rtc::Thread::Current()); + GetEndpoint(0)->network_manager_.set_mdns_responder( + absl::make_unique<webrtc::FakeMdnsResponder>(rtc::Thread::Current())); GetEndpoint(1)->async_resolver_factory_ = &mock_async_resolver_factory; CreateChannels(); // Pause sending candidates from both endpoints until we find out what port @@ -4891,6 +4898,50 @@ DestroyChannels(); } +class MockMdnsResponder : public webrtc::MdnsResponderInterface { + public: + MOCK_METHOD2(CreateNameForAddress, + void(const rtc::IPAddress&, NameCreatedCallback)); + MOCK_METHOD2(RemoveNameForAddress, + void(const rtc::IPAddress&, NameRemovedCallback)); +}; + +TEST_F(P2PTransportChannelTest, + SrflxCandidateCanBeGatheredBeforeMdnsCandidateToCreateConnection) { + // ep1 and ep2 will only gather host and srflx candidates with base addresses + // kPublicAddrs[0] and kPublicAddrs[1], respectively, and we use a shared + // socket in gathering. + const auto kOnlyLocalAndStunPorts = + cricket::PORTALLOCATOR_DISABLE_RELAY | + cricket::PORTALLOCATOR_DISABLE_TCP | + cricket::PORTALLOCATOR_ENABLE_SHARED_SOCKET; + // ep1 is configured with a NAT so that we do gather a srflx candidate. + ConfigureEndpoints(NAT_FULL_CONE, OPEN, kOnlyLocalAndStunPorts, + kOnlyLocalAndStunPorts); + // ICE parameter will be set up when creating the channels. + set_remote_ice_parameter_source(FROM_SETICEPARAMETERS); + // Use a mock mDNS responder, which does not complete the name registration by + // ignoring the completion callback. + auto mock_mdns_responder = absl::make_unique<MockMdnsResponder>(); + EXPECT_CALL(*mock_mdns_responder, CreateNameForAddress(_, _)) + .Times(1) + .WillOnce(Return()); + GetEndpoint(0)->network_manager_.set_mdns_responder( + std::move(mock_mdns_responder)); + + CreateChannels(); + + // We should be able to form a srflx-host connection to ep2. + ASSERT_TRUE_WAIT((ep1_ch1()->selected_connection()) != nullptr, + kMediumTimeout); + EXPECT_EQ(STUN_PORT_TYPE, + ep1_ch1()->selected_connection()->local_candidate().type()); + EXPECT_EQ(LOCAL_PORT_TYPE, + ep1_ch1()->selected_connection()->remote_candidate().type()); + + DestroyChannels(); +} + // Test that after changing the candidate filter from relay-only to allowing all // types of candidates when doing continual gathering, we can gather without ICE // restart the other types of candidates that are now enabled and form candidate
diff --git a/p2p/base/stun_port.cc b/p2p/base/stun_port.cc index f94ea65..03d9553 100644 --- a/p2p/base/stun_port.cc +++ b/p2p/base/stun_port.cc
@@ -196,7 +196,7 @@ username, password), requests_(thread), - socket_(NULL), + socket_(nullptr), error_(0), ready_(false), stun_keepalive_delay_(STUN_KEEPALIVE_INTERVAL), @@ -208,7 +208,7 @@ bool UDPPort::Init() { stun_keepalive_lifetime_ = GetStunKeepaliveLifetime(); if (!SharedSocket()) { - RTC_DCHECK(socket_ == NULL); + RTC_DCHECK(socket_ == nullptr); socket_ = socket_factory()->CreateUdpSocket( rtc::SocketAddress(Network()->GetBestIP(), 0), min_port(), max_port()); if (!socket_) { @@ -250,17 +250,35 @@ Connection* UDPPort::CreateConnection(const Candidate& address, CandidateOrigin origin) { if (!SupportsProtocol(address.protocol())) { - return NULL; + return nullptr; } if (!IsCompatibleAddress(address.address())) { - return NULL; + return nullptr; } - if (SharedSocket() && Candidates()[0].type() != LOCAL_PORT_TYPE) { + // In addition to DCHECK-ing the non-emptiness of local candidates, we also + // skip this Port with null if there are latent bugs to violate it; otherwise + // it would lead to a crash when accessing the local candidate of the + // connection that would be created below. + if (Candidates().empty()) { RTC_NOTREACHED(); - return NULL; + return nullptr; } + // When the socket is shared, the srflx candidate is gathered by the UDPPort. + // The assumption here is that + // 1) if the IP concealment with mDNS is not enabled, the gathering of the + // host candidate of this port (which is synchronous), + // 2) or otherwise if enabled, the start of name registration of the host + // candidate (as the start of asynchronous gathering) + // is always before the gathering of a srflx candidate (and any prflx + // candidate). + // + // See also the definition of MdnsNameRegistrationStatus::kNotStarted in + // port.h. + RTC_DCHECK(!SharedSocket() || Candidates()[0].type() == LOCAL_PORT_TYPE || + mdns_name_registration_status() != + MdnsNameRegistrationStatus::kNotStarted); Connection* conn = new ProxyConnection(this, 0, address); AddOrReplaceConnection(conn); @@ -418,7 +436,7 @@ } void UDPPort::OnResolveResult(const rtc::SocketAddress& input, int error) { - RTC_DCHECK(resolver_.get() != NULL); + RTC_DCHECK(resolver_.get() != nullptr); rtc::SocketAddress resolved; if (error != 0 || !resolver_->GetResolvedAddress(
diff --git a/p2p/client/basic_port_allocator_unittest.cc b/p2p/client/basic_port_allocator_unittest.cc index 067e757..a893554 100644 --- a/p2p/client/basic_port_allocator_unittest.cc +++ b/p2p/client/basic_port_allocator_unittest.cc
@@ -12,6 +12,7 @@ #include <ostream> // no-presubmit-check TODO(webrtc:8982) #include "absl/algorithm/container.h" +#include "absl/memory/memory.h" #include "p2p/base/basic_packet_socket_factory.h" #include "p2p/base/p2p_constants.h" #include "p2p/base/stun_port.h" @@ -22,6 +23,7 @@ #include "p2p/base/test_turn_server.h" #include "p2p/client/basic_port_allocator.h" #include "rtc_base/fake_clock.h" +#include "rtc_base/fake_mdns_responder.h" #include "rtc_base/fake_network.h" #include "rtc_base/firewall_socket_server.h" #include "rtc_base/gunit.h" @@ -2395,7 +2397,8 @@ AddTurnServers(kTurnUdpIntIPv6Addr, kTurnTcpIntIPv6Addr); ASSERT_EQ(&network_manager_, allocator().network_manager()); - network_manager_.CreateMdnsResponder(rtc::Thread::Current()); + network_manager_.set_mdns_responder( + absl::make_unique<webrtc::FakeMdnsResponder>(rtc::Thread::Current())); AddInterface(kClientAddr); ASSERT_TRUE(CreateSession(ICE_CANDIDATE_COMPONENT_RTP)); session_->StartGettingPorts();
diff --git a/pc/peer_connection_histogram_unittest.cc b/pc/peer_connection_histogram_unittest.cc index 97555a9..54c251a 100644 --- a/pc/peer_connection_histogram_unittest.cc +++ b/pc/peer_connection_histogram_unittest.cc
@@ -33,6 +33,7 @@ #include "pc/test/mock_peer_connection_observers.h" #include "rtc_base/arraysize.h" #include "rtc_base/checks.h" +#include "rtc_base/fake_mdns_responder.h" #include "rtc_base/fake_network.h" #include "rtc_base/gunit.h" #include "rtc_base/ref_counted_object.h" @@ -245,7 +246,8 @@ webrtc::PeerConnectionDependencies deps(nullptr /* observer_in */); auto fake_network = NewFakeNetwork(); - fake_network->CreateMdnsResponder(rtc::Thread::Current()); + fake_network->set_mdns_responder( + absl::make_unique<webrtc::FakeMdnsResponder>(rtc::Thread::Current())); fake_network->AddInterface(NextLocalAddress()); std::unique_ptr<cricket::BasicPortAllocator> port_allocator(
diff --git a/pc/peer_connection_integrationtest.cc b/pc/peer_connection_integrationtest.cc index 3a39971..0fc2e15 100644 --- a/pc/peer_connection_integrationtest.cc +++ b/pc/peer_connection_integrationtest.cc
@@ -61,6 +61,7 @@ #include "pc/test/fake_video_track_renderer.h" #include "pc/test/mock_peer_connection_observers.h" #include "rtc_base/fake_clock.h" +#include "rtc_base/fake_mdns_responder.h" #include "rtc_base/fake_network.h" #include "rtc_base/firewall_socket_server.h" #include "rtc_base/gunit.h" @@ -552,7 +553,7 @@ } } - rtc::FakeNetworkManager* network() const { + rtc::FakeNetworkManager* network_manager() const { return fake_network_manager_.get(); } cricket::PortAllocator* port_allocator() const { return port_allocator_; } @@ -565,6 +566,15 @@ return last_candidate_gathered_; } + // Sets the mDNS responder for the owned fake network manager and keeps a + // reference to the responder. + void SetMdnsResponder( + std::unique_ptr<webrtc::FakeMdnsResponder> mdns_responder) { + RTC_DCHECK(mdns_responder != nullptr); + mdns_responder_ = mdns_responder.get(); + network_manager()->set_mdns_responder(std::move(mdns_responder)); + } + private: explicit PeerConnectionWrapper(const std::string& debug_name) : debug_name_(debug_name) {} @@ -926,11 +936,10 @@ if (remote_async_resolver_) { const auto& local_candidate = candidate->candidate(); - const auto& mdns_responder = network()->GetMdnsResponderForTesting(); if (local_candidate.address().IsUnresolvedIP()) { RTC_DCHECK(local_candidate.type() == cricket::LOCAL_PORT_TYPE); rtc::SocketAddress resolved_addr(local_candidate.address()); - const auto resolved_ip = mdns_responder->GetMappedAddressForName( + const auto resolved_ip = mdns_responder_->GetMappedAddressForName( local_candidate.address().hostname()); RTC_DCHECK(!resolved_ip.IsNil()); resolved_addr.SetResolvedIP(resolved_ip); @@ -959,6 +968,8 @@ std::string debug_name_; std::unique_ptr<rtc::FakeNetworkManager> fake_network_manager_; + // Reference to the mDNS responder owned by |fake_network_manager_| after set. + webrtc::FakeMdnsResponder* mdns_responder_ = nullptr; rtc::scoped_refptr<webrtc::PeerConnectionInterface> peer_connection_; rtc::scoped_refptr<webrtc::PeerConnectionFactoryInterface> @@ -3825,8 +3836,10 @@ callee()->SetRemoteAsyncResolver(&caller_async_resolver); // Enable hostname candidates with mDNS names. - caller()->network()->CreateMdnsResponder(network_thread()); - callee()->network()->CreateMdnsResponder(network_thread()); + caller()->SetMdnsResponder( + absl::make_unique<webrtc::FakeMdnsResponder>(network_thread())); + callee()->SetMdnsResponder( + absl::make_unique<webrtc::FakeMdnsResponder>(network_thread())); SetPortAllocatorFlags(kOnlyLocalPorts, kOnlyLocalPorts); @@ -3891,15 +3904,15 @@ void SetUpNetworkInterfaces() { // Remove the default interfaces added by the test infrastructure. - caller()->network()->RemoveInterface(kDefaultLocalAddress); - callee()->network()->RemoveInterface(kDefaultLocalAddress); + caller()->network_manager()->RemoveInterface(kDefaultLocalAddress); + callee()->network_manager()->RemoveInterface(kDefaultLocalAddress); // Add network addresses for test. for (const auto& caller_address : CallerAddresses()) { - caller()->network()->AddInterface(caller_address); + caller()->network_manager()->AddInterface(caller_address); } for (const auto& callee_address : CalleeAddresses()) { - callee()->network()->AddInterface(callee_address); + callee()->network_manager()->AddInterface(callee_address); } }
diff --git a/rtc_base/fake_network.h b/rtc_base/fake_network.h index 1da9814..5913cc3 100644 --- a/rtc_base/fake_network.h +++ b/rtc_base/fake_network.h
@@ -18,7 +18,7 @@ #include "absl/memory/memory.h" #include "rtc_base/checks.h" -#include "rtc_base/fake_mdns_responder.h" +#include "rtc_base/mdns_responder_interface.h" #include "rtc_base/message_handler.h" #include "rtc_base/network.h" #include "rtc_base/socket_address.h" @@ -82,13 +82,6 @@ // MessageHandler interface. void OnMessage(Message* msg) override { DoUpdateNetworks(); } - void CreateMdnsResponder(rtc::Thread* network_thread) { - if (mdns_responder_ == nullptr) { - mdns_responder_ = - absl::make_unique<webrtc::FakeMdnsResponder>(network_thread); - } - } - using NetworkManagerBase::set_enumeration_permission; using NetworkManagerBase::set_default_local_addresses; @@ -97,8 +90,9 @@ return mdns_responder_.get(); } - webrtc::FakeMdnsResponder* GetMdnsResponderForTesting() const { - return mdns_responder_.get(); + void set_mdns_responder( + std::unique_ptr<webrtc::MdnsResponderInterface> mdns_responder) { + mdns_responder_ = std::move(mdns_responder); } private: @@ -134,7 +128,7 @@ int start_count_ = 0; bool sent_first_update_ = false; - std::unique_ptr<webrtc::FakeMdnsResponder> mdns_responder_; + std::unique_ptr<webrtc::MdnsResponderInterface> mdns_responder_; }; } // namespace rtc