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(