[Sync] Ensure new non-frontend datatypes release shared change processor

If we failed to startup, Stop() doesn't get called, so ensure we cleanup up 
properly in StartDoneImpl.

BUG=102009
TEST=unit_tests


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

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

3 files changed, 35 insertions, 9 deletions

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 e76b626..b642e65 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
@@ -124,7 +124,6 @@ void NewNonFrontendDataTypeController::StartAssociation() {
       new SharedChangeProcessorRef(shared_change_processor_));
   RecordAssociationTime(base::TimeTicks::Now() - start_time);
   if (error.IsSet()) {
-    local_service_->StopSyncing(type());
     StartFailed(ASSOCIATION_FAILED, error);
     return;
   }
@@ -154,8 +153,29 @@ void NewNonFrontendDataTypeController::StartDone(
           error));
 }
 
+void NewNonFrontendDataTypeController::StartDoneImpl(
+    DataTypeController::StartResult result,
+    DataTypeController::State new_state,
+    const SyncError& error) {
+  // 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.
+    StopLocalServiceAsync();
+  }
+  NonFrontendDataTypeController::StartDoneImpl(result, new_state, error);
+}
+
 void NewNonFrontendDataTypeController::Stop() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  if (state() == NOT_RUNNING) {
+    // Stop() should never be called for datatypes that are already stopped.
+    NOTREACHED();
+    return;
+  }
 
   // Disconnect the change processor. At this point, the SyncableService
   // can no longer interact with the Syncer, even if it hasn't finished
@@ -173,16 +193,18 @@ void NewNonFrontendDataTypeController::Stop() {
     case ASSOCIATING:
       set_state(STOPPING);
       StartDoneImpl(ABORTED, NOT_RUNNING, SyncError());
-      // We continue on to deactivate the datatype.
+      // We continue on to deactivate the datatype and stop the local service.
       break;
     case DISABLED:
-      // If we're disabled we never succeded assocaiting and never activated the
-      // datatype.
+      // If we're disabled we never succeded associating and never activated the
+      // datatype. We would have already stopped the local service in
+      // StartDoneImpl(..).
       set_state(NOT_RUNNING);
       StopModels();
       return;
     default:
-      // Datatype was fully started.
+      // Datatype was fully started. Need to deactivate and stop the local
+      // service.
       DCHECK_EQ(state(), RUNNING);
       set_state(STOPPING);
       StopModels();
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 44cf867..7bc58bd 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
@@ -33,6 +33,9 @@ class NewNonFrontendDataTypeController : public NonFrontendDataTypeController {
   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;
 
   // Calls local_service_->StopSyncing() and releases our references to it and
   // |shared_change_processor_|.
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 2ea660d..61c8cd26 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
@@ -200,6 +200,8 @@ class NewNonFrontendDataTypeControllerTest : public testing::Test {
   }
 
   void SetStartFailExpectations(DataTypeController::StartResult result) {
+    EXPECT_CALL(*dtc_mock_, StopLocalServiceAsync());
+    EXPECT_CALL(syncable_service_, StopSyncing(_));
     EXPECT_CALL(*dtc_mock_, StopModels());
     EXPECT_CALL(*dtc_mock_, RecordStartFailure(result));
     EXPECT_CALL(start_callback_, Run(result,_));
@@ -259,7 +261,9 @@ TEST_F(NewNonFrontendDataTypeControllerTest, StartFirstRun) {
 
 TEST_F(NewNonFrontendDataTypeControllerTest, AbortDuringStartModels) {
   EXPECT_CALL(*dtc_mock_, StartModels()).WillOnce(Return(false));
-  SetStartFailExpectations(DataTypeController::ABORTED);
+  EXPECT_CALL(*dtc_mock_, StopModels());
+  EXPECT_CALL(*dtc_mock_, RecordStartFailure(DataTypeController::ABORTED));
+  EXPECT_CALL(start_callback_, Run(DataTypeController::ABORTED,_));
   EXPECT_EQ(DataTypeController::NOT_RUNNING, new_non_frontend_dtc_->state());
   new_non_frontend_dtc_->Start(
       NewCallback(&start_callback_, &StartCallback::Run));
@@ -284,7 +288,6 @@ TEST_F(NewNonFrontendDataTypeControllerTest, StartAssociationFailed) {
                    Return(SyncError(FROM_HERE, "failed", AUTOFILL_PROFILE))));
   EXPECT_CALL(*dtc_mock_, RecordAssociationTime(_));
   SetStartFailExpectations(DataTypeController::ASSOCIATION_FAILED);
-  EXPECT_CALL(syncable_service_, StopSyncing(_));
   // Set up association to fail with an association failed error.
   EXPECT_EQ(DataTypeController::NOT_RUNNING, new_non_frontend_dtc_->state());
   new_non_frontend_dtc_->Start(
@@ -349,8 +352,6 @@ TEST_F(NewNonFrontendDataTypeControllerTest, AbortDuringAssociation) {
   EXPECT_CALL(*change_processor_, Disconnect()).
       WillOnce(DoAll(SignalEvent(&pause_db_thread), Return(true)));
   EXPECT_CALL(service_, DeactivateDataType(_));
-  EXPECT_CALL(*dtc_mock_, StopLocalServiceAsync());
-  EXPECT_CALL(syncable_service_, StopSyncing(_));
   EXPECT_EQ(DataTypeController::NOT_RUNNING, new_non_frontend_dtc_->state());
   new_non_frontend_dtc_->Start(
       NewCallback(&start_callback_, &StartCallback::Run));