[Sync] Fixed sync crash regression in ServerNotifierThread

Added unit tests for ServerNotifierThread.

Put DISABLE_RUNNABLE_METHOD_REFCOUNT() call for MediatorThreadImpl in the right place.

BUG=57898
TEST=New unittests

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

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

3 files changed, 152 insertions, 1 deletions

diff --git a/chrome/browser/sync/notifier/server_notifier_thread.cc b/chrome/browser/sync/notifier/server_notifier_thread.cc
index 24a1c47..f16587f 100644
--- a/chrome/browser/sync/notifier/server_notifier_thread.cc
+++ b/chrome/browser/sync/notifier/server_notifier_thread.cc
@@ -123,7 +123,9 @@ void ServerNotifierThread::DoListenForUpdates() {
 
 void ServerNotifierThread::RegisterTypesAndSignalSubscribed() {
   DCHECK_EQ(MessageLoop::current(), worker_message_loop());
-  DCHECK(chrome_invalidation_client_.get());
+  if (!chrome_invalidation_client_.get()) {
+    return;
+  }
   chrome_invalidation_client_->RegisterTypes();
   observers_->Notify(&Observer::OnSubscriptionStateChange, true);
 }
diff --git a/chrome/browser/sync/notifier/server_notifier_thread_unittest.cc b/chrome/browser/sync/notifier/server_notifier_thread_unittest.cc
new file mode 100644
index 0000000..416ffe5
--- /dev/null
+++ b/chrome/browser/sync/notifier/server_notifier_thread_unittest.cc
@@ -0,0 +1,148 @@
+// 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 "base/compiler_specific.h"
+#include "base/message_loop.h"
+#include "base/scoped_ptr.h"
+#include "base/task.h"
+#include "chrome/browser/sync/notifier/server_notifier_thread.h"
+#include "chrome/browser/sync/notifier/state_writer.h"
+#include "jingle/notifier/base/fake_base_task.h"
+#include "jingle/notifier/base/notifier_options.h"
+#include "talk/xmpp/xmppclientsettings.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace sync_notifier {
+
+class FakeServerNotifierThread : public ServerNotifierThread {
+ public:
+  FakeServerNotifierThread()
+      : ServerNotifierThread(notifier::NotifierOptions(), "fake state",
+                             ALLOW_THIS_IN_INITIALIZER_LIST(this)) {}
+
+  virtual ~FakeServerNotifierThread() {}
+
+  virtual void Start() {
+    DCHECK_EQ(MessageLoop::current(), parent_message_loop_);
+    ServerNotifierThread::Start();
+    worker_message_loop()->PostTask(
+        FROM_HERE,
+        NewRunnableMethod(this,
+                          &FakeServerNotifierThread::InitFakes));
+  }
+
+  virtual void Logout() {
+    DCHECK_EQ(MessageLoop::current(), parent_message_loop_);
+    worker_message_loop()->PostTask(
+        FROM_HERE,
+        NewRunnableMethod(this,
+                          &FakeServerNotifierThread::DestroyFakes));
+    ServerNotifierThread::Logout();
+  }
+
+  // We prevent the real connection attempt from happening and use
+  // SimulateConnection()/SimulateDisconnection() instead.
+  virtual void Login(const buzz::XmppClientSettings& settings) {
+    DCHECK_EQ(MessageLoop::current(), parent_message_loop_);
+  }
+
+  // We pass ourselves as the StateWriter in the constructor, so shim
+  // out WriteState() to prevent an infinite loop.
+  virtual void WriteState(const std::string& state) {
+    DCHECK_EQ(MessageLoop::current(), worker_message_loop());
+  }
+
+  void SimulateConnect() {
+    DCHECK_EQ(MessageLoop::current(), parent_message_loop_);
+    worker_message_loop()->PostTask(
+        FROM_HERE,
+        NewRunnableMethod(this,
+                          &FakeServerNotifierThread::DoSimulateConnect));
+  }
+
+  void SimulateDisconnect() {
+    DCHECK_EQ(MessageLoop::current(), parent_message_loop_);
+    worker_message_loop()->PostTask(
+        FROM_HERE,
+        NewRunnableMethod(this,
+                          &FakeServerNotifierThread::DoSimulateDisconnect));
+  }
+
+ private:
+  void InitFakes() {
+    DCHECK_EQ(MessageLoop::current(), worker_message_loop());
+    fake_base_task_.reset(new notifier::FakeBaseTask());
+  }
+
+  void DestroyFakes() {
+    DCHECK_EQ(MessageLoop::current(), worker_message_loop());
+    fake_base_task_.reset();
+  }
+
+  void DoSimulateConnect() {
+    DCHECK_EQ(MessageLoop::current(), worker_message_loop());
+    OnConnect(fake_base_task_->AsWeakPtr());
+  }
+
+  void DoSimulateDisconnect() {
+    DCHECK_EQ(MessageLoop::current(), worker_message_loop());
+    OnDisconnect();
+  }
+
+  // Used only on the worker thread.
+  scoped_ptr<notifier::FakeBaseTask> fake_base_task_;
+};
+
+}  // namespace sync_notifier
+
+DISABLE_RUNNABLE_METHOD_REFCOUNT(sync_notifier::FakeServerNotifierThread);
+
+namespace sync_notifier {
+
+namespace {
+
+class ServerNotifierThreadTest : public testing::Test {
+ protected:
+  MessageLoop message_loop_;
+};
+
+TEST_F(ServerNotifierThreadTest, Basic) {
+  FakeServerNotifierThread server_notifier_thread;
+
+  server_notifier_thread.Start();
+  server_notifier_thread.Login(buzz::XmppClientSettings());
+  server_notifier_thread.SimulateConnect();
+  server_notifier_thread.ListenForUpdates();
+  server_notifier_thread.SubscribeForUpdates(std::vector<std::string>());
+  server_notifier_thread.Logout();
+}
+
+TEST_F(ServerNotifierThreadTest, DisconnectBeforeListen) {
+  FakeServerNotifierThread server_notifier_thread;
+
+  server_notifier_thread.Start();
+  server_notifier_thread.Login(buzz::XmppClientSettings());
+  server_notifier_thread.ListenForUpdates();
+  server_notifier_thread.SubscribeForUpdates(std::vector<std::string>());
+  server_notifier_thread.Logout();
+}
+
+TEST_F(ServerNotifierThreadTest, Disconnected) {
+  FakeServerNotifierThread server_notifier_thread;
+
+  server_notifier_thread.Start();
+  server_notifier_thread.Login(buzz::XmppClientSettings());
+  server_notifier_thread.SimulateConnect();
+  server_notifier_thread.SimulateDisconnect();
+  server_notifier_thread.ListenForUpdates();
+  server_notifier_thread.SubscribeForUpdates(std::vector<std::string>());
+  server_notifier_thread.Logout();
+}
+
+}  // namespace
+
+}  // namespace sync_notifier
diff --git a/chrome/chrome_tests.gypi b/chrome/chrome_tests.gypi
index 88ca592..67b51dbc 100644
--- a/chrome/chrome_tests.gypi
+++ b/chrome/chrome_tests.gypi
@@ -2643,6 +2643,7 @@
         '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/notifier/server_notifier_thread_unittest.cc',
         'browser/sync/profile_sync_factory_mock.cc',
         'browser/sync/profile_sync_factory_mock.h',
         'browser/sync/sessions/ordered_commit_set_unittest.cc',