From 2e1546887be3362743e111b1e3b99637f9c73aba Mon Sep 17 00:00:00 2001
From: Tommi <tommi@webrtc.org>
Date: Mon, 7 Jun 2021 21:16:38 +0200
Subject: [PATCH] Avoid generating a random id for candidate stats.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

CandidateStats didn't use an initializer list which caused the
`candidate` member variable to be constructed with a random id
(calling an expensive rng method), only to be overwritten directly
thereafter.

Bug: webrtc:12840
Change-Id: I0366f674281d236896cb9539812dc2d88c1b37ef
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/221600
Reviewed-by: Henrik Boström <hbos@webrtc.org>
Commit-Queue: Tommi <tommi@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#34244}
---
 p2p/base/port.cc                   | 10 ----------
 p2p/base/port.h                    | 22 ++++++++++++++++------
 p2p/client/basic_port_allocator.cc |  6 ++++--
 pc/stats_collector.cc              |  6 +++---
 4 files changed, 23 insertions(+), 21 deletions(-)

diff --git a/p2p/base/port.cc b/p2p/base/port.cc
index 0a123b4d93..a03a0d6a66 100644
--- a/p2p/base/port.cc
+++ b/p2p/base/port.cc
@@ -105,16 +105,6 @@ std::string Port::ComputeFoundation(const std::string& type,
   return rtc::ToString(rtc::ComputeCrc32(sb.Release()));
 }
 
-CandidateStats::CandidateStats() = default;
-
-CandidateStats::CandidateStats(const CandidateStats&) = default;
-
-CandidateStats::CandidateStats(Candidate candidate) {
-  this->candidate = candidate;
-}
-
-CandidateStats::~CandidateStats() = default;
-
 Port::Port(rtc::Thread* thread,
            const std::string& type,
            rtc::PacketSocketFactory* factory,
diff --git a/p2p/base/port.h b/p2p/base/port.h
index 7759ade33b..2c18f1adeb 100644
--- a/p2p/base/port.h
+++ b/p2p/base/port.h
@@ -99,14 +99,24 @@ class StunStats {
 // Stats that we can return about a candidate.
 class CandidateStats {
  public:
-  CandidateStats();
-  explicit CandidateStats(Candidate candidate);
-  CandidateStats(const CandidateStats&);
-  ~CandidateStats();
+  CandidateStats() = default;
+  CandidateStats(const CandidateStats&) = default;
+  CandidateStats(CandidateStats&&) = default;
+  CandidateStats(Candidate candidate,
+                 absl::optional<StunStats> stats = absl::nullopt)
+      : candidate_(std::move(candidate)), stun_stats_(std::move(stats)) {}
+  ~CandidateStats() = default;
 
-  Candidate candidate;
+  CandidateStats& operator=(const CandidateStats& other) = default;
+
+  const Candidate& candidate() const { return candidate_; }
+
+  const absl::optional<StunStats>& stun_stats() const { return stun_stats_; }
+
+ private:
+  Candidate candidate_;
   // STUN port stats if this candidate is a STUN candidate.
-  absl::optional<StunStats> stun_stats;
+  absl::optional<StunStats> stun_stats_;
 };
 
 typedef std::vector<CandidateStats> CandidateStatsList;
diff --git a/p2p/client/basic_port_allocator.cc b/p2p/client/basic_port_allocator.cc
index 9d7a4f9005..1d38a4c19f 100644
--- a/p2p/client/basic_port_allocator.cc
+++ b/p2p/client/basic_port_allocator.cc
@@ -487,8 +487,10 @@ void BasicPortAllocatorSession::GetCandidateStatsFromReadyPorts(
   for (auto* port : ports) {
     auto candidates = port->Candidates();
     for (const auto& candidate : candidates) {
-      CandidateStats candidate_stats(allocator_->SanitizeCandidate(candidate));
-      port->GetStunStats(&candidate_stats.stun_stats);
+      absl::optional<StunStats> stun_stats;
+      port->GetStunStats(&stun_stats);
+      CandidateStats candidate_stats(allocator_->SanitizeCandidate(candidate),
+                                     std::move(stun_stats));
       candidate_stats_list->push_back(std::move(candidate_stats));
     }
   }
diff --git a/pc/stats_collector.cc b/pc/stats_collector.cc
index 1728cb4d5d..7376e24c8b 100644
--- a/pc/stats_collector.cc
+++ b/pc/stats_collector.cc
@@ -811,7 +811,7 @@ StatsReport* StatsCollector::AddConnectionInfoReport(
 StatsReport* StatsCollector::AddCandidateReport(
     const cricket::CandidateStats& candidate_stats,
     bool local) {
-  const auto& candidate = candidate_stats.candidate;
+  const auto& candidate = candidate_stats.candidate();
   StatsReport::Id id(StatsReport::NewCandidateId(local, candidate.id()));
   StatsReport* report = reports_.Find(id);
   if (!report) {
@@ -834,8 +834,8 @@ StatsReport* StatsCollector::AddCandidateReport(
   }
   report->set_timestamp(stats_gathering_started_);
 
-  if (local && candidate_stats.stun_stats.has_value()) {
-    const auto& stun_stats = candidate_stats.stun_stats.value();
+  if (local && candidate_stats.stun_stats().has_value()) {
+    const auto& stun_stats = candidate_stats.stun_stats().value();
     report->AddInt64(StatsReport::kStatsValueNameSentStunKeepaliveRequests,
                      stun_stats.stun_binding_requests_sent);
     report->AddInt64(StatsReport::kStatsValueNameRecvStunKeepaliveResponses,