[Sync] Fix thread-ordering-dependent NULL dereference in NewNonFrontendDTC

chrome/browser/sync/glue/autofill_data_type_controller_unittest.cc

Index: chrome/browser/sync/glue/autofill_data_type_controller_unittest.cc
diff --git a/chrome/browser/sync/glue/autofill_data_type_controller_unittest.cc b/chrome/browser/sync/glue/autofill_data_type_controller_unittest.cc
index 9640e35cf6bed054d5b817ed3e3eaa66cf1880a8..d8fc00b037fff5da458f896cbc15d0de997f7467 100644
--- a/chrome/browser/sync/glue/autofill_data_type_controller_unittest.cc
+++ b/chrome/browser/sync/glue/autofill_data_type_controller_unittest.cc
@@ -2,178 +2,162 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "testing/gtest/include/gtest/gtest.h"
-
 #include "base/callback.h"
+#include "base/compiler_specific.h"
 #include "base/memory/ref_counted.h"
-#include "base/memory/scoped_ptr.h"
+#include "base/memory/weak_ptr.h"
 #include "base/message_loop.h"
-#include "base/synchronization/waitable_event.h"
-#include "base/test/test_timeouts.h"
-#include "chrome/browser/autofill/personal_data_manager.h"
+#include "chrome/browser/sync/api/sync_error.h"
 #include "chrome/browser/sync/glue/autofill_data_type_controller.h"
 #include "chrome/browser/sync/glue/data_type_controller_mock.h"
+#include "chrome/browser/sync/glue/shared_change_processor_mock.h"
 #include "chrome/browser/sync/profile_sync_components_factory_mock.h"
 #include "chrome/browser/sync/profile_sync_service_mock.h"
-#include "chrome/browser/sync/profile_sync_test_util.h"
 #include "chrome/browser/webdata/web_data_service.h"
 #include "chrome/common/chrome_notification_types.h"
 #include "chrome/test/base/profile_mock.h"
+#include "content/public/browser/notification_service.h"
 #include "content/public/browser/notification_source.h"
 #include "content/public/browser/notification_types.h"
 #include "content/test/test_browser_thread.h"
 #include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
 
-using base::WaitableEvent;
-using browser_sync::AutofillDataTypeController;
-using browser_sync::DataTypeController;
-using content::BrowserThread;
-using testing::_;
-using testing::DoAll;
-using testing::Invoke;
-using testing::Return;
-using testing::SetArgumentPointee;
+namespace browser_sync {
 
 namespace {
 
-ACTION_P(WaitOnEvent, event) {
-  event->Wait();
-}
-
-ACTION_P(SignalEvent, event) {
-  event->Signal();
-}
-
-ACTION_P(GetAutocompleteSyncComponents, wds) {
-  return base::WeakPtr<SyncableService>();
-}
-
-ACTION_P(RaiseSignal, data_controller_mock) {
-  data_controller_mock->RaiseStartSignal();
-}
+using content::BrowserThread;
+using testing::_;
+using testing::NiceMock;
+using testing::Return;
 
-// This class mocks PersonalDataManager and provides a factory method to
-// serve back the mocked object to callers of
-// |PersonalDataManagerFactory::GetForProfile()|.
-class PersonalDataManagerMock : public PersonalDataManager {
+// Fake WebDataService implementation that stubs out the database
+// loading.
+class FakeWebDataService : public WebDataService {
  public:
-  PersonalDataManagerMock() : PersonalDataManager() {}
-  virtual ~PersonalDataManagerMock() {}
-
-  static ProfileKeyedService* Build(Profile* profile) {
-    return new PersonalDataManagerMock;
+  FakeWebDataService() : is_database_loaded_(false) {}
+  virtual ~FakeWebDataService() {}
+
+  // Mark the database as loaded and send out the appropriate
+  // notification.
+  void LoadDatabase() {
+    is_database_loaded_ = true;
+    // TODO(akalin): Expose WDS::NotifyDatabaseLoadedOnUIThread() and
+    // use that instead of sending this notification manually.
+    content::NotificationService::current()->Notify(
+        chrome::NOTIFICATION_WEB_DATABASE_LOADED,
+        content::Source<WebDataService>(this),
+        content::NotificationService::NoDetails());
   }
 
-  MOCK_CONST_METHOD0(IsDataLoaded, bool());
+  virtual bool IsDatabaseLoaded() OVERRIDE {
+    return is_database_loaded_;
+  }
 
  private:
-  DISALLOW_COPY_AND_ASSIGN(PersonalDataManagerMock);
-};
-
-class WebDataServiceFake : public WebDataService {
- public:
-  explicit WebDataServiceFake() {}
+  DISALLOW_COPY_AND_ASSIGN(FakeWebDataService);
 
-  MOCK_METHOD0(IsDatabaseLoaded, bool());
-
- private:
-  DISALLOW_COPY_AND_ASSIGN(WebDataServiceFake);
+  bool is_database_loaded_;
 };
 
-class AutofillDataTypeControllerMock : public AutofillDataTypeController {
+class SyncAutofillDataTypeControllerTest : public testing::Test {
  public:
-  AutofillDataTypeControllerMock(
-      ProfileSyncComponentsFactory* profile_sync_factory,
-      Profile* profile,
-      ProfileSyncService* sync_service)
-      : AutofillDataTypeController(profile_sync_factory,
-                                   profile,
-                                   sync_service),
-        start_called_(false, false) {}
-
-  MOCK_METHOD0(StartAssociation, void());
-
-  void RaiseStartSignal() {
-    start_called_.Signal();
-  }
+  SyncAutofillDataTypeControllerTest()
+      : weak_ptr_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)),
+        ui_thread_(BrowserThread::UI, &message_loop_),
+        last_start_result_(DataTypeController::OK) {}
 
-  void WaitUntilStartCalled() {
-    start_called_.Wait();
-  }
+  virtual ~SyncAutofillDataTypeControllerTest() {}
 
- private:
-  friend class AutofillDataTypeControllerTest;
-  FRIEND_TEST_ALL_PREFIXES(AutofillDataTypeControllerTest, StartWDSReady);
-  FRIEND_TEST_ALL_PREFIXES(AutofillDataTypeControllerTest, StartWDSNotReady);
+  // We deliberately do not set up a DB thread so that we always stop
+  // with an association failure.
 
-  WaitableEvent start_called_;
+  virtual void SetUp() {
+    change_processor_ = new NiceMock<SharedChangeProcessorMock>();
 
-  DISALLOW_COPY_AND_ASSIGN(AutofillDataTypeControllerMock);
-};
+    EXPECT_CALL(profile_sync_factory_,
+                CreateSharedChangeProcessor()).
+        WillRepeatedly(Return(change_processor_.get()));
 
-class AutofillDataTypeControllerTest : public testing::Test {
- public:
-  AutofillDataTypeControllerTest()
-      : ui_thread_(BrowserThread::UI, &message_loop_),
-        db_thread_(BrowserThread::DB) {}
+    web_data_service_ = new FakeWebDataService();
 
-  virtual void SetUp() {
-    db_thread_.Start();
-    db_notification_service_ = new ThreadNotificationService(
-        db_thread_.DeprecatedGetThreadObject());
-    db_notification_service_->Init();
-    web_data_service_ = new WebDataServiceFake();
-    autofill_dtc_ =
-        new AutofillDataTypeControllerMock(&profile_sync_factory_,
-                                           &profile_,
-                                           &service_);
     EXPECT_CALL(profile_, GetWebDataService(_)).
         WillRepeatedly(Return(web_data_service_.get()));
+
+    autofill_dtc_ =
+        new AutofillDataTypeController(&profile_sync_factory_,
+                                       &profile_,
+                                       &service_);
+  }
+
+  // Passed to AutofillDTC::Start().
+  void OnStartFinished(DataTypeController::StartResult result,
+                       const SyncError& error) {
+    last_start_result_ = result;
+    last_start_error_ = error;
   }
 
   virtual void TearDown() {
-    db_notification_service_->TearDown();
-    db_thread_.Stop();
+    autofill_dtc_ = NULL;
+    web_data_service_ = NULL;
+    change_processor_ = NULL;
   }
 
  protected:
+  base::WeakPtrFactory<SyncAutofillDataTypeControllerTest> weak_ptr_factory_;
   MessageLoopForUI message_loop_;
   content::TestBrowserThread ui_thread_;
-  content::TestBrowserThread db_thread_;
-  scoped_refptr<ThreadNotificationService> db_notification_service_;
-  scoped_refptr<AutofillDataTypeControllerMock> autofill_dtc_;
+
+  scoped_refptr<NiceMock<SharedChangeProcessorMock> > change_processor_;
   ProfileSyncComponentsFactoryMock profile_sync_factory_;
-  ProfileMock profile_;
   ProfileSyncServiceMock service_;
-  scoped_refptr<WebDataServiceFake> web_data_service_;
-};
+  ProfileMock profile_;
+  scoped_refptr<FakeWebDataService> web_data_service_;
+  scoped_refptr<AutofillDataTypeController> autofill_dtc_;
 
-TEST_F(AutofillDataTypeControllerTest, StartWDSReady) {
-  EXPECT_CALL(*(web_data_service_.get()), IsDatabaseLoaded()).
-      WillOnce(Return(true));
-  EXPECT_CALL(*(autofill_dtc_.get()), StartAssociation()).Times(0);
+  // Stores arguments of most recent call of OnStartFinished().
+  DataTypeController::StartResult last_start_result_;
+  SyncError last_start_error_;
+};
 
-  autofill_dtc_->set_state(DataTypeController::MODEL_STARTING);
-  EXPECT_TRUE(autofill_dtc_->StartModels());
+// Load the WDS's database, then start the Autofill DTC.  It should
+// immediately try to start association and fail (due to missing DB
+// thread).
+TEST_F(SyncAutofillDataTypeControllerTest, StartWDSReady) {
+  web_data_service_->LoadDatabase();
+  autofill_dtc_->Start(
+      base::Bind(&SyncAutofillDataTypeControllerTest::OnStartFinished,
+                 weak_ptr_factory_.GetWeakPtr()));
+
+  EXPECT_EQ(DataTypeController::ASSOCIATION_FAILED, last_start_result_);
+  EXPECT_TRUE(last_start_error_.IsSet());
+  EXPECT_EQ(DataTypeController::NOT_RUNNING, autofill_dtc_->state());
 }
 
-TEST_F(AutofillDataTypeControllerTest, StartWDSNotReady) {
-  EXPECT_CALL(*(web_data_service_.get()), IsDatabaseLoaded()).
-      WillOnce(Return(false));
-  EXPECT_CALL(*(autofill_dtc_.get()), StartAssociation()).
-      WillOnce(RaiseSignal(autofill_dtc_.get()));
-
-  autofill_dtc_->set_state(DataTypeController::MODEL_STARTING);
-  EXPECT_FALSE(autofill_dtc_->StartModels());
-
-  scoped_refptr<ThreadNotifier> notifier(new ThreadNotifier(
-      ui_thread_.DeprecatedGetThreadObject()));
-  autofill_dtc_->Observe(
-      chrome::NOTIFICATION_WEB_DATABASE_LOADED,
-      content::Source<WebDataService>(web_data_service_.get()),
-      content::NotificationService::NoDetails());
-  autofill_dtc_->WaitUntilStartCalled();
-  EXPECT_EQ(DataTypeController::ASSOCIATING, autofill_dtc_->state());
+// Start the autofill DTC without the WDS's database loaded, then
+// start the DB.  The Autofill DTC should be in the MODEL_STARTING
+// state until the database in loaded, when it should try to start
+// association and fail (due to missing DB thread).
+TEST_F(SyncAutofillDataTypeControllerTest, StartWDSNotReady) {
+  autofill_dtc_->Start(
+      base::Bind(&SyncAutofillDataTypeControllerTest::OnStartFinished,
+                 weak_ptr_factory_.GetWeakPtr()));
+
+  EXPECT_EQ(DataTypeController::OK, last_start_result_);
+  EXPECT_FALSE(last_start_error_.IsSet());
+  EXPECT_EQ(DataTypeController::MODEL_STARTING, autofill_dtc_->state());
+
+  web_data_service_->LoadDatabase();
+
+  EXPECT_EQ(DataTypeController::ASSOCIATION_FAILED, last_start_result_);
+  EXPECT_TRUE(last_start_error_.IsSet());
+  // There's a TODO for
+  // NonFrontendDataTypeController::StartAssociationAsync() that, when
+  // done, will make this consistent with the previous test.
+  EXPECT_EQ(DataTypeController::DISABLED, autofill_dtc_->state());
 }
 
 }  // namespace
+
+}  // namespace browser_sync