[Sync] Fixed crash on XMPP reconnection

Tied lifetime of invalidation packet callback to ChromeInvalidationClient.

Added some tests for ChromeInvalidationClient.

Created FakeBaseTask class for testing.

BUG=64652
TEST=New unittests

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

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

10 files changed, 241 insertions, 58 deletions

diff --git a/chrome/browser/sync/notifier/cache_invalidation_packet_handler.cc b/chrome/browser/sync/notifier/cache_invalidation_packet_handler.cc
index 7250c62..be46eca 100644
--- a/chrome/browser/sync/notifier/cache_invalidation_packet_handler.cc
+++ b/chrome/browser/sync/notifier/cache_invalidation_packet_handler.cc
@@ -198,20 +198,11 @@ 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),
       sid_(MakeSid()) {
   CHECK(base_task_.get());
-  CHECK(invalidation_client_);
-  invalidation::NetworkEndpoint* network_endpoint =
-      invalidation_client_->network_endpoint();
-  CHECK(network_endpoint);
-  network_endpoint->RegisterOutboundListener(
-      handle_outbound_packet_callback_.get());
   // Owned by base_task.  Takes ownership of the callback.
   CacheInvalidationListenTask* listen_task =
       new CacheInvalidationListenTask(
@@ -222,14 +213,10 @@ CacheInvalidationPacketHandler::CacheInvalidationPacketHandler(
 
 CacheInvalidationPacketHandler::~CacheInvalidationPacketHandler() {
   DCHECK(non_thread_safe_.CalledOnValidThread());
-  invalidation::NetworkEndpoint* network_endpoint =
-      invalidation_client_->network_endpoint();
-  CHECK(network_endpoint);
-  network_endpoint->RegisterOutboundListener(NULL);
 }
 
 void CacheInvalidationPacketHandler::HandleOutboundPacket(
-    invalidation::NetworkEndpoint* const& network_endpoint) {
+    invalidation::NetworkEndpoint* network_endpoint) {
   DCHECK(non_thread_safe_.CalledOnValidThread());
   if (!base_task_.get()) {
     return;
diff --git a/chrome/browser/sync/notifier/cache_invalidation_packet_handler.h b/chrome/browser/sync/notifier/cache_invalidation_packet_handler.h
index 16e86ff..ac2d0c4 100644
--- a/chrome/browser/sync/notifier/cache_invalidation_packet_handler.h
+++ b/chrome/browser/sync/notifier/cache_invalidation_packet_handler.h
@@ -12,13 +12,10 @@
 #include <string>
 
 #include "base/basictypes.h"
-#include "base/callback.h"
 #include "base/gtest_prod_util.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"
 
 namespace invalidation {
 class InvalidationClient;
@@ -46,19 +43,18 @@ class CacheInvalidationPacketHandler {
   // anymore.
   ~CacheInvalidationPacketHandler();
 
+  // If |base_task| is non-NULL, sends any existing pending outbound
+  // packets.
+  void HandleOutboundPacket(invalidation::NetworkEndpoint* network_endpoint);
+
  private:
   FRIEND_TEST(CacheInvalidationPacketHandlerTest, Basic);
 
-  void HandleOutboundPacket(
-      invalidation::NetworkEndpoint* const& network_endpoint);
-
   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
index 6bd1f02..1d30bbc 100644
--- a/chrome/browser/sync/notifier/cache_invalidation_packet_handler_unittest.cc
+++ b/chrome/browser/sync/notifier/cache_invalidation_packet_handler_unittest.cc
@@ -8,8 +8,7 @@
 #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 "jingle/notifier/base/fake_base_task.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "talk/base/task.h"
@@ -18,7 +17,6 @@
 namespace sync_notifier {
 
 using ::testing::_;
-using ::testing::NotNull;
 using ::testing::Return;
 
 class MockNetworkEndpoint : public invalidation::NetworkEndpoint {
@@ -38,20 +36,6 @@ class MockInvalidationClient : public invalidation::InvalidationClient {
   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() {}
@@ -60,38 +44,22 @@ class CacheInvalidationPacketHandlerTest : public testing::Test {
 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());
+  notifier::FakeBaseTask fake_base_task;
+
   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);
   const char kInboundMessage[] = "non-bogus";
   EXPECT_CALL(mock_network_endpoint,
               HandleInboundMessage(kInboundMessage)).Times(1);
   EXPECT_CALL(mock_network_endpoint, TakeOutboundMessage(_)).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);
+        fake_base_task.AsWeakPtr(), &mock_invalidation_client);
     // Take care of any tasks posted by the constructor.
     message_loop.RunAllPending();
 
diff --git a/chrome/browser/sync/notifier/chrome_invalidation_client.cc b/chrome/browser/sync/notifier/chrome_invalidation_client.cc
index 0192371..29fd987 100644
--- a/chrome/browser/sync/notifier/chrome_invalidation_client.cc
+++ b/chrome/browser/sync/notifier/chrome_invalidation_client.cc
@@ -21,6 +21,10 @@ ChromeInvalidationClient::Listener::~Listener() {}
 
 ChromeInvalidationClient::ChromeInvalidationClient()
     : chrome_system_resources_(ALLOW_THIS_IN_INITIALIZER_LIST(this)),
+      scoped_callback_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)),
+      handle_outbound_packet_callback_(
+          scoped_callback_factory_.NewCallback(
+              &ChromeInvalidationClient::HandleOutboundPacket)),
       listener_(NULL),
       state_writer_(NULL) {
   DCHECK(non_thread_safe_.CalledOnValidThread());
@@ -63,6 +67,11 @@ void ChromeInvalidationClient::Start(
           &chrome_system_resources_, client_type, client_id, client_config,
           this));
   invalidation_client_->Start(state);
+  invalidation::NetworkEndpoint* network_endpoint =
+      invalidation_client_->network_endpoint();
+  CHECK(network_endpoint);
+  network_endpoint->RegisterOutboundListener(
+      handle_outbound_packet_callback_.get());
   ChangeBaseTask(base_task);
   registration_manager_.reset(
       new RegistrationManager(invalidation_client_.get()));
@@ -182,7 +191,17 @@ void ChromeInvalidationClient::RegistrationLost(
 }
 
 void ChromeInvalidationClient::WriteState(const std::string& state) {
+  DCHECK(non_thread_safe_.CalledOnValidThread());
+  CHECK(state_writer_);
   state_writer_->WriteState(state);
 }
 
+void ChromeInvalidationClient::HandleOutboundPacket(
+    invalidation::NetworkEndpoint* const& network_endpoint) {
+  DCHECK(non_thread_safe_.CalledOnValidThread());
+  CHECK(cache_invalidation_packet_handler_.get());
+  cache_invalidation_packet_handler_->
+      HandleOutboundPacket(network_endpoint);
+}
+
 }  // namespace sync_notifier
diff --git a/chrome/browser/sync/notifier/chrome_invalidation_client.h b/chrome/browser/sync/notifier/chrome_invalidation_client.h
index e6219f7..e408085 100644
--- a/chrome/browser/sync/notifier/chrome_invalidation_client.h
+++ b/chrome/browser/sync/notifier/chrome_invalidation_client.h
@@ -13,6 +13,7 @@
 
 #include "base/basictypes.h"
 #include "base/non_thread_safe.h"
+#include "base/scoped_callback_factory.h"
 #include "base/scoped_ptr.h"
 #include "base/weak_ptr.h"
 #include "chrome/browser/sync/notifier/chrome_system_resources.h"
@@ -92,8 +93,17 @@ class ChromeInvalidationClient
   virtual void WriteState(const std::string& state);
 
  private:
+  friend class ChromeInvalidationClientTest;
+
+  // Should only be called between calls to Start() and Stop().
+  void HandleOutboundPacket(
+      invalidation::NetworkEndpoint* const& network_endpoint);
+
   NonThreadSafe non_thread_safe_;
   ChromeSystemResources chrome_system_resources_;
+  base::ScopedCallbackFactory<ChromeInvalidationClient>
+      scoped_callback_factory_;
+  scoped_ptr<invalidation::NetworkCallback> handle_outbound_packet_callback_;
   Listener* listener_;
   StateWriter* state_writer_;
   scoped_ptr<invalidation::InvalidationClient> invalidation_client_;
diff --git a/chrome/browser/sync/notifier/chrome_invalidation_client_unittest.cc b/chrome/browser/sync/notifier/chrome_invalidation_client_unittest.cc
new file mode 100644
index 0000000..6923904
--- /dev/null
+++ b/chrome/browser/sync/notifier/chrome_invalidation_client_unittest.cc
@@ -0,0 +1,95 @@
+// 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 <string>
+
+#include "chrome/browser/sync/notifier/chrome_invalidation_client.h"
+#include "chrome/browser/sync/notifier/state_writer.h"
+#include "jingle/notifier/base/fake_base_task.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace sync_notifier {
+
+using ::testing::_;
+using ::testing::Return;
+
+class MockListener : public ChromeInvalidationClient::Listener {
+ public:
+  MOCK_METHOD1(OnInvalidate, void(syncable::ModelType));
+  MOCK_METHOD0(OnInvalidateAll, void());
+};
+
+class MockStateWriter : public StateWriter {
+ public:
+  MOCK_METHOD1(WriteState, void(const std::string&));
+};
+
+class MockInvalidationClient : public invalidation::InvalidationClient {
+ public:
+  MOCK_METHOD1(Start, void(const std::string& str));
+  MOCK_METHOD1(Register, void(const invalidation::ObjectId&));
+  MOCK_METHOD1(Unregister, void(const invalidation::ObjectId&));
+  MOCK_METHOD0(network_endpoint, invalidation::NetworkEndpoint*());
+};
+
+namespace {
+const char kClientId[] = "client_id";
+const char kState[] = "state";
+}  // namespace
+
+class ChromeInvalidationClientTest : public testing::Test {
+ protected:
+  virtual void SetUp() {
+    client_.Start(kClientId, kState, &mock_listener_, &mock_state_writer_,
+                  fake_base_task_.AsWeakPtr());
+  }
+
+  virtual void TearDown() {
+    client_.Stop();
+    message_loop_.RunAllPending();
+  }
+
+  // Simulates DoInformOutboundListener() from network-manager.cc.
+  virtual void SimulateInformOutboundListener() {
+    // Explicitness hack here to work around broken callback
+    // implementations.
+    void (invalidation::NetworkCallback::*run_function)(
+        invalidation::NetworkEndpoint* const&) =
+        &invalidation::NetworkCallback::Run;
+
+    client_.chrome_system_resources_.ScheduleOnListenerThread(
+        invalidation::NewPermanentCallback(
+            client_.handle_outbound_packet_callback_.get(), run_function,
+            client_.invalidation_client_->network_endpoint()));
+  }
+
+  MessageLoop message_loop_;
+  MockListener mock_listener_;
+  MockStateWriter mock_state_writer_;
+  notifier::FakeBaseTask fake_base_task_;
+  ChromeInvalidationClient client_;
+};
+
+// Outbound packet sending should be resilient to
+// changing/disappearing base tasks.
+TEST_F(ChromeInvalidationClientTest, OutboundPackets) {
+  SimulateInformOutboundListener();
+
+  notifier::FakeBaseTask fake_base_task;
+  client_.ChangeBaseTask(fake_base_task.AsWeakPtr());
+
+  SimulateInformOutboundListener();
+
+  {
+    notifier::FakeBaseTask fake_base_task2;
+    client_.ChangeBaseTask(fake_base_task2.AsWeakPtr());
+  }
+
+  SimulateInformOutboundListener();
+}
+
+// TODO(akalin): Flesh out unit tests.
+
+}  // namespace sync_notifier
diff --git a/chrome/chrome_tests.gypi b/chrome/chrome_tests.gypi
index 82f6128c..cedf831 100644
--- a/chrome/chrome_tests.gypi
+++ b/chrome/chrome_tests.gypi
@@ -2632,6 +2632,7 @@
         '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_invalidation_client_unittest.cc',
         'browser/sync/notifier/chrome_system_resources_unittest.cc',
         'browser/sync/notifier/registration_manager_unittest.cc',
         'browser/sync/profile_sync_factory_mock.cc',
@@ -2676,6 +2677,7 @@
         'browser/sync/protocol/sync_proto.gyp:sync_proto_cpp',
         'common',
         'debugger',
+        '../jingle/jingle.gyp:notifier_test_util',
         '../skia/skia.gyp:skia',
         '../testing/gmock.gyp:gmock',
         '../testing/gtest.gyp:gtest',
diff --git a/jingle/jingle.gyp b/jingle/jingle.gyp
index 939c9d0..61d9320 100644
--- a/jingle/jingle.gyp
+++ b/jingle/jingle.gyp
@@ -92,6 +92,19 @@
       ],
     },
     {
+      'target_name': 'notifier_test_util',
+      'type': '<(library)',
+      'sources': [
+        'notifier/base/fake_base_task.cc',
+        'notifier/base/fake_base_task.h',
+      ],
+      'dependencies': [
+        'notifier',
+        '../base/base.gyp:base',
+        '../testing/gmock.gyp:gmock',
+      ],
+    },
+    {
       'target_name': 'notifier_unit_tests',
       'type': 'executable',
       'sources': [
@@ -113,6 +126,7 @@
       ],
       'dependencies': [
         'notifier',
+        'notifier_test_util',
         '../base/base.gyp:base',
         '../base/base.gyp:test_support_base',
         '../net/net.gyp:net',
diff --git a/jingle/notifier/base/fake_base_task.cc b/jingle/notifier/base/fake_base_task.cc
new file mode 100644
index 0000000..255ae64
--- /dev/null
+++ b/jingle/notifier/base/fake_base_task.cc
@@ -0,0 +1,54 @@
+// 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.
+//
+// A simple wrapper around invalidation::InvalidationClient that
+// handles all the startup/shutdown details and hookups.
+
+#include "base/message_loop.h"
+#include "jingle/notifier/base/fake_base_task.h"
+#include "jingle/notifier/base/weak_xmpp_client.h"
+#include "talk/xmpp/asyncsocket.h"
+#include "testing/gmock/include/gmock/gmock.h"
+
+namespace notifier {
+
+using ::testing::_;
+using ::testing::Return;
+
+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&));
+};
+
+FakeBaseTask::FakeBaseTask() {
+  // Owned by |task_pump_|.
+  notifier::WeakXmppClient* weak_xmpp_client =
+      new notifier::WeakXmppClient(&task_pump_);
+
+  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.
+  MessageLoop::current()->RunAllPending();
+
+  base_task_ = weak_xmpp_client->AsWeakPtr();
+}
+
+base::WeakPtr<talk_base::Task> FakeBaseTask::AsWeakPtr() {
+  return base_task_;
+}
+
+}  // namespace notifier
diff --git a/jingle/notifier/base/fake_base_task.h b/jingle/notifier/base/fake_base_task.h
new file mode 100644
index 0000000..c165561
--- /dev/null
+++ b/jingle/notifier/base/fake_base_task.h
@@ -0,0 +1,38 @@
+// 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.
+//
+// A simple wrapper around invalidation::InvalidationClient that
+// handles all the startup/shutdown details and hookups.
+
+#ifndef JINGLE_NOTIFIER_FAKE_XMPP_CLIENT_H_
+#define JINGLE_NOTIFIER_FAKE_XMPP_CLIENT_H_
+#pragma once
+
+#include "base/basictypes.h"
+#include "base/weak_ptr.h"
+#include "jingle/notifier/base/task_pump.h"
+
+namespace talk_base {
+class Task;
+}  // namespace talk_base
+
+namespace notifier {
+
+// This class expects a message loop to exist on the current thread.
+class FakeBaseTask {
+ public:
+  FakeBaseTask();
+
+  base::WeakPtr<talk_base::Task> AsWeakPtr();
+
+ private:
+  notifier::TaskPump task_pump_;
+  base::WeakPtr<talk_base::Task> base_task_;
+
+  DISALLOW_COPY_AND_ASSIGN(FakeBaseTask);
+};
+
+}  // namespace notifier
+
+#endif  // JINGLE_NOTIFIER_FAKE_XMPP_CLIENT_H_