From e2506670a4e57cbd351141d8ccf7635ffd2db093 Mon Sep 17 00:00:00 2001 From: "decurtis@webrtc.org" Date: Tue, 3 Feb 2015 23:18:39 +0000 Subject: [PATCH] Add RefCounting for TransportProxies BUG=1574 R=pthatcher@webrtc.org Review URL: https://webrtc-codereview.appspot.com/37869004 Cr-Commit-Position: refs/heads/master@{#8238} git-svn-id: http://webrtc.googlecode.com/svn/trunk@8238 4adac7df-926f-26a2-2b94-8c16560cd09d --- talk/app/webrtc/webrtcsession.cc | 6 +- webrtc/p2p/base/session.cc | 102 +++++++++++++++++++--------- webrtc/p2p/base/session.h | 20 ++++-- webrtc/p2p/base/session_unittest.cc | 54 +++++++++++++++ webrtc/p2p/p2p_tests.gypi | 1 + 5 files changed, 143 insertions(+), 40 deletions(-) create mode 100644 webrtc/p2p/base/session_unittest.cc diff --git a/talk/app/webrtc/webrtcsession.cc b/talk/app/webrtc/webrtcsession.cc index 7144e76bfb..83e613ee5e 100644 --- a/talk/app/webrtc/webrtcsession.cc +++ b/talk/app/webrtc/webrtcsession.cc @@ -1482,7 +1482,7 @@ void WebRtcSession::RemoveUnusedChannelsAndTransports( SignalVideoChannelDestroyed(); const std::string content_name = video_channel_->content_name(); channel_manager_->DestroyVideoChannel(video_channel_.release()); - DestroyTransportProxy(content_name); + DestroyTransportProxyWhenUnused(content_name); } const cricket::ContentInfo* voice_info = @@ -1492,7 +1492,7 @@ void WebRtcSession::RemoveUnusedChannelsAndTransports( SignalVoiceChannelDestroyed(); const std::string content_name = voice_channel_->content_name(); channel_manager_->DestroyVoiceChannel(voice_channel_.release()); - DestroyTransportProxy(content_name); + DestroyTransportProxyWhenUnused(content_name); } const cricket::ContentInfo* data_info = @@ -1502,7 +1502,7 @@ void WebRtcSession::RemoveUnusedChannelsAndTransports( SignalDataChannelDestroyed(); const std::string content_name = data_channel_->content_name(); channel_manager_->DestroyDataChannel(data_channel_.release()); - DestroyTransportProxy(content_name); + DestroyTransportProxyWhenUnused(content_name); } } diff --git a/webrtc/p2p/base/session.cc b/webrtc/p2p/base/session.cc index 93a4a2b57d..69b6a9cab5 100644 --- a/webrtc/p2p/base/session.cc +++ b/webrtc/p2p/base/session.cc @@ -30,11 +30,12 @@ namespace cricket { using rtc::Bind; TransportProxy::~TransportProxy() { - for (ChannelMap::iterator iter = channels_.begin(); - iter != channels_.end(); ++iter) { - iter->second->SignalDestroyed(iter->second); - delete iter->second; - } + ASSERT(channels_.empty()); +} + +bool TransportProxy::HasChannels() const { + ASSERT(rtc::Thread::Current() == worker_thread_); + return !channels_.empty(); } const std::string& TransportProxy::type() const { @@ -49,22 +50,35 @@ TransportChannel* TransportProxy::GetChannel(int component) { TransportChannel* TransportProxy::CreateChannel(const std::string& name, int component) { ASSERT(rtc::Thread::Current() == worker_thread_); - ASSERT(GetChannel(component) == NULL); - ASSERT(!transport_->get()->HasChannel(component)); - // We always create a proxy in case we need to change out the transport later. - TransportChannelProxy* channel_proxy = - new TransportChannelProxy(content_name(), name, component); - channels_[component] = channel_proxy; + TransportChannelProxyRef* channel_proxy; + if (channels_.find(component) == channels_.end()) { + channel_proxy = + new TransportChannelProxyRef(content_name(), name, component); + channels_[component] = channel_proxy; - // If we're already negotiated, create an impl and hook it up to the proxy - // channel. If we're connecting, create an impl but don't hook it up yet. - if (negotiated_) { - CreateChannelImpl_w(component); - SetChannelImplFromTransport_w(channel_proxy, component); - } else if (connecting_) { - CreateChannelImpl_w(component); + // TransportProxy maintains its own reference + // here. RefCountedObject automatically deletes the pointer when + // the refcount hits 0. This prevents RefCountedObject from + // deleting the object when all *external* references are + // gone. Things need to be done in DestroyChannel prior to the + // proxy being deleted. + channel_proxy->AddRef(); + + // If we're already negotiated, create an impl and hook it up to the proxy + // channel. If we're connecting, create an impl but don't hook it up yet. + if (negotiated_) { + CreateChannelImpl_w(component); + SetChannelImplFromTransport_w(channel_proxy, component); + } else if (connecting_) { + CreateChannelImpl_w(component); + } + } else { + channel_proxy = channels_[component]; } + + channel_proxy->AddRef(); + return channel_proxy; } @@ -74,22 +88,37 @@ bool TransportProxy::HasChannel(int component) { void TransportProxy::DestroyChannel(int component) { ASSERT(rtc::Thread::Current() == worker_thread_); - TransportChannelProxy* channel_proxy = GetChannelProxy(component); - if (channel_proxy) { - // If the state of TransportProxy is not NEGOTIATED then - // TransportChannelProxy and its impl are not connected. Both must - // be connected before deletion. - // - // However, if we haven't entered the connecting state then there - // is no implementation to hook up. - if (connecting_ && !negotiated_) { - SetChannelImplFromTransport_w(channel_proxy, component); - } - channels_.erase(component); - channel_proxy->SignalDestroyed(channel_proxy); - delete channel_proxy; + ChannelMap::const_iterator iter = channels_.find(component); + if (iter == channels_.end()) { + return; } + + TransportChannelProxyRef* channel_proxy = iter->second; + int ref_count = channel_proxy->Release(); + if (ref_count > 1) { + return; + } + // TransportProxy owns the last reference on the TransportChannelProxy. + // It should *never* be the case that ref_count is less than one + // here but this makes me sleep better at night. + ASSERT(ref_count == 1); + + // If the state of TransportProxy is not NEGOTIATED then + // TransportChannelProxy and its impl are not connected. Both must + // be connected before deletion. + // + // However, if we haven't entered the connecting state then there + // is no implementation to hook up. + if (connecting_ && !negotiated_) { + SetChannelImplFromTransport_w(channel_proxy, component); + } + + channels_.erase(component); + channel_proxy->SignalDestroyed(channel_proxy); + + // Implicitly deletes the object since ref_count is now 0. + channel_proxy->Release(); } void TransportProxy::ConnectChannels() { @@ -590,8 +619,15 @@ TransportProxy* BaseSession::GetFirstTransportProxy() { return transports_.begin()->second; } -void BaseSession::DestroyTransportProxy( +void BaseSession::DestroyTransportProxyWhenUnused( const std::string& content_name) { + TransportProxy *tp = GetTransportProxy(content_name); + if(tp && !tp->HasChannels()) { + DestroyTransportProxy(content_name); + } +} + +void BaseSession::DestroyTransportProxy(const std::string& content_name) { TransportMap::iterator iter = transports_.find(content_name); if (iter != transports_.end()) { delete iter->second; diff --git a/webrtc/p2p/base/session.h b/webrtc/p2p/base/session.h index 7f2d8168c3..7e89123ef4 100644 --- a/webrtc/p2p/base/session.h +++ b/webrtc/p2p/base/session.h @@ -19,6 +19,7 @@ #include "webrtc/p2p/base/candidate.h" #include "webrtc/p2p/base/port.h" #include "webrtc/p2p/base/transport.h" +#include "webrtc/p2p/base/transportchannelproxy.h" #include "webrtc/base/refcount.h" #include "webrtc/base/scoped_ptr.h" #include "webrtc/base/scoped_ref_ptr.h" @@ -30,11 +31,12 @@ class BaseSession; class P2PTransportChannel; class Transport; class TransportChannel; -class TransportChannelProxy; class TransportChannelImpl; -typedef rtc::RefCountedObject > -TransportWrapper; +// Transport and TransportChannelProxy are incomplete types. Thus we +// wrap them in a scoped_ptr. +typedef rtc::RefCountedObject > TransportWrapper; +typedef rtc::RefCountedObject TransportChannelProxyRef; // Bundles a Transport and ChannelMap together. ChannelMap is used to // create transport channels before receiving or sending a session @@ -42,7 +44,7 @@ TransportWrapper; // session had one ChannelMap and transport. Now, with multiple // transports per session, we need multiple ChannelMaps as well. -typedef std::map ChannelMap; +typedef std::map ChannelMap; class TransportProxy : public sigslot::has_slots<>, public CandidateTranslator { @@ -82,9 +84,14 @@ class TransportProxy : public sigslot::has_slots<>, } TransportChannel* GetChannel(int component); + + // Returns the channel for component. This channel may be used by + // others and should only be deleted by calling DestroyChannel. TransportChannel* CreateChannel(const std::string& channel_name, int component); bool HasChannel(int component); + + // Destroy the channel for component. void DestroyChannel(int component); void AddSentCandidates(const Candidates& candidates); @@ -118,6 +125,9 @@ class TransportProxy : public sigslot::has_slots<>, virtual bool GetComponentFromChannelName( const std::string& channel_name, int* component) const; + // Determine if TransportProxy has any active channels. + bool HasChannels() const; + // Called when a transport signals that it has new candidates. void OnTransportCandidatesReady(cricket::Transport* transport, const Candidates& candidates) { @@ -346,6 +356,8 @@ class BaseSession : public sigslot::has_slots<>, TransportProxy* GetTransportProxy(const Transport* transport); TransportProxy* GetFirstTransportProxy(); void DestroyTransportProxy(const std::string& content_name); + // Calls DestroyTransportProxy if the TransportProxy is not used. + void DestroyTransportProxyWhenUnused(const std::string& content_name); // TransportProxy is owned by session. Return proxy just for convenience. TransportProxy* GetOrCreateTransportProxy(const std::string& content_name); // Creates the actual transport object. Overridable for testing. diff --git a/webrtc/p2p/base/session_unittest.cc b/webrtc/p2p/base/session_unittest.cc new file mode 100644 index 0000000000..904cc886c7 --- /dev/null +++ b/webrtc/p2p/base/session_unittest.cc @@ -0,0 +1,54 @@ +/* + * Copyright 2004 The WebRTC Project Authors. All rights reserved. + * + * Use of this source code is governed by a BSD-style license + * that can be found in the LICENSE file in the root of the source + * tree. An additional intellectual property rights grant can be found + * in the file PATENTS. All contributing project authors may + * be found in the AUTHORS file in the root of the source tree. + */ + +#include "webrtc/p2p/base/session.h" + +#include "testing/gtest/include/gtest/gtest.h" + +namespace cricket { + +class BaseSessionTest : public testing::Test, public BaseSession { + protected: + BaseSessionTest() : + BaseSession(rtc::Thread::Current(), rtc::Thread::Current(), + NULL, "sid", "video?", true) { + } + + ~BaseSessionTest() {} +}; + +// Tests that channels are not being deleted until all refcounts are +// used and that the TransportProxy is not removed unless all channels +// are removed from the proxy. +TEST_F(BaseSessionTest, TransportChannelProxyRefCounter) { + std::string content_name = "no matter"; + int component = 10; + TransportChannel* channel = CreateChannel(content_name, "", component); + TransportChannel* channel_again = CreateChannel(content_name, "", component); + + EXPECT_EQ(channel, channel_again); + EXPECT_EQ(channel, GetChannel(content_name, component)); + + DestroyChannel(content_name, component); + EXPECT_EQ(channel, GetChannel(content_name, component)); + + // Try to destroy a non-existant content name. + DestroyTransportProxyWhenUnused("other content"); + EXPECT_TRUE(GetTransportProxy(content_name) != NULL); + + DestroyChannel(content_name, component); + EXPECT_EQ(NULL, GetChannel(content_name, component)); + EXPECT_TRUE(GetTransportProxy(content_name) != NULL); + + DestroyTransportProxyWhenUnused(content_name); + EXPECT_TRUE(GetTransportProxy(content_name) == NULL); +} + +} // namespace cricket diff --git a/webrtc/p2p/p2p_tests.gypi b/webrtc/p2p/p2p_tests.gypi index afab33bfaf..f9e6959433 100644 --- a/webrtc/p2p/p2p_tests.gypi +++ b/webrtc/p2p/p2p_tests.gypi @@ -22,6 +22,7 @@ 'base/pseudotcp_unittest.cc', 'base/relayport_unittest.cc', 'base/relayserver_unittest.cc', + 'base/session_unittest.cc', 'base/stun_unittest.cc', 'base/stunport_unittest.cc', 'base/stunrequest_unittest.cc',