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

Re-enable migration tests that were affected.

Rewrite autofill DTC tests.

Add basic unit test for SharedChangeProcessor.

Fix bug in SharedChangeProcessor where its GenericChangeProcessor could be deleted on the wrong thread.

Renamed some tests to have 'Sync' in their names.

BUG=107743,111552
TEST=

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=120698

Review URL: https://chromiumcodereview.appspot.com/9307079

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

10 files changed, 479 insertions, 259 deletions

diff --git a/chrome/browser/sync/glue/autofill_data_type_controller_unittest.cc b/chrome/browser/sync/glue/autofill_data_type_controller_unittest.cc
index 9640e35..d8fc00b 100644
--- a/chrome/browser/sync/glue/autofill_data_type_controller_unittest.cc
+++ b/chrome/browser/sync/glue/autofill_data_type_controller_unittest.cc
@@ -2,178 +2,162 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "testing/gtest/include/gtest/gtest.h"
-
 #include "base/callback.h"
+#include "base/compiler_specific.h"
 #include "base/memory/ref_counted.h"
-#include "base/memory/scoped_ptr.h"
+#include "base/memory/weak_ptr.h"
 #include "base/message_loop.h"
-#include "base/synchronization/waitable_event.h"
-#include "base/test/test_timeouts.h"
-#include "chrome/browser/autofill/personal_data_manager.h"
+#include "chrome/browser/sync/api/sync_error.h"
 #include "chrome/browser/sync/glue/autofill_data_type_controller.h"
 #include "chrome/browser/sync/glue/data_type_controller_mock.h"
+#include "chrome/browser/sync/glue/shared_change_processor_mock.h"
 #include "chrome/browser/sync/profile_sync_components_factory_mock.h"
 #include "chrome/browser/sync/profile_sync_service_mock.h"
-#include "chrome/browser/sync/profile_sync_test_util.h"
 #include "chrome/browser/webdata/web_data_service.h"
 #include "chrome/common/chrome_notification_types.h"
 #include "chrome/test/base/profile_mock.h"
+#include "content/public/browser/notification_service.h"
 #include "content/public/browser/notification_source.h"
 #include "content/public/browser/notification_types.h"
 #include "content/test/test_browser_thread.h"
 #include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
 
-using base::WaitableEvent;
-using browser_sync::AutofillDataTypeController;
-using browser_sync::DataTypeController;
-using content::BrowserThread;
-using testing::_;
-using testing::DoAll;
-using testing::Invoke;
-using testing::Return;
-using testing::SetArgumentPointee;
+namespace browser_sync {
 
 namespace {
 
-ACTION_P(WaitOnEvent, event) {
-  event->Wait();
-}
-
-ACTION_P(SignalEvent, event) {
-  event->Signal();
-}
-
-ACTION_P(GetAutocompleteSyncComponents, wds) {
-  return base::WeakPtr<SyncableService>();
-}
-
-ACTION_P(RaiseSignal, data_controller_mock) {
-  data_controller_mock->RaiseStartSignal();
-}
+using content::BrowserThread;
+using testing::_;
+using testing::NiceMock;
+using testing::Return;
 
-// This class mocks PersonalDataManager and provides a factory method to
-// serve back the mocked object to callers of
-// |PersonalDataManagerFactory::GetForProfile()|.
-class PersonalDataManagerMock : public PersonalDataManager {
+// Fake WebDataService implementation that stubs out the database
+// loading.
+class FakeWebDataService : public WebDataService {
  public:
-  PersonalDataManagerMock() : PersonalDataManager() {}
-  virtual ~PersonalDataManagerMock() {}
-
-  static ProfileKeyedService* Build(Profile* profile) {
-    return new PersonalDataManagerMock;
+  FakeWebDataService() : is_database_loaded_(false) {}
+  virtual ~FakeWebDataService() {}
+
+  // Mark the database as loaded and send out the appropriate
+  // notification.
+  void LoadDatabase() {
+    is_database_loaded_ = true;
+    // TODO(akalin): Expose WDS::NotifyDatabaseLoadedOnUIThread() and
+    // use that instead of sending this notification manually.
+    content::NotificationService::current()->Notify(
+        chrome::NOTIFICATION_WEB_DATABASE_LOADED,
+        content::Source<WebDataService>(this),
+        content::NotificationService::NoDetails());
   }
 
-  MOCK_CONST_METHOD0(IsDataLoaded, bool());
+  virtual bool IsDatabaseLoaded() OVERRIDE {
+    return is_database_loaded_;
+  }
 
  private:
-  DISALLOW_COPY_AND_ASSIGN(PersonalDataManagerMock);
-};
-
-class WebDataServiceFake : public WebDataService {
- public:
-  explicit WebDataServiceFake() {}
+  DISALLOW_COPY_AND_ASSIGN(FakeWebDataService);
 
-  MOCK_METHOD0(IsDatabaseLoaded, bool());
-
- private:
-  DISALLOW_COPY_AND_ASSIGN(WebDataServiceFake);
+  bool is_database_loaded_;
 };
 
-class AutofillDataTypeControllerMock : public AutofillDataTypeController {
+class SyncAutofillDataTypeControllerTest : public testing::Test {
  public:
-  AutofillDataTypeControllerMock(
-      ProfileSyncComponentsFactory* profile_sync_factory,
-      Profile* profile,
-      ProfileSyncService* sync_service)
-      : AutofillDataTypeController(profile_sync_factory,
-                                   profile,
-                                   sync_service),
-        start_called_(false, false) {}
-
-  MOCK_METHOD0(StartAssociation, void());
-
-  void RaiseStartSignal() {
-    start_called_.Signal();
-  }
+  SyncAutofillDataTypeControllerTest()
+      : weak_ptr_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)),
+        ui_thread_(BrowserThread::UI, &message_loop_),
+        last_start_result_(DataTypeController::OK) {}
 
-  void WaitUntilStartCalled() {
-    start_called_.Wait();
-  }
+  virtual ~SyncAutofillDataTypeControllerTest() {}
 
- private:
-  friend class AutofillDataTypeControllerTest;
-  FRIEND_TEST_ALL_PREFIXES(AutofillDataTypeControllerTest, StartWDSReady);
-  FRIEND_TEST_ALL_PREFIXES(AutofillDataTypeControllerTest, StartWDSNotReady);
+  // We deliberately do not set up a DB thread so that we always stop
+  // with an association failure.
 
-  WaitableEvent start_called_;
+  virtual void SetUp() {
+    change_processor_ = new NiceMock<SharedChangeProcessorMock>();
 
-  DISALLOW_COPY_AND_ASSIGN(AutofillDataTypeControllerMock);
-};
+    EXPECT_CALL(profile_sync_factory_,
+                CreateSharedChangeProcessor()).
+        WillRepeatedly(Return(change_processor_.get()));
 
-class AutofillDataTypeControllerTest : public testing::Test {
- public:
-  AutofillDataTypeControllerTest()
-      : ui_thread_(BrowserThread::UI, &message_loop_),
-        db_thread_(BrowserThread::DB) {}
+    web_data_service_ = new FakeWebDataService();
 
-  virtual void SetUp() {
-    db_thread_.Start();
-    db_notification_service_ = new ThreadNotificationService(
-        db_thread_.DeprecatedGetThreadObject());
-    db_notification_service_->Init();
-    web_data_service_ = new WebDataServiceFake();
-    autofill_dtc_ =
-        new AutofillDataTypeControllerMock(&profile_sync_factory_,
-                                           &profile_,
-                                           &service_);
     EXPECT_CALL(profile_, GetWebDataService(_)).
         WillRepeatedly(Return(web_data_service_.get()));
+
+    autofill_dtc_ =
+        new AutofillDataTypeController(&profile_sync_factory_,
+                                       &profile_,
+                                       &service_);
+  }
+
+  // Passed to AutofillDTC::Start().
+  void OnStartFinished(DataTypeController::StartResult result,
+                       const SyncError& error) {
+    last_start_result_ = result;
+    last_start_error_ = error;
   }
 
   virtual void TearDown() {
-    db_notification_service_->TearDown();
-    db_thread_.Stop();
+    autofill_dtc_ = NULL;
+    web_data_service_ = NULL;
+    change_processor_ = NULL;
   }
 
  protected:
+  base::WeakPtrFactory<SyncAutofillDataTypeControllerTest> weak_ptr_factory_;
   MessageLoopForUI message_loop_;
   content::TestBrowserThread ui_thread_;
-  content::TestBrowserThread db_thread_;
-  scoped_refptr<ThreadNotificationService> db_notification_service_;
-  scoped_refptr<AutofillDataTypeControllerMock> autofill_dtc_;
+
+  scoped_refptr<NiceMock<SharedChangeProcessorMock> > change_processor_;
   ProfileSyncComponentsFactoryMock profile_sync_factory_;
-  ProfileMock profile_;
   ProfileSyncServiceMock service_;
-  scoped_refptr<WebDataServiceFake> web_data_service_;
-};
+  ProfileMock profile_;
+  scoped_refptr<FakeWebDataService> web_data_service_;
+  scoped_refptr<AutofillDataTypeController> autofill_dtc_;
 
-TEST_F(AutofillDataTypeControllerTest, StartWDSReady) {
-  EXPECT_CALL(*(web_data_service_.get()), IsDatabaseLoaded()).
-      WillOnce(Return(true));
-  EXPECT_CALL(*(autofill_dtc_.get()), StartAssociation()).Times(0);
+  // Stores arguments of most recent call of OnStartFinished().
+  DataTypeController::StartResult last_start_result_;
+  SyncError last_start_error_;
+};
 
-  autofill_dtc_->set_state(DataTypeController::MODEL_STARTING);
-  EXPECT_TRUE(autofill_dtc_->StartModels());
+// Load the WDS's database, then start the Autofill DTC.  It should
+// immediately try to start association and fail (due to missing DB
+// thread).
+TEST_F(SyncAutofillDataTypeControllerTest, StartWDSReady) {
+  web_data_service_->LoadDatabase();
+  autofill_dtc_->Start(
+      base::Bind(&SyncAutofillDataTypeControllerTest::OnStartFinished,
+                 weak_ptr_factory_.GetWeakPtr()));
+
+  EXPECT_EQ(DataTypeController::ASSOCIATION_FAILED, last_start_result_);
+  EXPECT_TRUE(last_start_error_.IsSet());
+  EXPECT_EQ(DataTypeController::NOT_RUNNING, autofill_dtc_->state());
 }
 
-TEST_F(AutofillDataTypeControllerTest, StartWDSNotReady) {
-  EXPECT_CALL(*(web_data_service_.get()), IsDatabaseLoaded()).
-      WillOnce(Return(false));
-  EXPECT_CALL(*(autofill_dtc_.get()), StartAssociation()).
-      WillOnce(RaiseSignal(autofill_dtc_.get()));
-
-  autofill_dtc_->set_state(DataTypeController::MODEL_STARTING);
-  EXPECT_FALSE(autofill_dtc_->StartModels());
-
-  scoped_refptr<ThreadNotifier> notifier(new ThreadNotifier(
-      ui_thread_.DeprecatedGetThreadObject()));
-  autofill_dtc_->Observe(
-      chrome::NOTIFICATION_WEB_DATABASE_LOADED,
-      content::Source<WebDataService>(web_data_service_.get()),
-      content::NotificationService::NoDetails());
-  autofill_dtc_->WaitUntilStartCalled();
-  EXPECT_EQ(DataTypeController::ASSOCIATING, autofill_dtc_->state());
+// Start the autofill DTC without the WDS's database loaded, then
+// start the DB.  The Autofill DTC should be in the MODEL_STARTING
+// state until the database in loaded, when it should try to start
+// association and fail (due to missing DB thread).
+TEST_F(SyncAutofillDataTypeControllerTest, StartWDSNotReady) {
+  autofill_dtc_->Start(
+      base::Bind(&SyncAutofillDataTypeControllerTest::OnStartFinished,
+                 weak_ptr_factory_.GetWeakPtr()));
+
+  EXPECT_EQ(DataTypeController::OK, last_start_result_);
+  EXPECT_FALSE(last_start_error_.IsSet());
+  EXPECT_EQ(DataTypeController::MODEL_STARTING, autofill_dtc_->state());
+
+  web_data_service_->LoadDatabase();
+
+  EXPECT_EQ(DataTypeController::ASSOCIATION_FAILED, last_start_result_);
+  EXPECT_TRUE(last_start_error_.IsSet());
+  // There's a TODO for
+  // NonFrontendDataTypeController::StartAssociationAsync() that, when
+  // done, will make this consistent with the previous test.
+  EXPECT_EQ(DataTypeController::DISABLED, autofill_dtc_->state());
 }
 
 }  // namespace
+
+}  // namespace browser_sync
diff --git a/chrome/browser/sync/glue/new_non_frontend_data_type_controller.cc b/chrome/browser/sync/glue/new_non_frontend_data_type_controller.cc
index 51a2226..e4e4bd4 100644
--- a/chrome/browser/sync/glue/new_non_frontend_data_type_controller.cc
+++ b/chrome/browser/sync/glue/new_non_frontend_data_type_controller.cc
@@ -17,8 +17,7 @@ using content::BrowserThread;
 
 namespace browser_sync {
 
-NewNonFrontendDataTypeController::NewNonFrontendDataTypeController()
-    : shared_change_processor_(NULL) {}
+NewNonFrontendDataTypeController::NewNonFrontendDataTypeController() {}
 
 NewNonFrontendDataTypeController::NewNonFrontendDataTypeController(
     ProfileSyncComponentsFactory* profile_sync_factory,
@@ -26,9 +25,7 @@ NewNonFrontendDataTypeController::NewNonFrontendDataTypeController(
     ProfileSyncService* sync_service)
     : NonFrontendDataTypeController(profile_sync_factory,
                                     profile,
-                                    sync_service),
-      shared_change_processor_(NULL) {
-}
+                                    sync_service) {}
 
 NewNonFrontendDataTypeController::~NewNonFrontendDataTypeController() {}
 
@@ -43,8 +40,12 @@ void NewNonFrontendDataTypeController::Start(
 
   set_start_callback(start_callback);
 
+  // Since we can't be called multiple times before Stop() is called,
+  // |shared_change_processor_| must be NULL here.
+  DCHECK(!shared_change_processor_.get());
   shared_change_processor_ =
       profile_sync_factory()->CreateSharedChangeProcessor();
+  DCHECK(shared_change_processor_.get());
 
   set_state(MODEL_STARTING);
   if (!StartModels()) {
@@ -57,20 +58,34 @@ void NewNonFrontendDataTypeController::Start(
   // Kick off association on the thread the datatype resides on.
   set_state(ASSOCIATING);
   if (!StartAssociationAsync()) {
-    // Releasing the shared_change_processor_ here is safe since it was never
-    // connected.
-    shared_change_processor_ = NULL;
     SyncError error(FROM_HERE, "Failed to post StartAssociation", type());
     StartDoneImpl(ASSOCIATION_FAILED, NOT_RUNNING, error);
+    // StartDoneImpl should have called ClearSharedChangeProcessor();
+    DCHECK(!shared_change_processor_.get());
+    return;
   }
 }
 
+bool NewNonFrontendDataTypeController::StartAssociationAsync() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK_EQ(state(), ASSOCIATING);
+  return PostTaskOnBackendThread(
+      FROM_HERE,
+      base::Bind(
+          &NewNonFrontendDataTypeController::
+              StartAssociationWithSharedChangeProcessor,
+          this,
+          shared_change_processor_));
+}
+
 // This method can execute after we've already stopped (and possibly even
 // destroyed) both the Syncer and the SyncableService. As a result, all actions
 // must either have no side effects outside of the DTC or must be protected
-// by |shared_change_processor_|, which is guaranteed to have been Disconnected
+// by |shared_change_processor|, which is guaranteed to have been Disconnected
 // if the syncer shut down.
-void NewNonFrontendDataTypeController::StartAssociation() {
+void NewNonFrontendDataTypeController::
+    StartAssociationWithSharedChangeProcessor(
+        const scoped_refptr<SharedChangeProcessor>& shared_change_processor) {
   DCHECK(!BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK_EQ(state(), ASSOCIATING);
 
@@ -88,25 +103,27 @@ void NewNonFrontendDataTypeController::StartAssociation() {
     return;
   }
 
-  // Connect |shared_change_processor_| to the syncer and |local_service_|.
-  // Note that it's possible the shared_change_processor_ has already been
+  DCHECK(shared_change_processor.get());
+
+  // Connect |shared_change_processor| to the syncer and |local_service_|.
+  // Note that it's possible the shared_change_processor has already been
   // disconnected at this point, so all our accesses to the syncer from this
   // point on are through it.
-  if (!shared_change_processor_->Connect(profile_sync_factory(),
-                                         profile_sync_service(),
-                                         this,
-                                         local_service_)) {
+  if (!shared_change_processor->Connect(profile_sync_factory(),
+                                        profile_sync_service(),
+                                        this,
+                                        local_service_)) {
     SyncError error(FROM_HERE, "Failed to connect to syncer.", type());
     StartFailed(UNRECOVERABLE_ERROR, error);
   }
 
-  if (!shared_change_processor_->CryptoReadyIfNecessary(type())) {
+  if (!shared_change_processor->CryptoReadyIfNecessary(type())) {
     StartFailed(NEEDS_CRYPTO, SyncError());
     return;
   }
 
   bool sync_has_nodes = false;
-  if (!shared_change_processor_->SyncModelHasUserCreatedNodes(
+  if (!shared_change_processor->SyncModelHasUserCreatedNodes(
           type(), &sync_has_nodes)) {
     SyncError error(FROM_HERE, "Failed to load sync nodes", type());
     StartFailed(UNRECOVERABLE_ERROR, error);
@@ -116,17 +133,17 @@ void NewNonFrontendDataTypeController::StartAssociation() {
   base::TimeTicks start_time = base::TimeTicks::Now();
   SyncError error;
   SyncDataList initial_sync_data;
-  error = shared_change_processor_->GetSyncDataForType(type(),
-                                                       &initial_sync_data);
+  error = shared_change_processor->GetSyncDataForType(type(),
+                                                      &initial_sync_data);
   if (error.IsSet()) {
     StartFailed(ASSOCIATION_FAILED, error);
     return;
   }
-  // Passes a reference to the shared_change_processor_;
+  // Passes a reference to |shared_change_processor|.
   error = local_service_->MergeDataAndStartSyncing(
       type(),
       initial_sync_data,
-      new SharedChangeProcessorRef(shared_change_processor_));
+      new SharedChangeProcessorRef(shared_change_processor));
   RecordAssociationTime(base::TimeTicks::Now() - start_time);
   if (error.IsSet()) {
     StartFailed(ASSOCIATION_FAILED, error);
@@ -134,13 +151,13 @@ void NewNonFrontendDataTypeController::StartAssociation() {
   }
 
   // If we've been disconnected, profile_sync_service() may return an invalid
-  // pointer, but the shared_change_processor_ protects us from attempting to
+  // pointer, but |shared_change_processor| protects us from attempting to
   // access it.
   // Note: This must be done on the datatype's thread to ensure local_service_
   // doesn't start trying to push changes from it's thread before we activate
   // the datatype.
-  shared_change_processor_->ActivateDataType(profile_sync_service(),
-                                             type(), model_safe_group());
+  shared_change_processor->ActivateDataType(profile_sync_service(),
+                                            type(), model_safe_group());
   StartDone(!sync_has_nodes ? OK_FIRST_RUN : OK, RUNNING, SyncError());
 }
 
@@ -162,13 +179,11 @@ void NewNonFrontendDataTypeController::StartDoneImpl(
     DataTypeController::StartResult result,
     DataTypeController::State new_state,
     const SyncError& error) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   // If we failed to start up, and we haven't been stopped yet, we need to
   // ensure we clean up the local service and shared change processor properly.
-  if (new_state != RUNNING && state() != NOT_RUNNING && state() != STOPPING &&
-      shared_change_processor_.get()) {
-    shared_change_processor_->Disconnect();
-    // We release our reference to shared_change_processor on the datatype's
-    // thread.
+  if (new_state != RUNNING && state() != NOT_RUNNING && state() != STOPPING) {
+    ClearSharedChangeProcessor();
     StopLocalServiceAsync();
   }
   NonFrontendDataTypeController::StartDoneImpl(result, new_state, error);
@@ -182,12 +197,10 @@ void NewNonFrontendDataTypeController::Stop() {
     return;
   }
 
-  // Disconnect the change processor. At this point, the SyncableService
-  // can no longer interact with the Syncer, even if it hasn't finished
-  // MergeDataAndStartSyncing. It may still have ownership of the shared
-  // change processor though.
-  if (shared_change_processor_.get())
-    shared_change_processor_->Disconnect();
+  // Disconnect the change processor. At this point, the
+  // SyncableService can no longer interact with the Syncer, even if
+  // it hasn't finished MergeDataAndStartSyncing.
+  ClearSharedChangeProcessor();
 
   // If we haven't finished starting, we need to abort the start.
   switch (state()) {
@@ -227,6 +240,16 @@ void NewNonFrontendDataTypeController::Stop() {
   set_state(NOT_RUNNING);
 }
 
+void NewNonFrontendDataTypeController::ClearSharedChangeProcessor() {
+  // We are called only from StartDoneImpl() or Stop(), both of which
+  // can be called only after a call to Start().  Therefore,
+  // |shared_change_processor_| should not be NULL.
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK(shared_change_processor_.get());
+  shared_change_processor_->Disconnect();
+  shared_change_processor_ = NULL;
+}
+
 void NewNonFrontendDataTypeController::StopLocalServiceAsync() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   PostTaskOnBackendThread(
@@ -239,7 +262,6 @@ void NewNonFrontendDataTypeController::StopLocalService() {
   if (local_service_.get())
     local_service_->StopSyncing(type());
   local_service_.reset();
-  shared_change_processor_ = NULL;
 }
 
 void NewNonFrontendDataTypeController::CreateSyncComponents() {
diff --git a/chrome/browser/sync/glue/new_non_frontend_data_type_controller.h b/chrome/browser/sync/glue/new_non_frontend_data_type_controller.h
index 64e65d6..2dd3ffd 100644
--- a/chrome/browser/sync/glue/new_non_frontend_data_type_controller.h
+++ b/chrome/browser/sync/glue/new_non_frontend_data_type_controller.h
@@ -30,13 +30,13 @@ class NewNonFrontendDataTypeController : public NonFrontendDataTypeController {
   NewNonFrontendDataTypeController();
 
   // Overrides of NonFrontendDataTypeController methods.
-  virtual void StartAssociation() OVERRIDE;
   virtual void StartDone(DataTypeController::StartResult result,
                          DataTypeController::State new_state,
                          const SyncError& error) OVERRIDE;
   virtual void StartDoneImpl(DataTypeController::StartResult result,
                              DataTypeController::State new_state,
                              const SyncError& error) OVERRIDE;
+  virtual bool StartAssociationAsync() OVERRIDE;
 
   // Extract/create the syncable service from the profile and return a
   // WeakHandle to it.
@@ -44,34 +44,46 @@ class NewNonFrontendDataTypeController : public NonFrontendDataTypeController {
       const = 0;
 
  private:
+  // Posted on the backend thread by StartAssociationAsync().
+  void StartAssociationWithSharedChangeProcessor(
+      const scoped_refptr<SharedChangeProcessor>& shared_change_processor);
+
+  // Calls Disconnect() on |shared_change_processor_|, then sets it to
+  // NULL.  Must be called only by StartDoneImpl() or Stop() (on the
+  // UI thread) and only after a call to Start() (i.e.,
+  // |shared_change_processor_| must be non-NULL).
+  void ClearSharedChangeProcessor();
+
   // Posts StopLocalService() to the datatype's thread.
   void StopLocalServiceAsync();
 
-  // Calls local_service_->StopSyncing() and releases our references to it and
-  // |shared_change_processor_|.
+  // Calls local_service_->StopSyncing() and releases our references to it.
   void StopLocalService();
 
   // Deprecated.
   virtual void CreateSyncComponents() OVERRIDE;
 
+  // The shared change processor is the thread-safe interface to the
+  // datatype.  We hold a reference to it from the UI thread so that
+  // we can call Disconnect() on it from Stop()/StartDoneImpl().  Most
+  // of the work is done on the backend thread, and in
+  // StartAssociationWithSharedChangeProcessor() for this class in
+  // particular.
+  //
+  // Lifetime: The SharedChangeProcessor object is created on the UI
+  // thread and passed on to the backend thread.  This reference is
+  // released on the UI thread in Stop()/StartDoneImpl(), but the
+  // backend thread may still have references to it (which is okay,
+  // since we call Disconnect() before releasing the UI thread
+  // reference).
+  scoped_refptr<SharedChangeProcessor> shared_change_processor_;
+
   // A weak pointer to the actual local syncable service, which performs all the
   // real work. We do not own the object, and it is only safe to access on the
   // DataType's thread.
   // Lifetime: it gets set in StartAssociation() and released in
   // StopLocalService().
   base::WeakPtr<SyncableService> local_service_;
-
-  // The thread-safe layer between the datatype and the syncer. It allows us to
-  // disconnect both the datatype and ourselves from the syncer at shutdown
-  // time. All accesses to the syncer from any thread other than the UI thread
-  // should be through this. Once connected, it must be released on the
-  // datatype's thread (but if we never connect it we can destroy it on the UI
-  // thread as well).
-  // Lifetime: It gets created on the UI thread in Start(..), connected to
-  // the syncer in StartAssociation() on the datatype's thread, and if
-  // successfully connected released in StopLocalService() on the datatype's
-  // thread.
-  scoped_refptr<SharedChangeProcessor> shared_change_processor_;
 };
 
 }  // namespace browser_sync
diff --git a/chrome/browser/sync/glue/new_non_frontend_data_type_controller_unittest.cc b/chrome/browser/sync/glue/new_non_frontend_data_type_controller_unittest.cc
index ab80ada..71983bd 100644
--- a/chrome/browser/sync/glue/new_non_frontend_data_type_controller_unittest.cc
+++ b/chrome/browser/sync/glue/new_non_frontend_data_type_controller_unittest.cc
@@ -24,27 +24,21 @@
 #include "content/test/test_browser_thread.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+namespace browser_sync {
+
+namespace {
+
 using base::WaitableEvent;
-using browser_sync::GenericChangeProcessor;
-using browser_sync::SharedChangeProcessorMock;
-using browser_sync::DataTypeController;
-using browser_sync::GROUP_DB;
-using browser_sync::NewNonFrontendDataTypeController;
-using browser_sync::NewNonFrontendDataTypeControllerMock;
-using browser_sync::StartCallbackMock;
 using content::BrowserThread;
 using syncable::AUTOFILL_PROFILE;
 using testing::_;
 using testing::DoAll;
 using testing::InvokeWithoutArgs;
+using testing::Mock;
 using testing::Return;
 using testing::SetArgumentPointee;
 using testing::StrictMock;
 
-namespace browser_sync {
-
-namespace {
-
 ACTION_P(WaitOnEvent, event) {
   event->Wait();
 }
@@ -74,6 +68,7 @@ class NewNonFrontendDataTypeControllerFake
       : NewNonFrontendDataTypeController(profile_sync_factory,
                                          profile,
                                          sync_service),
+        blocked_(false),
         mock_(mock) {}
 
   virtual syncable::ModelType type() const OVERRIDE {
@@ -83,6 +78,23 @@ class NewNonFrontendDataTypeControllerFake
     return GROUP_DB;
   }
 
+  // Prevent tasks from being posted on the backend thread until
+  // UnblockBackendTasks() is called.
+  void BlockBackendTasks() {
+    blocked_ = true;
+  }
+
+  // Post pending tasks on the backend thread and start allowing tasks
+  // to be posted on the backend thread again.
+  void UnblockBackendTasks() {
+    blocked_ = false;
+    for (std::vector<PendingTask>::const_iterator it = pending_tasks_.begin();
+         it != pending_tasks_.end(); ++it) {
+      PostTaskOnBackendThread(it->from_here, it->task);
+    }
+    pending_tasks_.clear();
+  }
+
  protected:
   virtual base::WeakPtr<SyncableService>
       GetWeakPtrToSyncableService() const OVERRIDE {
@@ -92,7 +104,12 @@ class NewNonFrontendDataTypeControllerFake
   virtual bool PostTaskOnBackendThread(
       const tracked_objects::Location& from_here,
       const base::Closure& task) OVERRIDE {
-    return BrowserThread::PostTask(BrowserThread::DB, from_here, task);
+    if (blocked_) {
+      pending_tasks_.push_back(PendingTask(from_here, task));
+      return true;
+    } else {
+      return BrowserThread::PostTask(BrowserThread::DB, from_here, task);
+    }
   }
 
   // We mock the following methods because their default implementations do
@@ -115,13 +132,27 @@ class NewNonFrontendDataTypeControllerFake
       OVERRIDE {
     mock_->RecordStartFailure(result);
   }
+
  private:
+  DISALLOW_COPY_AND_ASSIGN(NewNonFrontendDataTypeControllerFake);
+
+  struct PendingTask {
+    PendingTask(const tracked_objects::Location& from_here,
+                const base::Closure& task)
+        : from_here(from_here), task(task) {}
+
+    tracked_objects::Location from_here;
+    base::Closure task;
+  };
+
+  bool blocked_;
+  std::vector<PendingTask> pending_tasks_;
   NewNonFrontendDataTypeControllerMock* mock_;
 };
 
-class NewNonFrontendDataTypeControllerTest : public testing::Test {
+class SyncNewNonFrontendDataTypeControllerTest : public testing::Test {
  public:
-  NewNonFrontendDataTypeControllerTest()
+  SyncNewNonFrontendDataTypeControllerTest()
       : ui_thread_(BrowserThread::UI, &message_loop_),
         db_thread_(BrowserThread::DB) {}
 
@@ -148,7 +179,7 @@ class NewNonFrontendDataTypeControllerTest : public testing::Test {
   void WaitForDTC() {
     WaitableEvent done(true, false);
     BrowserThread::PostTask(BrowserThread::DB, FROM_HERE,
-       base::Bind(&NewNonFrontendDataTypeControllerTest::SignalDone,
+       base::Bind(&SyncNewNonFrontendDataTypeControllerTest::SignalDone,
                   &done));
     done.TimedWait(base::TimeDelta::FromMilliseconds(
         TestTimeouts::action_timeout_ms()));
@@ -173,6 +204,7 @@ class NewNonFrontendDataTypeControllerTest : public testing::Test {
     EXPECT_CALL(*change_processor_, Connect(_,_,_,_)).WillOnce(Return(true));
     EXPECT_CALL(*change_processor_, CryptoReadyIfNecessary(_)).
         WillOnce(Return(true));
+    EXPECT_CALL(*change_processor_, ActivateDataType(_, _, _));
     EXPECT_CALL(*change_processor_, SyncModelHasUserCreatedNodes(_,_)).
         WillOnce(DoAll(SetArgumentPointee<1>(true), Return(true)));
     EXPECT_CALL(*change_processor_, GetSyncDataForType(_,_)).
@@ -184,7 +216,6 @@ class NewNonFrontendDataTypeControllerTest : public testing::Test {
   }
 
   void SetActivateExpectations(DataTypeController::StartResult result) {
-    EXPECT_CALL(service_, ActivateDataType(_, _, _));
     EXPECT_CALL(start_callback_, Run(result,_));
   }
 
@@ -221,7 +252,7 @@ class NewNonFrontendDataTypeControllerTest : public testing::Test {
   scoped_ptr<SyncChangeProcessor> saved_change_processor_;
 };
 
-TEST_F(NewNonFrontendDataTypeControllerTest, StartOk) {
+TEST_F(SyncNewNonFrontendDataTypeControllerTest, StartOk) {
   SetStartExpectations();
   SetAssociateExpectations();
   SetActivateExpectations(DataTypeController::OK);
@@ -232,7 +263,7 @@ TEST_F(NewNonFrontendDataTypeControllerTest, StartOk) {
   EXPECT_EQ(DataTypeController::RUNNING, new_non_frontend_dtc_->state());
 }
 
-TEST_F(NewNonFrontendDataTypeControllerTest, StartFirstRun) {
+TEST_F(SyncNewNonFrontendDataTypeControllerTest, StartFirstRun) {
   SetStartExpectations();
   EXPECT_CALL(*change_processor_, Connect(_,_,_,_)).WillOnce(Return(true));
   EXPECT_CALL(*change_processor_, CryptoReadyIfNecessary(_)).
@@ -253,7 +284,12 @@ TEST_F(NewNonFrontendDataTypeControllerTest, StartFirstRun) {
   EXPECT_EQ(DataTypeController::RUNNING, new_non_frontend_dtc_->state());
 }
 
-TEST_F(NewNonFrontendDataTypeControllerTest, AbortDuringStartModels) {
+// Start the DTC and have StartModels() return false.  Then, stop the
+// DTC without finishing model startup.  It should stop cleanly.
+TEST_F(SyncNewNonFrontendDataTypeControllerTest, AbortDuringStartModels) {
+  EXPECT_CALL(*profile_sync_factory_,
+              CreateSharedChangeProcessor()).
+      WillOnce(Return(change_processor_.get()));
   EXPECT_CALL(*dtc_mock_, StartModels()).WillOnce(Return(false));
   EXPECT_CALL(*dtc_mock_, StopModels());
   EXPECT_CALL(*dtc_mock_, RecordStartFailure(DataTypeController::ABORTED));
@@ -267,7 +303,7 @@ TEST_F(NewNonFrontendDataTypeControllerTest, AbortDuringStartModels) {
   EXPECT_EQ(DataTypeController::NOT_RUNNING, new_non_frontend_dtc_->state());
 }
 
-TEST_F(NewNonFrontendDataTypeControllerTest, StartAssociationFailed) {
+TEST_F(SyncNewNonFrontendDataTypeControllerTest, StartAssociationFailed) {
   SetStartExpectations();
   EXPECT_CALL(*change_processor_, Connect(_,_,_,_)).WillOnce(Return(true));
   EXPECT_CALL(*change_processor_, CryptoReadyIfNecessary(_)).
@@ -289,7 +325,7 @@ TEST_F(NewNonFrontendDataTypeControllerTest, StartAssociationFailed) {
   EXPECT_EQ(DataTypeController::DISABLED, new_non_frontend_dtc_->state());
 }
 
-TEST_F(NewNonFrontendDataTypeControllerTest,
+TEST_F(SyncNewNonFrontendDataTypeControllerTest,
        StartAssociationTriggersUnrecoverableError) {
   SetStartExpectations();
   SetStartFailExpectations(DataTypeController::UNRECOVERABLE_ERROR);
@@ -306,7 +342,8 @@ TEST_F(NewNonFrontendDataTypeControllerTest,
   EXPECT_EQ(DataTypeController::NOT_RUNNING, new_non_frontend_dtc_->state());
 }
 
-TEST_F(NewNonFrontendDataTypeControllerTest, StartAssociationCryptoNotReady) {
+TEST_F(SyncNewNonFrontendDataTypeControllerTest,
+       StartAssociationCryptoNotReady) {
   SetStartExpectations();
   SetStartFailExpectations(DataTypeController::NEEDS_CRYPTO);
   // Set up association to fail with a NEEDS_CRYPTO error.
@@ -322,7 +359,7 @@ TEST_F(NewNonFrontendDataTypeControllerTest, StartAssociationCryptoNotReady) {
 
 // Trigger a Stop() call when we check if the model associator has user created
 // nodes.
-TEST_F(NewNonFrontendDataTypeControllerTest, AbortDuringAssociation) {
+TEST_F(SyncNewNonFrontendDataTypeControllerTest, AbortDuringAssociation) {
   WaitableEvent wait_for_db_thread_pause(false, false);
   WaitableEvent pause_db_thread(false, false);
 
@@ -351,7 +388,25 @@ TEST_F(NewNonFrontendDataTypeControllerTest, AbortDuringAssociation) {
   EXPECT_EQ(DataTypeController::NOT_RUNNING, new_non_frontend_dtc_->state());
 }
 
-TEST_F(NewNonFrontendDataTypeControllerTest, Stop) {
+TEST_F(SyncNewNonFrontendDataTypeControllerTest, Stop) {
+  SetStartExpectations();
+  SetAssociateExpectations();
+  SetActivateExpectations(DataTypeController::OK);
+  SetStopExpectations();
+  EXPECT_EQ(DataTypeController::NOT_RUNNING, new_non_frontend_dtc_->state());
+  new_non_frontend_dtc_->Start(
+      base::Bind(&StartCallbackMock::Run, base::Unretained(&start_callback_)));
+  WaitForDTC();
+  EXPECT_EQ(DataTypeController::RUNNING, new_non_frontend_dtc_->state());
+  new_non_frontend_dtc_->Stop();
+  EXPECT_EQ(DataTypeController::NOT_RUNNING, new_non_frontend_dtc_->state());
+}
+
+// Start the DTC then block its backend tasks.  While its backend
+// tasks are blocked, stop and start it again, then unblock its
+// backend tasks.  The (delayed) running of the backend tasks from the
+// stop after the restart shouldn't cause any problems.
+TEST_F(SyncNewNonFrontendDataTypeControllerTest, StopStart) {
   SetStartExpectations();
   SetAssociateExpectations();
   SetActivateExpectations(DataTypeController::OK);
@@ -361,11 +416,23 @@ TEST_F(NewNonFrontendDataTypeControllerTest, Stop) {
       base::Bind(&StartCallbackMock::Run, base::Unretained(&start_callback_)));
   WaitForDTC();
   EXPECT_EQ(DataTypeController::RUNNING, new_non_frontend_dtc_->state());
+
+  new_non_frontend_dtc_->BlockBackendTasks();
   new_non_frontend_dtc_->Stop();
+  Mock::VerifyAndClearExpectations(&profile_sync_factory_);
+  SetStartExpectations();
+  SetAssociateExpectations();
+  SetActivateExpectations(DataTypeController::OK);
   EXPECT_EQ(DataTypeController::NOT_RUNNING, new_non_frontend_dtc_->state());
+  new_non_frontend_dtc_->Start(
+      base::Bind(&StartCallbackMock::Run, base::Unretained(&start_callback_)));
+  new_non_frontend_dtc_->UnblockBackendTasks();
+
+  WaitForDTC();
+  EXPECT_EQ(DataTypeController::RUNNING, new_non_frontend_dtc_->state());
 }
 
-TEST_F(NewNonFrontendDataTypeControllerTest, OnUnrecoverableError) {
+TEST_F(SyncNewNonFrontendDataTypeControllerTest, OnUnrecoverableError) {
   SetStartExpectations();
   SetAssociateExpectations();
   SetActivateExpectations(DataTypeController::OK);
diff --git a/chrome/browser/sync/glue/non_frontend_data_type_controller.h b/chrome/browser/sync/glue/non_frontend_data_type_controller.h
index 55f56d9..cfe7740 100644
--- a/chrome/browser/sync/glue/non_frontend_data_type_controller.h
+++ b/chrome/browser/sync/glue/non_frontend_data_type_controller.h
@@ -66,8 +66,8 @@ class NonFrontendDataTypeController : public DataTypeController {
   // associate models. The default implementation is a no-op.
   // Return value:
   //   True - if models are ready and association can proceed.
-  //   False - if models are not ready. KickOffAssociation should be called
-  //           when the models are ready. Refer to Start(_) implementation.
+  //   False - if models are not ready. StartAssociationAsync should be called
+  //           when the models are ready.
   // Note: this is performed on the frontend (UI) thread.
   virtual bool StartModels();
 
@@ -78,10 +78,6 @@ class NonFrontendDataTypeController : public DataTypeController {
       const tracked_objects::Location& from_here,
       const base::Closure& task) = 0;
 
-  // Build sync components and associate models.
-  // Note: this is performed on the datatype's thread.
-  virtual void StartAssociation();
-
   // Datatype specific creation of sync components.
   // Note: this is performed on the datatype's thread.
   virtual void CreateSyncComponents() = 0;
@@ -107,10 +103,6 @@ class NonFrontendDataTypeController : public DataTypeController {
   // Note: this is performed on the frontend (UI) thread.
   virtual void StopModels();
 
-  // Disassociate the models and destroy the sync components.
-  // Note: this is performed on the datatype's thread.
-  virtual void StopAssociation();
-
   // Implementation of OnUnrecoverableError that lives on UI thread.
   virtual void OnUnrecoverableErrorImpl(
       const tracked_objects::Location& from_here,
@@ -131,12 +123,12 @@ class NonFrontendDataTypeController : public DataTypeController {
   // Post the association task to the thread the datatype lives on.
   // Note: this is performed on the frontend (UI) thread.
   // Return value: True if task posted successfully, False otherwise.
-  bool StartAssociationAsync();
-
-  // Post the StopAssociation task to the thread the datatype lives on.
-  // Note: this is performed on the frontend (UI) thread.
-  // Return value: True if task posted successfully, False otherwise.
-  bool StopAssociationAsync();
+  //
+  // TODO(akalin): Callers handle false return values inconsistently;
+  // some set the state to NOT_RUNNING, and some set the state to
+  // DISABLED.  Move the error handling inside this function to be
+  // consistent.
+  virtual bool StartAssociationAsync();
 
   // Accessors and mutators used by derived classes.
   ProfileSyncComponentsFactory* profile_sync_factory() const;
@@ -151,6 +143,19 @@ class NonFrontendDataTypeController : public DataTypeController {
   virtual void set_change_processor(ChangeProcessor* change_processor);
 
  private:
+  // Build sync components and associate models.
+  // Note: this is performed on the datatype's thread.
+  void StartAssociation();
+
+  // Post the StopAssociation task to the thread the datatype lives on.
+  // Note: this is performed on the frontend (UI) thread.
+  // Return value: True if task posted successfully, False otherwise.
+  bool StopAssociationAsync();
+
+  // Disassociate the models and destroy the sync components.
+  // Note: this is performed on the datatype's thread.
+  void StopAssociation();
+
   ProfileSyncComponentsFactory* const profile_sync_factory_;
   Profile* const profile_;
   ProfileSyncService* const profile_sync_service_;
diff --git a/chrome/browser/sync/glue/shared_change_processor.cc b/chrome/browser/sync/glue/shared_change_processor.cc
index d0dd7c3..c6afd9e 100644
--- a/chrome/browser/sync/glue/shared_change_processor.cc
+++ b/chrome/browser/sync/glue/shared_change_processor.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 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.
 
@@ -16,18 +16,28 @@ using content::BrowserThread;
 namespace browser_sync {
 
 SharedChangeProcessor::SharedChangeProcessor()
-    : disconnected_(false) {
+    : disconnected_(false),
+      generic_change_processor_(NULL) {
   // We're always created on the UI thread.
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  DetachFromThread();
 }
 
 SharedChangeProcessor::~SharedChangeProcessor() {
-  // We can either be deleted when the DTC is destroyed (on UI thread),
-  // or when the SyncableService stop's syncing (datatype thread).
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI) ||
-         CalledOnValidThread());
-  DetachFromThread();
+  // We can either be deleted when the DTC is destroyed (on UI
+  // thread), or when the SyncableService stop's syncing (datatype
+  // thread).  |generic_change_processor_|, if non-NULL, must be
+  // deleted on |backend_loop_|.
+  if (BrowserThread::CurrentlyOn(BrowserThread::UI)) {
+    if (backend_loop_.get()) {
+      backend_loop_->DeleteSoon(FROM_HERE, generic_change_processor_);
+    } else {
+      DCHECK(!generic_change_processor_);
+    }
+  } else {
+    DCHECK(backend_loop_.get());
+    DCHECK(backend_loop_->BelongsToCurrentThread());
+    delete generic_change_processor_;
+  }
 }
 
 bool SharedChangeProcessor::Connect(
@@ -35,7 +45,7 @@ bool SharedChangeProcessor::Connect(
     ProfileSyncService* sync_service,
     UnrecoverableErrorHandler* error_handler,
     const base::WeakPtr<SyncableService>& local_service) {
-  DCHECK(CalledOnValidThread());
+  backend_loop_ = base::MessageLoopProxy::current();
   AutoLock lock(monitor_lock_);
   if (disconnected_)
     return false;
@@ -44,10 +54,10 @@ bool SharedChangeProcessor::Connect(
     disconnected_ = true;
     return false;
   }
-  generic_change_processor_.reset(
+  generic_change_processor_ =
       sync_factory->CreateGenericChangeProcessor(sync_service,
                                                  error_handler,
-                                                 local_service));
+                                                 local_service);
   return true;
 }
 
@@ -63,7 +73,8 @@ bool SharedChangeProcessor::Disconnect() {
 SyncError SharedChangeProcessor::GetSyncDataForType(
     syncable::ModelType type,
     SyncDataList* current_sync_data) {
-  DCHECK(CalledOnValidThread());
+  DCHECK(backend_loop_.get());
+  DCHECK(backend_loop_->BelongsToCurrentThread());
   AutoLock lock(monitor_lock_);
   if (disconnected_) {
     SyncError error(FROM_HERE, "Change processor disconnected.", type);
@@ -75,7 +86,8 @@ SyncError SharedChangeProcessor::GetSyncDataForType(
 SyncError SharedChangeProcessor::ProcessSyncChanges(
     const tracked_objects::Location& from_here,
     const SyncChangeList& list_of_changes) {
-  DCHECK(CalledOnValidThread());
+  DCHECK(backend_loop_.get());
+  DCHECK(backend_loop_->BelongsToCurrentThread());
   AutoLock lock(monitor_lock_);
   if (disconnected_) {
     // The DTC that disconnects us must ensure it posts a StopSyncing task.
@@ -94,7 +106,8 @@ SyncError SharedChangeProcessor::ProcessSyncChanges(
 bool SharedChangeProcessor::SyncModelHasUserCreatedNodes(
     syncable::ModelType type,
     bool* has_nodes) {
-  DCHECK(CalledOnValidThread());
+  DCHECK(backend_loop_.get());
+  DCHECK(backend_loop_->BelongsToCurrentThread());
   AutoLock lock(monitor_lock_);
   if (disconnected_) {
     LOG(ERROR) << "Change processor disconnected.";
@@ -105,7 +118,8 @@ bool SharedChangeProcessor::SyncModelHasUserCreatedNodes(
 }
 
 bool SharedChangeProcessor::CryptoReadyIfNecessary(syncable::ModelType type) {
-  DCHECK(CalledOnValidThread());
+  DCHECK(backend_loop_.get());
+  DCHECK(backend_loop_->BelongsToCurrentThread());
   AutoLock lock(monitor_lock_);
   if (disconnected_) {
     LOG(ERROR) << "Change processor disconnected.";
@@ -118,13 +132,15 @@ void SharedChangeProcessor::ActivateDataType(
     ProfileSyncService* sync_service,
     syncable::ModelType model_type,
     browser_sync::ModelSafeGroup model_safe_group) {
+  DCHECK(backend_loop_.get());
+  DCHECK(backend_loop_->BelongsToCurrentThread());
   AutoLock lock(monitor_lock_);
   if (disconnected_) {
     LOG(ERROR) << "Change processor disconnected.";
     return;
   }
   sync_service->ActivateDataType(
-      model_type, model_safe_group, generic_change_processor_.get());
+      model_type, model_safe_group, generic_change_processor_);
 }
 
 }  // namespace browser_sync
diff --git a/chrome/browser/sync/glue/shared_change_processor.h b/chrome/browser/sync/glue/shared_change_processor.h
index a15308e..3fa8550 100644
--- a/chrome/browser/sync/glue/shared_change_processor.h
+++ b/chrome/browser/sync/glue/shared_change_processor.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 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.
 
@@ -8,10 +8,9 @@
 
 #include "base/location.h"
 #include "base/memory/ref_counted.h"
-#include "base/memory/scoped_ptr.h"
 #include "base/memory/weak_ptr.h"
+#include "base/message_loop_proxy.h"
 #include "base/synchronization/lock.h"
-#include "base/threading/thread_checker.h"
 #include "chrome/browser/sync/api/sync_change_processor.h"
 #include "chrome/browser/sync/api/sync_error.h"
 #include "chrome/browser/sync/engine/model_safe_worker.h"
@@ -46,8 +45,7 @@ class UnrecoverableErrorHandler;
 //
 // We use virtual methods so that we can use mock's in testing.
 class SharedChangeProcessor
-    : public base::RefCountedThreadSafe<SharedChangeProcessor>,
-      public base::ThreadChecker {
+    : public base::RefCountedThreadSafe<SharedChangeProcessor> {
  public:
   // Create an uninitialized SharedChangeProcessor (to be later connected).
   SharedChangeProcessor();
@@ -103,7 +101,12 @@ class SharedChangeProcessor
   mutable base::Lock monitor_lock_;
   bool disconnected_;
 
-  scoped_ptr<GenericChangeProcessor> generic_change_processor_;
+  // The loop that all methods except the constructor, destructor, and
+  // Disconnect() should be called on.  Set in Connect().
+  scoped_refptr<base::MessageLoopProxy> backend_loop_;
+
+  // Used only on |backend_loop_|.
+  GenericChangeProcessor* generic_change_processor_;
 
   DISALLOW_COPY_AND_ASSIGN(SharedChangeProcessor);
 };
diff --git a/chrome/browser/sync/glue/shared_change_processor_mock.h b/chrome/browser/sync/glue/shared_change_processor_mock.h
index 6e2d55d..8fb91b4 100644
--- a/chrome/browser/sync/glue/shared_change_processor_mock.h
+++ b/chrome/browser/sync/glue/shared_change_processor_mock.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 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.
 
@@ -18,21 +18,24 @@ class SharedChangeProcessorMock : public SharedChangeProcessor {
   SharedChangeProcessorMock();
 
   MOCK_METHOD4(Connect, bool(
-      ProfileSyncComponentsFactory* sync_factory,
-      ProfileSyncService* sync_service,
-      UnrecoverableErrorHandler* error_handler,
-      const base::WeakPtr<SyncableService>& local_service));
+      ProfileSyncComponentsFactory*,
+      ProfileSyncService*,
+      UnrecoverableErrorHandler*,
+      const base::WeakPtr<SyncableService>&));
   MOCK_METHOD0(Disconnect, bool());
   MOCK_METHOD2(ProcessSyncChanges,
-               SyncError(const tracked_objects::Location& from_here,
-                         const SyncChangeList& change_list));
+               SyncError(const tracked_objects::Location&,
+                         const SyncChangeList&));
   MOCK_METHOD2(GetSyncDataForType,
-               SyncError(syncable::ModelType type,
-                         SyncDataList* current_sync_data));
+               SyncError(syncable::ModelType,
+                         SyncDataList*));
   MOCK_METHOD2(SyncModelHasUserCreatedNodes,
-               bool(syncable::ModelType type,
-                    bool* has_nodes));
-  MOCK_METHOD1(CryptoReadyIfNecessary, bool(syncable::ModelType type));
+               bool(syncable::ModelType,
+                    bool*));
+  MOCK_METHOD1(CryptoReadyIfNecessary, bool(syncable::ModelType));
+  MOCK_METHOD3(ActivateDataType,
+               void(ProfileSyncService*, syncable::ModelType,
+                    browser_sync::ModelSafeGroup));
 
  protected:
   virtual ~SharedChangeProcessorMock();
diff --git a/chrome/browser/sync/glue/shared_change_processor_unittest.cc b/chrome/browser/sync/glue/shared_change_processor_unittest.cc
new file mode 100644
index 0000000..ce62624
--- /dev/null
+++ b/chrome/browser/sync/glue/shared_change_processor_unittest.cc
@@ -0,0 +1,115 @@
+// Copyright (c) 2012 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 "chrome/browser/sync/glue/shared_change_processor.h"
+
+#include <cstddef>
+
+#include "base/bind.h"
+#include "base/bind_helpers.h"
+#include "base/compiler_specific.h"
+#include "base/message_loop.h"
+#include "chrome/browser/sync/api/syncable_service_mock.h"
+#include "chrome/browser/sync/profile_sync_components_factory_impl.h"
+#include "chrome/browser/sync/profile_sync_service_mock.h"
+#include "content/test/test_browser_thread.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace browser_sync {
+
+namespace {
+
+using content::BrowserThread;
+using ::testing::NiceMock;
+
+class SyncSharedChangeProcessorTest : public testing::Test {
+ public:
+  SyncSharedChangeProcessorTest()
+      : ui_thread_(BrowserThread::UI, &ui_loop_),
+        db_thread_(BrowserThread::DB),
+        sync_factory_(NULL, NULL),
+        db_syncable_service_(NULL) {}
+
+  virtual ~SyncSharedChangeProcessorTest() {
+    EXPECT_FALSE(db_syncable_service_);
+  }
+
+ protected:
+  virtual void SetUp() OVERRIDE {
+    shared_change_processor_ = new SharedChangeProcessor();
+    db_thread_.Start();
+    EXPECT_TRUE(BrowserThread::PostTask(
+        BrowserThread::DB,
+        FROM_HERE,
+        base::Bind(&SyncSharedChangeProcessorTest::SetUpDBSyncableService,
+                   base::Unretained(this))));
+  }
+
+  virtual void TearDown() OVERRIDE {
+    EXPECT_TRUE(BrowserThread::PostTask(
+        BrowserThread::DB,
+        FROM_HERE,
+        base::Bind(&SyncSharedChangeProcessorTest::TearDownDBSyncableService,
+                   base::Unretained(this))));
+    db_thread_.Stop();
+    shared_change_processor_ = NULL;
+  }
+
+  // Connect |shared_change_processor_| on the DB thread.
+  void Connect() {
+    EXPECT_TRUE(BrowserThread::PostTask(
+        BrowserThread::DB,
+        FROM_HERE,
+        base::Bind(&SyncSharedChangeProcessorTest::ConnectOnDBThread,
+                   base::Unretained(this))));
+  }
+
+ private:
+  // Used by SetUp().
+  void SetUpDBSyncableService() {
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::DB));
+    DCHECK(!db_syncable_service_);
+    db_syncable_service_ = new NiceMock<SyncableServiceMock>();
+  }
+
+  // Used by TearDown().
+  void TearDownDBSyncableService() {
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::DB));
+    DCHECK(db_syncable_service_);
+    delete db_syncable_service_;
+    db_syncable_service_ = NULL;
+  }
+
+  // Used by Connect().
+  void ConnectOnDBThread() {
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::DB));
+    EXPECT_TRUE(
+        shared_change_processor_->Connect(&sync_factory_,
+                                          &sync_service_,
+                                          &sync_service_,
+                                          db_syncable_service_->AsWeakPtr()));
+  }
+
+  MessageLoopForUI ui_loop_;
+  content::TestBrowserThread ui_thread_;
+  content::TestBrowserThread db_thread_;
+
+  scoped_refptr<SharedChangeProcessor> shared_change_processor_;
+  ProfileSyncComponentsFactoryImpl sync_factory_;
+  NiceMock<ProfileSyncServiceMock> sync_service_;
+
+  // Used only on DB thread.
+  NiceMock<SyncableServiceMock>* db_syncable_service_;
+};
+
+// Simply connect the shared change processor.  It should succeed, and
+// nothing further should happen.
+TEST_F(SyncSharedChangeProcessorTest, Basic) {
+  Connect();
+}
+
+}  // namespace
+
+}  // namespace browser_sync
diff --git a/chrome/browser/sync/test/integration/migration_errors_test.cc b/chrome/browser/sync/test/integration/migration_errors_test.cc
index 1c073c2..50f4080 100644
--- a/chrome/browser/sync/test/integration/migration_errors_test.cc
+++ b/chrome/browser/sync/test/integration/migration_errors_test.cc
@@ -270,9 +270,8 @@ IN_PROC_BROWSER_TEST_F(MigrationSingleClientTest, AllTypesIndividually) {
   RunSingleClientMigrationTest(GetPreferredDataTypesList(), MODIFY_BOOKMARK);
 }
 
-// This test is crashing on Win and Linux sync bots. http://crbug.com/107743
 IN_PROC_BROWSER_TEST_F(MigrationSingleClientTest,
-                       DISABLED_AllTypesIndividuallyTriggerNotification) {
+                       AllTypesIndividuallyTriggerNotification) {
   ASSERT_TRUE(SetupClients());
   RunSingleClientMigrationTest(GetPreferredDataTypesList(),
                                TRIGGER_NOTIFICATION);
@@ -359,12 +358,9 @@ IN_PROC_BROWSER_TEST_F(MigrationTwoClientTest,
       MODIFY_BOOKMARK);
 }
 
-// Flaky on Mac 10.6 Sync bot and crashes on Win7 sync bot:
-// http://crbug.com/107205.
 // Migrate every datatype in sequence; the catch being that the server
 // will only tell the client about the migrations one at a time.
-IN_PROC_BROWSER_TEST_F(MigrationTwoClientTest,
-                       DISABLED_MigrationHellWithoutNigori) {
+IN_PROC_BROWSER_TEST_F(MigrationTwoClientTest, MigrationHellWithoutNigori) {
   ASSERT_TRUE(SetupClients());
   MigrationList migration_list = GetPreferredDataTypesList();
   // Let the first nudge be a datatype that's neither prefs nor
@@ -373,10 +369,7 @@ IN_PROC_BROWSER_TEST_F(MigrationTwoClientTest,
   RunTwoClientMigrationTest(migration_list, MODIFY_BOOKMARK);
 }
 
-// Flaky on Mac 10.6 Sync bot and crashes on Win7 sync bot:
-// http://crbug.com/107205.
-IN_PROC_BROWSER_TEST_F(MigrationTwoClientTest,
-                       DISABLED_MigrationHellWithNigori) {
+IN_PROC_BROWSER_TEST_F(MigrationTwoClientTest, MigrationHellWithNigori) {
   ASSERT_TRUE(SetupClients());
   MigrationList migration_list = GetPreferredDataTypesList();
   // Let the first nudge be a datatype that's neither prefs nor