Revert "Add transaction id to CandidatePairEvents."

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 <zstein@webrtc.org>
> Reviewed-by: Qingsi Wang <qingsi@webrtc.org>
> Reviewed-by: Elad Alon <eladalon@webrtc.org>
> Reviewed-by: Björn Terelius <terelius@webrtc.org>
> 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 <sprang@webrtc.org>
Commit-Queue: Erik Språng <sprang@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#25852}
This commit is contained in:
Erik Språng
2018-11-30 09:28:14 +00:00
committed by Commit Bot
parent 3c19f2884c
commit 286ee0123e
10 changed files with 16 additions and 61 deletions

View File

@ -16,18 +16,14 @@ namespace webrtc {
RtcEventIceCandidatePair::RtcEventIceCandidatePair( RtcEventIceCandidatePair::RtcEventIceCandidatePair(
IceCandidatePairEventType type, IceCandidatePairEventType type,
uint32_t candidate_pair_id, uint32_t candidate_pair_id)
uint32_t transaction_id) : type_(type), candidate_pair_id_(candidate_pair_id) {}
: type_(type),
candidate_pair_id_(candidate_pair_id),
transaction_id_(transaction_id) {}
RtcEventIceCandidatePair::RtcEventIceCandidatePair( RtcEventIceCandidatePair::RtcEventIceCandidatePair(
const RtcEventIceCandidatePair& other) const RtcEventIceCandidatePair& other)
: RtcEvent(other.timestamp_us_), : RtcEvent(other.timestamp_us_),
type_(other.type_), type_(other.type_),
candidate_pair_id_(other.candidate_pair_id_), candidate_pair_id_(other.candidate_pair_id_) {}
transaction_id_(other.transaction_id_) {}
RtcEventIceCandidatePair::~RtcEventIceCandidatePair() = default; RtcEventIceCandidatePair::~RtcEventIceCandidatePair() = default;

View File

@ -29,8 +29,7 @@ enum class IceCandidatePairEventType {
class RtcEventIceCandidatePair final : public RtcEvent { class RtcEventIceCandidatePair final : public RtcEvent {
public: public:
RtcEventIceCandidatePair(IceCandidatePairEventType type, RtcEventIceCandidatePair(IceCandidatePairEventType type,
uint32_t candidate_pair_id, uint32_t candidate_pair_id);
uint32_t transaction_id);
~RtcEventIceCandidatePair() override; ~RtcEventIceCandidatePair() override;
@ -42,14 +41,12 @@ class RtcEventIceCandidatePair final : public RtcEvent {
IceCandidatePairEventType type() const { return type_; } IceCandidatePairEventType type() const { return type_; }
uint32_t candidate_pair_id() const { return candidate_pair_id_; } uint32_t candidate_pair_id() const { return candidate_pair_id_; }
uint32_t transaction_id() const { return transaction_id_; }
private: private:
RtcEventIceCandidatePair(const RtcEventIceCandidatePair& other); RtcEventIceCandidatePair(const RtcEventIceCandidatePair& other);
const IceCandidatePairEventType type_; const IceCandidatePairEventType type_;
const uint32_t candidate_pair_id_; const uint32_t candidate_pair_id_;
const uint32_t transaction_id_;
}; };
} // namespace webrtc } // namespace webrtc

View File

@ -31,13 +31,12 @@ void IceEventLog::LogCandidatePairConfig(
} }
void IceEventLog::LogCandidatePairEvent(IceCandidatePairEventType type, void IceEventLog::LogCandidatePairEvent(IceCandidatePairEventType type,
uint32_t candidate_pair_id, uint32_t candidate_pair_id) {
uint32_t transaction_id) {
if (event_log_ == nullptr) { if (event_log_ == nullptr) {
return; return;
} }
event_log_->Log(absl::make_unique<RtcEventIceCandidatePair>( event_log_->Log(
type, candidate_pair_id, transaction_id)); absl::make_unique<RtcEventIceCandidatePair>(type, candidate_pair_id));
} }
void IceEventLog::DumpCandidatePairDescriptionToMemoryAsConfigEvents() const { void IceEventLog::DumpCandidatePairDescriptionToMemoryAsConfigEvents() const {

View File

@ -36,8 +36,7 @@ class IceEventLog {
const IceCandidatePairDescription& candidate_pair_desc); const IceCandidatePairDescription& candidate_pair_desc);
void LogCandidatePairEvent(IceCandidatePairEventType type, void LogCandidatePairEvent(IceCandidatePairEventType type,
uint32_t candidate_pair_id, uint32_t candidate_pair_id);
uint32_t transaction_id);
// This method constructs a config event for each candidate pair with their // 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 // description and logs these config events. It is intended to be called when

View File

@ -214,10 +214,8 @@ EventGenerator::NewIceCandidatePair() {
static_cast<IceCandidatePairEventType>(prng_.Rand( static_cast<IceCandidatePairEventType>(prng_.Rand(
static_cast<uint32_t>(IceCandidatePairEventType::kNumValues) - 1)); static_cast<uint32_t>(IceCandidatePairEventType::kNumValues) - 1));
uint32_t pair_id = prng_.Rand<uint32_t>(); uint32_t pair_id = prng_.Rand<uint32_t>();
uint32_t transaction_id = prng_.Rand<uint32_t>();
return absl::make_unique<RtcEventIceCandidatePair>(type, pair_id, return absl::make_unique<RtcEventIceCandidatePair>(type, pair_id);
transaction_id);
} }
rtcp::ReportBlock EventGenerator::NewReportBlock() { rtcp::ReportBlock EventGenerator::NewReportBlock() {

View File

@ -860,8 +860,7 @@ void Port::SendBindingResponse(StunMessage* request,
conn->stats_.sent_ping_responses++; conn->stats_.sent_ping_responses++;
conn->LogCandidatePairEvent( conn->LogCandidatePairEvent(
webrtc::IceCandidatePairEventType::kCheckResponseSent, webrtc::IceCandidatePairEventType::kCheckResponseSent);
request->reduced_transaction_id());
} }
} }
@ -1341,8 +1340,7 @@ void Connection::HandleBindingRequest(IceMessage* msg) {
} }
stats_.recv_ping_requests++; stats_.recv_ping_requests++;
LogCandidatePairEvent(webrtc::IceCandidatePairEventType::kCheckReceived, LogCandidatePairEvent(webrtc::IceCandidatePairEventType::kCheckReceived);
msg->reduced_transaction_id());
// This is a validated stun request from remote peer. // This is a validated stun request from remote peer.
port_->SendBindingResponse(msg, remote_addr); port_->SendBindingResponse(msg, remote_addr);
@ -1679,12 +1677,11 @@ void Connection::LogCandidatePairConfig(
ice_event_log_->LogCandidatePairConfig(type, id(), ToLogDescription()); ice_event_log_->LogCandidatePairConfig(type, id(), ToLogDescription());
} }
void Connection::LogCandidatePairEvent(webrtc::IceCandidatePairEventType type, void Connection::LogCandidatePairEvent(webrtc::IceCandidatePairEventType type) {
uint32_t transaction_id) {
if (ice_event_log_ == nullptr) { if (ice_event_log_ == nullptr) {
return; return;
} }
ice_event_log_->LogCandidatePairEvent(type, id(), transaction_id); ice_event_log_->LogCandidatePairEvent(type, id());
} }
void Connection::OnConnectionRequestResponse(ConnectionRequest* request, void Connection::OnConnectionRequestResponse(ConnectionRequest* request,
@ -1711,8 +1708,7 @@ void Connection::OnConnectionRequestResponse(ConnectionRequest* request,
stats_.recv_ping_responses++; stats_.recv_ping_responses++;
LogCandidatePairEvent( LogCandidatePairEvent(
webrtc::IceCandidatePairEventType::kCheckResponseReceived, webrtc::IceCandidatePairEventType::kCheckResponseReceived);
response->reduced_transaction_id());
MaybeUpdateLocalCandidate(request, response); MaybeUpdateLocalCandidate(request, response);
} }
@ -1758,8 +1754,7 @@ void Connection::OnConnectionRequestSent(ConnectionRequest* request) {
<< ", use_candidate=" << use_candidate_attr() << ", use_candidate=" << use_candidate_attr()
<< ", nomination=" << nomination(); << ", nomination=" << nomination();
stats_.sent_ping_requests_total++; stats_.sent_ping_requests_total++;
LogCandidatePairEvent(webrtc::IceCandidatePairEventType::kCheckSent, LogCandidatePairEvent(webrtc::IceCandidatePairEventType::kCheckSent);
request->reduced_transaction_id());
if (stats_.recv_ping_responses == 0) { if (stats_.recv_ping_responses == 0) {
stats_.sent_ping_requests_before_first_response++; stats_.sent_ping_requests_before_first_response++;
} }

View File

@ -789,8 +789,7 @@ class Connection : public CandidatePairInterface,
StunMessage* response); StunMessage* response);
void LogCandidatePairConfig(webrtc::IceCandidatePairConfigType type); void LogCandidatePairConfig(webrtc::IceCandidatePairConfigType type);
void LogCandidatePairEvent(webrtc::IceCandidatePairEventType type, void LogCandidatePairEvent(webrtc::IceCandidatePairEventType type);
uint32_t transaction_id);
WriteState write_state_; WriteState write_state_;
bool receiving_; bool receiving_;

View File

@ -26,25 +26,6 @@
using rtc::ByteBufferReader; using rtc::ByteBufferReader;
using rtc::ByteBufferWriter; 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 { namespace cricket {
const char STUN_ERROR_REASON_TRY_ALTERNATE_SERVER[] = "Try Alternate Server"; const char STUN_ERROR_REASON_TRY_ALTERNATE_SERVER[] = "Try Alternate Server";
@ -87,7 +68,6 @@ bool StunMessage::SetTransactionID(const std::string& str) {
return false; return false;
} }
transaction_id_ = str; transaction_id_ = str;
reduced_transaction_id_ = ReduceTransactionId(transaction_id_);
return true; return true;
} }
@ -374,7 +354,6 @@ bool StunMessage::Read(ByteBufferReader* buf) {
} }
RTC_DCHECK(IsValidTransactionId(transaction_id)); RTC_DCHECK(IsValidTransactionId(transaction_id));
transaction_id_ = transaction_id; transaction_id_ = transaction_id;
reduced_transaction_id_ = ReduceTransactionId(transaction_id_);
if (length_ != buf->Length()) if (length_ != buf->Length())
return false; return false;

View File

@ -141,7 +141,6 @@ class StunMessage {
int type() const { return type_; } int type() const { return type_; }
size_t length() const { return length_; } size_t length() const { return length_; }
const std::string& transaction_id() const { return transaction_id_; } 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 // Returns true if the message confirms to RFC3489 rather than
// RFC5389. The main difference between two version of the STUN // RFC5389. The main difference between two version of the STUN
@ -215,7 +214,6 @@ class StunMessage {
uint16_t type_; uint16_t type_;
uint16_t length_; uint16_t length_;
std::string transaction_id_; std::string transaction_id_;
uint32_t reduced_transaction_id_;
std::vector<std::unique_ptr<StunAttribute>> attrs_; std::vector<std::unique_ptr<StunAttribute>> attrs_;
uint32_t stun_magic_cookie_; uint32_t stun_magic_cookie_;
}; };

View File

@ -100,11 +100,6 @@ class StunRequest : public rtc::MessageHandler {
// Returns the transaction ID of this request. // Returns the transaction ID of this request.
const std::string& id() { return msg_->transaction_id(); } 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 // the origin value
const std::string& origin() const { return origin_; } const std::string& origin() const { return origin_; }
void set_origin(const std::string& origin) { origin_ = origin; } void set_origin(const std::string& origin) { origin_ = origin; }