diff --git a/p2p/base/stun_port.cc b/p2p/base/stun_port.cc index e846194a4c..e1a6820bb0 100644 --- a/p2p/base/stun_port.cc +++ b/p2p/base/stun_port.cc @@ -40,17 +40,14 @@ class StunBindingRequest : public StunRequest { StunBindingRequest(UDPPort* port, const rtc::SocketAddress& addr, int64_t start_time) - : StunRequest(port->request_manager()), + : StunRequest(port->request_manager(), + std::make_unique(STUN_BINDING_REQUEST)), port_(port), server_addr_(addr), start_time_(start_time) {} const rtc::SocketAddress& server_addr() const { return server_addr_; } - void Prepare(StunMessage* message) override { - message->SetType(STUN_BINDING_REQUEST); - } - void OnResponse(StunMessage* response) override { const StunAddressAttribute* addr_attr = response->GetAddress(STUN_ATTR_MAPPED_ADDRESS); diff --git a/p2p/base/stun_request.cc b/p2p/base/stun_request.cc index 868db4c6c4..2a1ad65112 100644 --- a/p2p/base/stun_request.cc +++ b/p2p/base/stun_request.cc @@ -57,7 +57,6 @@ void StunRequestManager::Send(StunRequest* request) { void StunRequestManager::SendDelayed(StunRequest* request, int delay) { RTC_DCHECK_RUN_ON(thread_); RTC_DCHECK_EQ(this, request->manager()); - request->Construct(); auto [iter, was_inserted] = requests_.emplace(request->id(), absl::WrapUnique(request)); RTC_DCHECK(was_inserted); @@ -212,18 +211,6 @@ StunRequest::~StunRequest() { manager_.network_thread()->Clear(this); } -void StunRequest::Construct() { - // TODO(tommi): The implementation assumes that Construct() is only called - // once (see `StunRequestManager::SendDelayed` below). However, these steps to - // construct a request object are odd to have at this level (virtual method - // called from the parent class, _after_ construction) and also triggered - // from a different class... via a "Send" method. - // Remove `Prepare`, `Construct` and make construction of the message objects - // separate from the StunRequest and StunRequestManager classes. - Prepare(msg_.get()); - RTC_DCHECK_NE(msg_->type(), 0); -} - int StunRequest::type() { RTC_DCHECK(msg_ != NULL); return msg_->type(); diff --git a/p2p/base/stun_request.h b/p2p/base/stun_request.h index 94fc98f917..c6d10764e4 100644 --- a/p2p/base/stun_request.h +++ b/p2p/base/stun_request.h @@ -114,14 +114,7 @@ class StunRequest : public rtc::MessageHandler { protected: friend class StunRequestManager; - // Causes our wrapped StunMessage to be Prepared. - // Only called by StunRequestManager. - // TODO(tommi): get rid of this (see cc file). - void Construct(); - - // Fills in a request object to be sent. Note that request's transaction ID - // will already be set and cannot be changed. - virtual void Prepare(StunMessage* message) {} + StunMessage* mutable_msg() { return msg_.get(); } // Called when the message receives a response or times out. virtual void OnResponse(StunMessage* response) {} diff --git a/p2p/base/stun_request_unittest.cc b/p2p/base/stun_request_unittest.cc index 99847f8b7d..6831d9ffa2 100644 --- a/p2p/base/stun_request_unittest.cc +++ b/p2p/base/stun_request_unittest.cc @@ -77,16 +77,9 @@ class StunRequestTest : public ::testing::Test { // Forwards results to the test class. class StunRequestThunker : public StunRequest { public: - StunRequestThunker(StunRequestManager& manager, - StunMessageType message_type, - StunRequestTest* test) - : StunRequest(manager, CreateStunMessage(message_type)), test_(test) { - Construct(); // Triggers a call to `Prepare()` which sets the type. - } StunRequestThunker(StunRequestManager& manager, StunRequestTest* test) - : StunRequest(manager), test_(test) { - Construct(); // Triggers a call to `Prepare()` which sets the type. - } + : StunRequest(manager, CreateStunMessage(STUN_BINDING_REQUEST)), + test_(test) {} std::unique_ptr CreateResponseMessage(StunMessageType type) { return CreateStunMessage(type, msg()); @@ -99,16 +92,12 @@ class StunRequestThunker : public StunRequest { } virtual void OnTimeout() { test_->OnTimeout(); } - virtual void Prepare(StunMessage* message) { - message->SetType(STUN_BINDING_REQUEST); - } - StunRequestTest* test_; }; // Test handling of a normal binding response. TEST_F(StunRequestTest, TestSuccess) { - auto* request = new StunRequestThunker(manager_, STUN_BINDING_REQUEST, this); + auto* request = new StunRequestThunker(manager_, this); std::unique_ptr res = request->CreateResponseMessage(STUN_BINDING_RESPONSE); manager_.Send(request); @@ -122,7 +111,7 @@ TEST_F(StunRequestTest, TestSuccess) { // Test handling of an error binding response. TEST_F(StunRequestTest, TestError) { - auto* request = new StunRequestThunker(manager_, STUN_BINDING_REQUEST, this); + auto* request = new StunRequestThunker(manager_, this); std::unique_ptr res = request->CreateResponseMessage(STUN_BINDING_ERROR_RESPONSE); manager_.Send(request); @@ -136,7 +125,7 @@ TEST_F(StunRequestTest, TestError) { // Test handling of a binding response with the wrong transaction id. TEST_F(StunRequestTest, TestUnexpected) { - auto* request = new StunRequestThunker(manager_, STUN_BINDING_REQUEST, this); + auto* request = new StunRequestThunker(manager_, this); std::unique_ptr res = CreateStunMessage(STUN_BINDING_RESPONSE); manager_.Send(request); @@ -151,7 +140,7 @@ TEST_F(StunRequestTest, TestUnexpected) { // Test that requests are sent at the right times. TEST_F(StunRequestTest, TestBackoff) { rtc::ScopedFakeClock fake_clock; - auto* request = new StunRequestThunker(manager_, STUN_BINDING_REQUEST, this); + auto* request = new StunRequestThunker(manager_, this); std::unique_ptr res = request->CreateResponseMessage(STUN_BINDING_RESPONSE); @@ -176,7 +165,7 @@ TEST_F(StunRequestTest, TestBackoff) { // Test that we timeout properly if no response is received. TEST_F(StunRequestTest, TestTimeout) { rtc::ScopedFakeClock fake_clock; - auto* request = new StunRequestThunker(manager_, STUN_BINDING_REQUEST, this); + auto* request = new StunRequestThunker(manager_, this); std::unique_ptr res = request->CreateResponseMessage(STUN_BINDING_RESPONSE); @@ -213,7 +202,7 @@ TEST_F(StunRequestTest, TestNoEmptyRequest) { // 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_, STUN_BINDING_REQUEST, this); + auto* request = new StunRequestThunker(manager_, this); std::unique_ptr res = request->CreateResponseMessage(STUN_BINDING_ERROR_RESPONSE); diff --git a/p2p/base/turn_port.cc b/p2p/base/turn_port.cc index 148fc6b4e3..6cb3da43c1 100644 --- a/p2p/base/turn_port.cc +++ b/p2p/base/turn_port.cc @@ -73,7 +73,6 @@ static int GetRelayPreference(cricket::ProtocolType proto) { class TurnAllocateRequest : public StunRequest { public: explicit TurnAllocateRequest(TurnPort* port); - void Prepare(StunMessage* message) override; void OnSent() override; void OnResponse(StunMessage* response) override; void OnErrorResponse(StunMessage* response) override; @@ -90,17 +89,14 @@ class TurnAllocateRequest : public StunRequest { class TurnRefreshRequest : public StunRequest { public: - explicit TurnRefreshRequest(TurnPort* port); - void Prepare(StunMessage* message) override; + explicit TurnRefreshRequest(TurnPort* port, int lifetime = -1); void OnSent() override; void OnResponse(StunMessage* response) override; void OnErrorResponse(StunMessage* response) override; void OnTimeout() override; - void set_lifetime(int lifetime) { lifetime_ = lifetime; } private: TurnPort* port_; - int lifetime_; }; class TurnCreatePermissionRequest : public StunRequest, @@ -110,7 +106,6 @@ class TurnCreatePermissionRequest : public StunRequest, TurnEntry* entry, const rtc::SocketAddress& ext_addr, const std::string& remote_ufrag); - void Prepare(StunMessage* message) override; void OnSent() override; void OnResponse(StunMessage* response) override; void OnErrorResponse(StunMessage* response) override; @@ -131,7 +126,6 @@ class TurnChannelBindRequest : public StunRequest, public sigslot::has_slots<> { TurnEntry* entry, int channel_id, const rtc::SocketAddress& ext_addr); - void Prepare(StunMessage* message) override; void OnSent() override; void OnResponse(StunMessage* response) override; void OnErrorResponse(StunMessage* response) override; @@ -933,8 +927,7 @@ void TurnPort::Release() { request_manager_.Clear(); // Send refresh with lifetime 0. - TurnRefreshRequest* req = new TurnRefreshRequest(this); - req->set_lifetime(0); + TurnRefreshRequest* req = new TurnRefreshRequest(this, 0); SendRequest(req, 0); state_ = STATE_RECEIVEONLY; @@ -1376,9 +1369,8 @@ void TurnPort::MaybeAddTurnLoggingId(StunMessage* msg) { TurnAllocateRequest::TurnAllocateRequest(TurnPort* port) : StunRequest(port->request_manager(), std::make_unique(TURN_ALLOCATE_REQUEST)), - port_(port) {} - -void TurnAllocateRequest::Prepare(StunMessage* message) { + port_(port) { + StunMessage* message = mutable_msg(); // Create the request as indicated in RFC 5766, Section 6.1. RTC_DCHECK_EQ(message->type(), TURN_ALLOCATE_REQUEST); auto transport_attr = @@ -1563,19 +1555,17 @@ void TurnAllocateRequest::OnTryAlternate(StunMessage* response, int code) { TurnPort::MSG_TRY_ALTERNATE_SERVER); } -TurnRefreshRequest::TurnRefreshRequest(TurnPort* port) +TurnRefreshRequest::TurnRefreshRequest(TurnPort* port, int lifetime /*= -1*/) : StunRequest(port->request_manager(), std::make_unique(TURN_REFRESH_REQUEST)), - port_(port), - lifetime_(-1) {} - -void TurnRefreshRequest::Prepare(StunMessage* message) { + port_(port) { + StunMessage* message = mutable_msg(); // Create the request as indicated in RFC 5766, Section 7.1. // No attributes need to be included. RTC_DCHECK_EQ(message->type(), TURN_REFRESH_REQUEST); - if (lifetime_ > -1) { + if (lifetime > -1) { message->AddAttribute( - std::make_unique(STUN_ATTR_LIFETIME, lifetime_)); + std::make_unique(STUN_ATTR_LIFETIME, lifetime)); } port_->AddRequestAuthInfo(message); @@ -1657,9 +1647,7 @@ TurnCreatePermissionRequest::TurnCreatePermissionRequest( remote_ufrag_(remote_ufrag) { entry_->SignalDestroyed.connect( this, &TurnCreatePermissionRequest::OnEntryDestroyed); -} - -void TurnCreatePermissionRequest::Prepare(StunMessage* message) { + StunMessage* message = mutable_msg(); // Create the request as indicated in RFC5766, Section 9.1. message->SetType(TURN_CREATE_PERMISSION_REQUEST); message->AddAttribute(std::make_unique( @@ -1731,9 +1719,7 @@ TurnChannelBindRequest::TurnChannelBindRequest( ext_addr_(ext_addr) { entry_->SignalDestroyed.connect(this, &TurnChannelBindRequest::OnEntryDestroyed); -} - -void TurnChannelBindRequest::Prepare(StunMessage* message) { + StunMessage* message = mutable_msg(); // Create the request as indicated in RFC5766, Section 11.1. RTC_DCHECK_EQ(message->type(), TURN_CHANNEL_BIND_REQUEST); message->AddAttribute(std::make_unique(