From 286ee0123e02ec364acf70357d8192468e10a7eb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Erik=20Spr=C3=A5ng?= Date: Fri, 30 Nov 2018 09:28:14 +0000 Subject: [PATCH] Revert "Add transaction id to CandidatePairEvents." MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit c308bdfa451ee2ceac7096b6777fcbf756f4091a. Reason for revert: The msan bot has been consistently failing since this commit. See eg https://ci.chromium.org/p/webrtc/builders/luci.webrtc.ci/Linux%20MSan/16989 Original change's description: > Add transaction id to CandidatePairEvents. > > The transaction id is a randomly generated number used to link stun > requests and responses (https://tools.ietf.org/html/rfc5389#section-6). > Logging this will help us debug ICE network issues. > > Bug: webrtc:9972 > Change-Id: I93167cb119aad99156e8727b6e4eeeff5198f924 > Reviewed-on: https://webrtc-review.googlesource.com/c/109720 > Commit-Queue: Zach Stein > Reviewed-by: Qingsi Wang > Reviewed-by: Elad Alon > Reviewed-by: Björn Terelius > Cr-Commit-Position: refs/heads/master@{#25848} TBR=eladalon@webrtc.org,terelius@webrtc.org,zstein@webrtc.org,qingsi@webrtc.org,jeroendb@webrtc.org Change-Id: Ib3b0a845f2300f4fcba2061650e17522735f08b3 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: webrtc:9972 Reviewed-on: https://webrtc-review.googlesource.com/c/112581 Reviewed-by: Erik Språng Commit-Queue: Erik Språng Cr-Commit-Position: refs/heads/master@{#25852} --- .../events/rtc_event_ice_candidate_pair.cc | 10 +++------ .../events/rtc_event_ice_candidate_pair.h | 5 +---- logging/rtc_event_log/icelogger.cc | 7 +++---- logging/rtc_event_log/icelogger.h | 3 +-- .../rtc_event_log_unittest_helper.cc | 4 +--- p2p/base/port.cc | 17 ++++++--------- p2p/base/port.h | 3 +-- p2p/base/stun.cc | 21 ------------------- p2p/base/stun.h | 2 -- p2p/base/stunrequest.h | 5 ----- 10 files changed, 16 insertions(+), 61 deletions(-) diff --git a/logging/rtc_event_log/events/rtc_event_ice_candidate_pair.cc b/logging/rtc_event_log/events/rtc_event_ice_candidate_pair.cc index 225362d9e6..9c31737ea5 100644 --- a/logging/rtc_event_log/events/rtc_event_ice_candidate_pair.cc +++ b/logging/rtc_event_log/events/rtc_event_ice_candidate_pair.cc @@ -16,18 +16,14 @@ namespace webrtc { RtcEventIceCandidatePair::RtcEventIceCandidatePair( IceCandidatePairEventType type, - uint32_t candidate_pair_id, - uint32_t transaction_id) - : type_(type), - candidate_pair_id_(candidate_pair_id), - transaction_id_(transaction_id) {} + uint32_t candidate_pair_id) + : type_(type), candidate_pair_id_(candidate_pair_id) {} RtcEventIceCandidatePair::RtcEventIceCandidatePair( const RtcEventIceCandidatePair& other) : RtcEvent(other.timestamp_us_), type_(other.type_), - candidate_pair_id_(other.candidate_pair_id_), - transaction_id_(other.transaction_id_) {} + candidate_pair_id_(other.candidate_pair_id_) {} RtcEventIceCandidatePair::~RtcEventIceCandidatePair() = default; diff --git a/logging/rtc_event_log/events/rtc_event_ice_candidate_pair.h b/logging/rtc_event_log/events/rtc_event_ice_candidate_pair.h index 973a12a265..1dc1b8eaa9 100644 --- a/logging/rtc_event_log/events/rtc_event_ice_candidate_pair.h +++ b/logging/rtc_event_log/events/rtc_event_ice_candidate_pair.h @@ -29,8 +29,7 @@ enum class IceCandidatePairEventType { class RtcEventIceCandidatePair final : public RtcEvent { public: RtcEventIceCandidatePair(IceCandidatePairEventType type, - uint32_t candidate_pair_id, - uint32_t transaction_id); + uint32_t candidate_pair_id); ~RtcEventIceCandidatePair() override; @@ -42,14 +41,12 @@ class RtcEventIceCandidatePair final : public RtcEvent { IceCandidatePairEventType type() const { return type_; } uint32_t candidate_pair_id() const { return candidate_pair_id_; } - uint32_t transaction_id() const { return transaction_id_; } private: RtcEventIceCandidatePair(const RtcEventIceCandidatePair& other); const IceCandidatePairEventType type_; const uint32_t candidate_pair_id_; - const uint32_t transaction_id_; }; } // namespace webrtc diff --git a/logging/rtc_event_log/icelogger.cc b/logging/rtc_event_log/icelogger.cc index c1dbcd8c89..5f91c5e7bd 100644 --- a/logging/rtc_event_log/icelogger.cc +++ b/logging/rtc_event_log/icelogger.cc @@ -31,13 +31,12 @@ void IceEventLog::LogCandidatePairConfig( } void IceEventLog::LogCandidatePairEvent(IceCandidatePairEventType type, - uint32_t candidate_pair_id, - uint32_t transaction_id) { + uint32_t candidate_pair_id) { if (event_log_ == nullptr) { return; } - event_log_->Log(absl::make_unique( - type, candidate_pair_id, transaction_id)); + event_log_->Log( + absl::make_unique(type, candidate_pair_id)); } void IceEventLog::DumpCandidatePairDescriptionToMemoryAsConfigEvents() const { diff --git a/logging/rtc_event_log/icelogger.h b/logging/rtc_event_log/icelogger.h index f14cf0daec..b590fd7cb3 100644 --- a/logging/rtc_event_log/icelogger.h +++ b/logging/rtc_event_log/icelogger.h @@ -36,8 +36,7 @@ class IceEventLog { const IceCandidatePairDescription& candidate_pair_desc); void LogCandidatePairEvent(IceCandidatePairEventType type, - uint32_t candidate_pair_id, - uint32_t transaction_id); + uint32_t candidate_pair_id); // This method constructs a config event for each candidate pair with their // description and logs these config events. It is intended to be called when diff --git a/logging/rtc_event_log/rtc_event_log_unittest_helper.cc b/logging/rtc_event_log/rtc_event_log_unittest_helper.cc index 6787cf56a7..7e20d439b8 100644 --- a/logging/rtc_event_log/rtc_event_log_unittest_helper.cc +++ b/logging/rtc_event_log/rtc_event_log_unittest_helper.cc @@ -214,10 +214,8 @@ EventGenerator::NewIceCandidatePair() { static_cast(prng_.Rand( static_cast(IceCandidatePairEventType::kNumValues) - 1)); uint32_t pair_id = prng_.Rand(); - uint32_t transaction_id = prng_.Rand(); - return absl::make_unique(type, pair_id, - transaction_id); + return absl::make_unique(type, pair_id); } rtcp::ReportBlock EventGenerator::NewReportBlock() { diff --git a/p2p/base/port.cc b/p2p/base/port.cc index 4d93b4f92f..5b8e02d28f 100644 --- a/p2p/base/port.cc +++ b/p2p/base/port.cc @@ -860,8 +860,7 @@ void Port::SendBindingResponse(StunMessage* request, conn->stats_.sent_ping_responses++; conn->LogCandidatePairEvent( - webrtc::IceCandidatePairEventType::kCheckResponseSent, - request->reduced_transaction_id()); + webrtc::IceCandidatePairEventType::kCheckResponseSent); } } @@ -1341,8 +1340,7 @@ void Connection::HandleBindingRequest(IceMessage* msg) { } stats_.recv_ping_requests++; - LogCandidatePairEvent(webrtc::IceCandidatePairEventType::kCheckReceived, - msg->reduced_transaction_id()); + LogCandidatePairEvent(webrtc::IceCandidatePairEventType::kCheckReceived); // This is a validated stun request from remote peer. port_->SendBindingResponse(msg, remote_addr); @@ -1679,12 +1677,11 @@ void Connection::LogCandidatePairConfig( ice_event_log_->LogCandidatePairConfig(type, id(), ToLogDescription()); } -void Connection::LogCandidatePairEvent(webrtc::IceCandidatePairEventType type, - uint32_t transaction_id) { +void Connection::LogCandidatePairEvent(webrtc::IceCandidatePairEventType type) { if (ice_event_log_ == nullptr) { return; } - ice_event_log_->LogCandidatePairEvent(type, id(), transaction_id); + ice_event_log_->LogCandidatePairEvent(type, id()); } void Connection::OnConnectionRequestResponse(ConnectionRequest* request, @@ -1711,8 +1708,7 @@ void Connection::OnConnectionRequestResponse(ConnectionRequest* request, stats_.recv_ping_responses++; LogCandidatePairEvent( - webrtc::IceCandidatePairEventType::kCheckResponseReceived, - response->reduced_transaction_id()); + webrtc::IceCandidatePairEventType::kCheckResponseReceived); MaybeUpdateLocalCandidate(request, response); } @@ -1758,8 +1754,7 @@ void Connection::OnConnectionRequestSent(ConnectionRequest* request) { << ", use_candidate=" << use_candidate_attr() << ", nomination=" << nomination(); stats_.sent_ping_requests_total++; - LogCandidatePairEvent(webrtc::IceCandidatePairEventType::kCheckSent, - request->reduced_transaction_id()); + LogCandidatePairEvent(webrtc::IceCandidatePairEventType::kCheckSent); if (stats_.recv_ping_responses == 0) { stats_.sent_ping_requests_before_first_response++; } diff --git a/p2p/base/port.h b/p2p/base/port.h index ca4fedbbd8..9a8f92a96e 100644 --- a/p2p/base/port.h +++ b/p2p/base/port.h @@ -789,8 +789,7 @@ class Connection : public CandidatePairInterface, StunMessage* response); void LogCandidatePairConfig(webrtc::IceCandidatePairConfigType type); - void LogCandidatePairEvent(webrtc::IceCandidatePairEventType type, - uint32_t transaction_id); + void LogCandidatePairEvent(webrtc::IceCandidatePairEventType type); WriteState write_state_; bool receiving_; diff --git a/p2p/base/stun.cc b/p2p/base/stun.cc index 33b444884d..fa6b6f832e 100644 --- a/p2p/base/stun.cc +++ b/p2p/base/stun.cc @@ -26,25 +26,6 @@ using rtc::ByteBufferReader; using rtc::ByteBufferWriter; -namespace { - -uint32_t ReduceTransactionId(const std::string& transaction_id) { - RTC_DCHECK(transaction_id.length() == cricket::kStunTransactionIdLength || - transaction_id.length() == - cricket::kStunLegacyTransactionIdLength); - uint32_t transaction_id_as_ints[4]; - memcpy(transaction_id_as_ints, transaction_id.c_str(), - transaction_id.length()); - - uint32_t result = 0; - for (size_t i = 0; i < transaction_id.length() / 4; ++i) { - result ^= transaction_id_as_ints[i]; - } - return result; -} - -} // namespace - namespace cricket { const char STUN_ERROR_REASON_TRY_ALTERNATE_SERVER[] = "Try Alternate Server"; @@ -87,7 +68,6 @@ bool StunMessage::SetTransactionID(const std::string& str) { return false; } transaction_id_ = str; - reduced_transaction_id_ = ReduceTransactionId(transaction_id_); return true; } @@ -374,7 +354,6 @@ bool StunMessage::Read(ByteBufferReader* buf) { } RTC_DCHECK(IsValidTransactionId(transaction_id)); transaction_id_ = transaction_id; - reduced_transaction_id_ = ReduceTransactionId(transaction_id_); if (length_ != buf->Length()) return false; diff --git a/p2p/base/stun.h b/p2p/base/stun.h index ca664b02fc..6083772a1a 100644 --- a/p2p/base/stun.h +++ b/p2p/base/stun.h @@ -141,7 +141,6 @@ class StunMessage { int type() const { return type_; } size_t length() const { return length_; } const std::string& transaction_id() const { return transaction_id_; } - uint32_t reduced_transaction_id() const { return reduced_transaction_id_; } // Returns true if the message confirms to RFC3489 rather than // RFC5389. The main difference between two version of the STUN @@ -215,7 +214,6 @@ class StunMessage { uint16_t type_; uint16_t length_; std::string transaction_id_; - uint32_t reduced_transaction_id_; std::vector> attrs_; uint32_t stun_magic_cookie_; }; diff --git a/p2p/base/stunrequest.h b/p2p/base/stunrequest.h index 4a9a9b4a13..3fac2d3a8a 100644 --- a/p2p/base/stunrequest.h +++ b/p2p/base/stunrequest.h @@ -100,11 +100,6 @@ class StunRequest : public rtc::MessageHandler { // Returns the transaction ID of this request. const std::string& id() { return msg_->transaction_id(); } - // Returns the reduced transaction ID of this request. - uint32_t reduced_transaction_id() const { - return msg_->reduced_transaction_id(); - } - // the origin value const std::string& origin() const { return origin_; } void set_origin(const std::string& origin) { origin_ = origin; }