admin/webrtc_m130
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Add PeerConnection IsClosed check.

Browse Source 
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}
This commit is contained in:
zhihuang 2016-07-27 11:07:25 -07:00 committed by Commit bot
parent 93dd634561
commit 29ff8446c0
8 changed files with 168 additions and 53 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

47
webrtc/api/peerconnection.cc
View File

@ -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<cricket::TransportController>(
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<DtmfSenderInterface> 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<RtpSenderInterface> PeerConnection::CreateSender(
const std::string& kind,
const std::string& stream_id) {
TRACE_EVENT0("webrtc", "PeerConnection::CreateSender");
if (IsClosed()) {
return nullptr;
}
rtc::scoped_refptr<RtpSenderProxyWithInternal<RtpSenderInternal>> new_sender;
if (kind == MediaStreamTrackInterface::kAudioKind) {
new_sender = RtpSenderProxyWithInternal<RtpSenderInternal>::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<cricket::Candidate>& 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()

7
webrtc/api/peerconnectionfactory.cc
View File

@ -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().

2
webrtc/api/peerconnectionfactory.h
View File

@ -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();

99
webrtc/api/peerconnectioninterface_unittest.cc
View File

@ -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<cricket::Candidate>& 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<StreamCollection> remote_streams_;
bool renegotiation_needed_ = false;
bool ice_complete_ = false;
bool callback_triggered = false;
private:
scoped_refptr<MediaStreamInterface> 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<PeerConnectionFactoryForTest>();
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<webrtc::PeerConnectionFactoryInterface> pc_factory_;
scoped_refptr<PeerConnectionFactoryForTest> pc_factory_for_test_;
scoped_refptr<PeerConnectionInterface> pc_;
MockPeerConnectionObserver observer_;
rtc::scoped_refptr<StreamCollection> reference_collection_;
};
// Test that no callbacks on the PeerConnectionObserver are called after the
// PeerConnection is closed.
TEST_F(PeerConnectionInterfaceTest, CloseAndTestCallbackFunctions) {
scoped_refptr<PeerConnectionInterface> 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<cricket::Candidate> 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 {

15
webrtc/api/statscollector_unittest.cc
View File

@ -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<cricket::TransportController>(
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

16
webrtc/api/webrtcsession.cc
View File

@ -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<cricket::TransportController> 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),

12
webrtc/api/webrtcsession.h
View File

@ -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<cricket::TransportController> transport_controller);
virtual ~WebRtcSession();
// These are const to allow them to be called from const methods.

23
webrtc/api/webrtcsession_unittest.cc
View File

@ -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<cricket::TransportController> 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<cricket::TransportController>(
new cricket::TransportController(rtc::Thread::Current(),
rtc::Thread::Current(),
allocator_.get()))));
session_->SignalDataChannelOpenMessage.connect(
this, &WebRtcSessionTest::OnDataChannelOpenMessage);
session_->GetOnDestroyedSignal()->connect(