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}diff --git a/pc/peerconnection.cc b/pc/peerconnection.cc
index 4e65c31..da190ad 100644
--- a/pc/peerconnection.cc
+++ b/pc/peerconnection.cc
@@ -726,33 +726,16 @@
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 @@
});
}
+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 @@
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,
diff --git a/pc/peerconnection.h b/pc/peerconnection.h
index c2b743d..16185de 100644
--- a/pc/peerconnection.h
+++ b/pc/peerconnection.h
@@ -241,10 +241,18 @@
// 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 @@
// 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|.
diff --git a/pc/peerconnection_rtp_unittest.cc b/pc/peerconnection_rtp_unittest.cc
index e7ac8c7..4989726 100644
--- a/pc/peerconnection_rtp_unittest.cc
+++ b/pc/peerconnection_rtp_unittest.cc
@@ -543,13 +543,6 @@
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 @@
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
