Propagate clock into StunRequest While at it migrate some integers to strong time types Update Send function signature to use unique_ptr to communicate request is passed with ownership Bug: webrtc:42223992 Change-Id: I177dc20ce1d40d5dec9efaf9f37b46ee4f994ea1 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/407080 Reviewed-by: Per Kjellander <perkj@webrtc.org> Commit-Queue: Danil Chapovalov <danilchap@webrtc.org> Cr-Commit-Position: refs/heads/main@{#45510}
diff --git a/p2p/BUILD.gn b/p2p/BUILD.gn index 2baa0e2..281a0d0 100644 --- a/p2p/BUILD.gn +++ b/p2p/BUILD.gn
@@ -758,6 +758,8 @@ "../api:local_network_access_permission", "../api:packet_socket_factory", "../api/transport:stun_types", + "../api/units:time_delta", + "../api/units:timestamp", "../rtc_base:async_packet_socket", "../rtc_base:checks", "../rtc_base:dscp", @@ -787,10 +789,12 @@ deps = [ "../api:array_view", "../api:sequence_checker", + "../api/environment", "../api/task_queue", "../api/task_queue:pending_task_safety_flag", "../api/transport:stun_types", "../api/units:time_delta", + "../api/units:timestamp", "../rtc_base:byte_buffer", "../rtc_base:checks", "../rtc_base:crypto_random", @@ -920,6 +924,7 @@ "../rtc_base:logging", "../rtc_base:net_helper", "../rtc_base:network", + "../rtc_base:platform_thread_types", "../rtc_base:socket", "../rtc_base:socket_address", "../rtc_base:ssl",
diff --git a/p2p/base/connection.cc b/p2p/base/connection.cc index 5bdf070..1afd41b 100644 --- a/p2p/base/connection.cc +++ b/p2p/base/connection.cc
@@ -180,7 +180,8 @@ // A ConnectionRequest is a STUN binding used to determine writability. class Connection::ConnectionRequest : public StunRequest { public: - ConnectionRequest(StunRequestManager& manager, + ConnectionRequest(const Environment& env, + StunRequestManager& manager, Connection* connection, std::unique_ptr<IceMessage> message); void OnResponse(StunMessage* response) override; @@ -194,10 +195,11 @@ }; Connection::ConnectionRequest::ConnectionRequest( + const Environment& env, StunRequestManager& manager, Connection* connection, std::unique_ptr<IceMessage> message) - : StunRequest(manager, std::move(message)), connection_(connection) {} + : StunRequest(env, manager, std::move(message)), connection_(connection) {} void Connection::ConnectionRequest::OnResponse(StunMessage* response) { RTC_DCHECK_RUN_ON(connection_->network_thread_); @@ -1103,19 +1105,20 @@ bool has_delta = delta != nullptr; auto req = std::make_unique<ConnectionRequest>( - requests_, this, BuildPingRequest(std::move(delta))); + env_, requests_, this, BuildPingRequest(std::move(delta))); if (!has_delta && ShouldSendGoogPing(req->msg())) { auto message = std::make_unique<IceMessage>(GOOG_PING_REQUEST, req->id()); message->AddMessageIntegrity32(remote_candidate_.password()); - req.reset(new ConnectionRequest(requests_, this, std::move(message))); + req = std::make_unique<ConnectionRequest>(env_, requests_, this, + std::move(message)); } pings_since_last_response_.push_back(SentPing(req->id(), now, nomination)); RTC_LOG(LS_VERBOSE) << ToString() << ": Sending STUN ping, id=" << hex_encode(req->id()) << ", nomination=" << nomination_; - requests_.Send(req.release()); + requests_.Send(std::move(req)); state_ = IceCandidatePairState::IN_PROGRESS; num_pings_sent_++; } @@ -1513,7 +1516,7 @@ // connection. LoggingSeverity sev = !writable() ? LS_INFO : LS_VERBOSE; - TimeDelta rtt = TimeDelta::Millis(request->Elapsed()); + TimeDelta rtt = request->Elapsed(); if (RTC_LOG_CHECK_LEVEL_V(sev)) { std::string pings;
diff --git a/p2p/base/stun_port.cc b/p2p/base/stun_port.cc index b6b2c53..9903a16 100644 --- a/p2p/base/stun_port.cc +++ b/p2p/base/stun_port.cc
@@ -26,6 +26,8 @@ #include "api/local_network_access_permission.h" #include "api/packet_socket_factory.h" #include "api/transport/stun.h" +#include "api/units/time_delta.h" +#include "api/units/timestamp.h" #include "p2p/base/connection.h" #include "p2p/base/p2p_constants.h" #include "p2p/base/port.h" @@ -43,25 +45,27 @@ #include "rtc_base/socket.h" #include "rtc_base/socket_address.h" #include "rtc_base/strings/string_builder.h" -#include "rtc_base/time_utils.h" #include "system_wrappers/include/metrics.h" namespace webrtc { - +namespace { // TODO(?): Move these to a common place (used in relayport too) -const int RETRY_TIMEOUT = 50 * 1000; // 50 seconds +constexpr TimeDelta kRetryTimeout = TimeDelta::Seconds(50); // Stop logging errors in UDPPort::SendTo after we have logged // `kSendErrorLogLimit` messages. Start again after a successful send. -const int kSendErrorLogLimit = 5; +constexpr int kSendErrorLogLimit = 5; + +} // namespace // Handles a binding request sent to the STUN server. class StunBindingRequest : public StunRequest { public: StunBindingRequest(UDPPort* port, const SocketAddress& addr, - int64_t start_time) - : StunRequest(port->request_manager(), + Timestamp start_time) + : StunRequest(port->env(), + port->request_manager(), std::make_unique<StunMessage>(STUN_BINDING_REQUEST)), port_(port), server_addr_(addr), @@ -85,10 +89,10 @@ } // The keep-alive requests will be stopped after its lifetime has passed. - if (WithinLifetime(TimeMillis())) { - port_->request_manager_.SendDelayed( - new StunBindingRequest(port_, server_addr_, start_time_), - port_->stun_keepalive_delay()); + if (WithinLifetime(Connection::AlignTime(env().clock().CurrentTime()))) { + port_->request_manager_.Send(std::make_unique<StunBindingRequest>( + port_, server_addr_, start_time_), + /*delay=*/port_->stun_keepalive_delay()); } } @@ -108,11 +112,11 @@ attr ? attr->reason() : "STUN binding response with no error code attribute."); - int64_t now = TimeMillis(); - if (WithinLifetime(now) && TimeDiff(now, start_time_) < RETRY_TIMEOUT) { - port_->request_manager_.SendDelayed( - new StunBindingRequest(port_, server_addr_, start_time_), - port_->stun_keepalive_delay()); + Timestamp now = Connection::AlignTime(env().clock().CurrentTime()); + if (WithinLifetime(now) && now - start_time_ < kRetryTimeout) { + port_->request_manager_.Send(std::make_unique<StunBindingRequest>( + port_, server_addr_, start_time_), + /*delay=*/port_->stun_keepalive_delay()); } } void OnTimeout() override { @@ -125,17 +129,15 @@ } private: - // Returns true if `now` is within the lifetime of the request (a negative - // lifetime means infinite). - bool WithinLifetime(int64_t now) const { - int lifetime = port_->stun_keepalive_lifetime(); - return lifetime < 0 || TimeDiff(now, start_time_) <= lifetime; + // Returns true if `now` is within the lifetime of the request. + bool WithinLifetime(Timestamp now) const { + return now - start_time_ <= port_->stun_keepalive_lifetime(); } UDPPort* port_; const SocketAddress server_addr_; - int64_t start_time_; + Timestamp start_time_; }; UDPPort::AddressResolver::AddressResolver( @@ -508,8 +510,9 @@ return; } - request_manager_.Send( - new StunBindingRequest(this, stun_addr, TimeMillis())); + request_manager_.Send(std::make_unique<StunBindingRequest>( + this, stun_addr, + Connection::AlignTime(env().clock().CurrentTime()))); }); } @@ -531,9 +534,10 @@ } void UDPPort::OnStunBindingRequestSucceeded( - int rtt_ms, + TimeDelta rtt, const SocketAddress& stun_server_addr, const SocketAddress& stun_reflected_addr) { + int rtt_ms = rtt.ms(); RTC_DCHECK(stats_.stun_binding_responses_received < stats_.stun_binding_requests_sent); stats_.stun_binding_responses_received++;
diff --git a/p2p/base/stun_port.h b/p2p/base/stun_port.h index 1dc4da7..171e232 100644 --- a/p2p/base/stun_port.h +++ b/p2p/base/stun_port.h
@@ -24,6 +24,7 @@ #include "api/candidate.h" #include "api/field_trials_view.h" #include "api/packet_socket_factory.h" +#include "api/units/time_delta.h" #include "p2p/base/connection.h" #include "p2p/base/port.h" #include "p2p/base/port_interface.h" @@ -39,10 +40,9 @@ namespace webrtc { -// Lifetime chosen for STUN ports on low-cost networks. -static const int INFINITE_LIFETIME = -1; // Lifetime for STUN ports on high-cost networks: 2 minutes -static const int HIGH_COST_PORT_KEEPALIVE_LIFETIME = 2 * 60 * 1000; +inline constexpr TimeDelta kHighCostPortKeepaliveLifetime = + TimeDelta::Seconds(2 * 60); // Communicates using the address on the outside of a NAT. class RTC_EXPORT UDPPort : public Port { @@ -105,11 +105,13 @@ void GetStunStats(std::optional<StunStats>* stats) override; void set_stun_keepalive_delay(const std::optional<int>& delay); - int stun_keepalive_delay() const { return stun_keepalive_delay_; } + TimeDelta stun_keepalive_delay() const { + return TimeDelta::Millis(stun_keepalive_delay_); + } // Visible for testing. - int stun_keepalive_lifetime() const { return stun_keepalive_lifetime_; } - void set_stun_keepalive_lifetime(int lifetime) { + TimeDelta stun_keepalive_lifetime() const { return stun_keepalive_lifetime_; } + void set_stun_keepalive_lifetime(TimeDelta lifetime) { stun_keepalive_lifetime_ = lifetime; } @@ -201,7 +203,7 @@ void SendStunBindingRequest(const SocketAddress& stun_addr); // Below methods handles binding request responses. - void OnStunBindingRequestSucceeded(int rtt_ms, + void OnStunBindingRequestSucceeded(TimeDelta rtt, const SocketAddress& stun_server_addr, const SocketAddress& stun_reflected_addr); void OnStunBindingOrResolveRequestFailed( @@ -220,11 +222,10 @@ // If this is a low-cost network, it will keep on sending STUN binding // requests indefinitely to keep the NAT binding alive. Otherwise, stop - // sending STUN binding requests after HIGH_COST_PORT_KEEPALIVE_LIFETIME. - int GetStunKeepaliveLifetime() { - return (network_cost() >= kNetworkCostHigh) - ? HIGH_COST_PORT_KEEPALIVE_LIFETIME - : INFINITE_LIFETIME; + // sending STUN binding requests after kHighCostPortKeepaliveLifetime. + TimeDelta GetStunKeepaliveLifetime() { + return (network_cost() >= kNetworkCostHigh) ? kHighCostPortKeepaliveLifetime + : TimeDelta::PlusInfinity(); } ServerAddresses server_addresses_; @@ -237,7 +238,7 @@ std::unique_ptr<AddressResolver> resolver_; bool ready_; int stun_keepalive_delay_; - int stun_keepalive_lifetime_ = INFINITE_LIFETIME; + TimeDelta stun_keepalive_lifetime_ = TimeDelta::PlusInfinity(); DiffServCodePoint dscp_; StunStats stats_;
diff --git a/p2p/base/stun_port_unittest.cc b/p2p/base/stun_port_unittest.cc index 6fa8096..99e2e86 100644 --- a/p2p/base/stun_port_unittest.cc +++ b/p2p/base/stun_port_unittest.cc
@@ -98,9 +98,6 @@ // stun prio = 100 (srflx) << 24 | 40 (IPv6) << 8 | 256 - 1 (component) constexpr uint32_t kIPv6StunCandidatePriority = (100 << 24) | (40 << 8) | (256 - 1); -constexpr int kInfiniteLifetime = -1; -constexpr int kHighCostPortKeepaliveLifetimeMs = 2 * 60 * 1000; - constexpr uint64_t kTiebreakerDefault = 44444; struct IPAddressTypeTestConfig { @@ -156,8 +153,7 @@ nat_server_(CreateNatServer(nat_server_address, webrtc::NAT_OPEN_CONE)), done_(false), error_(false), - stun_keepalive_delay_(1), - stun_keepalive_lifetime_(-1) { + stun_keepalive_delay_(1) { network_ = MakeNetwork(address); RTC_CHECK(address.family() == nat_server_address.family()); for (const auto& addr : stun_server_addresses) { @@ -210,10 +206,10 @@ 0, 0, stun_servers, std::nullopt); stun_port_->SetIceTiebreaker(kTiebreakerDefault); stun_port_->set_stun_keepalive_delay(stun_keepalive_delay_); - // If `stun_keepalive_lifetime_` is negative, let the stun port - // choose its lifetime from the network type. - if (stun_keepalive_lifetime_ >= 0) { - stun_port_->set_stun_keepalive_lifetime(stun_keepalive_lifetime_); + // If `stun_keepalive_lifetime_` is not set, let the stun port choose its + // lifetime from the network type. + if (stun_keepalive_lifetime_.has_value()) { + stun_port_->set_stun_keepalive_lifetime(*stun_keepalive_lifetime_); } stun_port_->SignalPortComplete.connect(this, &StunPortTestBase::OnPortComplete); @@ -292,7 +288,7 @@ } void SetKeepaliveDelay(int delay) { stun_keepalive_delay_ = delay; } - void SetKeepaliveLifetime(int lifetime) { + void SetKeepaliveLifetime(TimeDelta lifetime) { stun_keepalive_lifetime_ = lifetime; } @@ -324,7 +320,7 @@ bool done_; bool error_; int stun_keepalive_delay_; - int stun_keepalive_lifetime_; + std::optional<TimeDelta> stun_keepalive_lifetime_; protected: webrtc::IceCandidateErrorEvent error_event_; @@ -653,43 +649,41 @@ // type on a STUN port. Also test that it will be updated if the network type // changes. TEST_F(StunPortTest, TestStunPortGetStunKeepaliveLifetime) { - // Lifetime for the default (unknown) network type is `kInfiniteLifetime`. + // Lifetime for the default (unknown) network type is infinite. CreateStunPort(kStunServerAddr1); - EXPECT_EQ(kInfiniteLifetime, port()->stun_keepalive_lifetime()); - // Lifetime for the cellular network is `kHighCostPortKeepaliveLifetimeMs` + EXPECT_EQ(port()->stun_keepalive_lifetime(), TimeDelta::PlusInfinity()); + // Lifetime for the cellular network is `kHighCostPortKeepaliveLifetime` SetNetworkType(webrtc::ADAPTER_TYPE_CELLULAR); - EXPECT_EQ(kHighCostPortKeepaliveLifetimeMs, - port()->stun_keepalive_lifetime()); + EXPECT_EQ(port()->stun_keepalive_lifetime(), kHighCostPortKeepaliveLifetime); - // Lifetime for the wifi network is `kInfiniteLifetime`. + // Lifetime for the wifi network is infinite. SetNetworkType(webrtc::ADAPTER_TYPE_WIFI); CreateStunPort(kStunServerAddr2); - EXPECT_EQ(kInfiniteLifetime, port()->stun_keepalive_lifetime()); + EXPECT_EQ(port()->stun_keepalive_lifetime(), TimeDelta::PlusInfinity()); } // Test that the stun_keepalive_lifetime is set correctly based on the network // type on a shared STUN port (UDPPort). Also test that it will be updated // if the network type changes. TEST_F(StunPortTest, TestUdpPortGetStunKeepaliveLifetime) { - // Lifetime for the default (unknown) network type is `kInfiniteLifetime`. + // Lifetime for the default (unknown) network type is infinite. CreateSharedUdpPort(kStunServerAddr1, nullptr); - EXPECT_EQ(kInfiniteLifetime, port()->stun_keepalive_lifetime()); - // Lifetime for the cellular network is `kHighCostPortKeepaliveLifetimeMs`. + EXPECT_EQ(port()->stun_keepalive_lifetime(), TimeDelta::PlusInfinity()); + // Lifetime for the cellular network is `kHighCostPortKeepaliveLifetime`. SetNetworkType(webrtc::ADAPTER_TYPE_CELLULAR); - EXPECT_EQ(kHighCostPortKeepaliveLifetimeMs, - port()->stun_keepalive_lifetime()); + EXPECT_EQ(port()->stun_keepalive_lifetime(), kHighCostPortKeepaliveLifetime); - // Lifetime for the wifi network type is `kInfiniteLifetime`. + // Lifetime for the wifi network type is infinite. SetNetworkType(webrtc::ADAPTER_TYPE_WIFI); CreateSharedUdpPort(kStunServerAddr2, nullptr); - EXPECT_EQ(kInfiniteLifetime, port()->stun_keepalive_lifetime()); + EXPECT_EQ(port()->stun_keepalive_lifetime(), TimeDelta::PlusInfinity()); } // Test that STUN binding requests will be stopped shortly if the keep-alive // lifetime is short. TEST_F(StunPortTest, TestStunBindingRequestShortLifetime) { SetKeepaliveDelay(101); - SetKeepaliveLifetime(100); + SetKeepaliveLifetime(TimeDelta::Millis(100)); CreateStunPort(kStunServerAddr1); PrepareAddress(); EXPECT_THAT(
diff --git a/p2p/base/stun_request.cc b/p2p/base/stun_request.cc index 1a31df5..874da79 100644 --- a/p2p/base/stun_request.cc +++ b/p2p/base/stun_request.cc
@@ -19,18 +19,18 @@ #include <utility> #include <vector> -#include "absl/memory/memory.h" #include "api/array_view.h" +#include "api/environment/environment.h" #include "api/sequence_checker.h" #include "api/task_queue/pending_task_safety_flag.h" #include "api/task_queue/task_queue_base.h" #include "api/transport/stun.h" #include "api/units/time_delta.h" +#include "api/units/timestamp.h" #include "rtc_base/byte_buffer.h" #include "rtc_base/checks.h" #include "rtc_base/logging.h" #include "rtc_base/string_encode.h" -#include "rtc_base/time_utils.h" // For TimeMillis namespace webrtc { @@ -58,11 +58,8 @@ StunRequestManager::~StunRequestManager() = default; -void StunRequestManager::Send(StunRequest* request) { - SendDelayed(request, 0); -} - -void StunRequestManager::SendDelayed(StunRequest* request, int delay) { +void StunRequestManager::Send(std::unique_ptr<StunRequest> request, + TimeDelta delay) { RTC_DCHECK_RUN_ON(thread_); RTC_DCHECK_EQ(this, request->manager()); RTC_DCHECK(!request->AuthenticationRequired() || @@ -70,9 +67,9 @@ StunMessage::IntegrityStatus::kNotSet) << "Sending request w/o integrity!"; auto [iter, was_inserted] = - requests_.emplace(request->id(), absl::WrapUnique(request)); + requests_.emplace(request->id(), std::move(request)); RTC_DCHECK(was_inserted); - request->Send(TimeDelta::Millis(delay)); + iter->second->Send(delay); } void StunRequestManager::FlushForTest(int msg_type) { @@ -233,20 +230,23 @@ send_packet_(data, size, request); } -StunRequest::StunRequest(StunRequestManager& manager) - : manager_(manager), - msg_(new StunMessage(STUN_INVALID_MESSAGE_TYPE)), - tstamp_(0), +StunRequest::StunRequest(const Environment& env, StunRequestManager& manager) + : env_(env), + manager_(manager), + msg_(std::make_unique<StunMessage>(STUN_INVALID_MESSAGE_TYPE)), + tstamp_(Timestamp::Zero()), count_(0), timeout_(false) { RTC_DCHECK_RUN_ON(network_thread()); } -StunRequest::StunRequest(StunRequestManager& manager, +StunRequest::StunRequest(const Environment& env, + StunRequestManager& manager, std::unique_ptr<StunMessage> message) - : manager_(manager), + : env_(env), + manager_(manager), msg_(std::move(message)), - tstamp_(0), + tstamp_(Timestamp::Zero()), count_(0), timeout_(false) { RTC_DCHECK_RUN_ON(network_thread()); @@ -264,9 +264,9 @@ return msg_.get(); } -int StunRequest::Elapsed() const { +TimeDelta StunRequest::Elapsed() const { RTC_DCHECK_RUN_ON(network_thread()); - return static_cast<int>(TimeMillis() - tstamp_); + return env_.clock().CurrentTime() - tstamp_; } void StunRequest::SendInternal() { @@ -277,7 +277,7 @@ return; } - tstamp_ = TimeMillis(); + tstamp_ = env_.clock().CurrentTime(); ByteBufferWriter buf; msg_->Write(&buf);
diff --git a/p2p/base/stun_request.h b/p2p/base/stun_request.h index 8d6a7f5..b7f2025 100644 --- a/p2p/base/stun_request.h +++ b/p2p/base/stun_request.h
@@ -18,10 +18,12 @@ #include <memory> #include <string> +#include "api/environment/environment.h" #include "api/task_queue/pending_task_safety_flag.h" #include "api/task_queue/task_queue_base.h" #include "api/transport/stun.h" #include "api/units/time_delta.h" +#include "api/units/timestamp.h" #include "rtc_base/thread_annotations.h" namespace webrtc { @@ -45,8 +47,8 @@ ~StunRequestManager(); // Starts sending the given request (perhaps after a delay). - void Send(StunRequest* request); - void SendDelayed(StunRequest* request, int delay); + void Send(std::unique_ptr<StunRequest> request, + TimeDelta delay = TimeDelta::Zero()); // If `msg_type` is kAllRequestsForTest, sends all pending requests right // away. Otherwise, sends those that have a matching type right away. Only for @@ -90,8 +92,9 @@ // constructed beforehand or built on demand. class StunRequest { public: - explicit StunRequest(StunRequestManager& manager); - StunRequest(StunRequestManager& manager, + StunRequest(const Environment& env, StunRequestManager& manager); + StunRequest(const Environment& env, + StunRequestManager& manager, std::unique_ptr<StunMessage> message); virtual ~StunRequest(); @@ -112,8 +115,8 @@ // Returns a const pointer to `msg_`. const StunMessage* msg() const; - // Time elapsed since last send (in ms) - int Elapsed() const; + // Time elapsed since last send. + TimeDelta Elapsed() const; // Add method to explitly allow requests w/o password. // - STUN_BINDINGs from StunPort to a stun server @@ -124,6 +127,8 @@ protected: friend class StunRequestManager; + const Environment& env() { return env_; } + // Called by StunRequestManager. void Send(TimeDelta delay); @@ -152,9 +157,10 @@ // specified timeout. void SendDelayed(TimeDelta delay); + const Environment env_; StunRequestManager& manager_; const std::unique_ptr<StunMessage> msg_; - int64_t tstamp_ RTC_GUARDED_BY(network_thread()); + Timestamp tstamp_ RTC_GUARDED_BY(network_thread()); int count_ RTC_GUARDED_BY(network_thread()); bool timeout_ RTC_GUARDED_BY(network_thread()); ScopedTaskSafety task_safety_{
diff --git a/p2p/base/stun_request_unittest.cc b/p2p/base/stun_request_unittest.cc index ec3bb74..2055a16 100644 --- a/p2p/base/stun_request_unittest.cc +++ b/p2p/base/stun_request_unittest.cc
@@ -13,8 +13,11 @@ #include <cstddef> #include <cstdint> #include <memory> +#include <string> +#include <utility> #include <vector> +#include "api/environment/environment.h" #include "api/test/rtc_error_matchers.h" #include "api/transport/stun.h" #include "api/units/time_delta.h" @@ -23,6 +26,7 @@ #include "rtc_base/logging.h" #include "rtc_base/thread.h" #include "rtc_base/time_utils.h" +#include "test/create_test_environment.h" #include "test/gmock.h" #include "test/gtest.h" #include "test/wait_until.h" @@ -47,10 +51,13 @@ } } // namespace +class StunRequestThunker; + class StunRequestTest : public ::testing::Test { public: StunRequestTest() - : manager_(Thread::Current(), + : env_(CreateTestEnvironment()), + manager_(Thread::Current(), [this](const void* data, size_t size, StunRequest* request) { OnSendPacket(data, size, request); }), @@ -60,6 +67,8 @@ failure_(false), timeout_(false) {} + std::unique_ptr<StunRequestThunker> CreateStunRequest(); + void OnSendPacket(const void* data, size_t size, StunRequest* req) { request_count_++; } @@ -76,6 +85,7 @@ protected: AutoThread main_thread_; + const Environment env_; StunRequestManager manager_; int request_count_; StunMessage* response_; @@ -87,8 +97,10 @@ // Forwards results to the test class. class StunRequestThunker : public StunRequest { public: - StunRequestThunker(StunRequestManager& manager, StunRequestTest* test) - : StunRequest(manager, CreateStunMessage(STUN_BINDING_REQUEST)), + StunRequestThunker(const Environment& env, + StunRequestManager& manager, + StunRequestTest* test) + : StunRequest(env, manager, CreateStunMessage(STUN_BINDING_REQUEST)), test_(test) { SetAuthenticationRequired(false); } @@ -107,12 +119,16 @@ StunRequestTest* test_; }; +std::unique_ptr<StunRequestThunker> StunRequestTest::CreateStunRequest() { + return std::make_unique<StunRequestThunker>(env_, manager_, this); +} + // Test handling of a normal binding response. TEST_F(StunRequestTest, TestSuccess) { - auto* request = new StunRequestThunker(manager_, this); + std::unique_ptr<StunRequestThunker> request = CreateStunRequest(); std::unique_ptr<StunMessage> res = request->CreateResponseMessage(STUN_BINDING_RESPONSE); - manager_.Send(request); + manager_.Send(std::move(request)); EXPECT_TRUE(manager_.CheckResponse(res.get())); EXPECT_TRUE(response_ == res.get()); @@ -123,10 +139,10 @@ // Test handling of an error binding response. TEST_F(StunRequestTest, TestError) { - auto* request = new StunRequestThunker(manager_, this); + std::unique_ptr<StunRequestThunker> request = CreateStunRequest(); std::unique_ptr<StunMessage> res = request->CreateResponseMessage(STUN_BINDING_ERROR_RESPONSE); - manager_.Send(request); + manager_.Send(std::move(request)); EXPECT_TRUE(manager_.CheckResponse(res.get())); EXPECT_TRUE(response_ == res.get()); @@ -137,10 +153,10 @@ // Test handling of a binding response with the wrong transaction id. TEST_F(StunRequestTest, TestUnexpected) { - auto* request = new StunRequestThunker(manager_, this); + std::unique_ptr<StunRequestThunker> request = CreateStunRequest(); std::unique_ptr<StunMessage> res = CreateStunMessage(STUN_BINDING_RESPONSE); - manager_.Send(request); + manager_.Send(std::move(request)); EXPECT_FALSE(manager_.CheckResponse(res.get())); EXPECT_TRUE(response_ == nullptr); @@ -152,12 +168,12 @@ // Test that requests are sent at the right times. TEST_F(StunRequestTest, TestBackoff) { ScopedFakeClock fake_clock; - auto* request = new StunRequestThunker(manager_, this); + std::unique_ptr<StunRequestThunker> request = CreateStunRequest(); std::unique_ptr<StunMessage> res = request->CreateResponseMessage(STUN_BINDING_RESPONSE); int64_t start = TimeMillis(); - manager_.Send(request); + manager_.Send(std::move(request)); for (int i = 0; i < 9; ++i) { EXPECT_THAT(WaitUntil([&] { return request_count_; }, Ne(i), {.timeout = TimeDelta::Millis(STUN_TOTAL_TIMEOUT), @@ -179,11 +195,11 @@ // Test that we timeout properly if no response is received. TEST_F(StunRequestTest, TestTimeout) { ScopedFakeClock fake_clock; - auto* request = new StunRequestThunker(manager_, this); + std::unique_ptr<StunRequestThunker> request = CreateStunRequest(); std::unique_ptr<StunMessage> res = request->CreateResponseMessage(STUN_BINDING_RESPONSE); - manager_.Send(request); + manager_.Send(std::move(request)); SIMULATED_WAIT(false, STUN_TOTAL_TIMEOUT, fake_clock); EXPECT_FALSE(manager_.CheckResponse(res.get())); @@ -196,11 +212,12 @@ // Regression test for specific crash where we receive a response with the // same id as a request that doesn't have an underlying StunMessage yet. TEST_F(StunRequestTest, TestNoEmptyRequest) { - StunRequestThunker* request = new StunRequestThunker(manager_, this); + std::unique_ptr<StunRequestThunker> request = CreateStunRequest(); + std::string request_id = request->id(); - manager_.SendDelayed(request, 100); + manager_.Send(std::move(request), /*delay=*/TimeDelta::Millis(100)); - StunMessage dummy_req(0, request->id()); + StunMessage dummy_req(0, request_id); std::unique_ptr<StunMessage> res = CreateStunMessage(STUN_BINDING_RESPONSE, &dummy_req); @@ -216,11 +233,11 @@ // which is not recognized, the transaction should be considered a failure and // the response should be ignored. TEST_F(StunRequestTest, TestUnrecognizedComprehensionRequiredAttribute) { - auto* request = new StunRequestThunker(manager_, this); + std::unique_ptr<StunRequestThunker> request = CreateStunRequest(); std::unique_ptr<StunMessage> res = request->CreateResponseMessage(STUN_BINDING_ERROR_RESPONSE); - manager_.Send(request); + manager_.Send(std::move(request)); res->AddAttribute(StunAttribute::CreateUInt32(0x7777)); EXPECT_FALSE(manager_.CheckResponse(res.get())); @@ -243,10 +260,10 @@ }; TEST_F(StunRequestReentranceTest, TestSuccess) { - auto* request = new StunRequestThunker(manager_, this); + std::unique_ptr<StunRequestThunker> request = CreateStunRequest(); std::unique_ptr<StunMessage> res = request->CreateResponseMessage(STUN_BINDING_RESPONSE); - manager_.Send(request); + manager_.Send(std::move(request)); EXPECT_TRUE(manager_.CheckResponse(res.get())); EXPECT_TRUE(response_ == res.get()); @@ -256,10 +273,10 @@ } TEST_F(StunRequestReentranceTest, TestError) { - auto* request = new StunRequestThunker(manager_, this); + std::unique_ptr<StunRequestThunker> request = CreateStunRequest(); std::unique_ptr<StunMessage> res = request->CreateResponseMessage(STUN_BINDING_ERROR_RESPONSE); - manager_.Send(request); + manager_.Send(std::move(request)); EXPECT_TRUE(manager_.CheckResponse(res.get())); EXPECT_TRUE(response_ == res.get());
diff --git a/p2p/base/turn_port.cc b/p2p/base/turn_port.cc index cff27b0..fe18fa6 100644 --- a/p2p/base/turn_port.cc +++ b/p2p/base/turn_port.cc
@@ -21,6 +21,7 @@ #include <vector> #include "absl/algorithm/container.h" +#include "absl/memory/memory.h" #include "absl/strings/match.h" #include "absl/strings/string_view.h" #include "api/array_view.h" @@ -51,6 +52,7 @@ #include "rtc_base/network.h" #include "rtc_base/network/received_packet.h" #include "rtc_base/network/sent_packet.h" +#include "rtc_base/platform_thread_types.h" #include "rtc_base/socket.h" #include "rtc_base/socket_address.h" #include "rtc_base/ssl_certificate.h" @@ -1162,7 +1164,7 @@ } void TurnPort::SendRequest(StunRequest* req, int delay) { - request_manager_.SendDelayed(req, delay); + request_manager_.Send(absl::WrapUnique(req), TimeDelta::Millis(delay)); } void TurnPort::AddRequestAuthInfo(StunMessage* msg) { @@ -1338,7 +1340,8 @@ } TurnAllocateRequest::TurnAllocateRequest(TurnPort* port) - : StunRequest(port->request_manager(), + : StunRequest(port->env(), + port->request_manager(), std::make_unique<TurnMessage>(TURN_ALLOCATE_REQUEST)), port_(port) { StunMessage* message = mutable_msg(); @@ -1536,7 +1539,8 @@ } TurnRefreshRequest::TurnRefreshRequest(TurnPort* port, int lifetime /*= -1*/) - : StunRequest(port->request_manager(), + : StunRequest(port->env(), + port->request_manager(), std::make_unique<TurnMessage>(TURN_REFRESH_REQUEST)), port_(port) { StunMessage* message = mutable_msg(); @@ -1623,6 +1627,7 @@ TurnEntry* entry, const SocketAddress& ext_addr) : StunRequest( + port->env(), port->request_manager(), std::make_unique<TurnMessage>(TURN_CREATE_PERMISSION_REQUEST)), port_(port), @@ -1692,7 +1697,8 @@ TurnEntry* entry, uint16_t channel_id, const SocketAddress& ext_addr) - : StunRequest(port->request_manager(), + : StunRequest(port->env(), + port->request_manager(), std::make_unique<TurnMessage>(TURN_CHANNEL_BIND_REQUEST)), port_(port), entry_(entry),
diff --git a/p2p/client/basic_port_allocator_unittest.cc b/p2p/client/basic_port_allocator_unittest.cc index 2d543ee..48eaff1 100644 --- a/p2p/client/basic_port_allocator_unittest.cc +++ b/p2p/client/basic_port_allocator_unittest.cc
@@ -145,7 +145,7 @@ port->GetProtocol() == webrtc::PROTO_UDP)) { EXPECT_EQ( static_cast<const webrtc::UDPPort*>(port)->stun_keepalive_delay(), - expected); + webrtc::TimeDelta::Millis(expected)); } } }