From 02d2a92d92d05c3014d779efd5c03e0822b7dbe7 Mon Sep 17 00:00:00 2001 From: hbos Date: Wed, 21 Dec 2016 01:29:05 -0800 Subject: [PATCH] RTCStatsReport::AddStats DCHECKs that the ID is unique. Previously it was allowed to call AddStats with stats of the same ID multiple times. This revealed a few things: - Local and remote streams can have the same label. RTCMediaStreamStats's ID is updated to include "local"/"remote". - The same certificate can show up multiple times (e.g. for local and remote in a loopback), so we skip creating RTCCertificateStats for the same certificate multiple times BUG=chromium:627816 Review-Url: https://codereview.webrtc.org/2593503003 Cr-Commit-Position: refs/heads/master@{#15730} --- webrtc/api/rtcstatscollector.cc | 16 +++++++++++++--- webrtc/api/rtcstatscollector_unittest.cc | 10 ++++++---- webrtc/api/stats/rtcstatsreport.h | 2 +- webrtc/stats/rtcstatsreport.cc | 9 ++++++--- 4 files changed, 26 insertions(+), 11 deletions(-) diff --git a/webrtc/api/rtcstatscollector.cc b/webrtc/api/rtcstatscollector.cc index f1c7ed7e1a..cf97057c1b 100644 --- a/webrtc/api/rtcstatscollector.cc +++ b/webrtc/api/rtcstatscollector.cc @@ -233,8 +233,17 @@ void ProduceCertificateStatsFromSSLCertificateStats( RTCCertificateStats* prev_certificate_stats = nullptr; for (const rtc::SSLCertificateStats* s = &certificate_stats; s; s = s->issuer.get()) { + std::string certificate_stats_id = + RTCCertificateIDFromFingerprint(s->fingerprint); + // It is possible for the same certificate to show up multiple times, e.g. + // if local and remote side use the same certificate in a loopback call. + // If the report already contains stats for this certificate, skip it. + if (report->Get(certificate_stats_id)) { + RTC_DCHECK_EQ(s, &certificate_stats); + break; + } RTCCertificateStats* certificate_stats = new RTCCertificateStats( - RTCCertificateIDFromFingerprint(s->fingerprint), timestamp_us); + certificate_stats_id, timestamp_us); certificate_stats->fingerprint = s->fingerprint; certificate_stats->fingerprint_algorithm = s->fingerprint_algorithm; certificate_stats->base64_certificate = s->base64_certificate; @@ -288,8 +297,9 @@ void ProduceMediaStreamAndTrackStats( MediaStreamInterface* stream = streams->at(i); std::unique_ptr stream_stats( - new RTCMediaStreamStats("RTCMediaStream_" + stream->label(), - timestamp_us)); + new RTCMediaStreamStats( + (is_local ? "RTCMediaStream_local_" : "RTCMediaStream_remote_") + + stream->label(), timestamp_us)); stream_stats->stream_identifier = stream->label(); stream_stats->track_ids = std::vector(); // Audio Tracks diff --git a/webrtc/api/rtcstatscollector_unittest.cc b/webrtc/api/rtcstatscollector_unittest.cc index 4be7491724..8941818fe9 100644 --- a/webrtc/api/rtcstatscollector_unittest.cc +++ b/webrtc/api/rtcstatscollector_unittest.cc @@ -969,6 +969,7 @@ TEST_F(RTCStatsCollectorTest, CollectRTCIceCandidateStats) { *a_local_prflx.get(); a_transport_channel_stats.connection_infos[1].remote_candidate = *a_remote_relay.get(); + session_stats.transport_stats["a"].transport_name = "a"; session_stats.transport_stats["a"].channel_stats.push_back( a_transport_channel_stats); @@ -979,6 +980,7 @@ TEST_F(RTCStatsCollectorTest, CollectRTCIceCandidateStats) { *b_local.get(); b_transport_channel_stats.connection_infos[0].remote_candidate = *b_remote.get(); + session_stats.transport_stats["b"].transport_name = "b"; session_stats.transport_stats["b"].channel_stats.push_back( b_transport_channel_stats); @@ -1164,7 +1166,7 @@ TEST_F(RTCStatsCollectorTest, rtc::scoped_refptr report = GetStatsReport(); RTCMediaStreamStats expected_local_stream( - "RTCMediaStream_LocalStreamLabel", report->timestamp_us()); + "RTCMediaStream_local_LocalStreamLabel", report->timestamp_us()); expected_local_stream.stream_identifier = local_stream->label(); expected_local_stream.track_ids = std::vector(); expected_local_stream.track_ids->push_back( @@ -1175,7 +1177,7 @@ TEST_F(RTCStatsCollectorTest, RTCMediaStreamStats>()); RTCMediaStreamStats expected_remote_stream( - "RTCMediaStream_RemoteStreamLabel", report->timestamp_us()); + "RTCMediaStream_remote_RemoteStreamLabel", report->timestamp_us()); expected_remote_stream.stream_identifier = remote_stream->label(); expected_remote_stream.track_ids = std::vector(); expected_remote_stream.track_ids->push_back( @@ -1301,7 +1303,7 @@ TEST_F(RTCStatsCollectorTest, rtc::scoped_refptr report = GetStatsReport(); RTCMediaStreamStats expected_local_stream( - "RTCMediaStream_LocalStreamLabel", report->timestamp_us()); + "RTCMediaStream_local_LocalStreamLabel", report->timestamp_us()); expected_local_stream.stream_identifier = local_stream->label(); expected_local_stream.track_ids = std::vector(); expected_local_stream.track_ids->push_back( @@ -1312,7 +1314,7 @@ TEST_F(RTCStatsCollectorTest, RTCMediaStreamStats>()); RTCMediaStreamStats expected_remote_stream( - "RTCMediaStream_RemoteStreamLabel", report->timestamp_us()); + "RTCMediaStream_remote_RemoteStreamLabel", report->timestamp_us()); expected_remote_stream.stream_identifier = remote_stream->label(); expected_remote_stream.track_ids = std::vector(); expected_remote_stream.track_ids->push_back( diff --git a/webrtc/api/stats/rtcstatsreport.h b/webrtc/api/stats/rtcstatsreport.h index 32514e765a..6d9ae6df2a 100644 --- a/webrtc/api/stats/rtcstatsreport.h +++ b/webrtc/api/stats/rtcstatsreport.h @@ -58,7 +58,7 @@ class RTCStatsReport : public rtc::RefCountInterface { RTCStatsReport(const RTCStatsReport& other) = delete; int64_t timestamp_us() const { return timestamp_us_; } - bool AddStats(std::unique_ptr stats); + void AddStats(std::unique_ptr stats); const RTCStats* Get(const std::string& id) const; size_t size() const { return stats_.size(); } diff --git a/webrtc/stats/rtcstatsreport.cc b/webrtc/stats/rtcstatsreport.cc index 3dd6eff39f..566aef8772 100644 --- a/webrtc/stats/rtcstatsreport.cc +++ b/webrtc/stats/rtcstatsreport.cc @@ -69,9 +69,12 @@ RTCStatsReport::RTCStatsReport(int64_t timestamp_us) RTCStatsReport::~RTCStatsReport() { } -bool RTCStatsReport::AddStats(std::unique_ptr stats) { - return !stats_.insert(std::make_pair(std::string(stats->id()), - std::move(stats))).second; +void RTCStatsReport::AddStats(std::unique_ptr stats) { + auto result = stats_.insert(std::make_pair(std::string(stats->id()), + std::move(stats))); + RTC_DCHECK(result.second) << + "A stats object with ID " << result.first->second->id() << " is already " + "present in this stats report."; } const RTCStats* RTCStatsReport::Get(const std::string& id) const {