commit dae07bae82c0c92e062c451a0648c70015593018
author Danil Chapovalov <danilchap@webrtc.org> Fri May 13 23:43:50 2016
committer Danil Chapovalov <danilchap@webrtc.org> Fri May 13 23:44:02 2016
tree 8277692f3ecbe60c2169630571e9c0505ba4ef76
parent 99f8cd0ae2f05c24410ddd6b2d9ea3d56f1fec5f

Fix BaseChannel destructor when network thread differ from worker thread

BUG=webrtc:5645
R=pthatcher@webrtc.org

Review URL: https://codereview.webrtc.org/1970223002 .

Cr-Commit-Position: refs/heads/master@{#12740}

webrtc/pc/channel.cc

diff --git a/webrtc/pc/channel.cc b/webrtc/pc/channel.cc
index 7e4d5d2..67c16ec 100644
--- a/webrtc/pc/channel.cc
+++ b/webrtc/pc/channel.cc
@@ -183,8 +183,6 @@
   ASSERT(worker_thread_ == rtc::Thread::Current());
   Deinit();
   StopConnectionMonitor();
-  // Send any outstanding RTCP packets.
-  network_thread_->Invoke<void>(Bind(&BaseChannel::FlushRtcpMessages_n, this));
   // Eats any outstanding messages or packets.
   worker_thread_->Clear(&invoker_);
   worker_thread_->Clear(this);
@@ -194,21 +192,40 @@
   delete media_channel_;
   // Note that we don't just call SetTransportChannel_n(nullptr) because that
   // would call a pure virtual method which we can't do from a destructor.
-  network_thread_->Invoke<void>(Bind(&BaseChannel::DeinitNetwork_n, this));
+  network_thread_->Invoke<void>(
+      Bind(&BaseChannel::DestroyTransportChannels_n, this));
   LOG(LS_INFO) << "Destroyed channel";
 }
 
-void BaseChannel::DeinitNetwork_n() {
+void BaseChannel::DisconnectTransportChannels_n() {
+  // Send any outstanding RTCP packets.
+  FlushRtcpMessages_n();
+
+  // Stop signals from transport channels, but keep them alive because
+  // media_channel may use them from a different thread.
   if (transport_channel_) {
     DisconnectFromTransportChannel(transport_channel_);
+  }
+  if (rtcp_transport_channel_) {
+    DisconnectFromTransportChannel(rtcp_transport_channel_);
+  }
+
+  // Clear pending read packets/messages.
+  network_thread_->Clear(&invoker_);
+  network_thread_->Clear(this);
+}
+
+void BaseChannel::DestroyTransportChannels_n() {
+  if (transport_channel_) {
     transport_controller_->DestroyTransportChannel_n(
         transport_name_, cricket::ICE_CANDIDATE_COMPONENT_RTP);
   }
   if (rtcp_transport_channel_) {
-    DisconnectFromTransportChannel(rtcp_transport_channel_);
     transport_controller_->DestroyTransportChannel_n(
         transport_name_, cricket::ICE_CANDIDATE_COMPONENT_RTCP);
   }
+  // Clear pending send packets/messages.
+  network_thread_->Clear(&invoker_);
   network_thread_->Clear(this);
 }
 
@@ -243,6 +260,11 @@
 void BaseChannel::Deinit() {
   RTC_DCHECK(worker_thread_->IsCurrent());
   media_channel_->SetInterface(NULL);
+  // Packets arrive on the network thread, processing packets calls virtual
+  // functions, so need to stop this process in Deinit that is called in
+  // derived classes destructor.
+  network_thread_->Invoke<void>(
+      Bind(&BaseChannel::DisconnectTransportChannels_n, this));
 }
 
 bool BaseChannel::SetTransport(const std::string& transport_name) {

webrtc/pc/channel.h

diff --git a/webrtc/pc/channel.h b/webrtc/pc/channel.h
index df75245..2f66f12 100644
--- a/webrtc/pc/channel.h
+++ b/webrtc/pc/channel.h
@@ -314,7 +314,8 @@
 
  private:
   bool InitNetwork_n();
-  void DeinitNetwork_n();
+  void DisconnectTransportChannels_n();
+  void DestroyTransportChannels_n();
   void SignalSentPacket_n(TransportChannel* channel,
                           const rtc::SentPacket& sent_packet);
   void SignalSentPacket_w(const rtc::SentPacket& sent_packet);

webrtc/pc/channel_unittest.cc

diff --git a/webrtc/pc/channel_unittest.cc b/webrtc/pc/channel_unittest.cc
index d15748f..81339cf 100644
--- a/webrtc/pc/channel_unittest.cc
+++ b/webrtc/pc/channel_unittest.cc
@@ -1000,6 +1000,19 @@
     EXPECT_TRUE(CheckNoRtp2());
   }
 
+  void TestDeinit() {
+    CreateChannels(RTCP, RTCP);
+    EXPECT_TRUE(SendInitiate());
+    EXPECT_TRUE(SendAccept());
+    SendRtp1();
+    SendRtp2();
+    SendRtcp1();
+    SendRtcp2();
+    // Do not wait, destroy channels.
+    channel1_.reset(nullptr);
+    channel2_.reset(nullptr);
+  }
+
   // Check that RTCP is not transmitted if both sides don't support RTCP.
   void SendNoRtcpToNoRtcp() {
     CreateChannels(0, 0);
@@ -2076,6 +2089,10 @@
   EXPECT_TRUE(media_channel1_->dtmf_info_queue().empty());
 }
 
+TEST_F(VoiceChannelSingleThreadTest, TestDeinit) {
+  Base::TestDeinit();
+}
+
 TEST_F(VoiceChannelSingleThreadTest, TestSetContents) {
   Base::TestSetContents();
 }
@@ -2402,6 +2419,10 @@
   EXPECT_TRUE(media_channel1_->dtmf_info_queue().empty());
 }
 
+TEST_F(VoiceChannelDoubleThreadTest, TestDeinit) {
+  Base::TestDeinit();
+}
+
 TEST_F(VoiceChannelDoubleThreadTest, TestSetContents) {
   Base::TestSetContents();
 }
@@ -2726,6 +2747,10 @@
   Base::TestInit();
 }
 
+TEST_F(VideoChannelSingleThreadTest, TestDeinit) {
+  Base::TestDeinit();
+}
+
 TEST_F(VideoChannelSingleThreadTest, TestSetContents) {
   Base::TestSetContents();
 }
@@ -2966,6 +2991,10 @@
   Base::TestInit();
 }
 
+TEST_F(VideoChannelDoubleThreadTest, TestDeinit) {
+  Base::TestDeinit();
+}
+
 TEST_F(VideoChannelDoubleThreadTest, TestSetContents) {
   Base::TestSetContents();
 }
@@ -3277,6 +3306,10 @@
   EXPECT_FALSE(media_channel1_->IsStreamMuted(0));
 }
 
+TEST_F(DataChannelSingleThreadTest, TestDeinit) {
+  Base::TestDeinit();
+}
+
 TEST_F(DataChannelSingleThreadTest, TestSetContents) {
   Base::TestSetContents();
 }
@@ -3417,6 +3450,10 @@
   EXPECT_FALSE(media_channel1_->IsStreamMuted(0));
 }
 
+TEST_F(DataChannelDoubleThreadTest, TestDeinit) {
+  Base::TestDeinit();
+}
+
 TEST_F(DataChannelDoubleThreadTest, TestSetContents) {
   Base::TestSetContents();
 }