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

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

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;

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<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 @@ TransportWrapper;
 // 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 @@ 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.

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

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',