From 87d5a7494a04f452ff0fbf04841d4ecd55ca9a6c Mon Sep 17 00:00:00 2001
From: Taylor Brandstetter <deadbeef@webrtc.org>
Date: Tue, 6 Mar 2018 09:42:25 -0800
Subject: [PATCH] Fix crash that occurs if GetStats is called from within
 OnStatsDelivered
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This was resulting in the currently executing stats callback to be
invoked *again*, possibly ad infinitum, resulting in stack overflow.

Bug: webrtc:8973
Change-Id: Ib3bcc8e6bdd991728214fa242dda2010efc919a7
Reviewed-on: https://webrtc-review.googlesource.com/59464
Commit-Queue: Taylor Brandstetter <deadbeef@webrtc.org>
Reviewed-by: Henrik Boström <hbos@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#22313}
---
 pc/rtcstatscollector.cc          |  9 +++++++--
 pc/rtcstatscollector_unittest.cc | 31 +++++++++++++++++++++++++++++++
 2 files changed, 38 insertions(+), 2 deletions(-)

diff --git a/pc/rtcstatscollector.cc b/pc/rtcstatscollector.cc
index 20cbc912a1..53a6158ab8 100644
--- a/pc/rtcstatscollector.cc
+++ b/pc/rtcstatscollector.cc
@@ -780,11 +780,16 @@ void RTCStatsCollector::DeliverCachedReport() {
   RTC_DCHECK(signaling_thread_->IsCurrent());
   RTC_DCHECK(!callbacks_.empty());
   RTC_DCHECK(cached_report_);
+
+  // Swap the list of callbacks, in case one of them recursively calls
+  // GetStatsReport again and modifies the callback list.
+  std::vector<rtc::scoped_refptr<RTCStatsCollectorCallback>> callbacks;
+  callbacks.swap(callbacks_);
+
   for (const rtc::scoped_refptr<RTCStatsCollectorCallback>& callback :
-       callbacks_) {
+       callbacks) {
     callback->OnStatsDelivered(cached_report_);
   }
-  callbacks_.clear();
 }
 
 void RTCStatsCollector::ProduceCertificateStats_n(
diff --git a/pc/rtcstatscollector_unittest.cc b/pc/rtcstatscollector_unittest.cc
index f180471a9b..5240e627af 100644
--- a/pc/rtcstatscollector_unittest.cc
+++ b/pc/rtcstatscollector_unittest.cc
@@ -1910,6 +1910,37 @@ TEST_F(RTCStatsCollectorTest, DoNotCrashOnSsrcChange) {
   EXPECT_EQ(1, track_stats.size());
 }
 
+// Used for test below, to test calling GetStatsReport during a callback.
+class ReentrantCallback : public RTCStatsCollectorCallback {
+ public:
+  explicit ReentrantCallback(RTCStatsCollectorWrapper* stats) : stats_(stats) {}
+
+  void OnStatsDelivered(
+      const rtc::scoped_refptr<const RTCStatsReport>& report) override {
+    stats_->GetStatsReport();
+    called_ = true;
+  }
+
+  bool called() const { return called_; }
+
+ private:
+  RTCStatsCollectorWrapper* stats_;
+  bool called_ = false;
+};
+
+// Test that nothing bad happens if a callback causes GetStatsReport to be
+// called again recursively. Regression test for crbug.com/webrtc/8973.
+TEST_F(RTCStatsCollectorTest, DoNotCrashOnReentrantInvocation) {
+  rtc::scoped_refptr<ReentrantCallback> callback1(
+      new rtc::RefCountedObject<ReentrantCallback>(stats_.get()));
+  rtc::scoped_refptr<ReentrantCallback> callback2(
+      new rtc::RefCountedObject<ReentrantCallback>(stats_.get()));
+  stats_->stats_collector()->GetStatsReport(callback1);
+  stats_->stats_collector()->GetStatsReport(callback2);
+  EXPECT_TRUE_WAIT(callback1->called(), kGetStatsReportTimeoutMs);
+  EXPECT_TRUE_WAIT(callback2->called(), kGetStatsReportTimeoutMs);
+}
+
 class RTCTestStats : public RTCStats {
  public:
   WEBRTC_RTCSTATS_DECL();