RTCRtpReceiverInternal destructor traits ensuring delete on main thread.

The internal class owns AdapterRefs which have to be destructed on the
main thread. As such, the last reference to the internal class had to
be freed on the main thread. In flaky cases, when tasks were posted to
the signaling thread, an operation performed and a task posted back to
the main thread - the main thread would finish before the signaling
thread task and the last reference to the internal class would be
released on the signaling thread, causing DCHECK crashes at
~AdapterRef for being on the wrong thread.

With destructor traits, if we are not already on the main thread we
post to the main thread and delete RTCRtpReceiverInternal there.

The TearDown of RTCRtpReceiverTest is updated to ensure that any pending
tasks get a chance to execute, in case the signaling thread was not
finished yet or else the destructor posted to the main thread does not
get a chance to execute and the test would flakily leak.

Before this CL: Flake's symptoms could be reproduced by adding a thread
sleep at RTCRtpReceiverInternal::GetStatsOnSignalingThread. After this CL:
Unable to repro flake.

Bug: 827450
Change-Id: Icdfdd3e22c3e86308ee7911e67eff43590fde581
Reviewed-on: https://chromium-review.googlesource.com/1013492
Commit-Queue: Henrik Boström <hbos@chromium.org>
Reviewed-by: Wez <wez@chromium.org>
Reviewed-by: Guido Urdaneta <guidou@chromium.org>
Cr-Commit-Position: refs/heads/master@{#553675}

content/renderer/media/webrtc/rtc_rtp_receiver.cc

@@ -13,7 +13,8 @@ namespace content {
 
 class RTCRtpReceiver::RTCRtpReceiverInternal
     : public base::RefCountedThreadSafe<
-          RTCRtpReceiver::RTCRtpReceiverInternal> {
+          RTCRtpReceiver::RTCRtpReceiverInternal,
+          RTCRtpReceiver::RTCRtpReceiverInternalTraits> {
  public:
   RTCRtpReceiverInternal(
       scoped_refptr<webrtc::PeerConnectionInterface> native_peer_connection,
@@ -94,8 +95,9 @@ class RTCRtpReceiver::RTCRtpReceiverInternal
   }
 
  private:
-  friend class base::RefCountedThreadSafe<RTCRtpReceiverInternal>;
-  ~RTCRtpReceiverInternal() {}
+  friend struct RTCRtpReceiver::RTCRtpReceiverInternalTraits;
+
+  ~RTCRtpReceiverInternal() { DCHECK(main_thread_->BelongsToCurrentThread()); }
 
   void GetStatsOnSignalingThread(
       std::unique_ptr<blink::WebRTCStatsReportCallback> callback) {
@@ -118,6 +120,26 @@ class RTCRtpReceiver::RTCRtpReceiverInternal
       stream_adapter_refs_;
 };
 
+struct RTCRtpReceiver::RTCRtpReceiverInternalTraits {
+ private:
+  friend class base::RefCountedThreadSafe<RTCRtpReceiverInternal,
+                                          RTCRtpReceiverInternalTraits>;
+
+  static void Destruct(const RTCRtpReceiverInternal* receiver) {
+    // RTCRtpReceiverInternal owns AdapterRefs which have to be destroyed on the
+    // main thread, this ensures delete always happens there.
+    if (!receiver->main_thread_->BelongsToCurrentThread()) {
+      receiver->main_thread_->PostTask(
+          FROM_HERE,
+          base::BindOnce(
+              &RTCRtpReceiver::RTCRtpReceiverInternalTraits::Destruct,
+              base::Unretained(receiver)));
+      return;
+    }
+    delete receiver;
+  }
+};
+
 uintptr_t RTCRtpReceiver::getId(
     const webrtc::RtpReceiverInterface* webrtc_rtp_receiver) {
   return reinterpret_cast<uintptr_t>(webrtc_rtp_receiver);

content/renderer/media/webrtc/rtc_rtp_receiver.h

@@ -60,6 +60,7 @@ class CONTENT_EXPORT RTCRtpReceiver : public blink::WebRTCRtpReceiver {
 
  private:
   class RTCRtpReceiverInternal;
+  struct RTCRtpReceiverInternalTraits;
 
   scoped_refptr<RTCRtpReceiverInternal> internal_;
 };

content/renderer/media/webrtc/rtc_rtp_receiver_unittest.cc

@@ -42,7 +42,24 @@ class RTCRtpReceiverTest : public ::testing::Test {
         dependency_factory_.get(), nullptr);
   }
 
-  void TearDown() override { blink::WebHeap::CollectAllGarbageForTesting(); }
+  void TearDown() override {
+    receiver_.reset();
+    // Syncing up with the signaling thread ensures any pending operations on
+    // that thread are executed. If they post back to the main thread, such as
+    // the sender's destructor traits, this is allowed to execute before the
+    // test shuts down the threads.
+    SyncWithSignalingThread();
+    blink::WebHeap::CollectAllGarbageForTesting();
+  }
+
+  // Wait for the signaling thread to perform any queued tasks, executing tasks
+  // posted to the current thread in the meantime while waiting.
+  void SyncWithSignalingThread() const {
+    base::RunLoop run_loop;
+    dependency_factory_->GetWebRtcSignalingThread()->PostTask(
+        FROM_HERE, run_loop.QuitClosure());
+    run_loop.Run();
+  }
 
   std::unique_ptr<RTCRtpReceiver> CreateReceiver(
       scoped_refptr<webrtc::MediaStreamTrackInterface> webrtc_track) {
@@ -127,8 +144,7 @@ TEST_F(RTCRtpReceiverTest, ShallowCopy) {
   EXPECT_EQ(copy->Track().UniqueId(), web_track_unique_id);
 }
 
-// Test is flaky (https://crbug.com/827450).
-TEST_F(RTCRtpReceiverTest, DISABLED_GetStats) {
+TEST_F(RTCRtpReceiverTest, GetStats) {
   scoped_refptr<MockWebRtcAudioTrack> webrtc_track =
       MockWebRtcAudioTrack::Create("webrtc_track");
   receiver_ = CreateReceiver(webrtc_track);

content/renderer/media/webrtc/rtc_rtp_sender.cc

@@ -37,8 +37,9 @@ void OnSetParametersCompleted(blink::WebRTCVoidRequest request,
 }  // namespace
 
 class RTCRtpSender::RTCRtpSenderInternal
-    : public base::RefCountedThreadSafe<RTCRtpSender::RTCRtpSenderInternal,
-                                        RTCRtpSender::RTCRtpSenderInternal> {
+    : public base::RefCountedThreadSafe<
+          RTCRtpSender::RTCRtpSenderInternal,
+          RTCRtpSender::RTCRtpSenderInternalTraits> {
  public:
   RTCRtpSenderInternal(
       scoped_refptr<webrtc::PeerConnectionInterface> native_peer_connection,
@@ -200,21 +201,7 @@ class RTCRtpSender::RTCRtpSenderInternal
   }
 
  private:
-  friend class base::RefCountedThreadSafe<RTCRtpSender::RTCRtpSenderInternal,
-                                          RTCRtpSender::RTCRtpSenderInternal>;
-
-  static void Destruct(const RTCRtpSenderInternal* sender) {
-    // RTCRtpSenderInternal owns AdapterRefs which have to be destroyed on the
-    // main thread, this ensures delete always happens there.
-    if (!sender->main_thread_->BelongsToCurrentThread()) {
-      sender->main_thread_->PostTask(
-          FROM_HERE,
-          base::BindOnce(&RTCRtpSender::RTCRtpSenderInternal::Destruct,
-                         base::Unretained(sender)));
-      return;
-    }
-    delete sender;
-  }
+  friend struct RTCRtpSender::RTCRtpSenderInternalTraits;
 
   ~RTCRtpSenderInternal() {
     // Ensured by destructor traits.
@@ -288,6 +275,25 @@ class RTCRtpSender::RTCRtpSenderInternal
   webrtc::RtpParameters parameters_;
 };
 
+struct RTCRtpSender::RTCRtpSenderInternalTraits {
+ private:
+  friend class base::RefCountedThreadSafe<RTCRtpSenderInternal,
+                                          RTCRtpSenderInternalTraits>;
+
+  static void Destruct(const RTCRtpSenderInternal* sender) {
+    // RTCRtpSenderInternal owns AdapterRefs which have to be destroyed on the
+    // main thread, this ensures delete always happens there.
+    if (!sender->main_thread_->BelongsToCurrentThread()) {
+      sender->main_thread_->PostTask(
+          FROM_HERE,
+          base::BindOnce(&RTCRtpSender::RTCRtpSenderInternalTraits::Destruct,
+                         base::Unretained(sender)));
+      return;
+    }
+    delete sender;
+  }
+};
+
 uintptr_t RTCRtpSender::getId(const webrtc::RtpSenderInterface* webrtc_sender) {
   return reinterpret_cast<uintptr_t>(webrtc_sender);
 }

content/renderer/media/webrtc/rtc_rtp_sender.h

@@ -82,6 +82,7 @@ class CONTENT_EXPORT RTCRtpSender : public blink::WebRTCRtpSender {
 
  private:
   class RTCRtpSenderInternal;
+  struct RTCRtpSenderInternalTraits;
 
   scoped_refptr<RTCRtpSenderInternal> internal_;
 };