Move SendBindingResponse to Connection
This patch moves the SendBindingResponse from Port
to Connection. This is a behavioural NOP, and I don't
understand why it was in Port in the firs place!
Found when working on GOOG_PING.
BUG=webrtc:11100
Change-Id: I0466c5381f08ec4926ca3380e6914f0bc0dfcf63
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/161081
Commit-Queue: Jonas Oreland <jonaso@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#29963}
diff --git a/p2p/base/connection.cc b/p2p/base/connection.cc
index bea98cf..fbbd853 100644
--- a/p2p/base/connection.cc
+++ b/p2p/base/connection.cc
@@ -536,7 +536,7 @@
msg->reduced_transaction_id());
// This is a validated stun request from remote peer.
- port_->SendBindingResponse(msg, remote_addr);
+ SendBindingResponse(msg);
// If it timed out on writing check, start up again
if (!pruned_ && write_state_ == STATE_WRITE_TIMEOUT) {
@@ -587,6 +587,72 @@
}
}
+void Connection::SendBindingResponse(const StunMessage* request) {
+ RTC_DCHECK(request->type() == STUN_BINDING_REQUEST);
+
+ // Where I send the response.
+ const rtc::SocketAddress& addr = remote_candidate_.address();
+
+ // Retrieve the username from the request.
+ const StunByteStringAttribute* username_attr =
+ request->GetByteString(STUN_ATTR_USERNAME);
+ RTC_DCHECK(username_attr != NULL);
+ if (username_attr == NULL) {
+ // No valid username, skip the response.
+ return;
+ }
+
+ // Fill in the response message.
+ StunMessage response;
+ response.SetType(STUN_BINDING_RESPONSE);
+ response.SetTransactionID(request->transaction_id());
+ const StunUInt32Attribute* retransmit_attr =
+ request->GetUInt32(STUN_ATTR_RETRANSMIT_COUNT);
+ if (retransmit_attr) {
+ // Inherit the incoming retransmit value in the response so the other side
+ // can see our view of lost pings.
+ response.AddAttribute(std::make_unique<StunUInt32Attribute>(
+ STUN_ATTR_RETRANSMIT_COUNT, retransmit_attr->value()));
+
+ if (retransmit_attr->value() > CONNECTION_WRITE_CONNECT_FAILURES) {
+ RTC_LOG(LS_INFO)
+ << ToString()
+ << ": Received a remote ping with high retransmit count: "
+ << retransmit_attr->value();
+ }
+ }
+
+ response.AddAttribute(std::make_unique<StunXorAddressAttribute>(
+ STUN_ATTR_XOR_MAPPED_ADDRESS, addr));
+ response.AddMessageIntegrity(local_candidate().password());
+ response.AddFingerprint();
+
+ // Send the response message.
+ rtc::ByteBufferWriter buf;
+ response.Write(&buf);
+ rtc::PacketOptions options(port_->StunDscpValue());
+ options.info_signaled_after_sent.packet_type =
+ rtc::PacketType::kIceConnectivityCheckResponse;
+ auto err = port_->SendTo(buf.Data(), buf.Length(), addr, options, false);
+ if (err < 0) {
+ RTC_LOG(LS_ERROR) << ToString()
+ << ": Failed to send STUN ping response, to="
+ << addr.ToSensitiveString() << ", err=" << err
+ << ", id=" << rtc::hex_encode(response.transaction_id());
+ } else {
+ // Log at LS_INFO if we send a stun ping response on an unwritable
+ // connection.
+ rtc::LoggingSeverity sev = (!writable()) ? rtc::LS_INFO : rtc::LS_VERBOSE;
+ RTC_LOG_V(sev) << ToString() << ": Sent STUN ping response, to="
+ << addr.ToSensitiveString()
+ << ", id=" << rtc::hex_encode(response.transaction_id());
+
+ stats_.sent_ping_responses++;
+ LogCandidatePairEvent(webrtc::IceCandidatePairEventType::kCheckResponseSent,
+ request->reduced_transaction_id());
+ }
+}
+
void Connection::OnReadyToSend() {
SignalReadyToSend(this);
}
diff --git a/p2p/base/connection.h b/p2p/base/connection.h
index bc37429..39066f4 100644
--- a/p2p/base/connection.h
+++ b/p2p/base/connection.h
@@ -184,13 +184,6 @@
// a nomination value. The controlling agent gets its |acked_nomination_| set
// when receiving a response to a nominating ping.
bool nominated() const { return acked_nomination_ || remote_nomination_; }
- // Public for unit tests.
- void set_remote_nomination(uint32_t remote_nomination) {
- remote_nomination_ = remote_nomination;
- }
- // Public for unit tests.
- uint32_t acked_nomination() const { return acked_nomination_; }
-
void set_remote_ice_mode(IceMode mode) { remote_ice_mode_ = mode; }
int receiving_timeout() const;
@@ -300,13 +293,23 @@
// Check if we sent |val| pings without receving a response.
bool TooManyOutstandingPings(const absl::optional<int>& val) const;
+ void SetIceFieldTrials(const IceFieldTrials* field_trials);
+ const rtc::EventBasedExponentialMovingAverage& GetRttEstimate() const {
+ return rtt_estimate_;
+ }
+
+ void SendBindingResponse(const StunMessage* request);
+
// An accessor for unit tests.
Port* PortForTest() { return port_; }
const Port* PortForTest() const { return port_; }
- void SetIceFieldTrials(const IceFieldTrials* field_trials);
- const rtc::EventBasedExponentialMovingAverage& GetRttEstimate() const {
- return rtt_estimate_;
+ // Public for unit tests.
+ uint32_t acked_nomination() const { return acked_nomination_; }
+
+ // Public for unit tests.
+ void set_remote_nomination(uint32_t remote_nomination) {
+ remote_nomination_ = remote_nomination;
}
protected:
diff --git a/p2p/base/fake_port_allocator.h b/p2p/base/fake_port_allocator.h
index ab61c80..4fafb54 100644
--- a/p2p/base/fake_port_allocator.h
+++ b/p2p/base/fake_port_allocator.h
@@ -48,15 +48,6 @@
}
return port;
}
- void SendBindingResponse(StunMessage* request,
- const rtc::SocketAddress& addr) override {
- UDPPort::SendBindingResponse(request, addr);
- sent_binding_response_ = true;
- }
- bool sent_binding_response() { return sent_binding_response_; }
- void set_sent_binding_response(bool response) {
- sent_binding_response_ = response;
- }
protected:
TestUDPPort(rtc::Thread* thread,
@@ -77,8 +68,6 @@
password,
origin,
emit_localhost_for_anyaddress) {}
-
- bool sent_binding_response_ = false;
};
// A FakePortAllocatorSession can be used with either a real or fake socket
diff --git a/p2p/base/p2p_transport_channel_unittest.cc b/p2p/base/p2p_transport_channel_unittest.cc
index 9cc58ac..ac28d46 100644
--- a/p2p/base/p2p_transport_channel_unittest.cc
+++ b/p2p/base/p2p_transport_channel_unittest.cc
@@ -3808,11 +3808,10 @@
&request, kIceUfrag[1], false);
Connection* conn1 = WaitForConnectionTo(&ch, "1.1.1.1", 1);
ASSERT_TRUE(conn1 != nullptr);
- EXPECT_TRUE(port->sent_binding_response());
+ EXPECT_EQ(conn1->stats().sent_ping_responses, 1u);
EXPECT_NE(conn1, ch.selected_connection());
conn1->ReceivedPingResponse(LOW_RTT, "id");
EXPECT_EQ_WAIT(conn1, ch.selected_connection(), kDefaultTimeout);
- port->set_sent_binding_response(false);
// Another connection is nominated via use_candidate.
ch.AddRemoteCandidate(CreateUdpCandidate(LOCAL_PORT_TYPE, "2.2.2.2", 2, 1));
@@ -3833,10 +3832,9 @@
&request, kIceUfrag[1], false);
Connection* conn3 = WaitForConnectionTo(&ch, "3.3.3.3", 3);
ASSERT_TRUE(conn3 != nullptr);
- EXPECT_TRUE(port->sent_binding_response());
+ EXPECT_EQ(conn3->stats().sent_ping_responses, 1u);
conn3->ReceivedPingResponse(LOW_RTT, "id"); // Become writable.
EXPECT_EQ(conn2, ch.selected_connection());
- port->set_sent_binding_response(false);
// However if the request contains use_candidate attribute, it will be
// selected as the selected connection.
@@ -3846,7 +3844,7 @@
&request, kIceUfrag[1], false);
Connection* conn4 = WaitForConnectionTo(&ch, "4.4.4.4", 4);
ASSERT_TRUE(conn4 != nullptr);
- EXPECT_TRUE(port->sent_binding_response());
+ EXPECT_EQ(conn4->stats().sent_ping_responses, 1u);
// conn4 is not the selected connection yet because it is not writable.
EXPECT_EQ(conn2, ch.selected_connection());
conn4->ReceivedPingResponse(LOW_RTT, "id"); // Become writable.
@@ -3854,14 +3852,14 @@
// Test that the request from an unknown address contains a ufrag from an old
// generation.
- port->set_sent_binding_response(false);
+ // port->set_sent_binding_response(false);
ch.SetRemoteIceParameters(kIceParams[2]);
ch.SetRemoteIceParameters(kIceParams[3]);
port->SignalUnknownAddress(port, rtc::SocketAddress("5.5.5.5", 5), PROTO_UDP,
&request, kIceUfrag[2], false);
Connection* conn5 = WaitForConnectionTo(&ch, "5.5.5.5", 5);
ASSERT_TRUE(conn5 != nullptr);
- EXPECT_TRUE(port->sent_binding_response());
+ EXPECT_EQ(conn5->stats().sent_ping_responses, 1u);
EXPECT_EQ(kIcePwd[2], conn5->remote_candidate().password());
}
diff --git a/p2p/base/port.cc b/p2p/base/port.cc
index 742c15d..b92f122 100644
--- a/p2p/base/port.cc
+++ b/p2p/base/port.cc
@@ -661,73 +661,6 @@
return false;
}
-void Port::SendBindingResponse(StunMessage* request,
- const rtc::SocketAddress& addr) {
- RTC_DCHECK(request->type() == STUN_BINDING_REQUEST);
-
- // Retrieve the username from the request.
- const StunByteStringAttribute* username_attr =
- request->GetByteString(STUN_ATTR_USERNAME);
- RTC_DCHECK(username_attr != NULL);
- if (username_attr == NULL) {
- // No valid username, skip the response.
- return;
- }
-
- // Fill in the response message.
- StunMessage response;
- response.SetType(STUN_BINDING_RESPONSE);
- response.SetTransactionID(request->transaction_id());
- const StunUInt32Attribute* retransmit_attr =
- request->GetUInt32(STUN_ATTR_RETRANSMIT_COUNT);
- if (retransmit_attr) {
- // Inherit the incoming retransmit value in the response so the other side
- // can see our view of lost pings.
- response.AddAttribute(std::make_unique<StunUInt32Attribute>(
- STUN_ATTR_RETRANSMIT_COUNT, retransmit_attr->value()));
-
- if (retransmit_attr->value() > CONNECTION_WRITE_CONNECT_FAILURES) {
- RTC_LOG(LS_INFO)
- << ToString()
- << ": Received a remote ping with high retransmit count: "
- << retransmit_attr->value();
- }
- }
-
- response.AddAttribute(std::make_unique<StunXorAddressAttribute>(
- STUN_ATTR_XOR_MAPPED_ADDRESS, addr));
- response.AddMessageIntegrity(password_);
- response.AddFingerprint();
-
- // Send the response message.
- rtc::ByteBufferWriter buf;
- response.Write(&buf);
- rtc::PacketOptions options(StunDscpValue());
- options.info_signaled_after_sent.packet_type =
- rtc::PacketType::kIceConnectivityCheckResponse;
- auto err = SendTo(buf.Data(), buf.Length(), addr, options, false);
- if (err < 0) {
- RTC_LOG(LS_ERROR) << ToString()
- << ": Failed to send STUN ping response, to="
- << addr.ToSensitiveString() << ", err=" << err
- << ", id=" << rtc::hex_encode(response.transaction_id());
- } else {
- // Log at LS_INFO if we send a stun ping response on an unwritable
- // connection.
- Connection* conn = GetConnection(addr);
- rtc::LoggingSeverity sev =
- (conn && !conn->writable()) ? rtc::LS_INFO : rtc::LS_VERBOSE;
- RTC_LOG_V(sev) << ToString() << ": Sent STUN ping response, to="
- << addr.ToSensitiveString()
- << ", id=" << rtc::hex_encode(response.transaction_id());
-
- conn->stats_.sent_ping_responses++;
- conn->LogCandidatePairEvent(
- webrtc::IceCandidatePairEventType::kCheckResponseSent,
- request->reduced_transaction_id());
- }
-}
-
void Port::SendBindingErrorResponse(StunMessage* request,
const rtc::SocketAddress& addr,
int error_code,
diff --git a/p2p/base/port.h b/p2p/base/port.h
index d609922..84340e8 100644
--- a/p2p/base/port.h
+++ b/p2p/base/port.h
@@ -287,11 +287,7 @@
virtual bool CanHandleIncomingPacketsFrom(
const rtc::SocketAddress& remote_addr) const;
- // Sends a response message (normal or error) to the given request. One of
- // these methods should be called as a response to SignalUnknownAddress.
- // NOTE: You MUST call CreateConnection BEFORE SendBindingResponse.
- void SendBindingResponse(StunMessage* request,
- const rtc::SocketAddress& addr) override;
+ // Sends a response error to the given request.
void SendBindingErrorResponse(StunMessage* request,
const rtc::SocketAddress& addr,
int error_code,
diff --git a/p2p/base/port_interface.h b/p2p/base/port_interface.h
index 24f2e2a..39eae18 100644
--- a/p2p/base/port_interface.h
+++ b/p2p/base/port_interface.h
@@ -105,9 +105,6 @@
// Sends a response message (normal or error) to the given request. One of
// these methods should be called as a response to SignalUnknownAddress.
- // NOTE: You MUST call CreateConnection BEFORE SendBindingResponse.
- virtual void SendBindingResponse(StunMessage* request,
- const rtc::SocketAddress& addr) = 0;
virtual void SendBindingErrorResponse(StunMessage* request,
const rtc::SocketAddress& addr,
int error_code,
diff --git a/p2p/base/port_unittest.cc b/p2p/base/port_unittest.cc
index ec2a872..55ff5be 100644
--- a/p2p/base/port_unittest.cc
+++ b/p2p/base/port_unittest.cc
@@ -302,7 +302,7 @@
c.set_address(remote_address_);
conn_ = port_->CreateConnection(c, Port::ORIGIN_MESSAGE);
conn_->SignalDestroyed.connect(this, &TestChannel::OnDestroyed);
- port_->SendBindingResponse(remote_request_.get(), remote_address_);
+ conn_->SendBindingResponse(remote_request_.get());
remote_request_.reset();
}
void Ping() { Ping(0); }
@@ -2618,11 +2618,10 @@
// NOTE: Ideally we should't create connection at this stage from lite
// port, as it should be done only after receiving ping with USE_CANDIDATE.
// But we need a connection to send a response message.
- ice_lite_port->CreateConnection(ice_full_port_ptr->Candidates()[0],
- cricket::Port::ORIGIN_MESSAGE);
+ auto* con = ice_lite_port->CreateConnection(
+ ice_full_port_ptr->Candidates()[0], cricket::Port::ORIGIN_MESSAGE);
std::unique_ptr<IceMessage> request = CopyStunMessage(*msg);
- ice_lite_port->SendBindingResponse(
- request.get(), ice_full_port_ptr->Candidates()[0].address());
+ con->SendBindingResponse(request.get());
// Feeding the respone message from litemode to the full mode connection.
ch1.conn()->OnReadPacket(ice_lite_port->last_stun_buf()->data<char>(),
