From 2e70294b7c22827a8a59d7caaadb242cfcc02ed6 Mon Sep 17 00:00:00 2001 From: Sameer Vijaykar Date: Fri, 2 Sep 2022 15:02:04 +0200 Subject: [PATCH] Ship family-specific STUN hostname resolution behind field trial param. Since multiple fixes are bundled behind the WebRTC-IPv6NetworkResolutionFixes field trial, this allows more flexibility if the launch is found to cause any issues. Bug: webrtc:14334, webrtc:14131 Change-Id: I2e73b8984db1f3d292cecab98e29c53cfac1c070 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/273903 Commit-Queue: Jonas Oreland Commit-Queue: Sameer Vijaykar Reviewed-by: Jonas Oreland Auto-Submit: Sameer Vijaykar Cr-Commit-Position: refs/heads/main@{#37995} --- p2p/base/stun_port.cc | 25 ++++++++++++-- p2p/base/stun_port_unittest.cc | 62 ++++++++++++++++++++++++++++++++-- 2 files changed, 81 insertions(+), 6 deletions(-) diff --git a/p2p/base/stun_port.cc b/p2p/base/stun_port.cc index f0b1778772..5d57d1ac54 100644 --- a/p2p/base/stun_port.cc +++ b/p2p/base/stun_port.cc @@ -21,6 +21,7 @@ #include "p2p/base/port_allocator.h" #include "rtc_base/async_resolver_interface.h" #include "rtc_base/checks.h" +#include "rtc_base/experiments/field_trial_parser.h" #include "rtc_base/helpers.h" #include "rtc_base/ip_address.h" #include "rtc_base/logging.h" @@ -28,6 +29,26 @@ namespace cricket { +namespace { + +bool ResolveStunHostnameForFamily(const webrtc::FieldTrialsView& field_trials) { + // Bug fix for STUN hostname resolution on IPv6. + // Field trial key reserved in bugs.webrtc.org/14334 + static constexpr char field_trial_name[] = + "WebRTC-IPv6NetworkResolutionFixes"; + if (!field_trials.IsEnabled(field_trial_name)) { + return false; + } + + webrtc::FieldTrialParameter resolve_stun_hostname_for_family( + "ResolveStunHostnameForFamily", /*default_value=*/false); + webrtc::ParseFieldTrial({&resolve_stun_hostname_for_family}, + field_trials.Lookup(field_trial_name)); + return resolve_stun_hostname_for_family; +} + +} // namespace + // TODO(?): Move these to a common place (used in relayport too) const int RETRY_TIMEOUT = 50 * 1000; // 50 seconds @@ -142,9 +163,7 @@ void UDPPort::AddressResolver::Resolve( done_(it->first, it->second->result().GetError()); } }; - // Bug fix for STUN hostname resolution on IPv6. - // Field trial key reserved in bugs.webrtc.org/14334 - if (field_trials.IsEnabled("WebRTC-IPv6NetworkResolutionFixes")) { + if (ResolveStunHostnameForFamily(field_trials)) { resolver_ptr->Start(address, family, std::move(callback)); } else { resolver_ptr->Start(address, std::move(callback)); diff --git a/p2p/base/stun_port_unittest.cc b/p2p/base/stun_port_unittest.cc index 58705b95d2..a6f5d67c5c 100644 --- a/p2p/base/stun_port_unittest.cc +++ b/p2p/base/stun_port_unittest.cc @@ -627,6 +627,7 @@ TEST_F(StunIPv6PortTestWithMockDnsResolver, TestPrepareAddressHostname) { SetDnsResolverExpectations( [](webrtc::MockAsyncDnsResolver* resolver, webrtc::MockAsyncDnsResolverResult* resolver_result) { + // Expect to call Resolver::Start without family arg. EXPECT_CALL(*resolver, Start(kValidHostnameAddr, _)) .WillOnce(InvokeArgument<1>()); EXPECT_CALL(*resolver, result) @@ -644,13 +645,68 @@ TEST_F(StunIPv6PortTestWithMockDnsResolver, TestPrepareAddressHostname) { EXPECT_EQ(kIPv6StunCandidatePriority, port()->Candidates()[0].priority()); } -TEST_F(StunIPv6PortTestWithMockDnsResolver, TestPrepareAddressHostnameFamily) { +TEST_F(StunIPv6PortTestWithMockDnsResolver, + TestPrepareAddressHostnameFamilyFieldTrialDisabled) { webrtc::test::ScopedKeyValueConfig field_trials( - "WebRTC-IPv6NetworkResolutionFixes/Enabled/"); + "WebRTC-IPv6NetworkResolutionFixes/Disabled/"); SetDnsResolverExpectations( [](webrtc::MockAsyncDnsResolver* resolver, webrtc::MockAsyncDnsResolverResult* resolver_result) { - EXPECT_CALL(*resolver, Start(kValidHostnameAddr, AF_INET6, _)) + // Expect to call Resolver::Start without family arg. + EXPECT_CALL(*resolver, Start(kValidHostnameAddr, _)) + .WillOnce(InvokeArgument<1>()); + EXPECT_CALL(*resolver, result) + .WillRepeatedly(ReturnPointee(resolver_result)); + EXPECT_CALL(*resolver_result, GetError).WillOnce(Return(0)); + EXPECT_CALL(*resolver_result, GetResolvedAddress(AF_INET6, _)) + .WillOnce(DoAll(SetArgPointee<1>(SocketAddress("::1", 5000)), + Return(true))); + }); + CreateStunPort(kValidHostnameAddr, &field_trials); + PrepareAddress(); + EXPECT_TRUE_SIMULATED_WAIT(done(), kTimeoutMs, fake_clock); + ASSERT_EQ(1U, port()->Candidates().size()); + EXPECT_TRUE(kIPv6LocalAddr.EqualIPs(port()->Candidates()[0].address())); + EXPECT_EQ(kIPv6StunCandidatePriority, port()->Candidates()[0].priority()); +} + +TEST_F(StunIPv6PortTestWithMockDnsResolver, + TestPrepareAddressHostnameFamilyFieldTrialParamDisabled) { + webrtc::test::ScopedKeyValueConfig field_trials( + "WebRTC-IPv6NetworkResolutionFixes/" + "Enabled,ResolveStunHostnameForFamily:false/"); + SetDnsResolverExpectations( + [](webrtc::MockAsyncDnsResolver* resolver, + webrtc::MockAsyncDnsResolverResult* resolver_result) { + // Expect to call Resolver::Start without family arg. + EXPECT_CALL(*resolver, Start(kValidHostnameAddr, _)) + .WillOnce(InvokeArgument<1>()); + EXPECT_CALL(*resolver, result) + .WillRepeatedly(ReturnPointee(resolver_result)); + EXPECT_CALL(*resolver_result, GetError).WillOnce(Return(0)); + EXPECT_CALL(*resolver_result, GetResolvedAddress(AF_INET6, _)) + .WillOnce(DoAll(SetArgPointee<1>(SocketAddress("::1", 5000)), + Return(true))); + }); + CreateStunPort(kValidHostnameAddr, &field_trials); + PrepareAddress(); + EXPECT_TRUE_SIMULATED_WAIT(done(), kTimeoutMs, fake_clock); + ASSERT_EQ(1U, port()->Candidates().size()); + EXPECT_TRUE(kIPv6LocalAddr.EqualIPs(port()->Candidates()[0].address())); + EXPECT_EQ(kIPv6StunCandidatePriority, port()->Candidates()[0].priority()); +} + +TEST_F(StunIPv6PortTestWithMockDnsResolver, + TestPrepareAddressHostnameFamilyFieldTrialEnabled) { + webrtc::test::ScopedKeyValueConfig field_trials( + "WebRTC-IPv6NetworkResolutionFixes/" + "Enabled,ResolveStunHostnameForFamily:true/"); + SetDnsResolverExpectations( + [](webrtc::MockAsyncDnsResolver* resolver, + webrtc::MockAsyncDnsResolverResult* resolver_result) { + // Expect to call Resolver::Start _with_ family arg. + EXPECT_CALL(*resolver, + Start(kValidHostnameAddr, /*family=*/AF_INET6, _)) .WillOnce(InvokeArgument<2>()); EXPECT_CALL(*resolver, result) .WillRepeatedly(ReturnPointee(resolver_result));