From b2a747822145590256ce53c34ce22d628b9b2323 Mon Sep 17 00:00:00 2001 From: Harald Alvestrand Date: Thu, 28 Jun 2018 13:54:07 +0200 Subject: [PATCH] Fix usage logging of TURN and STUN servers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Commit-Queue: Harald Alvestrand Cr-Commit-Position: refs/heads/master@{#23769} --- pc/iceserverparsing.cc | 1 + pc/peerconnection.cc | 36 ++++++++++++------ pc/peerconnection.h | 5 ++- pc/peerconnection_histogram_unittest.cc | 50 +++++++++++++++++++++++++ 4 files changed, 80 insertions(+), 12 deletions(-) diff --git a/pc/iceserverparsing.cc b/pc/iceserverparsing.cc index b260954f04..806fb3bc21 100644 --- a/pc/iceserverparsing.cc +++ b/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 diff --git a/pc/peerconnection.cc b/pc/peerconnection.cc index 15966bafe3..348569a6c3 100644 --- a/pc/peerconnection.cc +++ b/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 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( - 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& turn_servers, const RTCConfiguration& configuration) { - cricket::ServerAddresses stun_servers; - std::vector 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; diff --git a/pc/peerconnection.h b/pc/peerconnection.h index 4ea3540bf1..7d7315d4e4 100644 --- a/pc/peerconnection.h +++ b/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& 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( diff --git a/pc/peerconnection_histogram_unittest.cc b/pc/peerconnection_histogram_unittest.cc index e6255d5ddf..e5c0426341 100644 --- a/pc/peerconnection_histogram_unittest.cc +++ b/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