Refactor synchronous error handling

This is the first of hopefully only two changes to improve our error handling situation.  This change changes the way we use OnUnrecoverableError -- previously this was called whenever there was some kind of error in the MA or CP, but now it is only used for asynchronous errors.  It has been removed from synchronous methods in favor of boolean return values to report errors.  This simplifies some of the error handling.

The next step is to make sure asynchronous usage of OnUnrecoverableError actually works as it should.

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

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

21 files changed, 283 insertions, 298 deletions

diff --git a/chrome/browser/sync/glue/autofill_data_type_controller.cc b/chrome/browser/sync/glue/autofill_data_type_controller.cc
index 5d72603..3857459 100644
--- a/chrome/browser/sync/glue/autofill_data_type_controller.cc
+++ b/chrome/browser/sync/glue/autofill_data_type_controller.cc
@@ -39,7 +39,8 @@ void AutofillDataTypeController::Start(bool merge_allowed,
                                        StartCallback* start_callback) {
   LOG(INFO) << "Starting autofill data controller.";
   DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
-  if (state_ != NOT_RUNNING || start_callback_.get()) {
+  DCHECK(start_callback);
+  if (state() != NOT_RUNNING) {
     start_callback->Run(BUSY);
     delete start_callback;
     return;
@@ -50,12 +51,14 @@ void AutofillDataTypeController::Start(bool merge_allowed,
 
   web_data_service_ = profile_->GetWebDataService(Profile::IMPLICIT_ACCESS);
   if (web_data_service_.get() && web_data_service_->IsDatabaseLoaded()) {
+    set_state(ASSOCIATING);
     ChromeThread::PostTask(ChromeThread::DB, FROM_HERE,
                            NewRunnableMethod(
                                this,
                                &AutofillDataTypeController::StartImpl,
                                merge_allowed_));
   } else {
+    set_state(MODEL_STARTING);
     notification_registrar_.Add(this, NotificationType::WEB_DATABASE_LOADED,
                                 NotificationService::AllSources());
   }
@@ -86,6 +89,7 @@ void AutofillDataTypeController::Stop() {
   if (model_associator_ != NULL)
     model_associator_->DisassociateModels();
 
+  set_state(NOT_RUNNING);
   ChromeThread::PostTask(ChromeThread::DB, FROM_HERE,
                          NewRunnableMethod(
                              this,
@@ -104,49 +108,55 @@ void AutofillDataTypeController::StartImpl(bool merge_allowed) {
           this);
   model_associator_.reset(sync_components.model_associator);
   change_processor_.reset(sync_components.change_processor);
-  bool needs_merge =  model_associator_->ChromeModelHasUserCreatedNodes() &&
-      model_associator_->SyncModelHasUserCreatedNodes();
-  if (needs_merge && !merge_allowed) {
-    model_associator_.reset();
-    change_processor_.reset();
-    StartDone(NEEDS_MERGE);
+
+  bool chrome_has_nodes = false;
+  if (!model_associator_->ChromeModelHasUserCreatedNodes(&chrome_has_nodes)) {
+    StartFailed(UNRECOVERABLE_ERROR);
+    return;
+  }
+  bool sync_has_nodes = false;
+  if (!model_associator_->SyncModelHasUserCreatedNodes(&sync_has_nodes)) {
+    StartFailed(UNRECOVERABLE_ERROR);
+    return;
+  }
+
+  if (chrome_has_nodes && sync_has_nodes && !merge_allowed) {
+    StartFailed(NEEDS_MERGE);
     return;
   }
 
   base::TimeTicks start_time = base::TimeTicks::Now();
-  bool first_run = !model_associator_->SyncModelHasUserCreatedNodes();
   bool merge_success = model_associator_->AssociateModels();
   UMA_HISTOGRAM_TIMES("Sync.AutofillAssociationTime",
                       base::TimeTicks::Now() - start_time);
   if (!merge_success) {
-    model_associator_.reset();
-    change_processor_.reset();
-    StartDone(ASSOCIATION_FAILED);
+    StartFailed(NEEDS_MERGE);
     return;
   }
 
   sync_service_->ActivateDataType(this, change_processor_.get());
-  state_ = RUNNING;
-
-  StartDone(first_run ? OK_FIRST_RUN : OK);
+  StartDone(!sync_has_nodes ? OK_FIRST_RUN : OK, RUNNING);
 }
 
 void AutofillDataTypeController::StartDone(
-    DataTypeController::StartResult result) {
+    DataTypeController::StartResult result,
+    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));
+                             result,
+                             new_state));
 }
 
 void AutofillDataTypeController::StartDoneImpl(
-    DataTypeController::StartResult result) {
+    DataTypeController::StartResult result,
+    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();
 }
@@ -157,11 +167,17 @@ void AutofillDataTypeController::StopImpl() {
 
   change_processor_.reset();
   model_associator_.reset();
+}
 
-  state_ = NOT_RUNNING;
+void AutofillDataTypeController::StartFailed(StartResult result) {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::DB));
+  change_processor_.reset();
+  model_associator_.reset();
+  StartDone(result, NOT_RUNNING);
 }
 
 void AutofillDataTypeController::OnUnrecoverableError() {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::DB));
   ChromeThread::PostTask(
     ChromeThread::UI, FROM_HERE,
     NewRunnableMethod(this,
diff --git a/chrome/browser/sync/glue/autofill_data_type_controller.h b/chrome/browser/sync/glue/autofill_data_type_controller.h
index 9c73c81d..4e088d9 100644
--- a/chrome/browser/sync/glue/autofill_data_type_controller.h
+++ b/chrome/browser/sync/glue/autofill_data_type_controller.h
@@ -52,6 +52,7 @@ class AutofillDataTypeController : public DataTypeController,
   }
 
   virtual State state() {
+    DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
     return state_;
   }
 
@@ -65,11 +66,17 @@ class AutofillDataTypeController : public DataTypeController,
 
  private:
   void StartImpl(bool merge_allowed);
-  void StartDone(DataTypeController::StartResult result);
-  void StartDoneImpl(DataTypeController::StartResult result);
+  void StartDone(StartResult result, State state);
+  void StartDoneImpl(StartResult result, State state);
   void StopImpl();
+  void StartFailed(StartResult result);
   void OnUnrecoverableErrorImpl();
 
+  void set_state(State state) {
+    DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
+    state_ = state;
+  }
+
   ProfileSyncFactory* profile_sync_factory_;
   Profile* profile_;
   ProfileSyncService* sync_service_;
diff --git a/chrome/browser/sync/glue/autofill_model_associator.cc b/chrome/browser/sync/glue/autofill_model_associator.cc
index 499e9ab..ccda440 100644
--- a/chrome/browser/sync/glue/autofill_model_associator.cc
+++ b/chrome/browser/sync/glue/autofill_model_associator.cc
@@ -146,10 +146,11 @@ bool AutofillModelAssociator::DisassociateModels() {
   return true;
 }
 
-bool AutofillModelAssociator::SyncModelHasUserCreatedNodes() {
+bool AutofillModelAssociator::SyncModelHasUserCreatedNodes(bool* has_nodes) {
+  DCHECK(has_nodes);
+  *has_nodes = false;
   int64 autofill_sync_id;
   if (!GetSyncIdForTaggedNode(kAutofillTag, &autofill_sync_id)) {
-    error_handler_->OnUnrecoverableError();
     LOG(ERROR) << "Server did not create the top-level autofill node. We "
                << "might be running against an out-of-date server.";
     return false;
@@ -159,7 +160,6 @@ bool AutofillModelAssociator::SyncModelHasUserCreatedNodes() {
 
   sync_api::ReadNode autofill_node(&trans);
   if (!autofill_node.InitByIdLookup(autofill_sync_id)) {
-    error_handler_->OnUnrecoverableError();
     LOG(ERROR) << "Server did not create the top-level autofill node. We "
                << "might be running against an out-of-date server.";
     return false;
@@ -167,11 +167,14 @@ bool AutofillModelAssociator::SyncModelHasUserCreatedNodes() {
 
   // The sync model has user created nodes if the autofill folder has any
   // children.
-  return sync_api::kInvalidId != autofill_node.GetFirstChildId();
+  *has_nodes = sync_api::kInvalidId != autofill_node.GetFirstChildId();
+  return true;
 }
 
-bool AutofillModelAssociator::ChromeModelHasUserCreatedNodes() {
+bool AutofillModelAssociator::ChromeModelHasUserCreatedNodes(bool* has_nodes) {
+  DCHECK(has_nodes);
   // Assume the autofill model always have user-created nodes.
+  *has_nodes = true;
   return true;
 }
 
diff --git a/chrome/browser/sync/glue/autofill_model_associator.h b/chrome/browser/sync/glue/autofill_model_associator.h
index c869871..41a6387 100644
--- a/chrome/browser/sync/glue/autofill_model_associator.h
+++ b/chrome/browser/sync/glue/autofill_model_associator.h
@@ -48,12 +48,13 @@ class AutofillModelAssociator
   // Clears all associations.
   virtual bool DisassociateModels();
 
-  // Returns whether the sync model has nodes other than the permanent tagged
-  // nodes.
-  virtual bool SyncModelHasUserCreatedNodes();
+  // The has_nodes out param is true if the sync model has nodes other
+  // than the permanent tagged nodes.
+  virtual bool SyncModelHasUserCreatedNodes(bool* has_nodes);
 
-  // Returns whether the autofill model has any user-defined autofills.
-  virtual bool ChromeModelHasUserCreatedNodes();
+  // The has_nodes out param is true if the autofill model has any
+  // user-defined autofill entries.
+  virtual bool ChromeModelHasUserCreatedNodes(bool* has_nodes);
 
   // Not implemented.
   virtual const AutofillKey* GetChromeNodeFromSyncId(int64 sync_id) {
diff --git a/chrome/browser/sync/glue/bookmark_data_type_controller.cc b/chrome/browser/sync/glue/bookmark_data_type_controller.cc
index 3e8e474..60ed68d 100644
--- a/chrome/browser/sync/glue/bookmark_data_type_controller.cc
+++ b/chrome/browser/sync/glue/bookmark_data_type_controller.cc
@@ -40,6 +40,7 @@ BookmarkDataTypeController::~BookmarkDataTypeController() {
 void BookmarkDataTypeController::Start(bool merge_allowed,
                                        StartCallback* start_callback) {
   DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
+  DCHECK(start_callback);
   unrecoverable_error_detected_ = false;
   if (state_ != NOT_RUNNING) {
     start_callback->Run(BUSY);
@@ -116,38 +117,34 @@ void BookmarkDataTypeController::Associate() {
   model_associator_.reset(sync_components.model_associator);
   change_processor_.reset(sync_components.change_processor);
 
-  bool needs_merge =  model_associator_->ChromeModelHasUserCreatedNodes();
-  if (unrecoverable_error_detected_) return;
-  needs_merge &= model_associator_->SyncModelHasUserCreatedNodes();
-  if (unrecoverable_error_detected_) return;
+  bool chrome_has_nodes = false;
+  if (!model_associator_->ChromeModelHasUserCreatedNodes(&chrome_has_nodes)) {
+    StartFailed(UNRECOVERABLE_ERROR);
+    return;
+  }
+  bool sync_has_nodes = false;
+  if (!model_associator_->SyncModelHasUserCreatedNodes(&sync_has_nodes)) {
+    StartFailed(UNRECOVERABLE_ERROR);
+    return;
+  }
 
-  if (needs_merge && !merge_allowed_) {
-    model_associator_.reset();
-    change_processor_.reset();
-    state_ = NOT_RUNNING;
-    FinishStart(NEEDS_MERGE);
+  if (chrome_has_nodes && sync_has_nodes && !merge_allowed_) {
+    StartFailed(NEEDS_MERGE);
     return;
   }
 
   base::TimeTicks start_time = base::TimeTicks::Now();
-  bool first_run = !model_associator_->SyncModelHasUserCreatedNodes();
-  if (unrecoverable_error_detected_) return;
   bool merge_success = model_associator_->AssociateModels();
-  if (unrecoverable_error_detected_) return;
-
   UMA_HISTOGRAM_TIMES("Sync.BookmarkAssociationTime",
                       base::TimeTicks::Now() - start_time);
   if (!merge_success) {
-    model_associator_.reset();
-    change_processor_.reset();
-    state_ = NOT_RUNNING;
-    FinishStart(ASSOCIATION_FAILED);
+    StartFailed(ASSOCIATION_FAILED);
     return;
   }
 
   sync_service_->ActivateDataType(this, change_processor_.get());
   state_ = RUNNING;
-  FinishStart(first_run ? OK_FIRST_RUN : OK);
+  FinishStart(!sync_has_nodes ? OK_FIRST_RUN : OK);
 }
 
 void BookmarkDataTypeController::FinishStart(StartResult result) {
@@ -155,4 +152,12 @@ void BookmarkDataTypeController::FinishStart(StartResult result) {
   start_callback_.reset();
 }
 
+void BookmarkDataTypeController::StartFailed(StartResult result) {
+  model_associator_.reset();
+  change_processor_.reset();
+  state_ = NOT_RUNNING;
+  start_callback_->Run(result);
+  start_callback_.reset();
+}
+
 }  // namespace browser_sync
diff --git a/chrome/browser/sync/glue/bookmark_data_type_controller.h b/chrome/browser/sync/glue/bookmark_data_type_controller.h
index 14e326e..948bbe7 100644
--- a/chrome/browser/sync/glue/bookmark_data_type_controller.h
+++ b/chrome/browser/sync/glue/bookmark_data_type_controller.h
@@ -74,6 +74,9 @@ class BookmarkDataTypeController : public DataTypeController,
   // Helper method to run the stashed start callback with a given result.
   void FinishStart(StartResult result);
 
+  // Cleans up state and calls callback when star fails.
+  void StartFailed(StartResult result);
+
   ProfileSyncFactory* profile_sync_factory_;
   Profile* profile_;
   ProfileSyncService* sync_service_;
diff --git a/chrome/browser/sync/glue/bookmark_data_type_controller_unittest.cc b/chrome/browser/sync/glue/bookmark_data_type_controller_unittest.cc
index 78728c9..593b912 100644
--- a/chrome/browser/sync/glue/bookmark_data_type_controller_unittest.cc
+++ b/chrome/browser/sync/glue/bookmark_data_type_controller_unittest.cc
@@ -19,7 +19,7 @@
 #include "chrome/common/notification_service.h"
 #include "chrome/common/notification_source.h"
 #include "chrome/common/notification_type.h"
-#include "chrome/test/testing_profile.h"
+#include "chrome/test/profile_mock.h"
 #include "testing/gmock/include/gmock/gmock.h"
 
 using browser_sync::BookmarkDataTypeController;
@@ -30,17 +30,13 @@ using testing::_;
 using testing::DoAll;
 using testing::Invoke;
 using testing::Return;
+using testing::SetArgumentPointee;
 
 class StartCallback {
  public:
   MOCK_METHOD1(Run, void(DataTypeController::StartResult result));
 };
 
-class ProfileMock : public TestingProfile {
- public:
-  MOCK_METHOD0(GetBookmarkModel, BookmarkModel*());
-};
-
 class BookmarkModelMock : public BookmarkModel {
  public:
   BookmarkModelMock() : BookmarkModel(NULL) {}
@@ -73,10 +69,10 @@ class BookmarkDataTypeControllerTest : public testing::Test {
 
   void SetAssociateExpectations() {
     EXPECT_CALL(*profile_sync_factory_, CreateBookmarkSyncComponents(_, _));
-    EXPECT_CALL(*model_associator_, ChromeModelHasUserCreatedNodes()).
-        WillRepeatedly(Return(false));
-    EXPECT_CALL(*model_associator_, SyncModelHasUserCreatedNodes()).
-        WillRepeatedly(Return(true));
+    EXPECT_CALL(*model_associator_, ChromeModelHasUserCreatedNodes(_)).
+        WillRepeatedly(DoAll(SetArgumentPointee<0>(false), Return(true)));
+    EXPECT_CALL(*model_associator_, SyncModelHasUserCreatedNodes(_)).
+        WillRepeatedly(DoAll(SetArgumentPointee<0>(true), Return(true)));
     EXPECT_CALL(*model_associator_, AssociateModels()).
         WillRepeatedly(Return(true));
     EXPECT_CALL(service_, ActivateDataType(_, _));
@@ -132,8 +128,8 @@ TEST_F(BookmarkDataTypeControllerTest, StartBookmarkModelNotReady) {
 TEST_F(BookmarkDataTypeControllerTest, StartFirstRun) {
   SetStartExpectations();
   SetAssociateExpectations();
-  EXPECT_CALL(*model_associator_, SyncModelHasUserCreatedNodes()).
-      WillRepeatedly(Return(false));
+  EXPECT_CALL(*model_associator_, SyncModelHasUserCreatedNodes(_)).
+      WillRepeatedly(DoAll(SetArgumentPointee<0>(false), Return(true)));
   EXPECT_CALL(start_callback_, Run(DataTypeController::OK_FIRST_RUN));
   bookmark_dtc_->Start(false,
                        NewCallback(&start_callback_, &StartCallback::Run));
@@ -153,10 +149,10 @@ TEST_F(BookmarkDataTypeControllerTest, StartBusy) {
 TEST_F(BookmarkDataTypeControllerTest, StartNeedsMerge) {
   SetStartExpectations();
   EXPECT_CALL(*profile_sync_factory_, CreateBookmarkSyncComponents(_, _));
-  EXPECT_CALL(*model_associator_, ChromeModelHasUserCreatedNodes()).
-      WillRepeatedly(Return(true));
-  EXPECT_CALL(*model_associator_, SyncModelHasUserCreatedNodes()).
-      WillRepeatedly(Return(true));
+  EXPECT_CALL(*model_associator_, ChromeModelHasUserCreatedNodes(_)).
+      WillRepeatedly(DoAll(SetArgumentPointee<0>(true), Return(true)));
+  EXPECT_CALL(*model_associator_, SyncModelHasUserCreatedNodes(_)).
+      WillRepeatedly(DoAll(SetArgumentPointee<0>(true), Return(true)));
 
   EXPECT_CALL(start_callback_, Run(DataTypeController::NEEDS_MERGE));
   bookmark_dtc_->Start(false,
@@ -166,10 +162,10 @@ TEST_F(BookmarkDataTypeControllerTest, StartNeedsMerge) {
 TEST_F(BookmarkDataTypeControllerTest, StartMergeAllowed) {
   SetStartExpectations();
   SetAssociateExpectations();
-  EXPECT_CALL(*model_associator_, ChromeModelHasUserCreatedNodes()).
-      WillRepeatedly(Return(true));
-  EXPECT_CALL(*model_associator_, SyncModelHasUserCreatedNodes()).
-      WillRepeatedly(Return(true));
+  EXPECT_CALL(*model_associator_, ChromeModelHasUserCreatedNodes(_)).
+      WillRepeatedly(DoAll(SetArgumentPointee<0>(true), Return(true)));
+  EXPECT_CALL(*model_associator_, SyncModelHasUserCreatedNodes(_)).
+      WillRepeatedly(DoAll(SetArgumentPointee<0>(true), Return(true)));
 
   EXPECT_CALL(start_callback_, Run(DataTypeController::OK));
   bookmark_dtc_->Start(true,
@@ -180,10 +176,10 @@ TEST_F(BookmarkDataTypeControllerTest, StartAssociationFailed) {
   SetStartExpectations();
   // Set up association to fail.
   EXPECT_CALL(*profile_sync_factory_, CreateBookmarkSyncComponents(_, _));
-  EXPECT_CALL(*model_associator_, ChromeModelHasUserCreatedNodes()).
-      WillRepeatedly(Return(false));
-  EXPECT_CALL(*model_associator_, SyncModelHasUserCreatedNodes()).
-      WillRepeatedly(Return(true));
+  EXPECT_CALL(*model_associator_, ChromeModelHasUserCreatedNodes(_)).
+      WillRepeatedly(DoAll(SetArgumentPointee<0>(false), Return(true)));
+  EXPECT_CALL(*model_associator_, SyncModelHasUserCreatedNodes(_)).
+      WillRepeatedly(DoAll(SetArgumentPointee<0>(true), Return(true)));
   EXPECT_CALL(*model_associator_, AssociateModels()).
       WillRepeatedly(Return(false));
 
@@ -198,19 +194,10 @@ TEST_F(BookmarkDataTypeControllerTest,
   SetStartExpectations();
   // Set up association to fail with an unrecoverable error.
   EXPECT_CALL(*profile_sync_factory_, CreateBookmarkSyncComponents(_, _));
-  EXPECT_CALL(*model_associator_, ChromeModelHasUserCreatedNodes()).
-      WillOnce(Return(false));
-  EXPECT_CALL(*model_associator_, SyncModelHasUserCreatedNodes()).
-      WillOnce(DoAll(Invoke(bookmark_dtc_.get(),
-                            &BookmarkDataTypeController::OnUnrecoverableError),
-                     Return(false)));
-  EXPECT_CALL(service_, OnUnrecoverableError()).
-      WillOnce(Invoke(bookmark_dtc_.get(),
-                      &BookmarkDataTypeController::Stop));
-  EXPECT_CALL(service_, DeactivateDataType(bookmark_dtc_.get(),
-                                           change_processor_));
-  EXPECT_CALL(*model_associator_, DisassociateModels());
-
+  EXPECT_CALL(*model_associator_, ChromeModelHasUserCreatedNodes(_)).
+      WillRepeatedly(DoAll(SetArgumentPointee<0>(false), Return(true)));
+  EXPECT_CALL(*model_associator_, SyncModelHasUserCreatedNodes(_)).
+      WillRepeatedly(DoAll(SetArgumentPointee<0>(false), Return(false)));
   EXPECT_CALL(start_callback_, Run(DataTypeController::UNRECOVERABLE_ERROR));
   bookmark_dtc_->Start(true,
                        NewCallback(&start_callback_, &StartCallback::Run));
diff --git a/chrome/browser/sync/glue/bookmark_model_associator.cc b/chrome/browser/sync/glue/bookmark_model_associator.cc
index 19ddb7c..aaddd40 100644
--- a/chrome/browser/sync/glue/bookmark_model_associator.cc
+++ b/chrome/browser/sync/glue/bookmark_model_associator.cc
@@ -213,22 +213,25 @@ void BookmarkModelAssociator::Disassociate(int64 sync_id) {
   dirty_associations_sync_ids_.erase(sync_id);
 }
 
-bool BookmarkModelAssociator::ChromeModelHasUserCreatedNodes() {
+bool BookmarkModelAssociator::ChromeModelHasUserCreatedNodes(bool* has_nodes) {
+  DCHECK(has_nodes);
   BookmarkModel* model = sync_service_->profile()->GetBookmarkModel();
   DCHECK(model->IsLoaded());
-  return model->GetBookmarkBarNode()->GetChildCount() > 0 ||
-         model->other_node()->GetChildCount() > 0;
+
+  *has_nodes =  model->GetBookmarkBarNode()->GetChildCount() > 0 ||
+      model->other_node()->GetChildCount() > 0;
+  return true;
 }
 
-bool BookmarkModelAssociator::SyncModelHasUserCreatedNodes() {
+bool BookmarkModelAssociator::SyncModelHasUserCreatedNodes(bool* has_nodes) {
+  DCHECK(has_nodes);
+  *has_nodes = false;
   int64 bookmark_bar_sync_id;
   if (!GetSyncIdForTaggedNode(kBookmarkBarTag, &bookmark_bar_sync_id)) {
-    error_handler_->OnUnrecoverableError();
     return false;
   }
   int64 other_bookmarks_sync_id;
   if (!GetSyncIdForTaggedNode(kOtherBookmarksTag, &other_bookmarks_sync_id)) {
-    error_handler_->OnUnrecoverableError();
     return false;
   }
 
@@ -237,20 +240,19 @@ bool BookmarkModelAssociator::SyncModelHasUserCreatedNodes() {
 
   sync_api::ReadNode bookmark_bar_node(&trans);
   if (!bookmark_bar_node.InitByIdLookup(bookmark_bar_sync_id)) {
-    error_handler_->OnUnrecoverableError();
     return false;
   }
 
   sync_api::ReadNode other_bookmarks_node(&trans);
   if (!other_bookmarks_node.InitByIdLookup(other_bookmarks_sync_id)) {
-    error_handler_->OnUnrecoverableError();
     return false;
   }
 
   // Sync model has user created nodes if either one of the permanent nodes
   // has children.
-  return bookmark_bar_node.GetFirstChildId() != sync_api::kInvalidId ||
-         other_bookmarks_node.GetFirstChildId() != sync_api::kInvalidId;
+  *has_nodes = bookmark_bar_node.GetFirstChildId() != sync_api::kInvalidId ||
+      other_bookmarks_node.GetFirstChildId() != sync_api::kInvalidId;
+  return true;
 }
 
 bool BookmarkModelAssociator::NodesMatch(const BookmarkNode* bookmark,
diff --git a/chrome/browser/sync/glue/bookmark_model_associator.h b/chrome/browser/sync/glue/bookmark_model_associator.h
index 0e0f031..2fd96bd 100644
--- a/chrome/browser/sync/glue/bookmark_model_associator.h
+++ b/chrome/browser/sync/glue/bookmark_model_associator.h
@@ -51,17 +51,16 @@ class BookmarkModelAssociator
   // should abort the sync operation and report an error to the user.
   virtual bool AssociateModels();
 
-  // Clears all associations.
   virtual bool DisassociateModels();
 
-  // Returns whether the sync model has nodes other than the permanent tagged
-  // nodes.
-  virtual bool SyncModelHasUserCreatedNodes();
+  // The has_nodes out param is true if the sync model has nodes other
+  // than the permanent tagged nodes.
+  virtual bool SyncModelHasUserCreatedNodes(bool* has_nodes);
 
-  // Returns whether the bookmark model has user created nodes or not. That is,
-  // whether there are nodes in the bookmark model except the bookmark bar and
-  // other bookmarks.
-  virtual bool ChromeModelHasUserCreatedNodes();
+  // The has_nodes out param is true if the bookmark model has user
+  // created nodes or not. That is, whether there are nodes in the
+  // bookmark model except the bookmark bar and other bookmarks.
+  virtual bool ChromeModelHasUserCreatedNodes(bool* has_nodes);
 
   // Returns sync id for the given bookmark node id.
   // Returns sync_api::kInvalidId if the sync node is not found for the given
@@ -87,7 +86,7 @@ class BookmarkModelAssociator
   // Stores the id of the node with the given tag in |sync_id|.
   // Returns of that node was found successfully.
   // Tests override this.
-   virtual bool GetSyncIdForTaggedNode(const std::string& tag, int64* sync_id);
+  virtual bool GetSyncIdForTaggedNode(const std::string& tag, int64* sync_id);
 
   // Returns sync service instance.
   ProfileSyncService* sync_service() { return sync_service_; }
diff --git a/chrome/browser/sync/glue/data_type_manager.h b/chrome/browser/sync/glue/data_type_manager.h
index 5701acd..652fc06 100644
--- a/chrome/browser/sync/glue/data_type_manager.h
+++ b/chrome/browser/sync/glue/data_type_manager.h
@@ -27,8 +27,10 @@ class DataTypeManager {
     BUSY,               // Start() was called while start is already
                         // in progress.
     ASSOCIATION_FAILED, // An error occurred during model association.
-    ABORTED             // Start was aborted by calling Stop() before
+    ABORTED,            // Start was aborted by calling Stop() before
                         // all types were started.
+    UNRECOVERABLE_ERROR // A data type experienced an unrecoverable error
+                        // during startup.
   };
 
   typedef Callback1<StartResult>::Type StartCallback;
diff --git a/chrome/browser/sync/glue/data_type_manager_impl.cc b/chrome/browser/sync/glue/data_type_manager_impl.cc
index 1b84597..5713e91 100644
--- a/chrome/browser/sync/glue/data_type_manager_impl.cc
+++ b/chrome/browser/sync/glue/data_type_manager_impl.cc
@@ -112,6 +112,9 @@ void DataTypeManagerImpl::TypeStartCallback(
     case DataTypeController::ASSOCIATION_FAILED:
       start_result = DataTypeManager::ASSOCIATION_FAILED;
       break;
+    case DataTypeController::UNRECOVERABLE_ERROR:
+      start_result = DataTypeManager::UNRECOVERABLE_ERROR;
+      break;
     default:
       NOTREACHED();
       break;
diff --git a/chrome/browser/sync/glue/model_associator.h b/chrome/browser/sync/glue/model_associator.h
index 90bc4148..b7f7622 100644
--- a/chrome/browser/sync/glue/model_associator.h
+++ b/chrome/browser/sync/glue/model_associator.h
@@ -9,18 +9,12 @@
 #include <string>
 
 #include "base/basictypes.h"
-#include "base/logging.h"
-#include "base/stl_util-inl.h"
 #include "chrome/browser/sync/syncable/model_type.h"
 
 namespace sync_api {
 class BaseNode;
-class BaseTransaction;
-class ReadNode;
 }
 
-class ProfileSyncService;
-
 namespace browser_sync {
 
 // This represents the fundamental operations used for model association that
@@ -30,21 +24,25 @@ class AssociatorInterface {
  public:
   virtual ~AssociatorInterface() {}
 
-  // Iterates through both the sync and the chrome model looking for matched
-  // pairs of items. After successful completion, the models should be identical
-  // and corresponding. Returns true on success. On failure of this step, we
-  // should abort the sync operation and report an error to the user.
+  // Iterates through both the sync and the chrome model looking for
+  // matched pairs of items. After successful completion, the models
+  // should be identical and corresponding. Returns true on
+  // success. On failure of this step, we should abort the sync
+  // operation and report an error to the user.
   virtual bool AssociateModels() = 0;
 
   // Clears all the associations between the chrome and sync models.
   virtual bool DisassociateModels() = 0;
 
-  // Returns whether the sync model has nodes other than the permanent tagged
-  // nodes.
-  virtual bool SyncModelHasUserCreatedNodes() = 0;
+  // The has_nodes out parameter is set to true if the sync model has
+  // nodes other than the permanent tagged nodes.  The method may
+  // return false if an error occurred.
+  virtual bool SyncModelHasUserCreatedNodes(bool* has_nodes) = 0;
 
-  // Returns whether the chrome model has user-created nodes or not.
-  virtual bool ChromeModelHasUserCreatedNodes() = 0;
+  // The has_nodes out parameter is set to true if the chrome model
+  // has user-created nodes.  The method may return false if an error
+  // occurred.
+  virtual bool ChromeModelHasUserCreatedNodes(bool* has_nodes) = 0;
 };
 
 // In addition to the generic methods, association can refer to operations
@@ -81,90 +79,6 @@ class PerDataTypeAssociatorInterface : public AssociatorInterface {
   virtual void Disassociate(int64 sync_id) = 0;
 };
 
-// Contains all model association related logic to associate the chrome model
-// with the sync model.
-class ModelAssociator : public AssociatorInterface {
- public:
-  explicit ModelAssociator(ProfileSyncService* sync_service)
-      : sync_service_(sync_service) {
-    DCHECK(sync_service);
-  }
-  ~ModelAssociator() { CleanupAllAssociators(); }
-
-  // AssociatorInterface implementation. These are aggregated across all known
-  // data types (for example, if both a Bookmarks and Preferences association
-  // implementation is registered, AssociateModels() will associate both
-  // Bookmarks and Preferences with the sync model).
-  virtual bool AssociateModels() {
-    bool success = true;
-    for (ImplMap::iterator i = impl_map_.begin(); i != impl_map_.end(); ++i)
-      success &= i->second->AssociateModels();
-    return success;
-  }
-
-  virtual bool DisassociateModels() {
-    bool success = true;
-    for (ImplMap::iterator i = impl_map_.begin(); i != impl_map_.end(); ++i)
-      success &= i->second->DisassociateModels();
-    return success;
-  }
-
-  virtual bool SyncModelHasUserCreatedNodes() {
-    for (ImplMap::iterator i = impl_map_.begin(); i != impl_map_.end(); ++i) {
-      if (i->second->SyncModelHasUserCreatedNodes())
-        return true;
-    }
-    return false;
-  }
-
-  virtual bool ChromeModelHasUserCreatedNodes() {
-    for (ImplMap::iterator i = impl_map_.begin(); i != impl_map_.end(); ++i) {
-      if (i->second->ChromeModelHasUserCreatedNodes())
-        return true;
-    }
-    return false;
-  }
-
-  // Call to enable model association for a particular data type by registering
-  // a type specific association implementation.  This will unregister any
-  // previous implementation for the same model type, to enforce only one impl
-  // for a given type at a time.
-  template <class Impl>
-  void CreateAndRegisterPerDataTypeImpl() {
-    UnregisterPerDataTypeImpl<Impl>();
-    impl_map_[Impl::model_type()] = new Impl(sync_service_);
-  }
-
-  // Call to disable model association for a particular data type by
-  // unregistering the specific association implementation.
-  template <class Impl>
-  void UnregisterPerDataTypeImpl() {
-    ImplMap::iterator it = impl_map_.find(Impl::model_type());
-    if (it == impl_map_.end())
-      return;
-    delete it->second;
-    impl_map_.erase(it);
-  }
-
-  template <class Impl>
-  Impl* GetImpl() {
-    ImplMap::const_iterator it = impl_map_.find(Impl::model_type());
-    // The cast is safe because of the static model_type() pattern.
-    return reinterpret_cast<Impl*>(it->second);
-  }
-
-  void CleanupAllAssociators() {
-    STLDeleteValues(&impl_map_);
-  }
-
- private:
-  ProfileSyncService* sync_service_;
-
-  typedef std::map<syncable::ModelType, AssociatorInterface*> ImplMap;
-  ImplMap impl_map_;
-  DISALLOW_COPY_AND_ASSIGN(ModelAssociator);
-};
-
 }  // namespace browser_sync
 
 #endif  // CHROME_BROWSER_SYNC_GLUE_MODEL_ASSOCIATOR_H_
diff --git a/chrome/browser/sync/glue/model_associator_mock.h b/chrome/browser/sync/glue/model_associator_mock.h
index 7852dba..6efe02d 100644
--- a/chrome/browser/sync/glue/model_associator_mock.h
+++ b/chrome/browser/sync/glue/model_associator_mock.h
@@ -14,8 +14,8 @@ class ModelAssociatorMock : public AssociatorInterface {
  public:
   MOCK_METHOD0(AssociateModels, bool());
   MOCK_METHOD0(DisassociateModels, bool());
-  MOCK_METHOD0(SyncModelHasUserCreatedNodes, bool());
-  MOCK_METHOD0(ChromeModelHasUserCreatedNodes, bool());
+  MOCK_METHOD1(SyncModelHasUserCreatedNodes, bool(bool*));
+  MOCK_METHOD1(ChromeModelHasUserCreatedNodes, bool(bool*));
 };
 
 }  // 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 602d82a..e7ddb1f 100644
--- a/chrome/browser/sync/glue/preference_data_type_controller.cc
+++ b/chrome/browser/sync/glue/preference_data_type_controller.cc
@@ -32,6 +32,7 @@ PreferenceDataTypeController::~PreferenceDataTypeController() {
 void PreferenceDataTypeController::Start(bool merge_allowed,
                                          StartCallback* start_callback) {
   DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
+  DCHECK(start_callback);
   unrecoverable_error_detected_ = false;
   if (state_ != NOT_RUNNING) {
     start_callback->Run(BUSY);
@@ -40,6 +41,7 @@ void PreferenceDataTypeController::Start(bool merge_allowed,
   }
 
   start_callback_.reset(start_callback);
+
   // No additional services need to be started before we can proceed
   // with model association.
   ProfileSyncFactory::SyncComponents sync_components =
@@ -48,36 +50,34 @@ void PreferenceDataTypeController::Start(bool merge_allowed,
   model_associator_.reset(sync_components.model_associator);
   change_processor_.reset(sync_components.change_processor);
 
-  bool needs_merge =  model_associator_->ChromeModelHasUserCreatedNodes();
-  if (unrecoverable_error_detected_) return;
-  needs_merge &= model_associator_->SyncModelHasUserCreatedNodes();
-  if (unrecoverable_error_detected_) return;
+  bool chrome_has_nodes = false;
+  if (!model_associator_->ChromeModelHasUserCreatedNodes(&chrome_has_nodes)) {
+    StartFailed(UNRECOVERABLE_ERROR);
+    return;
+  }
+  bool sync_has_nodes = false;
+  if (!model_associator_->SyncModelHasUserCreatedNodes(&sync_has_nodes)) {
+    StartFailed(UNRECOVERABLE_ERROR);
+    return;
+  }
 
-  if (needs_merge && !merge_allowed) {
-    model_associator_.reset();
-    change_processor_.reset();
-    FinishStart(NEEDS_MERGE);
+  if (chrome_has_nodes && sync_has_nodes && !merge_allowed) {
+    StartFailed(NEEDS_MERGE);
     return;
   }
 
   base::TimeTicks start_time = base::TimeTicks::Now();
-  bool first_run = !model_associator_->SyncModelHasUserCreatedNodes();
-  if (unrecoverable_error_detected_) return;
   bool merge_success = model_associator_->AssociateModels();
-  if (unrecoverable_error_detected_) return;
-
   UMA_HISTOGRAM_TIMES("Sync.PreferenceAssociationTime",
                       base::TimeTicks::Now() - start_time);
   if (!merge_success) {
-    model_associator_.reset();
-    change_processor_.reset();
-    FinishStart(ASSOCIATION_FAILED);
+    StartFailed(ASSOCIATION_FAILED);
     return;
   }
 
   sync_service_->ActivateDataType(this, change_processor_.get());
   state_ = RUNNING;
-  FinishStart(first_run ? OK_FIRST_RUN : OK);
+  FinishStart(!sync_has_nodes ? OK_FIRST_RUN : OK);
 }
 
 void PreferenceDataTypeController::Stop() {
@@ -99,6 +99,7 @@ void PreferenceDataTypeController::Stop() {
 }
 
 void PreferenceDataTypeController::OnUnrecoverableError() {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
   unrecoverable_error_detected_ = true;
   sync_service_->OnUnrecoverableError();
 }
@@ -108,4 +109,11 @@ void PreferenceDataTypeController::FinishStart(StartResult result) {
   start_callback_.reset();
 }
 
+void PreferenceDataTypeController::StartFailed(StartResult result) {
+  model_associator_.reset();
+  change_processor_.reset();
+  start_callback_->Run(result);
+  start_callback_.reset();
+}
+
 }  // namespace browser_sync
diff --git a/chrome/browser/sync/glue/preference_data_type_controller.h b/chrome/browser/sync/glue/preference_data_type_controller.h
index d515b71..5347225 100644
--- a/chrome/browser/sync/glue/preference_data_type_controller.h
+++ b/chrome/browser/sync/glue/preference_data_type_controller.h
@@ -56,6 +56,9 @@ class PreferenceDataTypeController : public DataTypeController {
   // Helper method to run the stashed start callback with a given result.
   void FinishStart(StartResult result);
 
+  // Cleans up state and calls callback when start fails.
+  void StartFailed(StartResult result);
+
   ProfileSyncFactory* profile_sync_factory_;
   ProfileSyncService* sync_service_;
 
diff --git a/chrome/browser/sync/glue/preference_data_type_controller_unittest.cc b/chrome/browser/sync/glue/preference_data_type_controller_unittest.cc
index 7082698..8449222 100644
--- a/chrome/browser/sync/glue/preference_data_type_controller_unittest.cc
+++ b/chrome/browser/sync/glue/preference_data_type_controller_unittest.cc
@@ -23,6 +23,7 @@ using testing::_;
 using testing::DoAll;
 using testing::Invoke;
 using testing::Return;
+using testing::SetArgumentPointee;
 
 class StartCallback {
  public:
@@ -51,10 +52,10 @@ class PreferenceDataTypeControllerTest : public testing::Test {
   }
 
   void SetAssociateExpectations() {
-    EXPECT_CALL(*model_associator_, ChromeModelHasUserCreatedNodes()).
-        WillRepeatedly(Return(false));
-    EXPECT_CALL(*model_associator_, SyncModelHasUserCreatedNodes()).
-        WillRepeatedly(Return(true));
+    EXPECT_CALL(*model_associator_, ChromeModelHasUserCreatedNodes(_)).
+        WillRepeatedly(DoAll(SetArgumentPointee<0>(false), Return(true)));
+    EXPECT_CALL(*model_associator_, SyncModelHasUserCreatedNodes(_)).
+        WillRepeatedly(DoAll(SetArgumentPointee<0>(true), Return(true)));
     EXPECT_CALL(*model_associator_, AssociateModels()).
         WillRepeatedly(Return(true));
   }
@@ -93,8 +94,8 @@ TEST_F(PreferenceDataTypeControllerTest, StartFirstRun) {
   SetStartExpectations();
   SetAssociateExpectations();
   SetActivateExpectations();
-  EXPECT_CALL(*model_associator_, SyncModelHasUserCreatedNodes()).
-      WillRepeatedly(Return(false));
+  EXPECT_CALL(*model_associator_, SyncModelHasUserCreatedNodes(_)).
+      WillRepeatedly(DoAll(SetArgumentPointee<0>(false), Return(true)));
   EXPECT_CALL(start_callback_, Run(DataTypeController::OK_FIRST_RUN));
   preference_dtc_->Start(false,
                          NewCallback(&start_callback_, &StartCallback::Run));
@@ -103,11 +104,10 @@ TEST_F(PreferenceDataTypeControllerTest, StartFirstRun) {
 TEST_F(PreferenceDataTypeControllerTest, StartNeedsMerge) {
   SetStartExpectations();
   SetAssociateExpectations();
-  EXPECT_CALL(*model_associator_, ChromeModelHasUserCreatedNodes()).
-      WillRepeatedly(Return(true));
-  EXPECT_CALL(*model_associator_, SyncModelHasUserCreatedNodes()).
-      WillRepeatedly(Return(true));
-
+  EXPECT_CALL(*model_associator_, ChromeModelHasUserCreatedNodes(_)).
+      WillRepeatedly(DoAll(SetArgumentPointee<0>(true), Return(true)));
+  EXPECT_CALL(*model_associator_, SyncModelHasUserCreatedNodes(_)).
+      WillRepeatedly(DoAll(SetArgumentPointee<0>(true), Return(true)));
   EXPECT_CALL(start_callback_, Run(DataTypeController::NEEDS_MERGE));
   preference_dtc_->Start(false,
                          NewCallback(&start_callback_, &StartCallback::Run));
@@ -117,10 +117,10 @@ TEST_F(PreferenceDataTypeControllerTest, StartMergeAllowed) {
   SetStartExpectations();
   SetAssociateExpectations();
   SetActivateExpectations();
-  EXPECT_CALL(*model_associator_, ChromeModelHasUserCreatedNodes()).
-      WillRepeatedly(Return(true));
-  EXPECT_CALL(*model_associator_, SyncModelHasUserCreatedNodes()).
-      WillRepeatedly(Return(true));
+  EXPECT_CALL(*model_associator_, ChromeModelHasUserCreatedNodes(_)).
+      WillRepeatedly(DoAll(SetArgumentPointee<0>(true), Return(true)));
+  EXPECT_CALL(*model_associator_, SyncModelHasUserCreatedNodes(_)).
+      WillRepeatedly(DoAll(SetArgumentPointee<0>(true), Return(true)));
 
   EXPECT_CALL(start_callback_, Run(DataTypeController::OK));
   preference_dtc_->Start(true,
@@ -143,20 +143,10 @@ TEST_F(PreferenceDataTypeControllerTest,
        StartAssociationTriggersUnrecoverableError) {
   SetStartExpectations();
   // Set up association to fail with an unrecoverable error.
-  EXPECT_CALL(*model_associator_, ChromeModelHasUserCreatedNodes()).
-      WillOnce(Return(false));
-  EXPECT_CALL(*model_associator_, SyncModelHasUserCreatedNodes()).
-      WillOnce(DoAll(Invoke(
-          preference_dtc_.get(),
-          &PreferenceDataTypeController::OnUnrecoverableError),
-                     Return(false)));
-  EXPECT_CALL(service_, OnUnrecoverableError()).
-      WillOnce(Invoke(preference_dtc_.get(),
-                      &PreferenceDataTypeController::Stop));
-  EXPECT_CALL(service_, DeactivateDataType(preference_dtc_.get(),
-                                           change_processor_));
-  EXPECT_CALL(*model_associator_, DisassociateModels());
-
+  EXPECT_CALL(*model_associator_, ChromeModelHasUserCreatedNodes(_)).
+      WillRepeatedly(DoAll(SetArgumentPointee<0>(false), Return(true)));
+  EXPECT_CALL(*model_associator_, SyncModelHasUserCreatedNodes(_)).
+      WillRepeatedly(DoAll(SetArgumentPointee<0>(false), Return(false)));
   EXPECT_CALL(start_callback_, Run(DataTypeController::UNRECOVERABLE_ERROR));
   preference_dtc_->Start(true,
                          NewCallback(&start_callback_, &StartCallback::Run));
diff --git a/chrome/browser/sync/glue/preference_model_associator.cc b/chrome/browser/sync/glue/preference_model_associator.cc
index daa4e1f..ffd30f0 100644
--- a/chrome/browser/sync/glue/preference_model_associator.cc
+++ b/chrome/browser/sync/glue/preference_model_associator.cc
@@ -125,10 +125,11 @@ bool PreferenceModelAssociator::DisassociateModels() {
   return true;
 }
 
-bool PreferenceModelAssociator::SyncModelHasUserCreatedNodes() {
+bool PreferenceModelAssociator::SyncModelHasUserCreatedNodes(bool* has_nodes) {
+  DCHECK(has_nodes);
+  *has_nodes = false;
   int64 preferences_sync_id;
   if (!GetSyncIdForTaggedNode(kPreferencesTag, &preferences_sync_id)) {
-    sync_service()->OnUnrecoverableError();
     LOG(ERROR) << "Server did not create the top-level preferences node. We "
                << "might be running against an out-of-date server.";
     return false;
@@ -138,18 +139,22 @@ bool PreferenceModelAssociator::SyncModelHasUserCreatedNodes() {
 
   sync_api::ReadNode preferences_node(&trans);
   if (!preferences_node.InitByIdLookup(preferences_sync_id)) {
-    sync_service()->OnUnrecoverableError();
     LOG(ERROR) << "Server did not create the top-level preferences node. We "
                << "might be running against an out-of-date server.";
+    return false;
   }
 
   // The sync model has user created nodes if the preferences folder has any
   // children.
-  return sync_api::kInvalidId != preferences_node.GetFirstChildId();
+  *has_nodes = sync_api::kInvalidId != preferences_node.GetFirstChildId();
+  return true;
 }
 
-bool PreferenceModelAssociator::ChromeModelHasUserCreatedNodes() {
+bool PreferenceModelAssociator::ChromeModelHasUserCreatedNodes(
+    bool* has_nodes) {
+  DCHECK(has_nodes);
   // Assume the preferences model always have user-created nodes.
+  *has_nodes = true;
   return true;
 }
 
diff --git a/chrome/browser/sync/glue/preference_model_associator.h b/chrome/browser/sync/glue/preference_model_associator.h
index 1307c51..8f0bbca 100644
--- a/chrome/browser/sync/glue/preference_model_associator.h
+++ b/chrome/browser/sync/glue/preference_model_associator.h
@@ -49,10 +49,10 @@ class PreferenceModelAssociator
 
   // Returns whether the sync model has nodes other than the permanent tagged
   // nodes.
-  virtual bool SyncModelHasUserCreatedNodes();
+  virtual bool SyncModelHasUserCreatedNodes(bool* has_nodes);
 
   // Returns whether the preference model has any user-defined preferences.
-  virtual bool ChromeModelHasUserCreatedNodes();
+  virtual bool ChromeModelHasUserCreatedNodes(bool* has_nodes);
 
   // Not implemented.
   virtual const PrefService::Preference* GetChromeNodeFromSyncId(
diff --git a/chrome/browser/sync/profile_sync_service_autofill_unittest.cc b/chrome/browser/sync/profile_sync_service_autofill_unittest.cc
index 5b274eb..0c90209 100644
--- a/chrome/browser/sync/profile_sync_service_autofill_unittest.cc
+++ b/chrome/browser/sync/profile_sync_service_autofill_unittest.cc
@@ -29,7 +29,7 @@
 #include "chrome/browser/webdata/web_database.h"
 #include "chrome/common/notification_service.h"
 #include "chrome/test/sync/engine/test_id_factory.h"
-#include "chrome/test/testing_profile.h"
+#include "chrome/test/profile_mock.h"
 #include "testing/gmock/include/gmock/gmock.h"
 
 using base::Time;
@@ -97,23 +97,18 @@ class TestingProfileSyncService : public ProfileSyncService {
 class WebDatabaseMock : public WebDatabase {
  public:
   MOCK_METHOD2(RemoveFormElement,
-               bool(const string16& name, const string16& value)); // NOLINT
+               bool(const string16& name, const string16& value));  // NOLINT
   MOCK_METHOD1(GetAllAutofillEntries,
-               bool(std::vector<AutofillEntry>* entries)); // NOLINT
+               bool(std::vector<AutofillEntry>* entries));  // NOLINT
   MOCK_METHOD3(GetAutofillTimestamps,
-               bool(const string16& name, // NOLINT
+               bool(const string16& name,  // NOLINT
                     const string16& value,
                     std::vector<base::Time>* timestamps));
   MOCK_METHOD1(UpdateAutofillEntries,
-               bool(const std::vector<AutofillEntry>& entries)); // NOLINT
+               bool(const std::vector<AutofillEntry>& entries));  // NOLINT
 };
 
-class ProfileMock : public TestingProfile {
- public:
-  MOCK_METHOD1(GetWebDataService, WebDataService*(ServiceAccessType access));
-};
-
-class DBThreadNotificationService :
+class DBThreadNotificationService :  // NOLINT
     public base::RefCountedThreadSafe<DBThreadNotificationService> {
  public:
   DBThreadNotificationService() : done_event_(false, false) {}
@@ -226,10 +221,7 @@ class ProfileSyncServiceAutofillTest : public testing::Test {
 
       // State changes once for the backend init and once for startup done.
       EXPECT_CALL(observer_, OnStateChanged()).
-          WillOnce(DoAll(
-              Invoke(this,
-                     &ProfileSyncServiceAutofillTest::CreateAutofillRoot),
-              InvokeTask(task))).
+          WillOnce(InvokeTask(task)).
           WillOnce(QuitUIMessageLoop());
       service_->RegisterDataTypeController(data_type_controller);
       service_->Initialize();
@@ -329,6 +321,7 @@ class ProfileSyncServiceAutofillTest : public testing::Test {
     return MakeAutofillEntry(name, value, timestamp, -1);
   }
 
+  friend class CreateAutofillRootTask;
   friend class AddAutofillEntriesTask;
 
   MessageLoopForUI message_loop_;
@@ -347,12 +340,53 @@ class ProfileSyncServiceAutofillTest : public testing::Test {
   TestIdFactory ids_;
 };
 
-// TODO(sync): Test unrecoverable error during MA.
+class CreateAutofillRootTask : public Task {
+ public:
+  explicit CreateAutofillRootTask(ProfileSyncServiceAutofillTest* test)
+      : test_(test) {
+  }
+
+  virtual void Run() {
+    test_->CreateAutofillRoot();
+  }
+
+ private:
+  ProfileSyncServiceAutofillTest* test_;
+};
+
+class AddAutofillEntriesTask : public Task {
+ public:
+  AddAutofillEntriesTask(ProfileSyncServiceAutofillTest* test,
+                         const std::vector<AutofillEntry>& entries)
+      : test_(test), entries_(entries) {
+  }
+
+  virtual void Run() {
+    test_->CreateAutofillRoot();
+    for (size_t i = 0; i < entries_.size(); ++i) {
+      test_->AddAutofillSyncNode(entries_[i]);
+    }
+  }
+
+ private:
+  ProfileSyncServiceAutofillTest* test_;
+  const std::vector<AutofillEntry>& entries_;
+};
+
+// TODO(skrul): Test abort startup.
+// TODO(skrul): Test error while processing changes.
+
+TEST_F(ProfileSyncServiceAutofillTest, FailModelAssociation) {
+  // Don't create the root autofill node so startup fails.
+  StartSyncService(NULL);
+  EXPECT_TRUE(service_->unrecoverable_error_detected());
+}
 
 TEST_F(ProfileSyncServiceAutofillTest, EmptyNativeEmptySync) {
   EXPECT_CALL(web_database_, GetAllAutofillEntries(_)).WillOnce(Return(true));
   SetIdleChangeProcessorExpectations();
-  StartSyncService(NULL);
+  CreateAutofillRootTask task(this);
+  StartSyncService(&task);
   std::vector<AutofillEntry> sync_entries;
   GetAutofillEntriesFromSyncDB(&sync_entries);
   EXPECT_EQ(0U, sync_entries.size());
@@ -364,7 +398,8 @@ TEST_F(ProfileSyncServiceAutofillTest, HasNativeEmptySync) {
   EXPECT_CALL(web_database_, GetAllAutofillEntries(_)).
       WillOnce(DoAll(SetArgumentPointee<0>(entries), Return(true)));
   SetIdleChangeProcessorExpectations();
-  StartSyncService(NULL);
+  CreateAutofillRootTask task(this);
+  StartSyncService(&task);
   std::vector<AutofillEntry> sync_entries;
   GetAutofillEntriesFromSyncDB(&sync_entries);
   ASSERT_EQ(1U, entries.size());
@@ -381,30 +416,13 @@ TEST_F(ProfileSyncServiceAutofillTest, HasNativeWithDuplicatesEmptySync) {
   EXPECT_CALL(web_database_, GetAllAutofillEntries(_)).
       WillOnce(DoAll(SetArgumentPointee<0>(entries), Return(true)));
   SetIdleChangeProcessorExpectations();
-  StartSyncService(NULL);
+  CreateAutofillRootTask task(this);
+  StartSyncService(&task);
   std::vector<AutofillEntry> sync_entries;
   GetAutofillEntriesFromSyncDB(&sync_entries);
   EXPECT_EQ(2U, sync_entries.size());
 }
 
-class AddAutofillEntriesTask : public Task {
- public:
-  AddAutofillEntriesTask(ProfileSyncServiceAutofillTest* test,
-                         const std::vector<AutofillEntry>& entries)
-      : test_(test), entries_(entries) {
-  }
-
-  virtual void Run() {
-    for (size_t i = 0; i < entries_.size(); ++i) {
-      test_->AddAutofillSyncNode(entries_[i]);
-    }
-  }
-
- private:
-  ProfileSyncServiceAutofillTest* test_;
-  const std::vector<AutofillEntry>& entries_;
-};
-
 TEST_F(ProfileSyncServiceAutofillTest, HasNativeHasSyncNoMerge) {
   AutofillEntry native_entry(MakeAutofillEntry("native", "entry", 1));
   AutofillEntry sync_entry(MakeAutofillEntry("sync", "entry", 2));
diff --git a/chrome/chrome_tests.gypi b/chrome/chrome_tests.gypi
index 2fb9f52..5cbfb82 100644
--- a/chrome/chrome_tests.gypi
+++ b/chrome/chrome_tests.gypi
@@ -107,6 +107,7 @@
         'test/chrome_process_util.cc',
         'test/chrome_process_util.h',
         'test/chrome_process_util_mac.cc',
+        'test/profile_mock.h',
         'test/test_browser_window.h',
         'test/testing_profile.cc',
         'test/testing_profile.h',
@@ -848,12 +849,14 @@
         'browser/ssl/ssl_host_state_unittest.cc',
         'browser/status_icons/status_tray_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',
         'browser/sync/glue/data_type_controller_mock.h',
         'browser/sync/glue/data_type_manager_impl_unittest.cc',
         'browser/sync/glue/data_type_manager_mock.h',
         'browser/sync/glue/database_model_worker_unittest.cc',
         'browser/sync/glue/http_bridge_unittest.cc',
+        'browser/sync/glue/preference_data_type_controller_unittest.cc',
         'browser/sync/glue/ui_model_worker_unittest.cc',
         'browser/sync/profile_sync_service_mock.h',
         'browser/sync/profile_sync_service_startup_unittest.cc',
@@ -1546,9 +1549,7 @@
         'browser/sync/engine/syncer_unittest.cc',
         'browser/sync/engine/syncproto_unittest.cc',
         'browser/sync/engine/verify_updates_command_unittest.cc',
-        'browser/sync/glue/bookmark_data_type_controller_unittest.cc',
         'browser/sync/glue/change_processor_mock.h',
-        'browser/sync/glue/preference_data_type_controller_unittest.cc',
         'browser/sync/notifier/base/mac/network_status_detector_task_mac_unittest.cc',
         'browser/sync/notifier/listener/talk_mediator_unittest.cc',
         'browser/sync/notifier/listener/send_update_task_unittest.cc',
diff --git a/chrome/test/profile_mock.h b/chrome/test/profile_mock.h
new file mode 100644
index 0000000..c85a1ca
--- /dev/null
+++ b/chrome/test/profile_mock.h
@@ -0,0 +1,18 @@
+// 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.
+
+#ifndef CHROME_TEST_PROFILE_MOCK_H__
+#define CHROME_TEST_PROFILE_MOCK_H__
+
+#include "chrome/test/testing_profile.h"
+
+#include "testing/gmock/include/gmock/gmock.h"
+
+class ProfileMock : public TestingProfile {
+ public:
+  MOCK_METHOD0(GetBookmarkModel, BookmarkModel*());
+  MOCK_METHOD1(GetWebDataService, WebDataService*(ServiceAccessType access));
+};
+
+#endif  // CHROME_TEST_PROFILE_MOCK_H__