Fix usage logging of TURN and STUN servers

Also adds tests, and adds a bit of logging in ParseIceServers.

Bug: chromium:718508
Change-Id: Id41ccb7cccbdab5af76e380b32b4d8ba0c4a0a72
Reviewed-on: https://webrtc-review.googlesource.com/86121
Reviewed-by: Henrik Boström <hbos@webrtc.org>
Commit-Queue: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#23769}

pc/iceserverparsing.cc

@@ -231,6 +231,7 @@ static RTCErrorType ParseIceServerUrl(
       if (username.empty() || server.password.empty()) {
         // The WebRTC spec requires throwing an InvalidAccessError when username
         // or credential are ommitted; this is the native equivalent.
+        RTC_LOG(LS_ERROR) << "TURN URL without username, or password empty";
         return RTCErrorType::INVALID_PARAMETER;
       }
       // If the hostname field is not empty, then the server address must be

pc/peerconnection.cc

@@ -913,13 +913,31 @@ bool PeerConnection::Initialize(
   port_allocator_ = std::move(dependencies.allocator);
   tls_cert_verifier_ = std::move(dependencies.tls_cert_verifier);
 
+  cricket::ServerAddresses stun_servers;
+  std::vector<cricket::RelayServerConfig> turn_servers;
+
+  RTCErrorType parse_error =
+      ParseIceServers(configuration.servers, &stun_servers, &turn_servers);
+  if (parse_error != RTCErrorType::NONE) {
+    return false;
+  }
+
   // The port allocator lives on the network thread and should be initialized
   // there.
   if (!network_thread()->Invoke<bool>(
-          RTC_FROM_HERE, rtc::Bind(&PeerConnection::InitializePortAllocator_n,
-                                   this, configuration))) {
+          RTC_FROM_HERE,
+          rtc::Bind(&PeerConnection::InitializePortAllocator_n, this,
+                    stun_servers, turn_servers, configuration))) {
     return false;
   }
+  // If initialization was successful, note if STUN or TURN servers
+  // were supplied.
+  if (!stun_servers.empty()) {
+    NoteUsageEvent(UsageEvent::STUN_SERVER_ADDED);
+  }
+  if (!turn_servers.empty()) {
+    NoteUsageEvent(UsageEvent::TURN_SERVER_ADDED);
+  }
 
   const PeerConnectionFactoryInterface::Options& options = factory_->options();
 
@@ -4669,14 +4687,9 @@ DataChannel* PeerConnection::FindDataChannelBySid(int sid) const {
 }
 
 bool PeerConnection::InitializePortAllocator_n(
+    const cricket::ServerAddresses& stun_servers,
+    const std::vector<cricket::RelayServerConfig>& turn_servers,
     const RTCConfiguration& configuration) {
-  cricket::ServerAddresses stun_servers;
-  std::vector<cricket::RelayServerConfig> turn_servers;
-  if (ParseIceServers(configuration.servers, &stun_servers, &turn_servers) !=
-      RTCErrorType::NONE) {
-    return false;
-  }
-
   port_allocator_->Initialize();
   // To handle both internal and externally created port allocator, we will
   // enable BUNDLE here.
@@ -4721,15 +4734,16 @@ bool PeerConnection::InitializePortAllocator_n(
       ConvertIceTransportTypeToCandidateFilter(configuration.type));
   port_allocator_->set_max_ipv6_networks(configuration.max_ipv6_networks);
 
+  auto turn_servers_copy = turn_servers;
   if (tls_cert_verifier_ != nullptr) {
-    for (auto& turn_server : turn_servers) {
+    for (auto& turn_server : turn_servers_copy) {
       turn_server.tls_cert_verifier = tls_cert_verifier_.get();
     }
   }
   // Call this last since it may create pooled allocator sessions using the
   // properties set above.
   port_allocator_->SetConfiguration(
-      stun_servers, turn_servers, configuration.ice_candidate_pool_size,
+      stun_servers, turn_servers_copy, configuration.ice_candidate_pool_size,
       configuration.prune_turn_ports, configuration.turn_customizer,
       configuration.stun_candidate_keepalive_interval);
   return true;

pc/peerconnection.h

@@ -657,7 +657,10 @@ class PeerConnection : public PeerConnectionInternal,
   DataChannel* FindDataChannelBySid(int sid) const;
 
   // Called when first configuring the port allocator.
-  bool InitializePortAllocator_n(const RTCConfiguration& configuration);
+  bool InitializePortAllocator_n(
+      const cricket::ServerAddresses& stun_servers,
+      const std::vector<cricket::RelayServerConfig>& turn_servers,
+      const RTCConfiguration& configuration);
   // Called when SetConfiguration is called to apply the supported subset
   // of the configuration on the network thread.
   bool ReconfigurePortAllocator_n(

pc/peerconnection_histogram_unittest.cc

@@ -134,6 +134,11 @@ class PeerConnectionUsageHistogramTest : public ::testing::Test {
         RTCConfiguration(), PeerConnectionFactoryInterface::Options(), false);
   }
 
+  WrapperPtr CreatePeerConnection(const RTCConfiguration& config) {
+    return CreatePeerConnection(
+        config, PeerConnectionFactoryInterface::Options(), false);
+  }
+
   WrapperPtr CreatePeerConnectionWithImmediateReport() {
     return CreatePeerConnection(
         RTCConfiguration(), PeerConnectionFactoryInterface::Options(), true);
@@ -242,4 +247,49 @@ TEST_F(PeerConnectionUsageHistogramTest, FingerprintDataOnly) {
 #endif  // HAVE_SCTP
 #endif  // WEBRTC_ANDROID
 
+TEST_F(PeerConnectionUsageHistogramTest, FingerprintStunTurn) {
+  RTCConfiguration configuration;
+  PeerConnection::IceServer server;
+  server.urls = {"stun:dummy.stun.server/"};
+  configuration.servers.push_back(server);
+  server.urls = {"turn:dummy.turn.server/"};
+  server.username = "username";
+  server.password = "password";
+  configuration.servers.push_back(server);
+  auto caller = CreatePeerConnection(configuration);
+  ASSERT_TRUE(caller);
+  auto caller_observer = caller->RegisterFakeMetricsObserver();
+  caller->pc()->Close();
+  int expected_fingerprint =
+      MakeUsageFingerprint({PeerConnection::UsageEvent::STUN_SERVER_ADDED,
+                            PeerConnection::UsageEvent::TURN_SERVER_ADDED,
+                            PeerConnection::UsageEvent::CLOSE_CALLED});
+  EXPECT_TRUE(caller_observer->ExpectOnlySingleEnumCount(
+      webrtc::kEnumCounterUsagePattern, expected_fingerprint));
+}
+
+TEST_F(PeerConnectionUsageHistogramTest, FingerprintStunTurnInReconfiguration) {
+  RTCConfiguration configuration;
+  PeerConnection::IceServer server;
+  server.urls = {"stun:dummy.stun.server/"};
+  configuration.servers.push_back(server);
+  server.urls = {"turn:dummy.turn.server/"};
+  server.username = "username";
+  server.password = "password";
+  configuration.servers.push_back(server);
+  auto caller = CreatePeerConnection();
+  ASSERT_TRUE(caller);
+  auto caller_observer = caller->RegisterFakeMetricsObserver();
+  RTCError error;
+  caller->pc()->SetConfiguration(configuration, &error);
+  ASSERT_TRUE(error.ok());
+  caller->pc()->Close();
+  int expected_fingerprint =
+      MakeUsageFingerprint({PeerConnection::UsageEvent::STUN_SERVER_ADDED,
+                            PeerConnection::UsageEvent::TURN_SERVER_ADDED,
+                            PeerConnection::UsageEvent::CLOSE_CALLED});
+  EXPECT_TRUE(caller_observer->ExpectOnlySingleEnumCount(
+      webrtc::kEnumCounterUsagePattern, expected_fingerprint));
+}
+
 }  // namespace webrtc