From bf9f469a131577dbe3c1b444e27ff326d1a094f2 Mon Sep 17 00:00:00 2001
From: "hta@webrtc.org" <hta@webrtc.org@4adac7df-926f-26a2-2b94-8c16560cd09d>
Date: Mon, 23 Apr 2012 13:19:30 +0000
Subject: [PATCH] Lifetime management for UdpSocketManager

Make tests use Create/Destroy *or* new/delete for UdpSocketManager.
Move responsibility for calling Destroy on UdpSocketManager from transport
destructor to transport Destroy function.

This all ends up in not leaking memory in InitializeSourcePorts test.

BUG=
TEST=

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

git-svn-id: http://webrtc.googlecode.com/svn/trunk@2091 4adac7df-926f-26a2-2b94-8c16560cd09d
---
 .../udp_transport/source/udp_transport_impl.cc       |  9 +++++++--
 .../udp_transport/source/udp_transport_impl.h        |  4 ++--
 .../udp_transport/source/udp_transport_unittest.cc   | 12 +++++++++---
 3 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/src/modules/udp_transport/source/udp_transport_impl.cc b/src/modules/udp_transport/source/udp_transport_impl.cc
index e54edd0ef7..6ca61f5950 100644
--- a/src/modules/udp_transport/source/udp_transport_impl.cc
+++ b/src/modules/udp_transport/source/udp_transport_impl.cc
@@ -80,7 +80,9 @@ class SocketFactory : public UdpTransportImpl::SocketFactoryInterface {
   }
 };
 
-
+// Creates an UdpTransport using the definition of SocketFactory above,
+// and passes (creating if needed) a pointer to the static singleton
+// UdpSocketManager.
 UdpTransport* UdpTransport::Create(const WebRtc_Word32 id,
                                    WebRtc_UWord8& numSocketThreads)
 {
@@ -89,11 +91,15 @@ UdpTransport* UdpTransport::Create(const WebRtc_Word32 id,
                               UdpSocketManager::Create(id, numSocketThreads));
 }
 
+// Deletes the UdpTransport and decrements the refcount of the
+// static singleton UdpSocketManager, possibly destroying it.
+// Should only be used on UdpTransports that are created using Create.
 void UdpTransport::Destroy(UdpTransport* module)
 {
     if(module)
     {
         delete module;
+        UdpSocketManager::Return();
     }
 }
 
@@ -170,7 +176,6 @@ UdpTransportImpl::~UdpTransportImpl()
     delete _critPacketCallback;
     delete _cachLock;
     delete _socket_creator;
-    UdpSocketManager::Return();
 
     WEBRTC_TRACE(kTraceMemory, kTraceTransport, _id, "%s deleted",
                  __FUNCTION__);
diff --git a/src/modules/udp_transport/source/udp_transport_impl.h b/src/modules/udp_transport/source/udp_transport_impl.h
index 2c9530d101..9f4fd9fb06 100644
--- a/src/modules/udp_transport/source/udp_transport_impl.h
+++ b/src/modules/udp_transport/source/udp_transport_impl.h
@@ -35,8 +35,8 @@ public:
     };
 
     // Constructor, only called by UdpTransport::Create and tests.
-    // The constructor takes ownership of the "maker",
-    // and will call Return on the socket_manager at exit.
+    // The constructor takes ownership of the "maker".
+    // The constructor does not take ownership of socket_manager.
     UdpTransportImpl(const WebRtc_Word32 id,
                      SocketFactoryInterface* maker,
                      UdpSocketManager* socket_manager);
diff --git a/src/modules/udp_transport/source/udp_transport_unittest.cc b/src/modules/udp_transport/source/udp_transport_unittest.cc
index 25a9b9d5f1..3125e2e5fb 100644
--- a/src/modules/udp_transport/source/udp_transport_unittest.cc
+++ b/src/modules/udp_transport/source/udp_transport_unittest.cc
@@ -45,6 +45,10 @@ class MockUdpSocketWrapper : public webrtc::UdpSocketWrapper {
 
 class MockUdpSocketManager : public webrtc::UdpSocketManager {
  public:
+  // Access to protected destructor.
+  void Destroy() {
+    delete this;
+  }
   MOCK_METHOD2(Init, bool(WebRtc_Word32, WebRtc_UWord8&));
   MOCK_METHOD1(ChangeUniqueId, WebRtc_Word32(const WebRtc_Word32));
   MOCK_METHOD0(Start, bool());
@@ -118,18 +122,20 @@ TEST_F(UDPTransportTest, ConstructorDoesNotCreateSocket) {
   webrtc::UdpTransport* transport = new webrtc::UdpTransportImpl(id,
                                                                  null_maker,
                                                                  null_manager);
-  webrtc::UdpTransport::Destroy(transport);
+  delete transport;
 }
 
 TEST_F(UDPTransportTest, InitializeSourcePorts) {
   WebRtc_Word32 id = 0;
   webrtc::UdpTransportImpl::SocketFactoryInterface* mock_maker
       = new MockSocketFactory(sockets_created());
-  webrtc::UdpSocketManager* mock_manager = new MockUdpSocketManager();
+  MockUdpSocketManager* mock_manager = new MockUdpSocketManager();
   webrtc::UdpTransport* transport = new webrtc::UdpTransportImpl(id,
                                                                  mock_maker,
                                                                  mock_manager);
   EXPECT_EQ(0, transport->InitializeSourcePorts(4711, 4712));
   EXPECT_EQ(2, NumSocketsCreated());
-  webrtc::UdpTransport::Destroy(transport);
+
+  delete transport;
+  mock_manager->Destroy();
 }