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
diff --git a/talk/app/webrtc/webrtcsession.cc b/talk/app/webrtc/webrtcsession.cc
index 7144e76..83e613e 100644
--- a/talk/app/webrtc/webrtcsession.cc
+++ b/talk/app/webrtc/webrtcsession.cc
@@ -1482,7 +1482,7 @@
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 @@
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 @@
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 93a4a2b..69b6a9c 100644
--- a/webrtc/p2p/base/session.cc
+++ b/webrtc/p2p/base/session.cc
@@ -30,11 +30,12 @@
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::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 @@
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 @@
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 7f2d816..7e89123 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 P2PTransportChannel;
class Transport;
class TransportChannel;
-class TransportChannelProxy;
class TransportChannelImpl;
-typedef rtc::RefCountedObject<rtc::scoped_ptr<Transport> >
-TransportWrapper;
+// Transport and TransportChannelProxy are incomplete types. Thus we
+// wrap them in a scoped_ptr.
+typedef rtc::RefCountedObject<rtc::scoped_ptr<Transport> > TransportWrapper;
+typedef rtc::RefCountedObject<TransportChannelProxy> TransportChannelProxyRef;
// Bundles a Transport and ChannelMap together. ChannelMap is used to
// create transport channels before receiving or sending a session
@@ -42,7 +44,7 @@
// session had one ChannelMap and transport. Now, with multiple
// transports per session, we need multiple ChannelMaps as well.
-typedef std::map<int, TransportChannelProxy*> ChannelMap;
+typedef std::map<int, TransportChannelProxyRef*> ChannelMap;
class TransportProxy : public sigslot::has_slots<>,
public CandidateTranslator {
@@ -82,9 +84,14 @@
}
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 @@
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 @@
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 0000000..904cc88
--- /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 afab33b..f9e6959 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',
