Fix PeerConnection crashing on Close() when Unified Plan enabled

Bug: webrtc:8587
Change-Id: I283f6dbcf8ee7d0f99f528031137425afc35e4f4
Reviewed-on: https://webrtc-review.googlesource.com/31642
Reviewed-by: Peter Thatcher <pthatcher@webrtc.org>
Commit-Queue: Steve Anton <steveanton@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#21232}

pc/peerconnection.cc

@@ -726,33 +726,16 @@ PeerConnection::~PeerConnection() {
   TRACE_EVENT0("webrtc", "PeerConnection::~PeerConnection");
   RTC_DCHECK_RUN_ON(signaling_thread());
 
-  // Need to detach RTP transceivers from PeerConnection, since it's about to be
-  // destroyed.
-  for (auto transceiver : transceivers_) {
-    transceiver->Stop();
-  }
+  StopAndDestroyChannels();
 
-  // Destroy stats_ because it depends on session_.
+  // Destroy stats after stopping all transceivers because the senders/receivers
+  // will update the stats collector before stopping.
   stats_.reset(nullptr);
   if (stats_collector_) {
     stats_collector_->WaitForPendingRequest();
     stats_collector_ = nullptr;
   }
 
-  // Destroy video channels first since they may have a pointer to a voice
-  // channel.
-  for (auto transceiver : transceivers_) {
-    if (transceiver->internal()->media_type() == cricket::MEDIA_TYPE_VIDEO) {
-      DestroyTransceiverChannel(transceiver);
-    }
-  }
-  for (auto transceiver : transceivers_) {
-    if (transceiver->internal()->media_type() == cricket::MEDIA_TYPE_AUDIO) {
-      DestroyTransceiverChannel(transceiver);
-    }
-  }
-  DestroyDataChannel();
-
   RTC_LOG(LS_INFO) << "Session: " << session_id() << " is destroyed.";
 
   webrtc_session_desc_factory_.reset();
@@ -770,6 +753,25 @@ PeerConnection::~PeerConnection() {
   });
 }
 
+void PeerConnection::StopAndDestroyChannels() {
+  for (auto transceiver : transceivers_) {
+    transceiver->Stop();
+  }
+  // Destroy video channels first since they may have a pointer to a voice
+  // channel.
+  for (auto transceiver : transceivers_) {
+    if (transceiver->internal()->media_type() == cricket::MEDIA_TYPE_VIDEO) {
+      DestroyTransceiverChannel(transceiver);
+    }
+  }
+  for (auto transceiver : transceivers_) {
+    if (transceiver->internal()->media_type() == cricket::MEDIA_TYPE_AUDIO) {
+      DestroyTransceiverChannel(transceiver);
+    }
+  }
+  DestroyDataChannel();
+}
+
 bool PeerConnection::Initialize(
     const PeerConnectionInterface::RTCConfiguration& configuration,
     std::unique_ptr<cricket::PortAllocator> allocator,
@@ -2227,11 +2229,8 @@ void PeerConnection::Close() {
   stats_->UpdateStats(kStatsOutputLevelStandard);
 
   ChangeSignalingState(PeerConnectionInterface::kClosed);
-  RemoveUnusedChannels(nullptr);
-  RTC_DCHECK(!GetAudioTransceiver()->internal()->channel());
-  RTC_DCHECK(!GetVideoTransceiver()->internal()->channel());
-  RTC_DCHECK(!rtp_data_channel_);
-  RTC_DCHECK(!sctp_transport_);
+
+  StopAndDestroyChannels();
 
   network_thread()->Invoke<void>(
       RTC_FROM_HERE,

pc/peerconnection.h

@@ -241,10 +241,18 @@ class PeerConnection : public PeerConnectionInterface,
   // Exposed for stats collecting.
   // TODO(steveanton): Switch callers to use the plural form and remove these.
   virtual cricket::VoiceChannel* voice_channel() const {
+    if (IsUnifiedPlan()) {
+      // TODO(steveanton): Change stats collection to work with transceivers.
+      return nullptr;
+    }
     return static_cast<cricket::VoiceChannel*>(
         GetAudioTransceiver()->internal()->channel());
   }
   virtual cricket::VideoChannel* video_channel() const {
+    if (IsUnifiedPlan()) {
+      // TODO(steveanton): Change stats collection to work with transceivers.
+      return nullptr;
+    }
     return static_cast<cricket::VideoChannel*>(
         GetVideoTransceiver()->internal()->channel());
   }
@@ -649,6 +657,11 @@ class PeerConnection : public PeerConnectionInterface,
   // This enables media to flow on all configured audio/video channels and the
   // RtpDataChannel.
   void EnableSending();
+
+  // Stops all RtpTransceivers, destroys all BaseChannels and destroys the
+  // SCTP data channel.
+  void StopAndDestroyChannels();
+
   // Returns the media index for a local ice candidate given the content name.
   // Returns false if the local session description does not have a media
   // content called  |content_name|.

pc/peerconnection_rtp_unittest.cc

@@ -543,13 +543,6 @@ TEST_F(PeerConnectionRtpTest, AddTransceiverWithDirectionIsReflected) {
   EXPECT_EQ(RtpTransceiverDirection::kSendOnly, transceiver->direction());
 }
 
-TEST_F(PeerConnectionRtpTest, AddTransceiverWithInvalidKindReturnsError) {
-  auto caller = CreatePeerConnectionWithUnifiedPlan();
-
-  auto result = caller->pc()->AddTransceiver(cricket::MEDIA_TYPE_DATA);
-  EXPECT_EQ(RTCErrorType::INVALID_PARAMETER, result.error().type());
-}
-
 // Test that calling AddTransceiver with a track creates a transceiver which has
 // its sender's track set to the passed-in track.
 TEST_F(PeerConnectionRtpTest, AddTransceiverWithTrackCreatesSenderWithTrack) {
@@ -594,4 +587,19 @@ TEST_F(PeerConnectionRtpTest,
               UnorderedElementsAre(sender1, sender2));
 }
 
+// RtpTransceiver error handling tests.
+
+TEST_F(PeerConnectionRtpTest, AddTransceiverWithInvalidKindReturnsError) {
+  auto caller = CreatePeerConnectionWithUnifiedPlan();
+
+  auto result = caller->pc()->AddTransceiver(cricket::MEDIA_TYPE_DATA);
+  EXPECT_EQ(RTCErrorType::INVALID_PARAMETER, result.error().type());
+}
+
+TEST_F(PeerConnectionRtpTest, UnifiedPlanCanClosePeerConnection) {
+  auto caller = CreatePeerConnectionWithUnifiedPlan();
+
+  caller->pc()->Close();
+}
+
 }  // namespace webrtc