diff --git a/talk/app/webrtc/webrtcsession.cc b/talk/app/webrtc/webrtcsession.cc index 0145243fe..27b936bf0 100644 --- a/talk/app/webrtc/webrtcsession.cc +++ b/talk/app/webrtc/webrtcsession.cc @@ -1478,7 +1478,7 @@ void WebRtcSession::RemoveUnusedChannelsAndTransports( SignalVideoChannelDestroyed(); const std::string content_name = video_channel_->content_name(); channel_manager_->DestroyVideoChannel(video_channel_.release()); - DestroyTransportProxyWhenUnused(content_name); + DestroyTransportProxy(content_name); } const cricket::ContentInfo* voice_info = @@ -1488,7 +1488,7 @@ void WebRtcSession::RemoveUnusedChannelsAndTransports( SignalVoiceChannelDestroyed(); const std::string content_name = voice_channel_->content_name(); channel_manager_->DestroyVoiceChannel(voice_channel_.release()); - DestroyTransportProxyWhenUnused(content_name); + DestroyTransportProxy(content_name); } const cricket::ContentInfo* data_info = @@ -1498,7 +1498,7 @@ void WebRtcSession::RemoveUnusedChannelsAndTransports( SignalDataChannelDestroyed(); const std::string content_name = data_channel_->content_name(); channel_manager_->DestroyDataChannel(data_channel_.release()); - DestroyTransportProxyWhenUnused(content_name); + DestroyTransportProxy(content_name); } } diff --git a/webrtc/p2p/base/session.cc b/webrtc/p2p/base/session.cc index 1a126a698..ba10a1431 100644 --- a/webrtc/p2p/base/session.cc +++ b/webrtc/p2p/base/session.cc @@ -30,12 +30,11 @@ namespace cricket { using rtc::Bind; TransportProxy::~TransportProxy() { - ASSERT(channels_.empty()); -} - -bool TransportProxy::HasChannels() const { - ASSERT(rtc::Thread::Current() == worker_thread_); - return !channels_.empty(); + for (ChannelMap::iterator iter = channels_.begin(); + iter != channels_.end(); ++iter) { + iter->second->SignalDestroyed(iter->second); + delete iter->second; + } } const std::string& TransportProxy::type() const { @@ -50,35 +49,22 @@ 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)); - TransportChannelProxyRef* channel_proxy; - if (channels_.find(component) == channels_.end()) { - channel_proxy = - new TransportChannelProxyRef(content_name(), name, component); - channels_[component] = channel_proxy; + // 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; - // 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]; + // 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); } - - channel_proxy->AddRef(); - return channel_proxy; } @@ -88,37 +74,22 @@ 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); + } - ChannelMap::const_iterator iter = channels_.find(component); - if (iter == channels_.end()) { - return; + channels_.erase(component); + channel_proxy->SignalDestroyed(channel_proxy); + delete channel_proxy; } - - 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() { @@ -619,15 +590,8 @@ TransportProxy* BaseSession::GetFirstTransportProxy() { return transports_.begin()->second; } -void BaseSession::DestroyTransportProxyWhenUnused( +void BaseSession::DestroyTransportProxy( 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 f809b3095..d91fb57f9 100644 --- a/webrtc/p2p/base/session.h +++ b/webrtc/p2p/base/session.h @@ -19,7 +19,6 @@ #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" @@ -31,12 +30,11 @@ class BaseSession; class P2PTransportChannel; class Transport; class TransportChannel; +class TransportChannelProxy; class TransportChannelImpl; -// Transport and TransportChannelProxy are incomplete types. Thus we -// wrap them in a scoped_ptr. -typedef rtc::RefCountedObject > TransportWrapper; -typedef rtc::RefCountedObject TransportChannelProxyRef; +typedef rtc::RefCountedObject > +TransportWrapper; // Bundles a Transport and ChannelMap together. ChannelMap is used to // create transport channels before receiving or sending a session @@ -44,7 +42,7 @@ typedef rtc::RefCountedObject TransportChannelProxyRef; // 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 { @@ -84,14 +82,9 @@ 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); @@ -125,9 +118,6 @@ 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) { @@ -356,8 +346,6 @@ 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 deleted file mode 100644 index 904cc886c..000000000 --- a/webrtc/p2p/base/session_unittest.cc +++ /dev/null @@ -1,54 +0,0 @@ -/* - * 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 f9e695943..afab33bfa 100644 --- a/webrtc/p2p/p2p_tests.gypi +++ b/webrtc/p2p/p2p_tests.gypi @@ -22,7 +22,6 @@ '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',