[Sync] Fixed memory leak in CacheInvalidationPacketHandler.

Added unit tests.

Added NonThreadSafe member.

BUG=None
TEST=New unit tests

Review URL: http://codereview.chromium.org/4204001

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

5 files changed, 111 insertions, 5 deletions

diff --git a/chrome/browser/sync/notifier/DEPS b/chrome/browser/sync/notifier/DEPS
index dd31bd4..346b3b5 100644
--- a/chrome/browser/sync/notifier/DEPS
+++ b/chrome/browser/sync/notifier/DEPS
@@ -3,6 +3,8 @@ include_rules = [
   "+google/cacheinvalidation",
   # sync_notifier depends on the common jingle notifier classes.
   "+jingle/notifier",
+  # unit tests depend on talk/base.
+  "+talk/base",
   # sync_notifier depends on the xmpp part of libjingle.
   "+talk/xmpp",
 ]
diff --git a/chrome/browser/sync/notifier/cache_invalidation_packet_handler.cc b/chrome/browser/sync/notifier/cache_invalidation_packet_handler.cc
index dc1e7a3..7250c62 100644
--- a/chrome/browser/sync/notifier/cache_invalidation_packet_handler.cc
+++ b/chrome/browser/sync/notifier/cache_invalidation_packet_handler.cc
@@ -198,6 +198,9 @@ CacheInvalidationPacketHandler::CacheInvalidationPacketHandler(
     base::WeakPtr<talk_base::Task> base_task,
     invalidation::InvalidationClient* invalidation_client)
     : scoped_callback_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)),
+      handle_outbound_packet_callback_(
+          scoped_callback_factory_.NewCallback(
+              &CacheInvalidationPacketHandler::HandleOutboundPacket)),
       base_task_(base_task),
       invalidation_client_(invalidation_client),
       seq_(0),
@@ -208,9 +211,8 @@ CacheInvalidationPacketHandler::CacheInvalidationPacketHandler(
       invalidation_client_->network_endpoint();
   CHECK(network_endpoint);
   network_endpoint->RegisterOutboundListener(
-      scoped_callback_factory_.NewCallback(
-          &CacheInvalidationPacketHandler::HandleOutboundPacket));
-  // Owned by base_task.
+      handle_outbound_packet_callback_.get());
+  // Owned by base_task.  Takes ownership of the callback.
   CacheInvalidationListenTask* listen_task =
       new CacheInvalidationListenTask(
           base_task_, scoped_callback_factory_.NewCallback(
@@ -219,6 +221,7 @@ CacheInvalidationPacketHandler::CacheInvalidationPacketHandler(
 }
 
 CacheInvalidationPacketHandler::~CacheInvalidationPacketHandler() {
+  DCHECK(non_thread_safe_.CalledOnValidThread());
   invalidation::NetworkEndpoint* network_endpoint =
       invalidation_client_->network_endpoint();
   CHECK(network_endpoint);
@@ -227,6 +230,7 @@ CacheInvalidationPacketHandler::~CacheInvalidationPacketHandler() {
 
 void CacheInvalidationPacketHandler::HandleOutboundPacket(
     invalidation::NetworkEndpoint* const& network_endpoint) {
+  DCHECK(non_thread_safe_.CalledOnValidThread());
   if (!base_task_.get()) {
     return;
   }
@@ -251,6 +255,7 @@ void CacheInvalidationPacketHandler::HandleOutboundPacket(
 
 void CacheInvalidationPacketHandler::HandleInboundPacket(
     const std::string& packet) {
+  DCHECK(non_thread_safe_.CalledOnValidThread());
   invalidation::NetworkEndpoint* network_endpoint =
       invalidation_client_->network_endpoint();
   std::string decoded_message;
diff --git a/chrome/browser/sync/notifier/cache_invalidation_packet_handler.h b/chrome/browser/sync/notifier/cache_invalidation_packet_handler.h
index 3c3bfec..f4d89e3 100644
--- a/chrome/browser/sync/notifier/cache_invalidation_packet_handler.h
+++ b/chrome/browser/sync/notifier/cache_invalidation_packet_handler.h
@@ -12,7 +12,10 @@
 #include <string>
 
 #include "base/basictypes.h"
+#include "base/callback.h"
+#include "base/non_thread_safe.h"
 #include "base/scoped_callback_factory.h"
+#include "base/scoped_ptr.h"
 #include "base/weak_ptr.h"
 #include "talk/xmpp/jid.h"
 
@@ -27,8 +30,6 @@ class Task;
 
 namespace sync_notifier {
 
-// TODO(akalin): Add a NonThreadSafe member to this class and use it.
-
 class CacheInvalidationPacketHandler {
  public:
   // Starts routing packets from |invalidation_client| using
@@ -50,8 +51,11 @@ class CacheInvalidationPacketHandler {
 
   void HandleInboundPacket(const std::string& packet);
 
+  NonThreadSafe non_thread_safe_;
   base::ScopedCallbackFactory<CacheInvalidationPacketHandler>
       scoped_callback_factory_;
+  scoped_ptr<CallbackRunner<Tuple1<invalidation::NetworkEndpoint* const&> > >
+      handle_outbound_packet_callback_;
 
   base::WeakPtr<talk_base::Task> base_task_;
   invalidation::InvalidationClient* invalidation_client_;
diff --git a/chrome/browser/sync/notifier/cache_invalidation_packet_handler_unittest.cc b/chrome/browser/sync/notifier/cache_invalidation_packet_handler_unittest.cc
new file mode 100644
index 0000000..c636c97
--- /dev/null
+++ b/chrome/browser/sync/notifier/cache_invalidation_packet_handler_unittest.cc
@@ -0,0 +1,94 @@
+// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/sync/notifier/cache_invalidation_packet_handler.h"
+
+#include "base/message_loop.h"
+#include "base/weak_ptr.h"
+#include "google/cacheinvalidation/invalidation-client.h"
+#include "jingle/notifier/base/task_pump.h"
+#include "jingle/notifier/base/weak_xmpp_client.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "talk/base/task.h"
+#include "talk/xmpp/asyncsocket.h"
+
+namespace sync_notifier {
+
+namespace {
+
+using ::testing::_;
+using ::testing::NotNull;
+using ::testing::Return;
+
+class MockNetworkEndpoint : public invalidation::NetworkEndpoint {
+ public:
+  MOCK_METHOD1(RegisterOutboundListener,
+               void(invalidation::NetworkCallback*));
+  MOCK_METHOD1(HandleInboundMessage, void(const invalidation::string&));
+  MOCK_METHOD1(TakeOutboundMessage, void(invalidation::string*));
+  MOCK_METHOD1(AdviseNetworkStatus, void(bool));
+};
+
+class MockInvalidationClient : public invalidation::InvalidationClient {
+ public:
+  MOCK_METHOD1(Register, void(const invalidation::ObjectId&));
+  MOCK_METHOD1(Unregister, void(const invalidation::ObjectId&));
+  MOCK_METHOD0(network_endpoint, invalidation::NetworkEndpoint*());
+};
+
+class MockAsyncSocket : public buzz::AsyncSocket {
+ public:
+  virtual ~MockAsyncSocket() {}
+
+  MOCK_METHOD0(state, State());
+  MOCK_METHOD0(error, Error());
+  MOCK_METHOD0(GetError, int());
+  MOCK_METHOD1(Connect, bool(const talk_base::SocketAddress&));
+  MOCK_METHOD3(Read, bool(char*, size_t, size_t*));
+  MOCK_METHOD2(Write, bool(const char*, size_t));
+  MOCK_METHOD0(Close, bool());
+  MOCK_METHOD1(StartTls, bool(const std::string&));
+};
+
+class CacheInvalidationPacketHandlerTest : public testing::Test {
+ public:
+  virtual ~CacheInvalidationPacketHandlerTest() {}
+};
+
+TEST_F(CacheInvalidationPacketHandlerTest, Basic) {
+  MessageLoop message_loop;
+
+  notifier::TaskPump task_pump;
+  // Owned by |task_pump|.
+  notifier::WeakXmppClient* weak_xmpp_client =
+      new notifier::WeakXmppClient(&task_pump);
+  base::WeakPtr<talk_base::Task> base_task(weak_xmpp_client->AsWeakPtr());
+  MockNetworkEndpoint mock_network_endpoint;
+  MockInvalidationClient mock_invalidation_client;
+
+  EXPECT_CALL(mock_invalidation_client, network_endpoint()).
+      WillRepeatedly(Return(&mock_network_endpoint));
+
+  EXPECT_CALL(mock_network_endpoint,
+              RegisterOutboundListener(NotNull())).Times(1);
+  EXPECT_CALL(mock_network_endpoint,
+              RegisterOutboundListener(NULL)).Times(1);
+
+  weak_xmpp_client->Start();
+  buzz::XmppClientSettings settings;
+  // Owned by |weak_xmpp_client|.
+  MockAsyncSocket* mock_async_socket = new MockAsyncSocket();
+  EXPECT_CALL(*mock_async_socket, Connect(_)).WillOnce(Return(true));
+  weak_xmpp_client->Connect(settings, "en", mock_async_socket, NULL);
+  // Initialize the XMPP client.
+  message_loop.RunAllPending();
+
+  CacheInvalidationPacketHandler handler(
+      base_task, &mock_invalidation_client);
+}
+
+}  // namespace
+
+}  // namespace sync_notifier
diff --git a/chrome/chrome_tests.gypi b/chrome/chrome_tests.gypi
index 7bf15f7..7c6c46e 100644
--- a/chrome/chrome_tests.gypi
+++ b/chrome/chrome_tests.gypi
@@ -2500,6 +2500,7 @@
         'browser/sync/engine/syncproto_unittest.cc',
         'browser/sync/engine/verify_updates_command_unittest.cc',
         'browser/sync/glue/change_processor_mock.h',
+        'browser/sync/notifier/cache_invalidation_packet_handler_unittest.cc',
         'browser/sync/notifier/chrome_system_resources_unittest.cc',
         'browser/sync/notifier/registration_manager_unittest.cc',
         'browser/sync/profile_sync_factory_mock.cc',