From 3fe1b15413a6fdc1a01451e51b1ee634ad9faa4a Mon Sep 17 00:00:00 2001 From: Steve Anton Date: Tue, 12 Dec 2017 10:20:08 -0800 Subject: [PATCH] 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 Commit-Queue: Steve Anton Cr-Commit-Position: refs/heads/master@{#21232} --- pc/peerconnection.cc | 49 +++++++++++++++---------------- pc/peerconnection.h | 13 ++++++++ pc/peerconnection_rtp_unittest.cc | 22 +++++++++----- 3 files changed, 52 insertions(+), 32 deletions(-) diff --git a/pc/peerconnection.cc b/pc/peerconnection.cc index 4e65c311d0..da190ada39 100644 --- a/pc/peerconnection.cc +++ b/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 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( RTC_FROM_HERE, diff --git a/pc/peerconnection.h b/pc/peerconnection.h index c2b743dec1..16185de206 100644 --- a/pc/peerconnection.h +++ b/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( 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( 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|. diff --git a/pc/peerconnection_rtp_unittest.cc b/pc/peerconnection_rtp_unittest.cc index e7ac8c7f61..4989726020 100644 --- a/pc/peerconnection_rtp_unittest.cc +++ b/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