diff --git a/p2p/base/connection.cc b/p2p/base/connection.cc index 6fc93ed91b..2b04aa5147 100644 --- a/p2p/base/connection.cc +++ b/p2p/base/connection.cc @@ -152,6 +152,9 @@ void ConnectionRequest::Prepare(StunMessage* request) { std::string username; connection_->port()->CreateStunUsername( connection_->remote_candidate().username(), &username); + // Note that the order of attributes does not impact the parsing on the + // receiver side. The attribute is retrieved then by iterating and matching + // over all parsed attributes. See StunMessage::GetAttribute. request->AddAttribute( absl::make_unique(STUN_ATTR_USERNAME, username)); @@ -167,6 +170,13 @@ void ConnectionRequest::Prepare(StunMessage* request) { request->AddAttribute(absl::make_unique( STUN_ATTR_NETWORK_INFO, network_info)); + if (webrtc::field_trial::IsEnabled("WebRTC-PiggybackCheckAcknowledgement") && + connection_->last_ping_id_received()) { + request->AddAttribute(absl::make_unique( + STUN_ATTR_LAST_ICE_CHECK_RECEIVED, + connection_->last_ping_id_received().value())); + } + // Adding ICE_CONTROLLED or ICE_CONTROLLING attribute based on the role. if (connection_->port()->GetIceRole() == ICEROLE_CONTROLLING) { request->AddAttribute(absl::make_unique( @@ -455,7 +465,7 @@ void Connection::OnReadPacket(const char* data, // request. In this case |last_ping_received_| will be updated but no // response will be sent. case STUN_BINDING_INDICATION: - ReceivedPing(); + ReceivedPing(msg->transaction_id()); break; default: @@ -467,7 +477,7 @@ void Connection::OnReadPacket(const char* data, void Connection::HandleBindingRequest(IceMessage* msg) { // This connection should now be receiving. - ReceivedPing(); + ReceivedPing(msg->transaction_id()); if (webrtc::field_trial::IsEnabled("WebRTC-ExtraICEPing") && last_ping_response_received_ == 0) { if (local_candidate().type() == RELAY_PORT_TYPE || @@ -550,6 +560,10 @@ void Connection::HandleBindingRequest(IceMessage* msg) { SignalStateChange(this); } } + + if (webrtc::field_trial::IsEnabled("WebRTC-PiggybackCheckAcknowledgement")) { + HandlePiggybackCheckAcknowledgementIfAny(msg); + } } void Connection::OnReadyToSend() { @@ -678,24 +692,40 @@ void Connection::Ping(int64_t now) { num_pings_sent_++; } -void Connection::ReceivedPing() { +void Connection::ReceivedPing(const absl::optional& request_id) { last_ping_received_ = rtc::TimeMillis(); + last_ping_id_received_ = request_id; UpdateReceiving(last_ping_received_); } -void Connection::ReceivedPingResponse(int rtt, const std::string& request_id) { +void Connection::HandlePiggybackCheckAcknowledgementIfAny(StunMessage* msg) { + RTC_DCHECK(msg->type() == STUN_BINDING_REQUEST); + const StunByteStringAttribute* last_ice_check_received_attr = + msg->GetByteString(STUN_ATTR_LAST_ICE_CHECK_RECEIVED); + if (last_ice_check_received_attr) { + const std::string request_id = last_ice_check_received_attr->GetString(); + auto iter = absl::c_find_if( + pings_since_last_response_, + [&request_id](const SentPing& ping) { return ping.id == request_id; }); + if (iter != pings_since_last_response_.end()) { + const int64_t rtt = rtc::TimeMillis() - iter->sent_time; + ReceivedPingResponse(rtt, request_id, iter->nomination); + } + } +} + +void Connection::ReceivedPingResponse( + int rtt, + const std::string& request_id, + const absl::optional& nomination) { RTC_DCHECK_GE(rtt, 0); // We've already validated that this is a STUN binding response with // the correct local and remote username for this connection. // So if we're not already, become writable. We may be bringing a pruned // connection back to life, but if we don't really want it, we can always // prune it again. - auto iter = absl::c_find_if( - pings_since_last_response_, - [request_id](const SentPing& ping) { return ping.id == request_id; }); - if (iter != pings_since_last_response_.end() && - iter->nomination > acked_nomination_) { - acked_nomination_ = iter->nomination; + if (nomination && nomination.value() > acked_nomination_) { + acked_nomination_ = nomination.value(); } total_round_trip_time_ms_ += rtt; @@ -864,7 +894,15 @@ void Connection::OnConnectionRequestResponse(ConnectionRequest* request, ", rtt=" << rtt << ", pings_since_last_response=" << pings; } - ReceivedPingResponse(rtt, request->id()); + absl::optional nomination; + const std::string request_id = request->id(); + auto iter = absl::c_find_if( + pings_since_last_response_, + [&request_id](const SentPing& ping) { return ping.id == request_id; }); + if (iter != pings_since_last_response_.end()) { + nomination.emplace(iter->nomination); + } + ReceivedPingResponse(rtt, request_id, nomination); stats_.recv_ping_responses++; LogCandidatePairEvent( diff --git a/p2p/base/connection.h b/p2p/base/connection.h index 601d34cc08..82b2c8924e 100644 --- a/p2p/base/connection.h +++ b/p2p/base/connection.h @@ -202,20 +202,30 @@ class Connection : public CandidatePairInterface, // Called when this connection should try checking writability again. int64_t last_ping_sent() const { return last_ping_sent_; } void Ping(int64_t now); - void ReceivedPingResponse(int rtt, const std::string& request_id); + void ReceivedPingResponse( + int rtt, + const std::string& request_id, + const absl::optional& nomination = absl::nullopt); int64_t last_ping_response_received() const { return last_ping_response_received_; } + const absl::optional& last_ping_id_received() const { + return last_ping_id_received_; + } // Used to check if any STUN ping response has been received. int rtt_samples() const { return rtt_samples_; } // Called whenever a valid ping is received on this connection. This is // public because the connection intercepts the first ping for us. int64_t last_ping_received() const { return last_ping_received_; } - void ReceivedPing(); + void ReceivedPing( + const absl::optional& request_id = absl::nullopt); // Handles the binding request; sends a response if this is a valid request. void HandleBindingRequest(IceMessage* msg); - + // Handles the piggyback acknowledgement of the lastest connectivity check + // that the remote peer has received, if it is indicated in the incoming + // connectivity check from the peer. + void HandlePiggybackCheckAcknowledgementIfAny(StunMessage* msg); int64_t last_data_received() const { return last_data_received_; } // Debugging description of this connection @@ -347,8 +357,8 @@ class Connection : public CandidatePairInterface, // The last nomination that has been acknowledged. uint32_t acked_nomination_ = 0; // Used by the controlled side to remember the nomination value received from - // the controlling side. When the peer does not support ICE re-nomination, - // its value will be 1 if the connection has been nominated. + // the controlling side. When the peer does not support ICE re-nomination, its + // value will be 1 if the connection has been nominated. uint32_t remote_nomination_ = 0; IceMode remote_ice_mode_; @@ -366,6 +376,9 @@ class Connection : public CandidatePairInterface, int64_t last_ping_response_received_; int64_t receiving_unchanged_since_ = 0; std::vector pings_since_last_response_; + // Transaction ID of the last connectivity check received. Null if having not + // received a ping yet. + absl::optional last_ping_id_received_; absl::optional unwritable_timeout_; absl::optional unwritable_min_checks_; diff --git a/p2p/base/p2p_transport_channel_unittest.cc b/p2p/base/p2p_transport_channel_unittest.cc index c5bcbdd817..b97e66bb64 100644 --- a/p2p/base/p2p_transport_channel_unittest.cc +++ b/p2p/base/p2p_transport_channel_unittest.cc @@ -2256,6 +2256,55 @@ TEST_F(P2PTransportChannelTest, DestroyChannels(); } +// Test that the writability can be established with the piggyback +// acknowledgement in the connectivity check from the remote peer. +TEST_F(P2PTransportChannelTest, + CanConnectWithPiggybackCheckAcknowledgementWhenCheckResponseBlocked) { + webrtc::test::ScopedFieldTrials field_trials( + "WebRTC-PiggybackCheckAcknowledgement/Enabled/"); + rtc::ScopedFakeClock clock; + ConfigureEndpoints(OPEN, OPEN, kOnlyLocalPorts, kOnlyLocalPorts); + IceConfig ep1_config; + IceConfig ep2_config = CreateIceConfig(1000, GATHER_CONTINUALLY); + // Let ep2 be tolerable of the loss of connectivity checks, so that it keeps + // sending pings even after ep1 becomes unwritable as we configure the + // firewall below. + ep2_config.receiving_timeout = 30 * 1000; + ep2_config.ice_unwritable_timeout = 30 * 1000; + ep2_config.ice_unwritable_min_checks = 30; + ep2_config.ice_inactive_timeout = 60 * 1000; + + CreateChannels(ep1_config, ep2_config); + + // Wait until both sides become writable for the first time. + EXPECT_TRUE_SIMULATED_WAIT( + ep1_ch1() != nullptr && ep2_ch1() != nullptr && ep1_ch1()->receiving() && + ep1_ch1()->writable() && ep2_ch1()->receiving() && + ep2_ch1()->writable(), + kDefaultTimeout, clock); + // Block the ingress traffic to ep1 so that there is no check response from + // ep2. + ASSERT_NE(nullptr, LocalCandidate(ep1_ch1())); + fw()->AddRule(false, rtc::FP_ANY, rtc::FD_IN, + LocalCandidate(ep1_ch1())->address()); + // Wait until ep1 becomes unwritable. At the same time ep2 should be still + // fine so that it will keep sending pings. + EXPECT_TRUE_SIMULATED_WAIT(ep1_ch1() != nullptr && !ep1_ch1()->writable(), + kDefaultTimeout, clock); + EXPECT_TRUE(ep2_ch1() != nullptr && ep2_ch1()->writable()); + // Now let the pings from ep2 to flow but block any pings from ep1, so that + // ep1 can only become writable again after receiving an incoming ping from + // ep2 with piggyback acknowledgement of its previously sent pings. Note + // though that ep1 should have stopped sending pings after becoming unwritable + // in the current design. + fw()->ClearRules(); + fw()->AddRule(false, rtc::FP_ANY, rtc::FD_OUT, + LocalCandidate(ep1_ch1())->address()); + EXPECT_TRUE_SIMULATED_WAIT(ep1_ch1() != nullptr && ep1_ch1()->writable(), + kDefaultTimeout, clock); + DestroyChannels(); +} + // Test what happens when we have 2 users behind the same NAT. This can lead // to interesting behavior because the STUN server will only give out the // address of the outermost NAT. @@ -3187,10 +3236,12 @@ class P2PTransportChannelPingTest : public ::testing::Test, ++selected_candidate_pair_switches_; } - void ReceivePingOnConnection(Connection* conn, - const std::string& remote_ufrag, - int priority, - uint32_t nomination = 0) { + void ReceivePingOnConnection( + Connection* conn, + const std::string& remote_ufrag, + int priority, + uint32_t nomination, + const absl::optional& piggyback_ping_id) { IceMessage msg; msg.SetType(STUN_BINDING_REQUEST); msg.AddAttribute(absl::make_unique( @@ -3202,6 +3253,10 @@ class P2PTransportChannelPingTest : public ::testing::Test, msg.AddAttribute(absl::make_unique( STUN_ATTR_NOMINATION, nomination)); } + if (piggyback_ping_id) { + msg.AddAttribute(absl::make_unique( + STUN_ATTR_LAST_ICE_CHECK_RECEIVED, piggyback_ping_id.value())); + } msg.SetTransactionID(rtc::CreateRandomString(kStunTransactionIdLength)); msg.AddMessageIntegrity(conn->local_candidate().password()); msg.AddFingerprint(); @@ -3210,6 +3265,14 @@ class P2PTransportChannelPingTest : public ::testing::Test, conn->OnReadPacket(buf.Data(), buf.Length(), rtc::TimeMicros()); } + void ReceivePingOnConnection(Connection* conn, + const std::string& remote_ufrag, + int priority, + uint32_t nomination = 0) { + ReceivePingOnConnection(conn, remote_ufrag, priority, nomination, + absl::nullopt); + } + void OnReadyToSend(rtc::PacketTransportInternal* transport) { channel_ready_to_send_ = true; } diff --git a/p2p/base/stun.cc b/p2p/base/stun.cc index 3a44909a60..f40395bde3 100644 --- a/p2p/base/stun.cc +++ b/p2p/base/stun.cc @@ -465,6 +465,8 @@ StunAttributeValueType StunMessage::GetAttributeValueType(int type) const { return STUN_VALUE_BYTE_STRING; case STUN_ATTR_RETRANSMIT_COUNT: return STUN_VALUE_UINT32; + case STUN_ATTR_LAST_ICE_CHECK_RECEIVED: + return STUN_VALUE_BYTE_STRING; default: return STUN_VALUE_UNKNOWN; } diff --git a/p2p/base/stun.h b/p2p/base/stun.h index 33410b5bae..5b9b953db1 100644 --- a/p2p/base/stun.h +++ b/p2p/base/stun.h @@ -594,16 +594,26 @@ class TurnMessage : public StunMessage { StunMessage* CreateNew() const override; }; -// RFC 5245 ICE STUN attributes. enum IceAttributeType { + // RFC 5245 ICE STUN attributes. STUN_ATTR_PRIORITY = 0x0024, // UInt32 STUN_ATTR_USE_CANDIDATE = 0x0025, // No content, Length = 0 STUN_ATTR_ICE_CONTROLLED = 0x8029, // UInt64 STUN_ATTR_ICE_CONTROLLING = 0x802A, // UInt64 - STUN_ATTR_NOMINATION = 0xC001, // UInt32 + // The following attributes are in the comprehension-optional range + // (0xC000-0xFFFF) and are not registered with IANA. These STUN attributes are + // intended for ICE and should NOT be used in generic use cases of STUN + // messages. + // + // Note that the value 0xC001 has already been assigned by IANA to + // ENF-FLOW-DESCRIPTION + // (https://www.iana.org/assignments/stun-parameters/stun-parameters.xml). + STUN_ATTR_NOMINATION = 0xC001, // UInt32 // UInt32. The higher 16 bits are the network ID. The lower 16 bits are the // network cost. - STUN_ATTR_NETWORK_INFO = 0xC057 + STUN_ATTR_NETWORK_INFO = 0xC057, + // Experimental: Transaction ID of the last connectivity check received. + STUN_ATTR_LAST_ICE_CHECK_RECEIVED = 0xC058, }; // RFC 5245-defined errors.