RTCRtpSenderInternal 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 RTCRtpSenderInternal there.

The TearDown of RTCRtpSenderTest 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 RTCRtpSenderInternal::GetStatsOnSignalingThread. After this CL:
Unable to repro flake. The same applies for the other flaking tests
based on replaceTrack().

Bug: 827450
Change-Id: Ib594a53042e441e591ccc2c87ae8012bcb4ec75e
Reviewed-on: https://chromium-review.googlesource.com/1015002
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@{#553550}

content/renderer/media/webrtc/rtc_rtp_sender.cc

@@ -37,7 +37,8 @@ void OnSetParametersCompleted(blink::WebRTCVoidRequest request,
 }  // namespace
 
 class RTCRtpSender::RTCRtpSenderInternal
-    : public base::RefCountedThreadSafe<RTCRtpSender::RTCRtpSenderInternal> {
+    : public base::RefCountedThreadSafe<RTCRtpSender::RTCRtpSenderInternal,
+                                        RTCRtpSender::RTCRtpSenderInternal> {
  public:
   RTCRtpSenderInternal(
       scoped_refptr<webrtc::PeerConnectionInterface> native_peer_connection,
@@ -199,8 +200,26 @@ class RTCRtpSender::RTCRtpSenderInternal
   }
 
  private:
-  friend class base::RefCountedThreadSafe<RTCRtpSenderInternal>;
-  virtual ~RTCRtpSenderInternal() {}
+  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;
+  }
+
+  ~RTCRtpSenderInternal() {
+    // Ensured by destructor traits.
+    DCHECK(main_thread_->BelongsToCurrentThread());
+  }
 
   // |webrtc_track| is passed as an argument because |track_ref->webrtc_track()|
   // cannot be accessed on the signaling thread. https://crbug.com/756436

content/renderer/media/webrtc/rtc_rtp_sender_unittest.cc

@@ -49,7 +49,24 @@ class RTCRtpSenderTest : public ::testing::Test {
     mock_webrtc_sender_ = new rtc::RefCountedObject<webrtc::MockRtpSender>();
   }
 
-  void TearDown() override { blink::WebHeap::CollectAllGarbageForTesting(); }
+  void TearDown() override {
+    sender_.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();
+  }
 
   blink::WebMediaStreamTrack CreateWebTrack(const std::string& id) {
     blink::WebMediaStreamSource web_source;
@@ -141,14 +158,7 @@ TEST_F(RTCRtpSenderTest, CreateSenderWithNullTrack) {
   EXPECT_TRUE(sender_->Track().IsNull());
 }
 
-// This test is flaky on Android and Linux.
-// See crbug.com/800465 for detail.
-#if defined(OS_ANDROID) || defined(OS_LINUX)
-#define MAYBE_ReplaceTrackSetsTrack DISABLED_ReplaceTrackSetsTrack
-#else
-#define MAYBE_ReplaceTrackSetsTrack ReplaceTrackSetsTrack
-#endif
-TEST_F(RTCRtpSenderTest, MAYBE_ReplaceTrackSetsTrack) {
+TEST_F(RTCRtpSenderTest, ReplaceTrackSetsTrack) {
   auto web_track1 = CreateWebTrack("track1");
   sender_ = CreateSender(web_track1);
 
@@ -160,14 +170,7 @@ TEST_F(RTCRtpSenderTest, MAYBE_ReplaceTrackSetsTrack) {
   EXPECT_EQ(web_track2.UniqueId(), sender_->Track().UniqueId());
 }
 
-// This test is flaky on Android and Linux.
-// See crbug.com/803597 for detail.
-#if defined(OS_ANDROID) || defined(OS_LINUX)
-#define MAYBE_ReplaceTrackWithNullTrack DISABLED_ReplaceTrackWithNullTrack
-#else
-#define MAYBE_ReplaceTrackWithNullTrack ReplaceTrackWithNullTrack
-#endif
-TEST_F(RTCRtpSenderTest, MAYBE_ReplaceTrackWithNullTrack) {
+TEST_F(RTCRtpSenderTest, ReplaceTrackWithNullTrack) {
   auto web_track = CreateWebTrack("track_id");
   sender_ = CreateSender(web_track);
 
@@ -178,14 +181,7 @@ TEST_F(RTCRtpSenderTest, MAYBE_ReplaceTrackWithNullTrack) {
   EXPECT_TRUE(sender_->Track().IsNull());
 }
 
-// This test is flaky on Android and Linux.
-// See crbug.com/800465 for detail.
-#if defined(OS_ANDROID) || defined(OS_LINUX)
-#define MAYBE_ReplaceTrackCanFail DISABLED_ReplaceTrackCanFail
-#else
-#define MAYBE_ReplaceTrackCanFail ReplaceTrackCanFail
-#endif
-TEST_F(RTCRtpSenderTest, MAYBE_ReplaceTrackCanFail) {
+TEST_F(RTCRtpSenderTest, ReplaceTrackCanFail) {
   auto web_track = CreateWebTrack("track_id");
   sender_ = CreateSender(web_track);
   ASSERT_FALSE(sender_->Track().IsNull());
@@ -200,16 +196,7 @@ TEST_F(RTCRtpSenderTest, MAYBE_ReplaceTrackCanFail) {
   EXPECT_EQ(web_track.UniqueId(), sender_->Track().UniqueId());
 }
 
-// This test is flaky on Android and Linux.
-// See crbug.com/800465 for detail.
-#if defined(OS_ANDROID) || defined(OS_LINUX)
-#define MAYBE_ReplaceTrackIsNotSetSynchronously \
-  DISABLED_ReplaceTrackIsNotSetSynchronously
-#else
-#define MAYBE_ReplaceTrackIsNotSetSynchronously \
-  ReplaceTrackIsNotSetSynchronously
-#endif
-TEST_F(RTCRtpSenderTest, MAYBE_ReplaceTrackIsNotSetSynchronously) {
+TEST_F(RTCRtpSenderTest, ReplaceTrackIsNotSetSynchronously) {
   auto web_track1 = CreateWebTrack("track1");
   sender_ = CreateSender(web_track1);
 
@@ -223,8 +210,7 @@ TEST_F(RTCRtpSenderTest, MAYBE_ReplaceTrackIsNotSetSynchronously) {
   std::move(replaceTrackRunLoopAndGetResult).Run();
 }
 
-// Test is flaky (https://crbug.com/827450).
-TEST_F(RTCRtpSenderTest, DISABLED_GetStats) {
+TEST_F(RTCRtpSenderTest, GetStats) {
   auto web_track = CreateWebTrack("track_id");
   sender_ = CreateSender(web_track);
 
@@ -250,8 +236,7 @@ TEST_F(RTCRtpSenderTest, DISABLED_GetStats) {
   EXPECT_EQ(stats->Timestamp(), 1.234);
 }
 
-// TODO(crbug.com/812296): Disabled since flaky.
-TEST_F(RTCRtpSenderTest, DISABLED_CopiedSenderSharesInternalStates) {
+TEST_F(RTCRtpSenderTest, CopiedSenderSharesInternalStates) {
   auto web_track = CreateWebTrack("track_id");
   sender_ = CreateSender(web_track);
   auto copy = std::make_unique<RTCRtpSender>(*sender_);