AutofillDataTypeController should invoke start callback on abort.

BUG=41361

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

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

15 files changed, 349 insertions, 20 deletions

diff --git a/chrome/browser/sync/glue/autofill_data_type_controller.cc b/chrome/browser/sync/glue/autofill_data_type_controller.cc
index 7f2e1bc..e212e65 100644
--- a/chrome/browser/sync/glue/autofill_data_type_controller.cc
+++ b/chrome/browser/sync/glue/autofill_data_type_controller.cc
@@ -26,7 +26,9 @@ AutofillDataTypeController::AutofillDataTypeController(
       profile_(profile),
       sync_service_(sync_service),
       state_(NOT_RUNNING),
-      personal_data_(NULL) {
+      personal_data_(NULL),
+      abort_association_(false),
+      abort_association_complete_(false, false) {
   DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
   DCHECK(profile_sync_factory);
   DCHECK(profile);
@@ -48,6 +50,7 @@ void AutofillDataTypeController::Start(StartCallback* start_callback) {
   }
 
   start_callback_.reset(start_callback);
+  abort_association_ = false;
 
   // Waiting for the personal data is subtle:  we do this as the PDM resets
   // its cache of unique IDs once it gets loaded. If we were to proceed with
@@ -87,10 +90,8 @@ void AutofillDataTypeController::Observe(NotificationType type,
                                          const NotificationSource& source,
                                          const NotificationDetails& details) {
   LOG(INFO) << "Web database loaded observed.";
-  notification_registrar_.Remove(this,
-                                 NotificationType::WEB_DATABASE_LOADED,
-                                 NotificationService::AllSources());
-
+  notification_registrar_.RemoveAll();
+  set_state(ASSOCIATING);
   ChromeThread::PostTask(ChromeThread::DB, FROM_HERE,
                          NewRunnableMethod(
                              this,
@@ -101,6 +102,30 @@ void AutofillDataTypeController::Stop() {
   LOG(INFO) << "Stopping autofill data type controller.";
   DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
 
+  // If Stop() is called while Start() is waiting for association to
+  // complete, we need to abort the association.
+  if (state_ == ASSOCIATING) {
+    {
+      AutoLock lock(abort_association_lock_);
+      abort_association_ = true;
+      if (model_associator_.get())
+        model_associator_->Shutdown();
+    }
+    // Wait for the model association to abort.
+    abort_association_complete_.Wait();
+    StartDoneImpl(ABORTED, STOPPING);
+  }
+
+  // If Stop() is called while Start() is waiting for the personal web
+  // service or web data service to load, abort the start.
+  if (state_ == MODEL_STARTING)
+    StartDoneImpl(ABORTED, STOPPING);
+
+  // Note that we are doing most of the stop work here on the UI
+  // thread even though the associate & activate were done on the DB
+  // thread.  This is because Stop() must be synchronous.
+  notification_registrar_.RemoveAll();
+  personal_data_->RemoveObserver(this);
   if (change_processor_ != NULL)
     sync_service_->DeactivateDataType(this, change_processor_.get());
 
@@ -152,12 +177,18 @@ void AutofillDataTypeController::StartDone(
     DataTypeController::State new_state) {
   LOG(INFO) << "Autofill data type controller StartDone called.";
   DCHECK(ChromeThread::CurrentlyOn(ChromeThread::DB));
-  ChromeThread::PostTask(ChromeThread::UI, FROM_HERE,
-                         NewRunnableMethod(
-                             this,
-                             &AutofillDataTypeController::StartDoneImpl,
-                             result,
-                             new_state));
+
+  AutoLock lock(abort_association_lock_);
+  if (abort_association_) {
+    abort_association_complete_.Signal();
+  } else {
+    ChromeThread::PostTask(ChromeThread::UI, FROM_HERE,
+                           NewRunnableMethod(
+                               this,
+                               &AutofillDataTypeController::StartDoneImpl,
+                               result,
+                               new_state));
+  }
 }
 
 void AutofillDataTypeController::StartDoneImpl(
@@ -165,6 +196,13 @@ void AutofillDataTypeController::StartDoneImpl(
     DataTypeController::State new_state) {
   LOG(INFO) << "Autofill data type controller StartDoneImpl called.";
   DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
+
+  // If we were stopped while associating, just return.
+  if (state_ == NOT_RUNNING) {
+    LOG(WARNING) << "Stopped while associating.";
+    return;
+  }
+
   set_state(new_state);
   start_callback_->Run(result);
   start_callback_.reset();
diff --git a/chrome/browser/sync/glue/autofill_data_type_controller.h b/chrome/browser/sync/glue/autofill_data_type_controller.h
index b9b5983..e522dbd 100644
--- a/chrome/browser/sync/glue/autofill_data_type_controller.h
+++ b/chrome/browser/sync/glue/autofill_data_type_controller.h
@@ -7,6 +7,7 @@
 
 #include "base/basictypes.h"
 #include "base/scoped_ptr.h"
+#include "base/waitable_event.h"
 #include "chrome/browser/autofill/personal_data_manager.h"
 #include "chrome/browser/sync/profile_sync_service.h"
 #include "chrome/browser/sync/glue/data_type_controller.h"
@@ -99,6 +100,10 @@ class AutofillDataTypeController : public DataTypeController,
 
   NotificationRegistrar notification_registrar_;
 
+  Lock abort_association_lock_;
+  bool abort_association_;
+  base::WaitableEvent abort_association_complete_;
+
   DISALLOW_COPY_AND_ASSIGN(AutofillDataTypeController);
 };
 
diff --git a/chrome/browser/sync/glue/autofill_data_type_controller_unittest.cc b/chrome/browser/sync/glue/autofill_data_type_controller_unittest.cc
new file mode 100644
index 0000000..4897a88
--- /dev/null
+++ b/chrome/browser/sync/glue/autofill_data_type_controller_unittest.cc
@@ -0,0 +1,233 @@
+// 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 "testing/gtest/include/gtest/gtest.h"
+
+#include "base/callback.h"
+#include "base/message_loop.h"
+#include "base/ref_counted.h"
+#include "base/scoped_ptr.h"
+#include "base/waitable_event.h"
+#include "chrome/browser/autofill/personal_data_manager.h"
+#include "chrome/browser/chrome_thread.h"
+#include "chrome/browser/profile.h"
+#include "chrome/browser/sync/glue/autofill_data_type_controller.h"
+#include "chrome/browser/sync/glue/change_processor_mock.h"
+#include "chrome/browser/sync/glue/model_associator_mock.h"
+#include "chrome/browser/sync/profile_sync_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/notification_service.h"
+#include "chrome/common/notification_source.h"
+#include "chrome/common/notification_type.h"
+#include "chrome/test/profile_mock.h"
+#include "testing/gmock/include/gmock/gmock.h"
+
+using base::WaitableEvent;
+using browser_sync::AutofillDataTypeController;
+using browser_sync::ChangeProcessorMock;
+using browser_sync::DataTypeController;
+using browser_sync::ModelAssociatorMock;
+using testing::_;
+using testing::DoAll;
+using testing::Invoke;
+using testing::Return;
+using testing::SetArgumentPointee;
+
+namespace {
+
+ACTION_P(WaitOnEvent, event) {
+  event->Wait();
+}
+
+ACTION_P(SignalEvent, event) {
+  event->Signal();
+}
+
+class StartCallback {
+ public:
+  MOCK_METHOD1(Run, void(DataTypeController::StartResult result));
+};
+
+class PersonalDataManagerMock : public PersonalDataManager {
+ public:
+  MOCK_CONST_METHOD0(IsDataLoaded, bool());
+};
+
+class WebDataServiceFake : public WebDataService {
+ public:
+  WebDataServiceFake(bool is_database_loaded)
+      : is_database_loaded_(is_database_loaded) {}
+
+  virtual bool IsDatabaseLoaded() {
+    return is_database_loaded_;
+  }
+ private:
+  bool is_database_loaded_;
+};
+
+class SignalEventTask : public Task {
+ public:
+  SignalEventTask(WaitableEvent* done_event) : done_event_(done_event) {}
+
+  virtual void Run() {
+    done_event_->Signal();
+  }
+
+ private:
+  WaitableEvent* done_event_;
+};
+
+class AutofillDataTypeControllerTest : public testing::Test {
+ public:
+  AutofillDataTypeControllerTest()
+      : ui_thread_(ChromeThread::UI, &message_loop_),
+        db_thread_(ChromeThread::DB) {}
+
+  virtual void SetUp() {
+    db_thread_.Start();
+    web_data_service_ = new WebDataServiceFake(true);
+    autofill_dtc_ =
+        new AutofillDataTypeController(&profile_sync_factory_,
+                                       &profile_,
+                                       &service_);
+  }
+
+  virtual void TearDown() {
+    WaitForEmptyDBMessageLoop();
+    db_thread_.Stop();
+  }
+
+ protected:
+  void SetStartExpectations() {
+    EXPECT_CALL(profile_, GetPersonalDataManager()).
+        WillRepeatedly(Return(&personal_data_manager_));
+    EXPECT_CALL(personal_data_manager_, IsDataLoaded()).
+        WillRepeatedly(Return(true));
+    EXPECT_CALL(profile_, GetWebDataService(_)).
+        WillOnce(Return(web_data_service_.get()));
+  }
+
+  void SetAssociateExpectations() {
+    model_associator_ = new ModelAssociatorMock();
+    change_processor_ = new ChangeProcessorMock();
+    EXPECT_CALL(profile_sync_factory_,
+                CreateAutofillSyncComponents(_, _, _, _)).
+        WillOnce(Return(
+            ProfileSyncFactory::SyncComponents(model_associator_,
+                                               change_processor_)));
+
+    EXPECT_CALL(*model_associator_, SyncModelHasUserCreatedNodes(_)).
+        WillRepeatedly(DoAll(SetArgumentPointee<0>(true), Return(true)));
+    EXPECT_CALL(*model_associator_, AssociateModels()).
+        WillRepeatedly(Return(true));
+    EXPECT_CALL(service_, ActivateDataType(_, _));
+  }
+
+  void SetStopExpectations() {
+    EXPECT_CALL(service_, DeactivateDataType(_, _));
+    EXPECT_CALL(*model_associator_, DisassociateModels());
+  }
+
+  void WaitForEmptyDBMessageLoop() {
+    // Run a task through the DB message loop to ensure that
+    // everything before it has been run.
+    WaitableEvent done_event(false, false);
+    ChromeThread::PostTask(ChromeThread::DB,
+                           FROM_HERE,
+                           new SignalEventTask(&done_event));
+    done_event.Wait();
+  }
+
+  MessageLoopForUI message_loop_;
+  ChromeThread ui_thread_;
+  ChromeThread db_thread_;
+  scoped_refptr<AutofillDataTypeController> autofill_dtc_;
+  ProfileSyncFactoryMock profile_sync_factory_;
+  ProfileMock profile_;
+  PersonalDataManagerMock personal_data_manager_;
+  scoped_refptr<WebDataService> web_data_service_;
+  ProfileSyncServiceMock service_;
+  ModelAssociatorMock* model_associator_;
+  ChangeProcessorMock* change_processor_;
+  StartCallback start_callback_;
+};
+
+TEST_F(AutofillDataTypeControllerTest, StartPDMAndWDSReady) {
+  SetStartExpectations();
+  SetAssociateExpectations();
+  EXPECT_EQ(DataTypeController::NOT_RUNNING, autofill_dtc_->state());
+  EXPECT_CALL(start_callback_, Run(DataTypeController::OK)).
+      WillOnce(QuitUIMessageLoop());;
+  autofill_dtc_->Start(NewCallback(&start_callback_, &StartCallback::Run));
+  MessageLoop::current()->Run();
+  EXPECT_EQ(DataTypeController::RUNNING, autofill_dtc_->state());
+  SetStopExpectations();
+  autofill_dtc_->Stop();
+  EXPECT_EQ(DataTypeController::NOT_RUNNING, autofill_dtc_->state());
+}
+
+TEST_F(AutofillDataTypeControllerTest, AbortWhileSPDStarting) {
+  EXPECT_CALL(profile_, GetPersonalDataManager()).
+      WillRepeatedly(Return(&personal_data_manager_));
+  EXPECT_CALL(personal_data_manager_, IsDataLoaded()).
+      WillRepeatedly(Return(false));
+  autofill_dtc_->Start(NewCallback(&start_callback_, &StartCallback::Run));
+  EXPECT_EQ(DataTypeController::MODEL_STARTING, autofill_dtc_->state());
+
+  EXPECT_CALL(service_, DeactivateDataType(_, _)).Times(0);
+  EXPECT_CALL(start_callback_, Run(DataTypeController::ABORTED));
+  autofill_dtc_->Stop();
+  EXPECT_EQ(DataTypeController::NOT_RUNNING, autofill_dtc_->state());
+}
+
+TEST_F(AutofillDataTypeControllerTest, AbortWhileWDSStarting) {
+  EXPECT_CALL(profile_, GetPersonalDataManager()).
+      WillRepeatedly(Return(&personal_data_manager_));
+  EXPECT_CALL(personal_data_manager_, IsDataLoaded()).
+      WillRepeatedly(Return(true));
+  scoped_refptr<WebDataServiceFake> web_data_service_not_loaded =
+      new WebDataServiceFake(false);
+  EXPECT_CALL(profile_, GetWebDataService(_)).
+      WillOnce(Return(web_data_service_not_loaded.get()));
+  autofill_dtc_->Start(NewCallback(&start_callback_, &StartCallback::Run));
+  EXPECT_EQ(DataTypeController::MODEL_STARTING, autofill_dtc_->state());
+
+  EXPECT_CALL(service_, DeactivateDataType(_, _)).Times(0);
+  EXPECT_CALL(start_callback_, Run(DataTypeController::ABORTED));
+  autofill_dtc_->Stop();
+  EXPECT_EQ(DataTypeController::NOT_RUNNING, autofill_dtc_->state());
+}
+
+TEST_F(AutofillDataTypeControllerTest, AbortWhileAssociating) {
+  SetStartExpectations();
+
+  model_associator_ = new ModelAssociatorMock();
+  change_processor_ = new ChangeProcessorMock();
+  EXPECT_CALL(profile_sync_factory_,
+              CreateAutofillSyncComponents(_, _, _, _)).
+      WillOnce(Return(
+          ProfileSyncFactory::SyncComponents(model_associator_,
+                                             change_processor_)));
+
+  WaitableEvent pause_db_thread(false, false);
+  EXPECT_CALL(*model_associator_, SyncModelHasUserCreatedNodes(_)).
+      WillOnce(DoAll(
+          WaitOnEvent(&pause_db_thread),
+          SetArgumentPointee<0>(true),
+          Return(false)));
+  EXPECT_CALL(*model_associator_, Shutdown()).
+      WillOnce(SignalEvent(&pause_db_thread));
+
+  autofill_dtc_->Start(NewCallback(&start_callback_, &StartCallback::Run));
+  EXPECT_EQ(DataTypeController::ASSOCIATING, autofill_dtc_->state());
+
+  EXPECT_CALL(service_, DeactivateDataType(_, _)).Times(0);
+  EXPECT_CALL(start_callback_, Run(DataTypeController::ABORTED));
+  autofill_dtc_->Stop();
+  EXPECT_EQ(DataTypeController::NOT_RUNNING, autofill_dtc_->state());
+}
+
+}  // namespace
diff --git a/chrome/browser/sync/glue/autofill_model_associator.cc b/chrome/browser/sync/glue/autofill_model_associator.cc
index ca3ae96..ec76cf5 100644
--- a/chrome/browser/sync/glue/autofill_model_associator.cc
+++ b/chrome/browser/sync/glue/autofill_model_associator.cc
@@ -38,7 +38,8 @@ AutofillModelAssociator::AutofillModelAssociator(
       web_database_(web_database),
       personal_data_(personal_data),
       error_handler_(error_handler),
-      autofill_node_id_(sync_api::kInvalidId) {
+      autofill_node_id_(sync_api::kInvalidId),
+      shutdown_(false) {
   DCHECK(ChromeThread::CurrentlyOn(ChromeThread::DB));
   DCHECK(sync_service_);
   DCHECK(web_database_);
@@ -60,6 +61,8 @@ bool AutofillModelAssociator::TraverseAndAssociateChromeAutofillEntries(
   const std::vector<AutofillEntry>& entries = all_entries_from_db;
   for (std::vector<AutofillEntry>::const_iterator ix = entries.begin();
        ix != entries.end(); ++ix) {
+    if (ShouldShutdown())
+      return false;
     std::string tag = KeyToTag(ix->key().name(), ix->key().value());
     if (id_map_.find(tag) != id_map_.end()) {
       // It seems that name/value pairs are not unique in the web database.
@@ -114,6 +117,8 @@ bool AutofillModelAssociator::TraverseAndAssociateChromeAutoFillProfiles(
   const std::vector<AutoFillProfile*>& profiles = all_profiles_from_db;
   for (std::vector<AutoFillProfile*>::const_iterator ix = profiles.begin();
        ix != profiles.end(); ++ix) {
+    if (ShouldShutdown())
+      return false;
     string16 label((*ix)->Label());
     std::string tag(ProfileLabelToTag(label));
 
@@ -262,11 +267,15 @@ bool AutofillModelAssociator::SaveChangesToWebData(const DataBundle& bundle) {
   }
 
   for (size_t i = 0; i < bundle.new_profiles.size(); i++) {
+    if (ShouldShutdown())
+      return false;
     if (!web_database_->AddAutoFillProfile(*bundle.new_profiles[i]))
       return false;
   }
 
   for (size_t i = 0; i < bundle.updated_profiles.size(); i++) {
+    if (ShouldShutdown())
+      return false;
     if (!web_database_->UpdateAutoFillProfile(*bundle.updated_profiles[i]))
       return false;
   }
@@ -281,6 +290,8 @@ bool AutofillModelAssociator::TraverseAndAssociateAllSyncNodes(
 
   int64 sync_child_id = autofill_root.GetFirstChildId();
   while (sync_child_id != sync_api::kInvalidId) {
+    if (ShouldShutdown())
+      return false;
     sync_api::ReadNode sync_child(write_trans);
     if (!sync_child.InitByIdLookup(sync_child_id)) {
       LOG(ERROR) << "Failed to fetch child node.";
@@ -373,6 +384,12 @@ bool AutofillModelAssociator::ChromeModelHasUserCreatedNodes(bool* has_nodes) {
   return true;
 }
 
+void AutofillModelAssociator::Shutdown() {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
+  AutoLock lock(shutdown_lock_);
+  shutdown_ = true;
+}
+
 int64 AutofillModelAssociator::GetSyncIdFromChromeId(
     const std::string autofill) {
   AutofillToSyncIdMap::const_iterator iter = id_map_.find(autofill);
@@ -409,6 +426,11 @@ bool AutofillModelAssociator::GetSyncIdForTaggedNode(const std::string& tag,
   return true;
 }
 
+bool AutofillModelAssociator::ShouldShutdown() {
+  AutoLock lock(shutdown_lock_);
+  return shutdown_;
+}
+
 // static
 std::string AutofillModelAssociator::KeyToTag(const string16& name,
                                               const string16& value) {
diff --git a/chrome/browser/sync/glue/autofill_model_associator.h b/chrome/browser/sync/glue/autofill_model_associator.h
index b1869b9..9dcf899 100644
--- a/chrome/browser/sync/glue/autofill_model_associator.h
+++ b/chrome/browser/sync/glue/autofill_model_associator.h
@@ -11,6 +11,7 @@
 #include <vector>
 
 #include "base/basictypes.h"
+#include "base/lock.h"
 #include "base/scoped_ptr.h"
 #include "chrome/browser/autofill/personal_data_manager.h"
 #include "chrome/browser/chrome_thread.h"
@@ -80,6 +81,10 @@ class AutofillModelAssociator
   // user-defined autofill entries.
   virtual bool ChromeModelHasUserCreatedNodes(bool* has_nodes);
 
+  // Call this from the main thread if the autofill model associator
+  // should stop what it is doing and shutdown.
+  virtual void Shutdown();
+
   // Not implemented.
   virtual const std::string* GetChromeNodeFromSyncId(int64 sync_id) {
     return NULL;
@@ -189,6 +194,8 @@ class AutofillModelAssociator
       const AutoFillProfile& profile,
       int64* sync_id);
 
+  bool ShouldShutdown();
+
   ProfileSyncService* sync_service_;
   WebDatabase* web_database_;
   PersonalDataManager* personal_data_;
@@ -198,6 +205,11 @@ class AutofillModelAssociator
   AutofillToSyncIdMap id_map_;
   SyncIdToAutofillMap id_map_inverse_;
 
+  // Shutdown flag and lock.  If this is set to true (via the Shutdown
+  // method), return from the current method as soon as possible.
+  Lock shutdown_lock_;
+  bool shutdown_;
+
   DISALLOW_COPY_AND_ASSIGN(AutofillModelAssociator);
 };
 
diff --git a/chrome/browser/sync/glue/bookmark_data_type_controller.cc b/chrome/browser/sync/glue/bookmark_data_type_controller.cc
index e9cf83f..65f649a 100644
--- a/chrome/browser/sync/glue/bookmark_data_type_controller.cc
+++ b/chrome/browser/sync/glue/bookmark_data_type_controller.cc
@@ -73,11 +73,8 @@ void BookmarkDataTypeController::Stop() {
   DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
   // If Stop() is called while Start() is waiting for the bookmark
   // model to load, abort the start.
-  if (unrecoverable_error_detected_) {
-    FinishStart(UNRECOVERABLE_ERROR);
-  } else if (state_ == MODEL_STARTING) {
+  if (state_ == MODEL_STARTING)
     FinishStart(ABORTED);
-  }
 
   registrar_.RemoveAll();
   if (change_processor_ != NULL)
diff --git a/chrome/browser/sync/glue/bookmark_model_associator.h b/chrome/browser/sync/glue/bookmark_model_associator.h
index 9df77cd..ded5f2c 100644
--- a/chrome/browser/sync/glue/bookmark_model_associator.h
+++ b/chrome/browser/sync/glue/bookmark_model_associator.h
@@ -82,6 +82,11 @@ class BookmarkModelAssociator
   // Remove the association that corresponds to the given sync id.
   virtual void Disassociate(int64 sync_id);
 
+  virtual void Shutdown() {
+    // No implementation needed, this associator runs on the main
+    // thread.
+  }
+
  protected:
   // Stores the id of the node with the given tag in |sync_id|.
   // Returns of that node was found successfully.
diff --git a/chrome/browser/sync/glue/change_processor.cc b/chrome/browser/sync/glue/change_processor.cc
index 0c608b8..6b25382 100644
--- a/chrome/browser/sync/glue/change_processor.cc
+++ b/chrome/browser/sync/glue/change_processor.cc
@@ -8,7 +8,7 @@
 namespace browser_sync {
 
 ChangeProcessor::~ChangeProcessor() {
-  Stop();
+  DCHECK(!running_) << "ChangeProcessor dtor while running";
 }
 
 void ChangeProcessor::Start(Profile* profile,
diff --git a/chrome/browser/sync/glue/model_associator.h b/chrome/browser/sync/glue/model_associator.h
index b7f7622..a000291 100644
--- a/chrome/browser/sync/glue/model_associator.h
+++ b/chrome/browser/sync/glue/model_associator.h
@@ -43,6 +43,11 @@ class AssociatorInterface {
   // has user-created nodes.  The method may return false if an error
   // occurred.
   virtual bool ChromeModelHasUserCreatedNodes(bool* has_nodes) = 0;
+
+  // For associators that work off the main thread, calling this
+  // method will signal that they must stop and return "false" from
+  // any method they are currently in.
+  virtual void Shutdown() = 0;
 };
 
 // In addition to the generic methods, association can refer to operations
diff --git a/chrome/browser/sync/glue/model_associator_mock.h b/chrome/browser/sync/glue/model_associator_mock.h
index 6efe02d..9b33f72 100644
--- a/chrome/browser/sync/glue/model_associator_mock.h
+++ b/chrome/browser/sync/glue/model_associator_mock.h
@@ -16,6 +16,7 @@ class ModelAssociatorMock : public AssociatorInterface {
   MOCK_METHOD0(DisassociateModels, bool());
   MOCK_METHOD1(SyncModelHasUserCreatedNodes, bool(bool*));
   MOCK_METHOD1(ChromeModelHasUserCreatedNodes, bool(bool*));
+  MOCK_METHOD0(Shutdown, void());
 };
 
 }  // namespace browser_sync
diff --git a/chrome/browser/sync/glue/preference_data_type_controller.cc b/chrome/browser/sync/glue/preference_data_type_controller.cc
index 75b964f..6309d94d 100644
--- a/chrome/browser/sync/glue/preference_data_type_controller.cc
+++ b/chrome/browser/sync/glue/preference_data_type_controller.cc
@@ -73,9 +73,6 @@ void PreferenceDataTypeController::Start(StartCallback* start_callback) {
 
 void PreferenceDataTypeController::Stop() {
   DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
-  if (unrecoverable_error_detected_) {
-    FinishStart(UNRECOVERABLE_ERROR);
-  }
 
   if (change_processor_ != NULL)
     sync_service_->DeactivateDataType(this, change_processor_.get());
diff --git a/chrome/browser/sync/glue/preference_model_associator.h b/chrome/browser/sync/glue/preference_model_associator.h
index 7495b59..de04e90 100644
--- a/chrome/browser/sync/glue/preference_model_associator.h
+++ b/chrome/browser/sync/glue/preference_model_associator.h
@@ -56,6 +56,11 @@ class PreferenceModelAssociator
   // Returns whether the preference model has any user-defined preferences.
   virtual bool ChromeModelHasUserCreatedNodes(bool* has_nodes);
 
+  virtual void Shutdown() {
+    // No implementation needed, this associator runs on the main
+    // thread.
+  }
+
   // Not implemented.
   virtual const PrefService::Preference* GetChromeNodeFromSyncId(
       int64 sync_id) {
diff --git a/chrome/browser/sync/glue/theme_model_associator.h b/chrome/browser/sync/glue/theme_model_associator.h
index 12cbc62..8c90f75 100644
--- a/chrome/browser/sync/glue/theme_model_associator.h
+++ b/chrome/browser/sync/glue/theme_model_associator.h
@@ -31,6 +31,10 @@ class ThemeModelAssociator : public AssociatorInterface {
   virtual bool DisassociateModels();
   virtual bool SyncModelHasUserCreatedNodes(bool* has_nodes);
   virtual bool ChromeModelHasUserCreatedNodes(bool* has_nodes);
+  virtual void Shutdown() {
+    // No implementation needed, this associator runs on the main
+    // thread.
+  }
 
  private:
   ProfileSyncService* sync_service_;
diff --git a/chrome/browser/sync/glue/typed_url_model_associator.h b/chrome/browser/sync/glue/typed_url_model_associator.h
index b3087c6..65a0d9c 100644
--- a/chrome/browser/sync/glue/typed_url_model_associator.h
+++ b/chrome/browser/sync/glue/typed_url_model_associator.h
@@ -77,6 +77,10 @@ class TypedUrlModelAssociator
   // user-defined typed_url entries.
   virtual bool ChromeModelHasUserCreatedNodes(bool* has_nodes);
 
+  virtual void Shutdown() {
+    // TODO(zork): Implement this.
+  }
+
   // Not implemented.
   virtual const std::string* GetChromeNodeFromSyncId(int64 sync_id) {
     return NULL;
diff --git a/chrome/chrome_tests.gypi b/chrome/chrome_tests.gypi
index 5c5f7d8..85fd8de 100644
--- a/chrome/chrome_tests.gypi
+++ b/chrome/chrome_tests.gypi
@@ -890,6 +890,7 @@
         'browser/ssl/ssl_host_state_unittest.cc',
         'browser/status_icons/status_icon_unittest.cc',
         'browser/status_icons/status_tray_unittest.cc',
+        'browser/sync/glue/autofill_data_type_controller_unittest.cc',
         'browser/sync/glue/autofill_model_associator_unittest.cc',
         'browser/sync/glue/bookmark_data_type_controller_unittest.cc',
         'browser/sync/glue/change_processor_mock.h',