diff --git a/pc/rtcstatscollector.cc b/pc/rtcstatscollector.cc index 53a6158ab8..e442ffe78c 100644 --- a/pc/rtcstatscollector.cc +++ b/pc/rtcstatscollector.cc @@ -648,8 +648,16 @@ void RTCStatsCollector::GetStatsReport( int64_t cache_now_us = rtc::TimeMicros(); if (cached_report_ && cache_now_us - cache_timestamp_us_ <= cache_lifetime_us_) { - // We have a fresh cached report to deliver. - DeliverCachedReport(); + // We have a fresh cached report to deliver. Deliver asynchronously, since + // the caller may not be expecting a synchronous callback, and it avoids + // reentrancy problems. + std::vector> callbacks; + callbacks.swap(callbacks_); + invoker_.AsyncInvoke( + RTC_FROM_HERE, signaling_thread_, + rtc::Bind(&RTCStatsCollector::DeliverCachedReport, this, cached_report_, + std::move(callbacks))); + callbacks_.clear(); } else if (!num_pending_partial_reports_) { // Only start gathering stats if we're not already gathering stats. In the // case of already gathering stats, |callback_| will be invoked when there @@ -772,23 +780,25 @@ void RTCStatsCollector::AddPartialResults_s( // select the "webrtc_stats" category when recording traces. TRACE_EVENT_INSTANT1("webrtc_stats", "webrtc_stats", "report", cached_report_->ToJson()); - DeliverCachedReport(); + + // Swap the list of callbacks, in case one of them recursively calls + // GetStatsReport again and modifies the callback list. + std::vector> callbacks; + callbacks.swap(callbacks_); + DeliverCachedReport(cached_report_, std::move(callbacks)); } } -void RTCStatsCollector::DeliverCachedReport() { +void RTCStatsCollector::DeliverCachedReport( + rtc::scoped_refptr cached_report, + std::vector> callbacks) { 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> callbacks; - callbacks.swap(callbacks_); + RTC_DCHECK(!callbacks.empty()); + RTC_DCHECK(cached_report); for (const rtc::scoped_refptr& callback : callbacks) { - callback->OnStatsDelivered(cached_report_); + callback->OnStatsDelivered(cached_report); } } diff --git a/pc/rtcstatscollector.h b/pc/rtcstatscollector.h index 2ff4c49065..be5980bffe 100644 --- a/pc/rtcstatscollector.h +++ b/pc/rtcstatscollector.h @@ -94,7 +94,9 @@ class RTCStatsCollector : public virtual rtc::RefCountInterface, }; void AddPartialResults_s(rtc::scoped_refptr partial_report); - void DeliverCachedReport(); + void DeliverCachedReport( + rtc::scoped_refptr cached_report, + std::vector> callbacks); // Produces |RTCCertificateStats|. void ProduceCertificateStats_n( diff --git a/pc/rtcstatscollector_unittest.cc b/pc/rtcstatscollector_unittest.cc index 5240e627af..42b42dca0d 100644 --- a/pc/rtcstatscollector_unittest.cc +++ b/pc/rtcstatscollector_unittest.cc @@ -1911,9 +1911,9 @@ TEST_F(RTCStatsCollectorTest, DoNotCrashOnSsrcChange) { } // Used for test below, to test calling GetStatsReport during a callback. -class ReentrantCallback : public RTCStatsCollectorCallback { +class RecursiveCallback : public RTCStatsCollectorCallback { public: - explicit ReentrantCallback(RTCStatsCollectorWrapper* stats) : stats_(stats) {} + explicit RecursiveCallback(RTCStatsCollectorWrapper* stats) : stats_(stats) {} void OnStatsDelivered( const rtc::scoped_refptr& report) override { @@ -1930,11 +1930,11 @@ class ReentrantCallback : public RTCStatsCollectorCallback { // 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 callback1( - new rtc::RefCountedObject(stats_.get())); - rtc::scoped_refptr callback2( - new rtc::RefCountedObject(stats_.get())); +TEST_F(RTCStatsCollectorTest, DoNotCrashWhenGetStatsCalledDuringCallback) { + rtc::scoped_refptr callback1( + new rtc::RefCountedObject(stats_.get())); + rtc::scoped_refptr callback2( + new rtc::RefCountedObject(stats_.get())); stats_->stats_collector()->GetStatsReport(callback1); stats_->stats_collector()->GetStatsReport(callback2); EXPECT_TRUE_WAIT(callback1->called(), kGetStatsReportTimeoutMs);