[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
hylschwy · Dec 10, 2010 · 5827135 · 5827135
1 parent 62e43e2
commit 5827135
Show file tree

Hide file tree

Showing 7 changed files with 159 additions and 13 deletions.
diff --git 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
@@ -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
@@ -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',

diff --git a/jingle/notifier/base/fake_base_task.cc b/jingle/notifier/base/fake_base_task.cc
@@ -1,11 +1,7 @@
 // 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"
@@ -42,7 +38,7 @@ FakeBaseTask::FakeBaseTask() {
   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();
+  task_pump_.RunTasks();
 
   base_task_ = weak_xmpp_client->AsWeakPtr();
 }

diff --git a/jingle/notifier/base/fake_base_task.h b/jingle/notifier/base/fake_base_task.h
@@ -2,8 +2,8 @@
 // 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.
+// A stand-in for stuff that expects a weak pointer to a BaseTask for
+// testing.
 
 #ifndef JINGLE_NOTIFIER_FAKE_XMPP_CLIENT_H_
 #define JINGLE_NOTIFIER_FAKE_XMPP_CLIENT_H_
@@ -19,7 +19,6 @@ class Task;
 
 namespace notifier {
 
-// This class expects a message loop to exist on the current thread.
 class FakeBaseTask {
  public:
   FakeBaseTask();

diff --git a/jingle/notifier/listener/mediator_thread_impl.cc b/jingle/notifier/listener/mediator_thread_impl.cc
@@ -6,7 +6,6 @@
 
 #include "base/logging.h"
 #include "base/message_loop.h"
-#include "base/task.h"
 #include "jingle/notifier/base/task_pump.h"
 #include "jingle/notifier/communicator/connection_options.h"
 #include "jingle/notifier/communicator/const_communicator.h"
@@ -18,9 +17,6 @@
 #include "net/base/host_resolver.h"
 #include "talk/xmpp/xmppclientsettings.h"
 
-// We manage the lifetime of notifier::MediatorThreadImpl ourselves.
-DISABLE_RUNNABLE_METHOD_REFCOUNT(notifier::MediatorThreadImpl);
-
 namespace notifier {
 
 MediatorThreadImpl::MediatorThreadImpl(const NotifierOptions& notifier_options)

diff --git a/jingle/notifier/listener/mediator_thread_impl.h b/jingle/notifier/listener/mediator_thread_impl.h
@@ -27,6 +27,7 @@
 #include "base/observer_list_threadsafe.h"
 #include "base/ref_counted.h"
 #include "base/scoped_ptr.h"
+#include "base/task.h"
 #include "base/thread.h"
 #include "base/weak_ptr.h"
 #include "jingle/notifier/base/notifier_options.h"
@@ -112,4 +113,7 @@ class MediatorThreadImpl : public MediatorThread, public LoginDelegate,
 
 }  // namespace notifier
 
+// We manage the lifetime of notifier::MediatorThreadImpl ourselves.
+DISABLE_RUNNABLE_METHOD_REFCOUNT(notifier::MediatorThreadImpl);
+
 #endif  // JINGLE_NOTIFIER_LISTENER_MEDIATOR_THREAD_IMPL_H_