Fix segfault when PeerConnection is destroyed during stats collection.

webrtc/api/peerconnection.cc

 /*
  *  Copyright 2012 The WebRTC project authors. All Rights Reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
@@ skipping 590 matching lines @@
   // Need to detach RTP senders/receivers from WebRtcSession,
   // since it's about to be destroyed.
   for (const auto& sender : senders_) {
     sender->internal()->Stop();
   }
   for (const auto& receiver : receivers_) {
     receiver->internal()->Stop();
   }
   // Destroy stats_ because it depends on session_.
   stats_.reset(nullptr);
+  if (stats_collector_) {
+    stats_collector_->WaitForPendingRequest();

[Taylor Brandstetter, 2016/12/16 18:53:27]

nit: I'd just put this in the RTCStatsCollector destructor instead of making it
a public method. But I do see the reasoning of "a destructor shouldn't be doing
things like waiting for other threads".

If you choose to leave it like this, could you add something like:

RTC_DCHECK(num_pending_partial_reports_ == 0) << "You must call
WaitForPendingRequest before safely destroying the RTCStatsCollector."

... in the RTCStatsCollector destructor? And leave a comment in the header
saying WaitForPendingRequest must be called before safely destroying?

[hbos, 2016/12/19 09:52:48]

It's not that you have to wait before destroying the collector, in fact the
collector is reference counted and will not be destroyed during a pending
request by design. The wait is to ensure that resources the collector is relying
on (that are not reference counted or owned by the collector) are not destroyed,
e.g. making sure PeerConnection and friends is not destroyed before the
collector.

I added a DCHECK, but if it is ever hit we have a bug and not just a problem of
not calling WaitForPendingRequest.

That said, now that we have a WaitForPendingRequests we could remove the
reference counting if we wanted to (not in this CL).

+    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...
   session_.reset(nullptr);
   // port_allocator_ lives on the network thread and should be destroyed there.
   network_thread()->Invoke<void>(RTC_FROM_HERE,
                                  [this] { port_allocator_.reset(nullptr); });
 }
 
 bool PeerConnection::Initialize(
@@ skipping 1779 matching lines @@
 
 bool PeerConnection::StartRtcEventLog_w(rtc::PlatformFile file,
                                         int64_t max_size_bytes) {
   return event_log_->StartLogging(file, max_size_bytes);
 }
 
 void PeerConnection::StopRtcEventLog_w() {
   event_log_->StopLogging();
 }
 }  // namespace webrtc