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}

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 @@ TEST_F(P2PTransportChannelTest,
   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 @@ TEST_F(P2PTransportChannelTest,
   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 @@ TEST_F(P2PTransportChannelTest, CanConnectWithHostCandidateWithMdnsName) {
   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 @@ TEST_F(P2PTransportChannelTest,
                      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 @@ TEST_F(P2PTransportChannelTest,
   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

p2p/base/stun_port.cc

@@ -196,7 +196,7 @@ UDPPort::UDPPort(rtc::Thread* thread,
            username,
            password),
       requests_(thread),
-      socket_(NULL),
+      socket_(nullptr),
       error_(0),
       ready_(false),
       stun_keepalive_delay_(STUN_KEEPALIVE_INTERVAL),
@@ -208,7 +208,7 @@ UDPPort::UDPPort(rtc::Thread* thread,
 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 @@ void UDPPort::MaybePrepareStunCandidate() {
 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::ResolveStunAddress(const rtc::SocketAddress& stun_addr) {
 }
 
 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(

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 @@ TEST_F(BasicPortAllocatorTest, HostCandidateAddressIsReplacedByHostname) {
   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();

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 @@ class PeerConnectionUsageHistogramTest : public ::testing::Test {
     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(

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 @@ class PeerConnectionWrapper : public webrtc::PeerConnectionObserver,
     }
   }
 
-  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 @@ class PeerConnectionWrapper : public webrtc::PeerConnectionObserver,
     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 @@ class PeerConnectionWrapper : public webrtc::PeerConnectionObserver,
 
     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 @@ class PeerConnectionWrapper : public webrtc::PeerConnectionObserver,
   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 @@ TEST_P(PeerConnectionIntegrationTest,
   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 @@ class PeerConnectionIntegrationIceStatesTest
 
   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);
     }
   }
 

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 @@ class FakeNetworkManager : public NetworkManagerBase, public MessageHandler {
   // 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 @@ class FakeNetworkManager : public NetworkManagerBase, public MessageHandler {
     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 @@ class FakeNetworkManager : public NetworkManagerBase, public MessageHandler {
   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