Multiple memory leak fixes in unittest.

- Simplify Close() to always tear down the underlying socket
- Always close the TestCastSocket in TearDown()
- Keep track of whether the mock socket was actually passed to CastSocket and destroy it if necessary
- Fix virtual dtor

TESTED=$ASAN_OPTIONS="detect_leaks=1 strict_memcmp=0" out/{Debug,Release}/unit_tests --gtest_filter="CastSocket*"

BUG=304970

Review URL: https://codereview.chromium.org/26358004

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@227709 0039d316-1c4b-4281-b951-d872f2087c98

5 files changed, 34 insertions, 30 deletions

diff --git a/chrome/browser/extensions/api/cast_channel/OWNERS b/chrome/browser/extensions/api/cast_channel/OWNERS
new file mode 100644
index 0000000..a0088fc
--- /dev/null
+++ b/chrome/browser/extensions/api/cast_channel/OWNERS
@@ -0,0 +1,2 @@
+mfoltz@chromium.org
+munjal@chromium.org
diff --git a/chrome/browser/extensions/api/cast_channel/cast_socket.cc b/chrome/browser/extensions/api/cast_channel/cast_socket.cc
index 20ad6d9..46b9195 100644
--- a/chrome/browser/extensions/api/cast_channel/cast_socket.cc
+++ b/chrome/browser/extensions/api/cast_channel/cast_socket.cc
@@ -134,16 +134,6 @@ void CastSocket::OnConnectComplete(int result) {
 void CastSocket::Close(const net::CompletionCallback& callback) {
   DCHECK(CalledOnValidThread());
   DVLOG(1) << "Close ReadyState = " << ready_state_;
-  if (ready_state_ == READY_STATE_CLOSING ||
-      ready_state_ == READY_STATE_CLOSED) {
-    callback.Run(net::OK);
-    return;
-  }
-  if (ready_state_ != READY_STATE_OPEN &&
-      ready_state_ != READY_STATE_CONNECTING) {
-    callback.Run(net::ERR_FAILED);
-    return;
-  }
   socket_.reset(NULL);
   ready_state_ = READY_STATE_CLOSED;
   callback.Run(net::OK);
diff --git a/chrome/browser/extensions/api/cast_channel/cast_socket_unittest.cc b/chrome/browser/extensions/api/cast_channel/cast_socket_unittest.cc
index 96a4dd0..0dad98a 100644
--- a/chrome/browser/extensions/api/cast_channel/cast_socket_unittest.cc
+++ b/chrome/browser/extensions/api/cast_channel/cast_socket_unittest.cc
@@ -35,6 +35,7 @@ class MockTCPClientSocket : public net::TCPClientSocket {
  public:
   explicit MockTCPClientSocket(const net::AddressList& addresses) :
     TCPClientSocket(addresses, NULL, net::NetLog::Source()) { }
+  virtual ~MockTCPClientSocket() { }
 
   MOCK_METHOD1(Connect, int(const net::CompletionCallback& callback));
   MOCK_METHOD2(SetKeepAlive, bool(bool, int));
@@ -56,30 +57,47 @@ class CompleteHandler {
   DISALLOW_COPY_AND_ASSIGN(CompleteHandler);
 };
 
-
 class TestCastSocket : public CastSocket {
  public:
   explicit TestCastSocket(MockCastSocketDelegate* delegate) :
     CastSocket("abcdefg", GURL("cast://192.0.0.1:8009"), delegate,
-               &capturing_net_log_) {
+               &capturing_net_log_), owns_socket_(true) {
     net::AddressList addresses;
     mock_tcp_socket_ = new MockTCPClientSocket(addresses);
   }
-  virtual ~TestCastSocket() { }
 
-  // Unowned ptr to the underlying socket.  Do not reference after calling
-  // Close().
+  virtual ~TestCastSocket() {
+    if (owns_socket_) {
+      DCHECK(mock_tcp_socket_);
+      delete mock_tcp_socket_;
+    }
+  }
+
+  virtual void Close(const net::CompletionCallback& callback) OVERRIDE {
+    if (!owns_socket_)
+      mock_tcp_socket_ = NULL;
+    CastSocket::Close(callback);
+  }
+
+  // Ptr to the mock socket.  Ownership is transferred to CastSocket when it is
+  // returned from CreateSocket().  CastSocket will destroy it on Close(),
+  // so don't refer to |mock_tcp_socket_| afterwards.
   MockTCPClientSocket* mock_tcp_socket_;
 
  protected:
+  // Transfers ownership of |mock_tcp_socket_| to CastSocket.
   virtual net::TCPClientSocket* CreateSocket(
       const net::AddressList& addresses,
       net::NetLog* net_log,
       const net::NetLog::Source& source) OVERRIDE {
+    owns_socket_ = false;
     return mock_tcp_socket_;
   }
+
  private:
   net::CapturingNetLog capturing_net_log_;
+  // Whether this object or the parent owns |mock_tcp_socket_|.
+  bool owns_socket_;
 };
 
 class CastSocketTest : public testing::Test {
@@ -91,6 +109,12 @@ class CastSocketTest : public testing::Test {
     socket_.reset(new TestCastSocket(&mock_delegate_));
   }
 
+  virtual void TearDown() OVERRIDE {
+    EXPECT_CALL(handler_, OnCloseComplete(net::OK));
+    socket_->Close(base::Bind(&CompleteHandler::OnCloseComplete,
+                              base::Unretained(&handler_)));
+  }
+
   // Sets expectations when the socket is connected.  Connecting the socket also
   // starts the read loop; we expect the call to Read(), but never fire the read
   // callback.
@@ -121,7 +145,9 @@ class CastSocketTest : public testing::Test {
 
  protected:
   MockTCPClientSocket& mock_tcp_socket() {
-    return *(socket_->mock_tcp_socket_);
+    MockTCPClientSocket* mock_socket = socket_->mock_tcp_socket_;
+    DCHECK(mock_socket);
+    return *mock_socket;
   }
 
   MockCastSocketDelegate mock_delegate_;
diff --git a/tools/heapcheck/suppressions.txt b/tools/heapcheck/suppressions.txt
index 4bb02ec..fbdb16e 100644
--- a/tools/heapcheck/suppressions.txt
+++ b/tools/heapcheck/suppressions.txt
@@ -852,10 +852,3 @@
    fun:chromeos::KeyboardDrivenEventRewriterTest::GetRewrittenEventAsString
    fun:chromeos::KeyboardDrivenEventRewriterTest_PassThrough_Test::TestBody
 }
-{
-   bug_304970
-   Heapcheck:Leak
-   ...
-   fun:TestCastSocket
-   fun:extensions::api::cast_channel::CastSocketTest::SetUp
-}
diff --git a/tools/valgrind/memcheck/suppressions.txt b/tools/valgrind/memcheck/suppressions.txt
index 2356554..fb89992 100644
--- a/tools/valgrind/memcheck/suppressions.txt
+++ b/tools/valgrind/memcheck/suppressions.txt
@@ -7237,10 +7237,3 @@
    fun:_ZN8SkCanvas10readPixelsEP8SkBitmapiiNS_10Config8888E
    fun:_ZN7WebCore15GraphicsContext10readPixelsEP8SkBitmapiiN8SkCanvas10Config8888E
 }
-{
-   bug_304970
-   Memcheck:Leak
-   fun:_Znw*
-   fun:_ZN10extensions3api12cast_channel14TestCastSocketC1EPNS1_22MockCastSocketDelegateE
-   fun:_ZN10extensions3api12cast_channel14CastSocketTest5SetUpEv
-}