From 59cfd35438753acd69859e8a2f98f6490e68d6a0 Mon Sep 17 00:00:00 2001
From: Yves Gerey <yvesg@webrtc.org>
Date: Mon, 26 Nov 2018 16:22:20 +0100
Subject: [PATCH] Address vptr race condition while PeerConnection is
 destructed.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Auxiliary threads (worker, network) are still active
while PeerConnection is destructed, leading to race condition
in tests such as:
  * RTCStatsIntegrationTest.GetStatsFromCaller
  * RTCStatsIntegrationTest.GetsStatsWhileDestroyingPeerConnection

This CL prevents the conflict to happen by explicitly
closing the PeerConnection.

Bug: webrtc:9847
Change-Id: I40880bb9b193201711031b8c4563c6bbd4983c71
Reviewed-on: https://webrtc-review.googlesource.com/c/104820
Reviewed-by: Karl Wiberg <kwiberg@webrtc.org>
Reviewed-by: Henrik Boström <hbos@webrtc.org>
Reviewed-by: Niels Moller <nisse@webrtc.org>
Commit-Queue: Yves Gerey <yvesg@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#25801}
---
 pc/rtcstats_integrationtest.cc       |  8 ++++----
 pc/test/peerconnectiontestwrapper.cc | 18 ++++++++++++++++--
 pc/test/peerconnectiontestwrapper.h  |  2 ++
 3 files changed, 22 insertions(+), 6 deletions(-)

diff --git a/pc/rtcstats_integrationtest.cc b/pc/rtcstats_integrationtest.cc
index 46687bab5f..ef29cbc71e 100644
--- a/pc/rtcstats_integrationtest.cc
+++ b/pc/rtcstats_integrationtest.cc
@@ -809,11 +809,11 @@ TEST_F(RTCStatsIntegrationTest, GetStatsWithInvalidReceiverSelector) {
   EXPECT_FALSE(report->size());
 }
 
-// TODO(bugs.webrtc.org/9847) Remove this test altogether if a proper fix cannot
-// be found. For now it is lying, as we cannot safely gets stats while
-// destroying PeerConnection.
+// TODO(bugs.webrtc.org/10041) For now this is equivalent to the following
+// test GetsStatsWhileClosingPeerConnection, because pc() is closed by
+// PeerConnectionTestWrapper. See: bugs.webrtc.org/9847
 TEST_F(RTCStatsIntegrationTest,
-       DISABLED_GetsStatsWhileDestroyingPeerConnection) {
+       DISABLED_GetStatsWhileDestroyingPeerConnection) {
   StartCall();
 
   rtc::scoped_refptr<RTCStatsObtainer> stats_obtainer =
diff --git a/pc/test/peerconnectiontestwrapper.cc b/pc/test/peerconnectiontestwrapper.cc
index 0010edb0f8..a1db9ed9e6 100644
--- a/pc/test/peerconnectiontestwrapper.cc
+++ b/pc/test/peerconnectiontestwrapper.cc
@@ -23,6 +23,7 @@
 #include "pc/test/mockpeerconnectionobservers.h"
 #include "pc/test/peerconnectiontestwrapper.h"
 #include "rtc_base/gunit.h"
+#include "rtc_base/thread_checker.h"
 #include "rtc_base/timeutils.h"
 
 using webrtc::FakeConstraints;
@@ -65,9 +66,20 @@ PeerConnectionTestWrapper::PeerConnectionTestWrapper(
     rtc::Thread* worker_thread)
     : name_(name),
       network_thread_(network_thread),
-      worker_thread_(worker_thread) {}
+      worker_thread_(worker_thread) {
+  pc_thread_checker_.DetachFromThread();
+}
 
-PeerConnectionTestWrapper::~PeerConnectionTestWrapper() {}
+PeerConnectionTestWrapper::~PeerConnectionTestWrapper() {
+  RTC_DCHECK_RUN_ON(&pc_thread_checker_);
+  // Either network_thread or worker_thread might be active at this point.
+  // Relying on ~PeerConnection to properly wait for them doesn't work,
+  // as a vptr race might occur (before we enter the destruction body).
+  // See: bugs.webrtc.org/9847
+  if (pc()) {
+    pc()->Close();
+  }
+}
 
 bool PeerConnectionTestWrapper::CreatePc(
     const webrtc::PeerConnectionInterface::RTCConfiguration& config,
@@ -76,6 +88,8 @@ bool PeerConnectionTestWrapper::CreatePc(
   std::unique_ptr<cricket::PortAllocator> port_allocator(
       new cricket::FakePortAllocator(network_thread_, nullptr));
 
+  RTC_DCHECK_RUN_ON(&pc_thread_checker_);
+
   fake_audio_capture_module_ = FakeAudioCaptureModule::Create();
   if (fake_audio_capture_module_ == NULL) {
     return false;
diff --git a/pc/test/peerconnectiontestwrapper.h b/pc/test/peerconnectiontestwrapper.h
index 21ba89acbf..b6a57f35d3 100644
--- a/pc/test/peerconnectiontestwrapper.h
+++ b/pc/test/peerconnectiontestwrapper.h
@@ -20,6 +20,7 @@
 #include "pc/test/fakeaudiocapturemodule.h"
 #include "pc/test/fakevideotrackrenderer.h"
 #include "rtc_base/third_party/sigslot/sigslot.h"
+#include "rtc_base/thread_checker.h"
 
 class PeerConnectionTestWrapper
     : public webrtc::PeerConnectionObserver,
@@ -106,6 +107,7 @@ class PeerConnectionTestWrapper
   std::string name_;
   rtc::Thread* const network_thread_;
   rtc::Thread* const worker_thread_;
+  rtc::ThreadChecker pc_thread_checker_;
   rtc::scoped_refptr<webrtc::PeerConnectionInterface> peer_connection_;
   rtc::scoped_refptr<webrtc::PeerConnectionFactoryInterface>
       peer_connection_factory_;