From 29ff8446c0c89114729f6b8cfe49063c899b5764 Mon Sep 17 00:00:00 2001 From: zhihuang Date: Wed, 27 Jul 2016 11:07:25 -0700 Subject: [PATCH] Add PeerConnection IsClosed check. Add IsClosed check when excuting some functions so that they can return early if the PeerConnection is closed. The observer will not be called after the PeerConnection is closed. BUG=webrtc:5861 Review-Url: https://codereview.webrtc.org/1975453002 Cr-Commit-Position: refs/heads/master@{#13544} --- webrtc/api/peerconnection.cc | 47 ++++++++- webrtc/api/peerconnectionfactory.cc | 7 ++ webrtc/api/peerconnectionfactory.h | 2 + .../api/peerconnectioninterface_unittest.cc | 99 ++++++++++++++----- webrtc/api/statscollector_unittest.cc | 15 ++- webrtc/api/webrtcsession.cc | 16 +-- webrtc/api/webrtcsession.h | 12 ++- webrtc/api/webrtcsession_unittest.cc | 23 +++-- 8 files changed, 168 insertions(+), 53 deletions(-) diff --git a/webrtc/api/peerconnection.cc b/webrtc/api/peerconnection.cc index be614727b9..e217f8089f 100644 --- a/webrtc/api/peerconnection.cc +++ b/webrtc/api/peerconnection.cc @@ -622,10 +622,13 @@ bool PeerConnection::Initialize( media_controller_.reset( factory_->CreateMediaController(configuration.media_config)); - session_.reset( - new WebRtcSession(media_controller_.get(), factory_->network_thread(), - factory_->worker_thread(), factory_->signaling_thread(), - port_allocator_.get())); + session_.reset(new WebRtcSession( + media_controller_.get(), factory_->network_thread(), + factory_->worker_thread(), factory_->signaling_thread(), + port_allocator_.get(), + std::unique_ptr( + factory_->CreateTransportController(port_allocator_.get())))); + stats_.reset(new StatsCollector(this)); // Initialize the WebRtcSession. It creates transport channels etc. @@ -800,6 +803,9 @@ bool PeerConnection::RemoveTrack(RtpSenderInterface* sender) { rtc::scoped_refptr PeerConnection::CreateDtmfSender( AudioTrackInterface* track) { TRACE_EVENT0("webrtc", "PeerConnection::CreateDtmfSender"); + if (IsClosed()) { + return nullptr; + } if (!track) { LOG(LS_ERROR) << "CreateDtmfSender - track is NULL."; return NULL; @@ -822,6 +828,9 @@ rtc::scoped_refptr PeerConnection::CreateSender( const std::string& kind, const std::string& stream_id) { TRACE_EVENT0("webrtc", "PeerConnection::CreateSender"); + if (IsClosed()) { + return nullptr; + } rtc::scoped_refptr> new_sender; if (kind == MediaStreamTrackInterface::kAudioKind) { new_sender = RtpSenderProxyWithInternal::Create( @@ -1033,6 +1042,9 @@ void PeerConnection::SetLocalDescription( SetSessionDescriptionObserver* observer, SessionDescriptionInterface* desc) { TRACE_EVENT0("webrtc", "PeerConnection::SetLocalDescription"); + if (IsClosed()) { + return; + } if (!VERIFY(observer != nullptr)) { LOG(LS_ERROR) << "SetLocalDescription - observer is NULL."; return; @@ -1112,6 +1124,9 @@ void PeerConnection::SetRemoteDescription( SetSessionDescriptionObserver* observer, SessionDescriptionInterface* desc) { TRACE_EVENT0("webrtc", "PeerConnection::SetRemoteDescription"); + if (IsClosed()) { + return; + } if (!VERIFY(observer != nullptr)) { LOG(LS_ERROR) << "SetRemoteDescription - observer is NULL."; return; @@ -1234,6 +1249,9 @@ bool PeerConnection::SetConfiguration(const RTCConfiguration& configuration) { bool PeerConnection::AddIceCandidate( const IceCandidateInterface* ice_candidate) { TRACE_EVENT0("webrtc", "PeerConnection::AddIceCandidate"); + if (IsClosed()) { + return false; + } return session_->ProcessIceMessage(ice_candidate); } @@ -1420,17 +1438,26 @@ void PeerConnection::OnIceGatheringChange( void PeerConnection::OnIceCandidate(const IceCandidateInterface* candidate) { RTC_DCHECK(signaling_thread()->IsCurrent()); + if (IsClosed()) { + return; + } observer_->OnIceCandidate(candidate); } void PeerConnection::OnIceCandidatesRemoved( const std::vector& candidates) { RTC_DCHECK(signaling_thread()->IsCurrent()); + if (IsClosed()) { + return; + } observer_->OnIceCandidatesRemoved(candidates); } void PeerConnection::OnIceConnectionReceivingChange(bool receiving) { RTC_DCHECK(signaling_thread()->IsCurrent()); + if (IsClosed()) { + return; + } observer_->OnIceConnectionReceivingChange(receiving); } @@ -1450,6 +1477,9 @@ void PeerConnection::ChangeSignalingState( void PeerConnection::OnAudioTrackAdded(AudioTrackInterface* track, MediaStreamInterface* stream) { + if (IsClosed()) { + return; + } auto sender = FindSenderForTrack(track); if (sender != senders_.end()) { // We already have a sender for this track, so just change the stream_id @@ -1482,6 +1512,9 @@ void PeerConnection::OnAudioTrackAdded(AudioTrackInterface* track, // indefinitely, when we have unified plan SDP. void PeerConnection::OnAudioTrackRemoved(AudioTrackInterface* track, MediaStreamInterface* stream) { + if (IsClosed()) { + return; + } auto sender = FindSenderForTrack(track); if (sender == senders_.end()) { LOG(LS_WARNING) << "RtpSender for track with id " << track->id() @@ -1494,6 +1527,9 @@ void PeerConnection::OnAudioTrackRemoved(AudioTrackInterface* track, void PeerConnection::OnVideoTrackAdded(VideoTrackInterface* track, MediaStreamInterface* stream) { + if (IsClosed()) { + return; + } auto sender = FindSenderForTrack(track); if (sender != senders_.end()) { // We already have a sender for this track, so just change the stream_id @@ -1517,6 +1553,9 @@ void PeerConnection::OnVideoTrackAdded(VideoTrackInterface* track, void PeerConnection::OnVideoTrackRemoved(VideoTrackInterface* track, MediaStreamInterface* stream) { + if (IsClosed()) { + return; + } auto sender = FindSenderForTrack(track); if (sender == senders_.end()) { LOG(LS_WARNING) << "RtpSender for track with id " << track->id() diff --git a/webrtc/api/peerconnectionfactory.cc b/webrtc/api/peerconnectionfactory.cc index b79e7a2544..26ca66604c 100644 --- a/webrtc/api/peerconnectionfactory.cc +++ b/webrtc/api/peerconnectionfactory.cc @@ -300,6 +300,13 @@ webrtc::MediaControllerInterface* PeerConnectionFactory::CreateMediaController( channel_manager_.get()); } +cricket::TransportController* PeerConnectionFactory::CreateTransportController( + cricket::PortAllocator* port_allocator) { + RTC_DCHECK(signaling_thread_->IsCurrent()); + return new cricket::TransportController(signaling_thread_, network_thread_, + port_allocator); +} + rtc::Thread* PeerConnectionFactory::signaling_thread() { // This method can be called on a different thread when the factory is // created in CreatePeerConnectionFactory(). diff --git a/webrtc/api/peerconnectionfactory.h b/webrtc/api/peerconnectionfactory.h index 39a64027b6..c209fdbba4 100644 --- a/webrtc/api/peerconnectionfactory.h +++ b/webrtc/api/peerconnectionfactory.h @@ -92,6 +92,8 @@ class PeerConnectionFactory : public PeerConnectionFactoryInterface { virtual webrtc::MediaControllerInterface* CreateMediaController( const cricket::MediaConfig& config) const; + virtual cricket::TransportController* CreateTransportController( + cricket::PortAllocator* port_allocator); virtual rtc::Thread* signaling_thread(); virtual rtc::Thread* worker_thread(); virtual rtc::Thread* network_thread(); diff --git a/webrtc/api/peerconnectioninterface_unittest.cc b/webrtc/api/peerconnectioninterface_unittest.cc index 843a312292..419f69c563 100644 --- a/webrtc/api/peerconnectioninterface_unittest.cc +++ b/webrtc/api/peerconnectioninterface_unittest.cc @@ -40,6 +40,7 @@ #include "webrtc/media/base/fakevideocapturer.h" #include "webrtc/media/sctp/sctpdataengine.h" #include "webrtc/p2p/base/fakeportallocator.h" +#include "webrtc/p2p/base/faketransportcontroller.h" #include "webrtc/pc/mediasession.h" static const char kStreamLabel1[] = "local_stream_1"; @@ -491,11 +492,13 @@ class MockPeerConnectionObserver : public PeerConnectionObserver { void OnIceConnectionChange( PeerConnectionInterface::IceConnectionState new_state) override { EXPECT_EQ(pc_->ice_connection_state(), new_state); + callback_triggered = true; } void OnIceGatheringChange( PeerConnectionInterface::IceGatheringState new_state) override { EXPECT_EQ(pc_->ice_gathering_state(), new_state); ice_complete_ = new_state == PeerConnectionInterface::kIceGatheringComplete; + callback_triggered = true; } void OnIceCandidate(const webrtc::IceCandidateInterface* candidate) override { EXPECT_NE(PeerConnectionInterface::kIceGatheringNew, @@ -507,6 +510,16 @@ class MockPeerConnectionObserver : public PeerConnectionObserver { last_candidate_.reset(webrtc::CreateIceCandidate(candidate->sdp_mid(), candidate->sdp_mline_index(), sdp, NULL)); EXPECT_TRUE(last_candidate_.get() != NULL); + callback_triggered = true; + } + + void OnIceCandidatesRemoved( + const std::vector& candidates) override { + callback_triggered = true; + } + + void OnIceConnectionReceivingChange(bool receiving) override { + callback_triggered = true; } // Returns the label of the last added stream. @@ -529,6 +542,7 @@ class MockPeerConnectionObserver : public PeerConnectionObserver { rtc::scoped_refptr remote_streams_; bool renegotiation_needed_ = false; bool ice_complete_ = false; + bool callback_triggered = false; private: scoped_refptr last_added_stream_; @@ -537,6 +551,36 @@ class MockPeerConnectionObserver : public PeerConnectionObserver { } // namespace +// The PeerConnectionMediaConfig tests below verify that configuration +// and constraints are propagated into the MediaConfig passed to +// CreateMediaController. These settings are intended for MediaChannel +// constructors, but that is not exercised by these unittest. +class PeerConnectionFactoryForTest : public webrtc::PeerConnectionFactory { + public: + webrtc::MediaControllerInterface* CreateMediaController( + const cricket::MediaConfig& config) const override { + create_media_controller_called_ = true; + create_media_controller_config_ = config; + + webrtc::MediaControllerInterface* mc = + PeerConnectionFactory::CreateMediaController(config); + EXPECT_TRUE(mc != nullptr); + return mc; + } + + cricket::TransportController* CreateTransportController( + cricket::PortAllocator* port_allocator) override { + transport_controller = new cricket::TransportController( + rtc::Thread::Current(), rtc::Thread::Current(), port_allocator); + return transport_controller; + } + + cricket::TransportController* transport_controller; + // Mutable, so they can be modified in the above const-declared method. + mutable bool create_media_controller_called_ = false; + mutable cricket::MediaConfig create_media_controller_config_; +}; + class PeerConnectionInterfaceTest : public testing::Test { protected: PeerConnectionInterfaceTest() { @@ -550,6 +594,9 @@ class PeerConnectionInterfaceTest : public testing::Test { rtc::Thread::Current(), rtc::Thread::Current(), rtc::Thread::Current(), nullptr, nullptr, nullptr); ASSERT_TRUE(pc_factory_); + pc_factory_for_test_ = + new rtc::RefCountedObject(); + pc_factory_for_test_->Initialize(); } void CreatePeerConnection() { @@ -732,7 +779,9 @@ class PeerConnectionInterfaceTest : public testing::Test { } else { pc_->SetRemoteDescription(observer, desc); } - EXPECT_EQ_WAIT(true, observer->called(), kTimeout); + if (pc_->signaling_state() != PeerConnectionInterface::kClosed) { + EXPECT_EQ_WAIT(true, observer->called(), kTimeout); + } return observer->result(); } @@ -994,11 +1043,37 @@ class PeerConnectionInterfaceTest : public testing::Test { cricket::FakePortAllocator* port_allocator_ = nullptr; scoped_refptr pc_factory_; + scoped_refptr pc_factory_for_test_; scoped_refptr pc_; MockPeerConnectionObserver observer_; rtc::scoped_refptr reference_collection_; }; +// Test that no callbacks on the PeerConnectionObserver are called after the +// PeerConnection is closed. +TEST_F(PeerConnectionInterfaceTest, CloseAndTestCallbackFunctions) { + scoped_refptr pc( + pc_factory_for_test_->CreatePeerConnection( + PeerConnectionInterface::RTCConfiguration(), nullptr, nullptr, + nullptr, &observer_)); + observer_.SetPeerConnectionInterface(pc.get()); + pc->Close(); + + // No callbacks is expected to be called. + observer_.callback_triggered = false; + std::vector candidates; + pc_factory_for_test_->transport_controller->SignalGatheringState( + cricket::IceGatheringState{}); + pc_factory_for_test_->transport_controller->SignalCandidatesGathered( + "", candidates); + pc_factory_for_test_->transport_controller->SignalConnectionState( + cricket::IceConnectionState{}); + pc_factory_for_test_->transport_controller->SignalCandidatesRemoved( + candidates); + pc_factory_for_test_->transport_controller->SignalReceiving(false); + EXPECT_FALSE(observer_.callback_triggered); +} + // Generate different CNAMEs when PeerConnections are created. // The CNAMEs are expected to be generated randomly. It is possible // that the test fails, though the possibility is very low. @@ -2555,28 +2630,6 @@ TEST_F(PeerConnectionInterfaceTest, EXPECT_TRUE(ContainsSender(new_senders, kVideoTracks[0], kStreams[1])); } -// The PeerConnectionMediaConfig tests below verify that configuration -// and constraints are propagated into the MediaConfig passed to -// CreateMediaController. These settings are intended for MediaChannel -// constructors, but that is not exercised by these unittest. -class PeerConnectionFactoryForTest : public webrtc::PeerConnectionFactory { - public: - webrtc::MediaControllerInterface* CreateMediaController( - const cricket::MediaConfig& config) const override { - create_media_controller_called_ = true; - create_media_controller_config_ = config; - - webrtc::MediaControllerInterface* mc = - PeerConnectionFactory::CreateMediaController(config); - EXPECT_TRUE(mc != nullptr); - return mc; - } - - // Mutable, so they can be modified in the above const-declared method. - mutable bool create_media_controller_called_ = false; - mutable cricket::MediaConfig create_media_controller_config_; -}; - class PeerConnectionMediaConfigTest : public testing::Test { protected: void SetUp() override { diff --git a/webrtc/api/statscollector_unittest.cc b/webrtc/api/statscollector_unittest.cc index 7953515072..e0657a1b95 100644 --- a/webrtc/api/statscollector_unittest.cc +++ b/webrtc/api/statscollector_unittest.cc @@ -72,11 +72,16 @@ class MockWebRtcSession : public webrtc::WebRtcSession { // warnings from -Winconsistent-missing-override. See // http://crbug.com/428099. explicit MockWebRtcSession(webrtc::MediaControllerInterface* media_controller) - : WebRtcSession(media_controller, - rtc::Thread::Current(), - rtc::Thread::Current(), - rtc::Thread::Current(), - nullptr) {} + : WebRtcSession( + media_controller, + rtc::Thread::Current(), + rtc::Thread::Current(), + rtc::Thread::Current(), + nullptr, + std::unique_ptr( + new cricket::TransportController(rtc::Thread::Current(), + rtc::Thread::Current(), + nullptr))) {} MOCK_METHOD0(voice_channel, cricket::VoiceChannel*()); MOCK_METHOD0(video_channel, cricket::VideoChannel*()); // Libjingle uses "local" for a outgoing track, and "remote" for a incoming diff --git a/webrtc/api/webrtcsession.cc b/webrtc/api/webrtcsession.cc index 8c83dbbc62..7497e2c7e0 100644 --- a/webrtc/api/webrtcsession.cc +++ b/webrtc/api/webrtcsession.cc @@ -453,20 +453,20 @@ bool CheckForRemoteIceRestart(const SessionDescriptionInterface* old_desc, return false; } -WebRtcSession::WebRtcSession(webrtc::MediaControllerInterface* media_controller, - rtc::Thread* network_thread, - rtc::Thread* worker_thread, - rtc::Thread* signaling_thread, - cricket::PortAllocator* port_allocator) +WebRtcSession::WebRtcSession( + webrtc::MediaControllerInterface* media_controller, + rtc::Thread* network_thread, + rtc::Thread* worker_thread, + rtc::Thread* signaling_thread, + cricket::PortAllocator* port_allocator, + std::unique_ptr transport_controller) : worker_thread_(worker_thread), signaling_thread_(signaling_thread), // RFC 3264: The numeric value of the session id and version in the // o line MUST be representable with a "64 bit signed integer". // Due to this constraint session id |sid_| is max limited to LLONG_MAX. sid_(rtc::ToString(rtc::CreateRandomId64() & LLONG_MAX)), - transport_controller_(new cricket::TransportController(signaling_thread, - network_thread, - port_allocator)), + transport_controller_(std::move(transport_controller)), media_controller_(media_controller), channel_manager_(media_controller_->channel_manager()), ice_observer_(NULL), diff --git a/webrtc/api/webrtcsession.h b/webrtc/api/webrtcsession.h index d69abc0ded..1314c59d1b 100644 --- a/webrtc/api/webrtcsession.h +++ b/webrtc/api/webrtcsession.h @@ -136,11 +136,13 @@ class WebRtcSession : ERROR_TRANSPORT = 2, // transport error of some kind }; - WebRtcSession(webrtc::MediaControllerInterface* media_controller, - rtc::Thread* network_thread, - rtc::Thread* worker_thread, - rtc::Thread* signaling_thread, - cricket::PortAllocator* port_allocator); + WebRtcSession( + webrtc::MediaControllerInterface* media_controller, + rtc::Thread* network_thread, + rtc::Thread* worker_thread, + rtc::Thread* signaling_thread, + cricket::PortAllocator* port_allocator, + std::unique_ptr transport_controller); virtual ~WebRtcSession(); // These are const to allow them to be called from const methods. diff --git a/webrtc/api/webrtcsession_unittest.cc b/webrtc/api/webrtcsession_unittest.cc index b90daee510..b96236d218 100644 --- a/webrtc/api/webrtcsession_unittest.cc +++ b/webrtc/api/webrtcsession_unittest.cc @@ -211,17 +211,20 @@ class MockIceObserver : public webrtc::IceObserver { class WebRtcSessionForTest : public webrtc::WebRtcSession { public: - WebRtcSessionForTest(webrtc::MediaControllerInterface* media_controller, - rtc::Thread* network_thread, - rtc::Thread* worker_thread, - rtc::Thread* signaling_thread, - cricket::PortAllocator* port_allocator, - webrtc::IceObserver* ice_observer) + WebRtcSessionForTest( + webrtc::MediaControllerInterface* media_controller, + rtc::Thread* network_thread, + rtc::Thread* worker_thread, + rtc::Thread* signaling_thread, + cricket::PortAllocator* port_allocator, + webrtc::IceObserver* ice_observer, + std::unique_ptr transport_controller) : WebRtcSession(media_controller, network_thread, worker_thread, signaling_thread, - port_allocator) { + port_allocator, + std::move(transport_controller)) { RegisterIceObserver(ice_observer); } virtual ~WebRtcSessionForTest() {} @@ -380,7 +383,11 @@ class WebRtcSessionTest ASSERT_TRUE(session_.get() == NULL); session_.reset(new WebRtcSessionForTest( media_controller_.get(), rtc::Thread::Current(), rtc::Thread::Current(), - rtc::Thread::Current(), allocator_.get(), &observer_)); + rtc::Thread::Current(), allocator_.get(), &observer_, + std::unique_ptr( + new cricket::TransportController(rtc::Thread::Current(), + rtc::Thread::Current(), + allocator_.get())))); session_->SignalDataChannelOpenMessage.connect( this, &WebRtcSessionTest::OnDataChannelOpenMessage); session_->GetOnDestroyedSignal()->connect(