Revert 8238 "Add RefCounting for TransportProxies"

Failing on Mac64_Debug

> Add RefCounting for TransportProxies
> 
> BUG=1574
> R=pthatcher@webrtc.org
> 
> Review URL: https://webrtc-codereview.appspot.com/37869004

TBR=decurtis@webrtc.org

Review URL: https://webrtc-codereview.appspot.com/37159004

Cr-Commit-Position: refs/heads/master@{#8243}
git-svn-id: http://webrtc.googlecode.com/svn/trunk@8243 4adac7df-926f-26a2-2b94-8c16560cd09d

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

webrtc/p2p/base/session.cc

@@ -30,12 +30,11 @@ namespace cricket {
 using rtc::Bind;
 
 TransportProxy::~TransportProxy() {
-  ASSERT(channels_.empty());
+  for (ChannelMap::iterator iter = channels_.begin();
-}
+       iter != channels_.end(); ++iter) {
-
+    iter->second->SignalDestroyed(iter->second);
-bool TransportProxy::HasChannels() const {
+    delete iter->second;
-  ASSERT(rtc::Thread::Current() == worker_thread_);
+  }
-  return !channels_.empty();
 }
 
 const std::string& TransportProxy::type() const {
@@ -50,21 +49,14 @@ 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;
+  // We always create a proxy in case we need to change out the transport later.
-  if (channels_.find(component) == channels_.end()) {
+  TransportChannelProxy* channel_proxy =
-    channel_proxy =
+      new TransportChannelProxy(content_name(), name, component);
-        new TransportChannelProxyRef(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_) {
@@ -73,12 +65,6 @@ TransportChannel* TransportProxy::CreateChannel(const std::string& name,
   } else if (connecting_) {
     CreateChannelImpl_w(component);
   }
-  } else {
-    channel_proxy = channels_[component];
-  }
-
-  channel_proxy->AddRef();
-
   return channel_proxy;
 }
 
@@ -88,22 +74,8 @@ bool TransportProxy::HasChannel(int component) {
 
 void TransportProxy::DestroyChannel(int component) {
   ASSERT(rtc::Thread::Current() == worker_thread_);
-
+  TransportChannelProxy* channel_proxy = GetChannelProxy(component);
-  ChannelMap::const_iterator iter = channels_.find(component);
+  if (channel_proxy) {
-  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.
@@ -116,9 +88,8 @@ void TransportProxy::DestroyChannel(int component) {
 
     channels_.erase(component);
     channel_proxy->SignalDestroyed(channel_proxy);
-
+    delete 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;

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
+typedef rtc::RefCountedObject<rtc::scoped_ptr<Transport> >
-// wrap them in a scoped_ptr.
+TransportWrapper;
-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
@@ -44,7 +42,7 @@ typedef rtc::RefCountedObject<TransportChannelProxy> TransportChannelProxyRef;
 // session had one ChannelMap and transport.  Now, with multiple
 // transports per session, we need multiple ChannelMaps as well.
 
-typedef std::map<int, TransportChannelProxyRef*> ChannelMap;
+typedef std::map<int, TransportChannelProxy*> 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.

webrtc/p2p/base/session_unittest.cc

@@ -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

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