From 25e022fd5c00914af5d7dde472260d0a6adb334c Mon Sep 17 00:00:00 2001 From: Taylor Brandstetter Date: Thu, 8 Mar 2018 09:53:47 -0800 Subject: [PATCH] Deliver cached stats reports asynchronously. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This has the following benefits: * Stats reports are always delivered asynchronously. This means the API client doesn't need to worry about *possibly* getting a synchronous callback depending on when the last report was generated. * Stats callbacks will always be invoked in the same order that the GetStats calls were made, even in cases where a callback recursively calls GetStats again. Bug: webrtc:8973 Change-Id: I94ca4b5dc5c21a8f2df42adfcddf357f40a32025 Reviewed-on: https://webrtc-review.googlesource.com/60473 Commit-Queue: Taylor Brandstetter Reviewed-by: Henrik Boström Cr-Commit-Position: refs/heads/master@{#22348} --- pc/rtcstatscollector.cc | 34 +++++++++++++++++++++----------- pc/rtcstatscollector.h | 4 +++- pc/rtcstatscollector_unittest.cc | 14 ++++++------- 3 files changed, 32 insertions(+), 20 deletions(-) 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);