[Sync] Split model association into multiple UI tasks

UI datatypes perform model association synchronously without any
chance of intervening tasks. This change posts the association
task for each datatype, allowing other work to intervene, both reducing
jank as well as allowing OOM warnings to be processed on platforms that
support them.

BUG=458406

Review URL: https://codereview.chromium.org/1024223002

Cr-Commit-Position: refs/heads/master@{#321859}

chrome/browser/sync/glue/bookmark_data_type_controller_unittest.cc

@@ -161,6 +161,7 @@ class SyncBookmarkDataTypeControllerTest : public testing::Test {
     bookmark_dtc_->StartAssociating(
         base::Bind(&StartCallbackMock::Run,
                    base::Unretained(&start_callback_)));
+     base::MessageLoop::current()->RunUntilIdle();
   }
 
   void NotifyHistoryServiceLoaded() {
@@ -215,6 +216,8 @@ TEST_F(SyncBookmarkDataTypeControllerTest, StartBookmarkModelNotReady) {
   bookmark_dtc_->StartAssociating(
       base::Bind(&StartCallbackMock::Run,
                  base::Unretained(&start_callback_)));
+  base::MessageLoop::current()->RunUntilIdle();
+
 
   EXPECT_EQ(DataTypeController::RUNNING, bookmark_dtc_->state());
 }

chrome/browser/sync/glue/frontend_data_type_controller.cc

@@ -83,13 +83,9 @@ void FrontendDataTypeController::StartAssociating(
 
   start_callback_ = start_callback;
   state_ = ASSOCIATING;
-  if (!Associate()) {
-    // It's possible StartDone(..) resulted in a Stop() call, or that
-    // association failed, so we just verify that the state has moved forward.
-    DCHECK_NE(state_, ASSOCIATING);
-    return;
-  }
-  DCHECK_EQ(state_, RUNNING);
+
+  base::MessageLoop::current()->PostTask(
+    FROM_HERE, base::Bind(&FrontendDataTypeController::Associate, this));
 }
 
 void FrontendDataTypeController::Stop() {
@@ -187,14 +183,19 @@ void FrontendDataTypeController::RecordUnrecoverableError(
     error_callback_.Run();
 }
 
-bool FrontendDataTypeController::Associate() {
-  DCHECK_EQ(state_, ASSOCIATING);
+void FrontendDataTypeController::Associate() {
+  if (state_ != ASSOCIATING) {
+    // Stop() must have been called while Associate() task have been waiting.
+    DCHECK_EQ(state_, NOT_RUNNING);
+    return;
+  }
+
   syncer::SyncMergeResult local_merge_result(type());
   syncer::SyncMergeResult syncer_merge_result(type());
   CreateSyncComponents();
   if (!model_associator()->CryptoReadyIfNecessary()) {
     StartDone(NEEDS_CRYPTO, local_merge_result, syncer_merge_result);
-    return false;
+    return;
   }
 
   bool sync_has_nodes = false;
@@ -205,7 +206,7 @@ bool FrontendDataTypeController::Associate() {
                             type());
     local_merge_result.set_error(error);
     StartDone(UNRECOVERABLE_ERROR, local_merge_result, syncer_merge_result);
-    return false;
+    return;
   }
 
   // TODO(zea): Have AssociateModels fill the local and syncer merge results.
@@ -219,7 +220,7 @@ bool FrontendDataTypeController::Associate() {
   if (error.IsSet()) {
     local_merge_result.set_error(error);
     StartDone(ASSOCIATION_FAILED, local_merge_result, syncer_merge_result);
-    return false;
+    return;
   }
 
   state_ = RUNNING;
@@ -229,12 +230,6 @@ bool FrontendDataTypeController::Associate() {
   StartDone(!sync_has_nodes ? OK_FIRST_RUN : OK,
             local_merge_result,
             syncer_merge_result);
-  // Return false if we're not in the RUNNING state (due to Stop() being called
-  // from FinishStart()).
-  // TODO(zea/atwilson): Should we maybe move the call to FinishStart() out of
-  // Associate() and into Start(), so we don't need this logic here? It seems
-  // cleaner to call FinishStart() from Start().
-  return state_ == RUNNING;
 }
 
 void FrontendDataTypeController::CleanUpState() {

chrome/browser/sync/glue/frontend_data_type_controller.h

@@ -129,11 +129,7 @@ class FrontendDataTypeController : public sync_driver::DataTypeController {
 
  private:
   // Build sync components and associate models.
-  // Return value:
-  //   True - if association was successful. FinishStart should have been
-  //          invoked.
-  //   False - if association failed. StartFailed should have been invoked.
-  virtual bool Associate();
+  virtual void Associate();
 
   void AbortModelLoad();
 

chrome/browser/sync/glue/frontend_data_type_controller_mock.h

@@ -32,7 +32,7 @@ class FrontendDataTypeControllerMock : public FrontendDataTypeController {
 
   // FrontendDataTypeController mocks.
   MOCK_METHOD0(StartModels, bool());
-  MOCK_METHOD0(Associate, bool());
+  MOCK_METHOD0(Associate, void());
   MOCK_METHOD0(CreateSyncComponents, void());
   MOCK_METHOD2(StartFailed, void(ConfigureResult result,
                                  const syncer::SyncError& error));

chrome/browser/sync/glue/frontend_data_type_controller_unittest.cc

@@ -140,6 +140,7 @@ class SyncFrontendDataTypeControllerTest : public testing::Test {
     frontend_dtc_->StartAssociating(
         base::Bind(&StartCallbackMock::Run,
                    base::Unretained(&start_callback_)));
+    PumpLoop();
   }
 
   void PumpLoop() { base::MessageLoop::current()->RunUntilIdle(); }
@@ -180,6 +181,17 @@ TEST_F(SyncFrontendDataTypeControllerTest, StartFirstRun) {
   EXPECT_EQ(DataTypeController::RUNNING, frontend_dtc_->state());
 }
 
+TEST_F(SyncFrontendDataTypeControllerTest, StartStopBeforeAssociation) {
+  EXPECT_CALL(*dtc_mock_.get(), StartModels()).WillOnce(Return(true));
+  EXPECT_CALL(*dtc_mock_.get(), CleanUpState());
+  EXPECT_CALL(model_load_callback_, Run(_, _));
+  EXPECT_EQ(DataTypeController::NOT_RUNNING, frontend_dtc_->state());
+  base::MessageLoop::current()->PostTask(
+      FROM_HERE, base::Bind(&FrontendDataTypeController::Stop, frontend_dtc_));
+  Start();
+  EXPECT_EQ(DataTypeController::NOT_RUNNING, frontend_dtc_->state());
+}
+
 TEST_F(SyncFrontendDataTypeControllerTest, AbortDuringStartModels) {
   EXPECT_CALL(*dtc_mock_.get(), StartModels()).WillOnce(Return(false));
   EXPECT_CALL(*dtc_mock_.get(), CleanUpState());

chrome/browser/sync/glue/search_engine_data_type_controller_unittest.cc

@@ -74,6 +74,7 @@ class SyncSearchEngineDataTypeControllerTest : public testing::Test {
     search_engine_dtc_->StartAssociating(
         base::Bind(&sync_driver::StartCallbackMock::Run,
                    base::Unretained(&start_callback_)));
+    base::MessageLoop::current()->RunUntilIdle();
   }
 
   content::TestBrowserThreadBundle thread_bundle_;

components/sync_driver/ui_data_type_controller.cc

@@ -96,10 +96,8 @@ void UIDataTypeController::StartAssociating(
 
   start_callback_ = start_callback;
   state_ = ASSOCIATING;
-  Associate();
-  // It's possible StartDone(..) resulted in a Stop() call, or that association
-  // failed, so we just verify that the state has moved foward.
-  DCHECK_NE(state_, ASSOCIATING);
+  base::MessageLoop::current()->PostTask(
+      FROM_HERE, base::Bind(&UIDataTypeController::Associate, this));
 }
 
 bool UIDataTypeController::StartModels() {
@@ -110,7 +108,12 @@ bool UIDataTypeController::StartModels() {
 }
 
 void UIDataTypeController::Associate() {
-  DCHECK_EQ(state_, ASSOCIATING);
+  if (state_ != ASSOCIATING) {
+    // Stop() must have been called while Associate() task have been waiting.
+    DCHECK_EQ(state_, NOT_RUNNING);
+    return;
+  }
+
   syncer::SyncMergeResult local_merge_result(type());
   syncer::SyncMergeResult syncer_merge_result(type());
   base::WeakPtrFactory<syncer::SyncMergeResult> weak_ptr_factory(

components/sync_driver/ui_data_type_controller_unittest.cc

@@ -83,6 +83,7 @@ class SyncUIDataTypeControllerTest : public testing::Test,
     preference_dtc_->StartAssociating(
         base::Bind(&StartCallbackMock::Run,
                    base::Unretained(&start_callback_)));
+    PumpLoop();
   }
 
   void PumpLoop() {
@@ -126,6 +127,19 @@ TEST_F(SyncUIDataTypeControllerTest, StartStop) {
   EXPECT_FALSE(syncable_service_.syncing());
 }
 
+// Start and then stop the DTC before the Start had a chance to perform
+// association. Verify that the service never started and is NOT_RUNNING.
+TEST_F(SyncUIDataTypeControllerTest, StartStopBeforeAssociation) {
+  EXPECT_EQ(DataTypeController::NOT_RUNNING, preference_dtc_->state());
+  EXPECT_FALSE(syncable_service_.syncing());
+  message_loop_.PostTask(FROM_HERE,
+                         base::Bind(&UIDataTypeController::Stop,
+                                    preference_dtc_));
+  Start();
+  EXPECT_EQ(DataTypeController::NOT_RUNNING, preference_dtc_->state());
+  EXPECT_FALSE(syncable_service_.syncing());
+}
+
 // Start the DTC when no user nodes are created. Verify that the callback
 // is called with OK_FIRST_RUN. Stop the DTC.
 TEST_F(SyncUIDataTypeControllerTest, StartStopFirstRun) {