Fix missing USS controller stop completion

DataTypeManager is allowed to call Stop() while a datatype is
NOT_RUNNING or FAILED. In both cases, the function should be a no-op,
but the completion callback should be called.

Prior to this patch, the completion callback would NOT be run and hence
future sync reconfigurations may never complete, leading to weird
behavior including UI implications (missing updates), UMA metric
biases (error-related scenarios may be underrepresented) and issues
related custom passphrase encryption (which may never start or never
complete, observed in tests and the reason why this bug got surfaced).

Bug: 898453,870624
Change-Id: I47a9c04ab34ac48e0fdf000df2acb70bbf3e4446
Reviewed-on: https://chromium-review.googlesource.com/c/1297419Reviewed-by: Jan Krcal <jkrcal@chromium.org>
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#602300}

components/sync/driver/model_type_controller.cc

@@ -246,6 +246,7 @@ void ModelTypeController::Stop(ShutdownReason shutdown_reason,
       // Nothing to stop. |metadata_fate| might require CLEAR_METADATA,
       // which could lead to leaking sync metadata, but it doesn't seem a
       // realistic scenario (disable sync during shutdown?).
+      std::move(callback).Run();
       return;
 
     case STOPPING:

components/sync/driver/model_type_controller_unittest.cc

@@ -195,11 +195,17 @@ class ModelTypeControllerTest : public testing::Test {
     controller_.StartAssociating(callback.Get());
   }
 
+  void StopAndWait(ShutdownReason shutdown_reason) {
+    // ModelTypeProcessorProxy does posting of tasks, so we need a runloop. This
+    // also verifies that the completion callback is run.
+    base::RunLoop loop;
+    controller_.Stop(shutdown_reason, loop.QuitClosure());
+    loop.Run();
+  }
+
   void DeactivateDataTypeAndStop(ShutdownReason shutdown_reason) {
     controller_.DeactivateDataType(&configurer_);
-    controller_.Stop(shutdown_reason, base::DoNothing());
-    // ModelTypeProcessorProxy does posting of tasks.
-    base::RunLoop().RunUntilIdle();
+    StopAndWait(shutdown_reason);
   }
 
   MockDelegate* delegate() { return &mock_delegate_; }
@@ -329,11 +335,40 @@ TEST_F(ModelTypeControllerTest, StopBeforeLoadModels) {
 
   ASSERT_EQ(DataTypeController::NOT_RUNNING, controller()->state());
 
-  controller()->Stop(DISABLE_SYNC, base::DoNothing());
+  StopAndWait(DISABLE_SYNC);
 
   EXPECT_EQ(DataTypeController::NOT_RUNNING, controller()->state());
 }
 
+// Test emulates disabling sync when datatype is in error state. Metadata should
+// not be cleared as the delegate is potentially not ready to handle it.
+TEST_F(ModelTypeControllerTest, StopDuringFailedState) {
+  EXPECT_CALL(*delegate(), OnSyncStopping(CLEAR_METADATA)).Times(0);
+
+  ModelErrorHandler error_handler;
+  EXPECT_CALL(*delegate(), OnSyncStarting(_, _))
+      .WillOnce([&](const DataTypeActivationRequest& request,
+                    ModelTypeControllerDelegate::StartCallback callback) {
+        error_handler = request.error_handler;
+      });
+
+  base::MockCallback<DataTypeController::ModelLoadCallback> load_models_done;
+  controller()->LoadModels(MakeConfigureContext(), load_models_done.Get());
+  ASSERT_EQ(DataTypeController::MODEL_STARTING, controller()->state());
+  ASSERT_TRUE(error_handler);
+  // Mimic completion for OnSyncStarting(), with an error.
+  error_handler.Run(ModelError(FROM_HERE, "Test error"));
+  // TODO(mastiz): We shouldn't need RunUntilIdle() here, but
+  // ModelTypeController currently uses task-posting for errors.
+  base::RunLoop().RunUntilIdle();
+
+  ASSERT_EQ(DataTypeController::FAILED, controller()->state());
+
+  StopAndWait(DISABLE_SYNC);
+
+  EXPECT_EQ(DataTypeController::FAILED, controller()->state());
+}
+
 // Test emulates disabling sync when datatype is loading. The controller should
 // wait for completion of the delegate, before stopping it.
 TEST_F(ModelTypeControllerTest, StopWhileStarting) {
@@ -425,7 +460,7 @@ TEST_F(ModelTypeControllerTest, StopWhileErrorInFlight) {
   // At this point, the UI stops the datatype, but it's possible that the
   // backend has already posted a task to the UI thread, which we'll process
   // later below.
-  controller()->Stop(DISABLE_SYNC, base::DoNothing());
+  StopAndWait(DISABLE_SYNC);
   ASSERT_EQ(DataTypeController::NOT_RUNNING, controller()->state());
 
   base::HistogramTester histogram_tester;