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
