Fix segfault when PeerConnection is destroyed during stats collection.
RTCStatsCollector relies on PeerConnection and its WebRtcSession. If the
PeerConnection is destroyed, reference counting keeps the
RTCStatsCollector alive until the request has completed. But the request
is using PeerConnection/WebRtcSession resources that are destroyed in
~PeerConnection().
To get around this problem, RTCStatsCollector::WaitForPendingRequest()
is added, which is invoked at ~PeerConnection().
Integration test added, it caused a segmentation fault before this
change / EXPECT failure.
BUG=chromium:627816
Review-Url: https://codereview.webrtc.org/2583613003
Cr-Commit-Position: refs/heads/master@{#15674}
diff --git a/webrtc/api/peerconnection.cc b/webrtc/api/peerconnection.cc
index 80c8a11..aeb03c6 100644
--- a/webrtc/api/peerconnection.cc
+++ b/webrtc/api/peerconnection.cc
@@ -614,6 +614,10 @@
}
// Destroy stats_ because it depends on session_.
stats_.reset(nullptr);
+ if (stats_collector_) {
+ stats_collector_->WaitForPendingRequest();
+ stats_collector_ = nullptr;
+ }
// Now destroy session_ before destroying other members,
// because its destruction fires signals (such as VoiceChannelDestroyed)
// which will trigger some final actions in PeerConnection...
diff --git a/webrtc/api/rtcstats_integrationtest.cc b/webrtc/api/rtcstats_integrationtest.cc
index db54b17..c28130fe 100644
--- a/webrtc/api/rtcstats_integrationtest.cc
+++ b/webrtc/api/rtcstats_integrationtest.cc
@@ -537,6 +537,19 @@
RTCStatsReportVerifier(report.get()).VerifyReport();
}
+TEST_F(RTCStatsIntegrationTest, GetsStatsWhileDestroyingPeerConnections) {
+ StartCall();
+
+ rtc::scoped_refptr<RTCStatsObtainer> stats_obtainer =
+ RTCStatsObtainer::Create();
+ caller_->pc()->GetStats(stats_obtainer);
+ // This will destroy the peer connection.
+ caller_ = nullptr;
+ // Any pending stats requests should have completed in the act of destroying
+ // the peer connection.
+ EXPECT_TRUE(stats_obtainer->report());
+}
+
} // namespace
} // namespace webrtc
diff --git a/webrtc/api/rtcstatscollector.cc b/webrtc/api/rtcstatscollector.cc
index 3628544..f1c7ed7 100644
--- a/webrtc/api/rtcstatscollector.cc
+++ b/webrtc/api/rtcstatscollector.cc
@@ -389,6 +389,10 @@
this, &RTCStatsCollector::OnDataChannelCreated);
}
+RTCStatsCollector::~RTCStatsCollector() {
+ RTC_DCHECK_EQ(num_pending_partial_reports_, 0);
+}
+
void RTCStatsCollector::GetStatsReport(
rtc::scoped_refptr<RTCStatsCollectorCallback> callback) {
RTC_DCHECK(signaling_thread_->IsCurrent());
@@ -453,6 +457,17 @@
cached_report_ = nullptr;
}
+void RTCStatsCollector::WaitForPendingRequest() {
+ RTC_DCHECK(signaling_thread_->IsCurrent());
+ if (num_pending_partial_reports_) {
+ rtc::Thread::Current()->ProcessMessages(0);
+ while (num_pending_partial_reports_) {
+ rtc::Thread::Current()->SleepMs(1);
+ rtc::Thread::Current()->ProcessMessages(0);
+ }
+ }
+}
+
void RTCStatsCollector::ProducePartialResultsOnSignalingThread(
int64_t timestamp_us) {
RTC_DCHECK(signaling_thread_->IsCurrent());
diff --git a/webrtc/api/rtcstatscollector.h b/webrtc/api/rtcstatscollector.h
index 7b23d24..4268a21 100644
--- a/webrtc/api/rtcstatscollector.h
+++ b/webrtc/api/rtcstatscollector.h
@@ -71,8 +71,13 @@
// calling |GetStatsReport| guarantees fresh stats.
void ClearCachedStatsReport();
+ // If there is a |GetStatsReport| requests in-flight, waits until it has been
+ // completed. Must be called on the signaling thread.
+ void WaitForPendingRequest();
+
protected:
RTCStatsCollector(PeerConnection* pc, int64_t cache_lifetime_us);
+ ~RTCStatsCollector();
// Stats gathering on a particular thread. Calls |AddPartialResults| before
// returning. Virtual for the sake of testing.