AutofillDataTypeController should invoke start callback on abort. Also gracefully handle stopping the data type startup while waiting for the PDM, WDS, and association.

BUG=41361
TEST=unittest

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

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

17 files changed, 425 insertions, 34 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..1d436ca 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,7 +102,33 @@ void AutofillDataTypeController::Stop() {
   LOG(INFO) << "Stopping autofill data type controller.";
   DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
 
-  if (change_processor_ != NULL)
+  // If Stop() is called while Start() is waiting for association to
+  // complete, we need to abort the association and wait for the DB
+  // thread to finish the StartImpl() task.
+  if (state_ == ASSOCIATING) {
+    {
+      AutoLock lock(abort_association_lock_);
+      abort_association_ = true;
+      if (model_associator_.get())
+        model_associator_->AbortAssociation();
+    }
+    // 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
+  // data manager 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 (deactivate and
+  // disassociate) 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 && change_processor_->IsRunning())
     sync_service_->DeactivateDataType(this, change_processor_.get());
 
   if (model_associator_ != NULL)
@@ -119,14 +146,21 @@ void AutofillDataTypeController::StartImpl() {
   DCHECK(ChromeThread::CurrentlyOn(ChromeThread::DB));
   // No additional services need to be started before we can proceed
   // with model association.
-  ProfileSyncFactory::SyncComponents sync_components =
-      profile_sync_factory_->CreateAutofillSyncComponents(
-          sync_service_,
-          web_data_service_->GetDatabase(),
-          profile_->GetPersonalDataManager(),
-          this);
-  model_associator_.reset(sync_components.model_associator);
-  change_processor_.reset(sync_components.change_processor);
+  {
+    AutoLock lock(abort_association_lock_);
+    if (abort_association_) {
+      abort_association_complete_.Signal();
+      return;
+    }
+    ProfileSyncFactory::SyncComponents sync_components =
+        profile_sync_factory_->CreateAutofillSyncComponents(
+            sync_service_,
+            web_data_service_->GetDatabase(),
+            profile_->GetPersonalDataManager(),
+            this);
+    model_associator_.reset(sync_components.model_associator);
+    change_processor_.reset(sync_components.change_processor);
+  }
 
   bool sync_has_nodes = false;
   if (!model_associator_->SyncModelHasUserCreatedNodes(&sync_has_nodes)) {
@@ -152,12 +186,17 @@ 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));
+
+  abort_association_complete_.Signal();
+  AutoLock lock(abort_association_lock_);
+  if (!abort_association_) {
+    ChromeThread::PostTask(ChromeThread::UI, FROM_HERE,
+                           NewRunnableMethod(
+                               this,
+                               &AutofillDataTypeController::StartDoneImpl,
+                               result,
+                               new_state));
+  }
 }
 
 void AutofillDataTypeController::StartDoneImpl(
@@ -165,6 +204,7 @@ void AutofillDataTypeController::StartDoneImpl(
     DataTypeController::State new_state) {
   LOG(INFO) << "Autofill data type controller StartDoneImpl called.";
   DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
+
   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..854ea80
--- /dev/null
+++ b/chrome/browser/sync/glue/autofill_data_type_controller_unittest.cc
@@ -0,0 +1,282 @@
+// 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:
+  explicit 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:
+  explicit 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(_, _));
+    EXPECT_CALL(*change_processor_, IsRunning()).WillRepeatedly(Return(true));
+  }
+
+  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, AbortWhilePDMStarting) {
+  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, AbortWhileAssociatingNotActivated) {
+  SetStartExpectations();
+
+  model_associator_ = new ModelAssociatorMock();
+  change_processor_ = new ChangeProcessorMock();
+  EXPECT_CALL(profile_sync_factory_,
+              CreateAutofillSyncComponents(_, _, _, _)).
+      WillOnce(Return(
+          ProfileSyncFactory::SyncComponents(model_associator_,
+                                             change_processor_)));
+
+  // Use the pause_db_thread WaitableEvent to pause the DB thread when
+  // the model association process reaches the
+  // SyncModelHasUserCreatedNodes method.  This lets us call Stop()
+  // while model association is in progress.  When we call Stop(),
+  // this will call the model associator's AbortAssociation() method
+  // that signals the event and lets the DB thread continue.
+  WaitableEvent pause_db_thread(false, false);
+  WaitableEvent wait_for_db_thread_pause(false, false);
+  EXPECT_CALL(*model_associator_, SyncModelHasUserCreatedNodes(_)).
+      WillOnce(DoAll(
+          SignalEvent(&wait_for_db_thread_pause),
+          WaitOnEvent(&pause_db_thread),
+          SetArgumentPointee<0>(true),
+          Return(false)));
+  EXPECT_CALL(*model_associator_, AbortAssociation()).
+      WillOnce(SignalEvent(&pause_db_thread));
+
+  autofill_dtc_->Start(NewCallback(&start_callback_, &StartCallback::Run));
+  wait_for_db_thread_pause.Wait();
+  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());
+}
+
+TEST_F(AutofillDataTypeControllerTest, AbortWhileAssociatingActivated) {
+  SetStartExpectations();
+
+  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(*change_processor_, IsRunning()).WillRepeatedly(Return(true));
+
+  // The usage of pause_db_thread is similar as in the previous test,
+  // but here we will wait until the DB thread calls
+  // ActivateDataType() before  allowing it to continue.
+  WaitableEvent pause_db_thread(false, false);
+  WaitableEvent wait_for_db_thread_pause(false, false);
+  EXPECT_CALL(service_, ActivateDataType(_, _)).
+      WillOnce(DoAll(
+          SignalEvent(&wait_for_db_thread_pause),
+          WaitOnEvent(&pause_db_thread)));
+  EXPECT_CALL(*model_associator_, AbortAssociation()).
+      WillOnce(SignalEvent(&pause_db_thread));
+
+  autofill_dtc_->Start(NewCallback(&start_callback_, &StartCallback::Run));
+  wait_for_db_thread_pause.Wait();
+  EXPECT_EQ(DataTypeController::ASSOCIATING, autofill_dtc_->state());
+
+  SetStopExpectations();
+  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..b22970c 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),
+      abort_association_pending_(false) {
   DCHECK(ChromeThread::CurrentlyOn(ChromeThread::DB));
   DCHECK(sync_service_);
   DCHECK(web_database_);
@@ -195,9 +196,13 @@ bool AutofillModelAssociator::MakeNewAutofillProfileSyncNode(
 bool AutofillModelAssociator::LoadAutofillData(
     std::vector<AutofillEntry>* entries,
     std::vector<AutoFillProfile*>* profiles) {
+  if (IsAbortPending())
+    return false;
   if (!web_database_->GetAllAutofillEntries(entries))
     return false;
 
+  if (IsAbortPending())
+    return false;
   if (!web_database_->GetAutoFillProfiles(profiles))
     return false;
 
@@ -207,6 +212,10 @@ bool AutofillModelAssociator::LoadAutofillData(
 bool AutofillModelAssociator::AssociateModels() {
   LOG(INFO) << "Associating Autofill Models";
   DCHECK(ChromeThread::CurrentlyOn(ChromeThread::DB));
+  {
+    AutoLock lock(abort_association_pending_lock_);
+    abort_association_pending_ = false;
+  }
 
   // TODO(zork): Attempt to load the model association from storage.
   std::vector<AutofillEntry> entries;
@@ -256,17 +265,25 @@ bool AutofillModelAssociator::AssociateModels() {
 
 bool AutofillModelAssociator::SaveChangesToWebData(const DataBundle& bundle) {
   DCHECK(ChromeThread::CurrentlyOn(ChromeThread::DB));
+
+  if (IsAbortPending())
+    return false;
+
   if (bundle.new_entries.size() &&
       !web_database_->UpdateAutofillEntries(bundle.new_entries)) {
     return false;
   }
 
   for (size_t i = 0; i < bundle.new_profiles.size(); i++) {
+    if (IsAbortPending())
+      return false;
     if (!web_database_->AddAutoFillProfile(*bundle.new_profiles[i]))
       return false;
   }
 
   for (size_t i = 0; i < bundle.updated_profiles.size(); i++) {
+    if (IsAbortPending())
+      return false;
     if (!web_database_->UpdateAutoFillProfile(*bundle.updated_profiles[i]))
       return false;
   }
@@ -373,6 +390,12 @@ bool AutofillModelAssociator::ChromeModelHasUserCreatedNodes(bool* has_nodes) {
   return true;
 }
 
+void AutofillModelAssociator::AbortAssociation() {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
+  AutoLock lock(abort_association_pending_lock_);
+  abort_association_pending_ = true;
+}
+
 int64 AutofillModelAssociator::GetSyncIdFromChromeId(
     const std::string autofill) {
   AutofillToSyncIdMap::const_iterator iter = id_map_.find(autofill);
@@ -409,6 +432,11 @@ bool AutofillModelAssociator::GetSyncIdForTaggedNode(const std::string& tag,
   return true;
 }
 
+bool AutofillModelAssociator::IsAbortPending() {
+  AutoLock lock(abort_association_pending_lock_);
+  return abort_association_pending_;
+}
+
 // 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..cfaddd9 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,9 @@ class AutofillModelAssociator
   // user-defined autofill entries.
   virtual bool ChromeModelHasUserCreatedNodes(bool* has_nodes);
 
+  // See ModelAssociator interface.
+  virtual void AbortAssociation();
+
   // Not implemented.
   virtual const std::string* GetChromeNodeFromSyncId(int64 sync_id) {
     return NULL;
@@ -189,6 +193,10 @@ class AutofillModelAssociator
       const AutoFillProfile& profile,
       int64* sync_id);
 
+  // Called at various points in model association to determine if the
+  // user requested an abort.
+  bool IsAbortPending();
+
   ProfileSyncService* sync_service_;
   WebDatabase* web_database_;
   PersonalDataManager* personal_data_;
@@ -198,6 +206,12 @@ class AutofillModelAssociator
   AutofillToSyncIdMap id_map_;
   SyncIdToAutofillMap id_map_inverse_;
 
+  // Abort association pending flag and lock.  If this is set to true
+  // (via the AbortAssociation method), return from the
+  // AssociateModels method as soon as possible.
+  Lock abort_association_pending_lock_;
+  bool abort_association_pending_;
+
   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 c109a16..92abb7a 100644
--- a/chrome/browser/sync/glue/bookmark_data_type_controller.cc
+++ b/chrome/browser/sync/glue/bookmark_data_type_controller.cc
@@ -26,8 +26,7 @@ BookmarkDataTypeController::BookmarkDataTypeController(
     : profile_sync_factory_(profile_sync_factory),
       profile_(profile),
       sync_service_(sync_service),
-      state_(NOT_RUNNING),
-      unrecoverable_error_detected_(false) {
+      state_(NOT_RUNNING) {
   DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
   DCHECK(profile_sync_factory);
   DCHECK(profile);
@@ -41,7 +40,6 @@ BookmarkDataTypeController::~BookmarkDataTypeController() {
 void BookmarkDataTypeController::Start(StartCallback* start_callback) {
   DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
   DCHECK(start_callback);
-  unrecoverable_error_detected_ = false;
   if (state_ != NOT_RUNNING) {
     start_callback->Run(BUSY);
     delete start_callback;
@@ -73,10 +71,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 (state_ == MODEL_STARTING) {
-    FinishStart(unrecoverable_error_detected_ ?
-                UNRECOVERABLE_ERROR : ABORTED);
-  }
+  if (state_ == MODEL_STARTING)
+    FinishStart(ABORTED);
 
   registrar_.RemoveAll();
   if (change_processor_ != NULL)
@@ -92,7 +88,6 @@ void BookmarkDataTypeController::Stop() {
 }
 
 void BookmarkDataTypeController::OnUnrecoverableError() {
-  unrecoverable_error_detected_ = true;
   // The ProfileSyncService will invoke our Stop() method in response to this.
   UMA_HISTOGRAM_COUNTS("Sync.BookmarkRunFailures", 1);
   sync_service_->OnUnrecoverableError();
diff --git a/chrome/browser/sync/glue/bookmark_data_type_controller.h b/chrome/browser/sync/glue/bookmark_data_type_controller.h
index 33a0089..ecb6255 100644
--- a/chrome/browser/sync/glue/bookmark_data_type_controller.h
+++ b/chrome/browser/sync/glue/bookmark_data_type_controller.h
@@ -82,7 +82,6 @@ class BookmarkDataTypeController : public DataTypeController,
   ProfileSyncService* sync_service_;
 
   State state_;
-  bool unrecoverable_error_detected_;
 
   scoped_ptr<StartCallback> start_callback_;
   scoped_ptr<AssociatorInterface> model_associator_;
diff --git a/chrome/browser/sync/glue/bookmark_model_associator.h b/chrome/browser/sync/glue/bookmark_model_associator.h
index 9df77cd..e4bf930 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 AbortAssociation() {
+    // 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/change_processor.h b/chrome/browser/sync/glue/change_processor.h
index 21052a7..7e020e6 100644
--- a/chrome/browser/sync/glue/change_processor.h
+++ b/chrome/browser/sync/glue/change_processor.h
@@ -33,7 +33,7 @@ class ChangeProcessor {
   // |StartImpl|.
   void Start(Profile* profile, sync_api::UserShare* share_handle);
   void Stop();
-  bool IsRunning() const { return running_; }
+  virtual bool IsRunning() const { return running_; }
 
   // Changes have been applied to the backend model and are ready to be
   // applied to the frontend model. See syncapi.h for detailed instructions on
diff --git a/chrome/browser/sync/glue/change_processor_mock.h b/chrome/browser/sync/glue/change_processor_mock.h
index 42bad36..2860c4b 100644
--- a/chrome/browser/sync/glue/change_processor_mock.h
+++ b/chrome/browser/sync/glue/change_processor_mock.h
@@ -22,6 +22,7 @@ class ChangeProcessorMock : public ChangeProcessor {
                     int change_count));
   MOCK_METHOD1(StartImpl, void(Profile* profile));
   MOCK_METHOD0(StopImpl, void());
+  MOCK_CONST_METHOD0(IsRunning, bool());
 };
 
 }  // namespace browser_sync
diff --git a/chrome/browser/sync/glue/model_associator.h b/chrome/browser/sync/glue/model_associator.h
index b7f7622..70ff078 100644
--- a/chrome/browser/sync/glue/model_associator.h
+++ b/chrome/browser/sync/glue/model_associator.h
@@ -43,6 +43,13 @@ class AssociatorInterface {
   // has user-created nodes.  The method may return false if an error
   // occurred.
   virtual bool ChromeModelHasUserCreatedNodes(bool* has_nodes) = 0;
+
+  // Calling this method while AssociateModels() is in progress will
+  // cause the method to exit early with a "false" return value.  This
+  // is useful for aborting model associations for shutdown.  This
+  // method is only implemented for model associators that are invoked
+  // off the main thread.
+  virtual void AbortAssociation() = 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..f8ffae7 100644
--- a/chrome/browser/sync/glue/model_associator_mock.h
+++ b/chrome/browser/sync/glue/model_associator_mock.h
@@ -14,8 +14,9 @@ class ModelAssociatorMock : public AssociatorInterface {
  public:
   MOCK_METHOD0(AssociateModels, bool());
   MOCK_METHOD0(DisassociateModels, bool());
-  MOCK_METHOD1(SyncModelHasUserCreatedNodes, bool(bool*));
-  MOCK_METHOD1(ChromeModelHasUserCreatedNodes, bool(bool*));
+  MOCK_METHOD1(SyncModelHasUserCreatedNodes, bool(bool* has_nodes));
+  MOCK_METHOD1(ChromeModelHasUserCreatedNodes, bool(bool* has_nodes));
+  MOCK_METHOD0(AbortAssociation, void());
 };
 
 }  // namespace browser_sync
diff --git a/chrome/browser/sync/glue/preference_model_associator.h b/chrome/browser/sync/glue/preference_model_associator.h
index 7495b59..a449b3c 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 AbortAssociation() {
+    // 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..cb0ae6d 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 AbortAssociation() {
+    // 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..82f4fa6 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 AbortAssociation() {
+    // 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',