Wait until STOPPING finishes during configuration

The process of stopping a datatype is asynchronous by nature, most
prominently in USS where we want to take special care about sync
metadata being cleared.

Prior to this patch, ModelTypeController could silently drop stop
events, which could lead to the DataTypeManager (effectively the UI)
think a datatype is stopped, while in reality it could remain
running. This is problematic in multiple ways, most importantly due
to the risk of leaking sync metadata or even data.

Instead, what we want is that calls to OnSyncStarting() always have a
corresponding StopSync(), and guarantee that OnSyncStarting() is not
called twice without a StopSync() call in between.

In order to do that, ModelTypeController now enters state STOPPING
if the Stop() cannot be immediately executed, because it's currently
STARTING. Once the start is finalized, stop is immediately issued and
the corresponding callback informed, which allows
ModelAssociationManager to continue its job.

Bug: 855375
Change-Id: I6e22940fcb56816c43181c34e04c4a6e53c70a21
Reviewed-on: https://chromium-review.googlesource.com/1122863
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#572555}

components/sync/driver/data_type_controller.h

@@ -67,6 +67,8 @@ class DataTypeController : public base::SupportsWeakPtr<DataTypeController> {
 
   using ModelLoadCallback = base::Callback<void(ModelType, const SyncError&)>;
 
+  using StopCallback = base::OnceClosure;
+
   using AllNodesCallback =
       base::Callback<void(const ModelType, std::unique_ptr<base::ListValue>)>;
 
@@ -127,9 +129,12 @@ class DataTypeController : public base::SupportsWeakPtr<DataTypeController> {
   // See comments for ModelAssociationManager::OnSingleDataTypeWillStop.
   virtual void DeactivateDataType(ModelTypeConfigurer* configurer) = 0;
 
-  // Synchronously stops the data type. If StartAssociating has already been
-  // called but is not done yet it will be aborted. Similarly if LoadModels
-  // has not completed it will also be aborted.
+  // Stops the data type. If StartAssociating has already been called but is not
+  // done yet it will be aborted. Similarly if LoadModels has not completed it
+  // will also be aborted. Implementations may enter STOPPING state
+  // transitionaly but should eventually become STOPPED. At this point,
+  // |callback| will be run. |callback| must not be null.
+  //
   // NOTE: Stop() should be called after sync backend machinery has stopped
   // routing changes to this data type. Stop() should ensure the data type
   // logic shuts down gracefully by flushing remaining changes and calling
@@ -137,7 +142,8 @@ class DataTypeController : public base::SupportsWeakPtr<DataTypeController> {
   // propagate from sync again from point where Stop() is called.
   // KEEP_METADATA is used when the engine just stops sync, and CLEAR_METADATA
   // is used when the user disables sync for data type.
-  virtual void Stop(SyncStopMetadataFate metadata_fate) = 0;
+  virtual void Stop(SyncStopMetadataFate metadata_fate,
+                    StopCallback callback) = 0;
 
   // Name of this data type.  For logging purposes only.
   std::string name() const { return ModelTypeToString(type()); }

components/sync/driver/data_type_manager_impl.cc

@@ -804,6 +804,11 @@ void DataTypeManagerImpl::StopImpl(ShutdownReason reason) {
   // callback will do nothing because state is set to STOPPING above.
   model_association_manager_.Stop(metadata_fate);
 
+  // Individual data type controllers might still be STOPPING, but we don't
+  // reflect that in |state_| because, for all practical matters, the manager is
+  // in a ready state and reconfguration can be triggered.
+  // TODO(mastiz): Reconsider waiting in STOPPING state until all datatypes have
+  // stopped.
   state_ = STOPPED;
 }
 

components/sync/driver/directory_data_type_controller.cc

@@ -60,6 +60,13 @@ void DirectoryDataTypeController::DeactivateDataType(
   configurer->UnregisterDirectoryDataType(type());
 }
 
+void DirectoryDataTypeController::Stop(SyncStopMetadataFate metadata_fate,
+                                       StopCallback callback) {
+  DCHECK(CalledOnValidThread());
+  Stop(metadata_fate);
+  std::move(callback).Run();
+}
+
 void DirectoryDataTypeController::GetAllNodes(
     const AllNodesCallback& callback) {
   std::unique_ptr<base::ListValue> node_list = GetAllNodesForTypeFromDirectory(

components/sync/driver/directory_data_type_controller.h

@@ -46,11 +46,15 @@ class DirectoryDataTypeController : public DataTypeController {
   // the data type's ChangeProcessor registration with the backend).
   // See ModelTypeConfigurer::DeactivateDataType for more details.
   void DeactivateDataType(ModelTypeConfigurer* configurer) override;
-
+  void Stop(SyncStopMetadataFate metadata_fate, StopCallback callback) final;
   void GetAllNodes(const AllNodesCallback& callback) override;
   void GetStatusCounters(const StatusCountersCallback& callback) override;
   void RecordMemoryUsageHistogram() override;
 
+  // Convenience overload with synchronous API, since directory types are always
+  // capable of stopping immediately.
+  virtual void Stop(SyncStopMetadataFate metadata_fate) = 0;
+
   // Returns a ListValue representing all nodes for a specified type by querying
   // the directory.
   static std::unique_ptr<base::ListValue> GetAllNodesForTypeFromDirectory(

components/sync/driver/model_association_manager.cc

@@ -9,7 +9,9 @@
 
 #include <algorithm>
 #include <functional>
+#include <utility>
 
+#include "base/barrier_closure.h"
 #include "base/logging.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/trace_event/trace_event.h"
@@ -126,40 +128,55 @@ void ModelAssociationManager::Initialize(ModelTypeSet desired_types,
   state_ = INITIALIZED;
   notified_about_ready_for_configure_ = false;
 
-  StopDisabledTypes(preferred_types);
-  LoadEnabledTypes();
-}
-
-void ModelAssociationManager::StopDatatype(const SyncError& error,
-                                           SyncStopMetadataFate metadata_fate,
-                                           DataTypeController* dtc) {
-  loaded_types_.Remove(dtc->type());
-  associated_types_.Remove(dtc->type());
-  associating_types_.Remove(dtc->type());
-
-  if (error.IsSet() || dtc->state() != DataTypeController::NOT_RUNNING) {
-    // If an error was set, the delegate must be informed of the error.
-    delegate_->OnSingleDataTypeWillStop(dtc->type(), error);
-    dtc->Stop(metadata_fate);
-  }
-}
-
-void ModelAssociationManager::StopDisabledTypes(ModelTypeSet preferred_types) {
   DVLOG(1) << "ModelAssociationManager: Stopping disabled types.";
+  std::map<DataTypeController*, SyncStopMetadataFate> types_to_stop;
   for (DataTypeController::TypeMap::const_iterator it = controllers_->begin();
        it != controllers_->end(); ++it) {
     DataTypeController* dtc = (*it).second.get();
-    if (dtc->state() != DataTypeController::NOT_RUNNING &&
-        !desired_types_.Has(dtc->type())) {
+    // We stop a datatype if it's not desired. Independently of being desired,
+    // if the datatype is already STOPPING, we also wait for it to stop, to make
+    // sure it's ready to start again (if appropriate).
+    if ((dtc->state() != DataTypeController::NOT_RUNNING &&
+         !desired_types_.Has(dtc->type())) ||
+        dtc->state() == DataTypeController::STOPPING) {
       const SyncStopMetadataFate metadata_fate =
           preferred_types.Has(dtc->type()) ? KEEP_METADATA : CLEAR_METADATA;
+      types_to_stop[dtc] = metadata_fate;
+    }
+  }
+
+  // Run LoadEnabledTypes() only after all relevant types are stopped.
+  // TODO(mastiz): Add test coverage to this waiting logic, including the
+  // case where the datatype is STOPPING when this function is called.
+  base::RepeatingClosure barrier_closure = base::BarrierClosure(
+      types_to_stop.size(),
+      base::BindOnce(&ModelAssociationManager::LoadEnabledTypes,
+                     weak_ptr_factory_.GetWeakPtr()));
+
+  for (const auto& dtc_and_metadata_fate : types_to_stop) {
+    DataTypeController* dtc = dtc_and_metadata_fate.first;
+    const SyncStopMetadataFate metadata_fate = dtc_and_metadata_fate.second;
     DVLOG(1) << "ModelAssociationManager: stop " << dtc->name() << " with "
              << SyncStopMetadataFateToString(metadata_fate);
-      StopDatatype(SyncError(), metadata_fate, dtc);
-    }
+    StopDatatype(SyncError(), metadata_fate, dtc, barrier_closure);
   }
 }
 
+void ModelAssociationManager::StopDatatype(
+    const SyncError& error,
+    SyncStopMetadataFate metadata_fate,
+    DataTypeController* dtc,
+    DataTypeController::StopCallback callback) {
+  loaded_types_.Remove(dtc->type());
+  associated_types_.Remove(dtc->type());
+  associating_types_.Remove(dtc->type());
+
+  DCHECK(error.IsSet() || (dtc->state() != DataTypeController::NOT_RUNNING));
+
+  delegate_->OnSingleDataTypeWillStop(dtc->type(), error);
+  dtc->Stop(metadata_fate, std::move(callback));
+}
+
 void ModelAssociationManager::LoadEnabledTypes() {
   for (auto it = desired_types_.First(); it.Good(); it.Inc()) {
     auto dtc_iter = controllers_->find(it.Get());
@@ -248,8 +265,11 @@ void ModelAssociationManager::Stop(SyncStopMetadataFate metadata_fate) {
   for (DataTypeController::TypeMap::const_iterator it = controllers_->begin();
        it != controllers_->end(); ++it) {
     DataTypeController* dtc = (*it).second.get();
-    if (dtc->state() != DataTypeController::NOT_RUNNING) {
-      StopDatatype(SyncError(), metadata_fate, dtc);
+    if (dtc->state() != DataTypeController::NOT_RUNNING &&
+        dtc->state() != DataTypeController::STOPPING) {
+      // We don't really wait until all datatypes have been fully stopped, which
+      // is only required (and in fact waited for) when Initialize() is called.
+      StopDatatype(SyncError(), metadata_fate, dtc, base::DoNothing());
       DVLOG(1) << "ModelAssociationManager: Stopped " << dtc->name();
     }
   }
@@ -319,7 +339,8 @@ void ModelAssociationManager::TypeStartCallback(
     DVLOG(1) << "ModelAssociationManager: Type encountered an error.";
     desired_types_.Remove(type);
     DataTypeController* dtc = controllers_->find(type)->second.get();
-    StopDatatype(local_merge_result.error(), KEEP_METADATA, dtc);
+    StopDatatype(local_merge_result.error(), KEEP_METADATA, dtc,
+                 base::DoNothing());
     NotifyDelegateIfReadyForConfigure();
 
     // Update configuration result.
@@ -386,14 +407,15 @@ void ModelAssociationManager::ModelAssociationDone(State new_state) {
        it != controllers_->end(); ++it) {
     DataTypeController* dtc = (*it).second.get();
     if (associating_types_.Has(dtc->type()) &&
-        dtc->state() != DataTypeController::NOT_RUNNING) {
+        dtc->state() != DataTypeController::NOT_RUNNING &&
+        dtc->state() != DataTypeController::STOPPING) {
       // TODO(wychen): enum uma should be strongly typed. crbug.com/661401
       UMA_HISTOGRAM_ENUMERATION("Sync.ConfigureFailed",
                                 ModelTypeToHistogramInt(dtc->type()),
                                 static_cast<int>(MODEL_TYPE_COUNT));
       StopDatatype(SyncError(FROM_HERE, SyncError::DATATYPE_ERROR,
                              "Association timed out.", dtc->type()),
-                   KEEP_METADATA, dtc);
+                   KEEP_METADATA, dtc, base::DoNothing());
     }
   }
 

components/sync/driver/model_association_manager.h

@@ -111,10 +111,6 @@ class ModelAssociationManager {
   // round of association.
   void ResetForNextAssociation();
 
-  // Called by Initialize() to stop types that are not in |desired_types_|.
-  // For types that not in |preferred_types| also clears sync metadata.
-  void StopDisabledTypes(ModelTypeSet preferred_types);
-
   // Start loading non-running types that are in |desired_types_|.
   void LoadEnabledTypes();
 
@@ -138,7 +134,8 @@ class ModelAssociationManager {
   // A helper to stop an individual datatype.
   void StopDatatype(const SyncError& error,
                     SyncStopMetadataFate metadata_fate,
-                    DataTypeController* dtc);
+                    DataTypeController* dtc,
+                    DataTypeController::StopCallback callback);
 
   // Calls delegate's OnAllDataTypesReadyForConfigure when all datatypes from
   // desired_types_ are ready for configure. Ensures that for every call to

components/sync/driver/model_type_controller.cc

@@ -85,6 +85,19 @@ void RunModelTask(DelegateProvider delegate_provider, ModelTask task) {
     std::move(task).Run(delegate);
 }
 
+// Takes the strictest policy for clearing sync metadata.
+SyncStopMetadataFate TakeStrictestMetadataFate(SyncStopMetadataFate fate1,
+                                               SyncStopMetadataFate fate2) {
+  switch (fate1) {
+    case CLEAR_METADATA:
+      return CLEAR_METADATA;
+    case KEEP_METADATA:
+      return fate2;
+  }
+  NOTREACHED();
+  return KEEP_METADATA;
+}
+
 }  // namespace
 
 ModelTypeController::ModelTypeController(
@@ -146,15 +159,27 @@ void ModelTypeController::BeforeLoadModels(ModelTypeConfigurer* configurer) {}
 void ModelTypeController::LoadModelsDone(ConfigureResult result,
                                          const SyncError& error) {
   DCHECK(CalledOnValidThread());
+  DCHECK_NE(NOT_RUNNING, state_);
 
-  if (state_ == NOT_RUNNING) {
-    // The callback arrived on the UI thread after the type has been already
-    // stopped.
+  if (state_ == STOPPING) {
+    DCHECK(!model_stop_callbacks_.empty());
+    // This reply to OnSyncStarting() has arrived after the type has been
+    // requested to stop.
     DVLOG(1) << "Sync start completion received late for "
              << ModelTypeToString(type()) << ", it has been stopped meanwhile";
-    // TODO(mastiz): Call to Stop() here, but think through if that's enough,
-    // because perhaps the datatype was reenabled.
     RecordStartFailure(ABORTED);
+    PostModelTask(FROM_HERE, base::BindOnce(&StopSyncHelperOnModelThread,
+                                            model_stop_metadata_fate_));
+    state_ = NOT_RUNNING;
+
+    // We make a copy in case running the callbacks has side effects and
+    // modifies the vector, although we don't expect that in practice.
+    std::vector<StopCallback> model_stop_callbacks =
+        std::move(model_stop_callbacks_);
+    DCHECK(model_stop_callbacks_.empty());
+    for (StopCallback& stop_callback : model_stop_callbacks) {
+      std::move(stop_callback).Run();
+    }
     return;
   }
 
@@ -234,13 +259,13 @@ void ModelTypeController::DeactivateDataType(ModelTypeConfigurer* configurer) {
   }
 }
 
-void ModelTypeController::Stop(SyncStopMetadataFate metadata_fate) {
+void ModelTypeController::Stop(SyncStopMetadataFate metadata_fate,
+                               StopCallback callback) {
   DCHECK(CalledOnValidThread());
 
   switch (state()) {
     case ASSOCIATING:
-    case STOPPING:
-      // We don't really use these states in this class.
+      // We don't really use this state in this class.
       NOTREACHED();
       break;
 
@@ -251,9 +276,23 @@ void ModelTypeController::Stop(SyncStopMetadataFate metadata_fate) {
       // realistic scenario (disable sync during shutdown?).
       return;
 
+    case STOPPING:
+      DCHECK(!model_stop_callbacks_.empty());
+      model_stop_metadata_fate_ =
+          TakeStrictestMetadataFate(model_stop_metadata_fate_, metadata_fate);
+      model_stop_callbacks_.push_back(std::move(callback));
+      break;
+
     case MODEL_STARTING:
-      DLOG(WARNING) << "Shortcutting stop for " << ModelTypeToString(type())
+      DCHECK(!model_load_callback_.is_null());
+      DCHECK(model_stop_callbacks_.empty());
+      DLOG(WARNING) << "Deferring stop for " << ModelTypeToString(type())
                     << " because it's still starting";
+      model_stop_metadata_fate_ = metadata_fate;
+      model_stop_callbacks_.push_back(std::move(callback));
+      // The actual stop will be executed in LoadModelsDone(), when the starting
+      // process is finished.
+      state_ = STOPPING;
       break;
 
     case MODEL_LOADED:
@@ -261,10 +300,10 @@ void ModelTypeController::Stop(SyncStopMetadataFate metadata_fate) {
       DVLOG(1) << "Stopping sync for " << ModelTypeToString(type());
       PostModelTask(FROM_HERE, base::BindOnce(&StopSyncHelperOnModelThread,
                                               metadata_fate));
+      state_ = NOT_RUNNING;
+      std::move(callback).Run();
       break;
   }
-
-  state_ = NOT_RUNNING;
 }
 
 DataTypeController::State ModelTypeController::state() const {

components/sync/driver/model_type_controller.h

@@ -7,6 +7,7 @@
 
 #include <memory>
 #include <string>
+#include <vector>
 
 #include "base/callback.h"
 #include "base/macros.h"
@@ -46,7 +47,7 @@ class ModelTypeController : public DataTypeController {
   void StartAssociating(const StartCallback& start_callback) override;
   void ActivateDataType(ModelTypeConfigurer* configurer) override;
   void DeactivateDataType(ModelTypeConfigurer* configurer) override;
-  void Stop(SyncStopMetadataFate metadata_fate) override;
+  void Stop(SyncStopMetadataFate metadata_fate, StopCallback callback) override;
   State state() const override;
   void GetAllNodes(const AllNodesCallback& callback) override;
   void GetStatusCounters(const StatusCountersCallback& callback) override;
@@ -89,9 +90,19 @@ class ModelTypeController : public DataTypeController {
   // State of this datatype controller.
   State state_;
 
-  // Callbacks for use when starting the datatype.
+  // Callback for use when starting the datatype (usually MODEL_STARTING, but
+  // STOPPING if abort requested while starting).
   ModelLoadCallback model_load_callback_;
 
+  // Callbacks for use when stopping the datatype (STOPPING), which also
+  // includes aborting a start. This is important because STOPPING is a state
+  // used to make sure we don't request two starts in parallel to the delegate,
+  // which is hard to support (most notably in ClientTagBasedProcessor). We
+  // use a vector because it's allowed to call Stop() multiple times (i.e. while
+  // STOPPING).
+  std::vector<StopCallback> model_stop_callbacks_;
+  SyncStopMetadataFate model_stop_metadata_fate_;
+
   // Controller receives |activation_response_| from
   // ClientTagBasedModelTypeProcessor callback and must temporarily own it until
   // ActivateDataType is called.

components/sync/driver/model_type_controller_unittest.cc

@@ -228,7 +228,7 @@ class ModelTypeControllerTest : public testing::Test {
 
   void DeactivateDataTypeAndStop(SyncStopMetadataFate metadata_fate) {
     controller_->DeactivateDataType(&configurer_);
-    controller_->Stop(metadata_fate);
+    controller_->Stop(metadata_fate, base::DoNothing());
   }
 
   // These threads can ping-pong for a bit so we run the model thread twice.
@@ -378,7 +378,7 @@ TEST_F(ModelTypeControllerTest, StopWhenDatatypeDisabled) {
 TEST_F(ModelTypeControllerTest, StopBeforeLoadModels) {
   EXPECT_EQ(DataTypeController::NOT_RUNNING, controller()->state());
 
-  controller()->Stop(CLEAR_METADATA);
+  controller()->Stop(CLEAR_METADATA, base::DoNothing());
 
   EXPECT_EQ(DataTypeController::NOT_RUNNING, controller()->state());
   // Ensure that DisableSync is not called.

components/sync/driver/proxy_data_type_controller.cc

@@ -4,6 +4,8 @@
 
 #include "components/sync/driver/proxy_data_type_controller.h"
 
+#include <utility>
+
 #include "base/values.h"
 #include "components/sync/engine/model_safe_worker.h"
 #include "components/sync/engine/model_type_configurer.h"
@@ -53,8 +55,10 @@ void ProxyDataTypeController::StartAssociating(
                      syncer_merge_result);
 }
 
-void ProxyDataTypeController::Stop(SyncStopMetadataFate metadata_fate) {
+void ProxyDataTypeController::Stop(SyncStopMetadataFate metadata_fate,
+                                   StopCallback callback) {
   state_ = NOT_RUNNING;
+  std::move(callback).Run();
 }
 
 DataTypeController::State ProxyDataTypeController::state() const {

components/sync/driver/proxy_data_type_controller.h

@@ -28,7 +28,7 @@ class ProxyDataTypeController : public DataTypeController {
   void RegisterWithBackend(base::Callback<void(bool)> set_downloaded,
                            ModelTypeConfigurer* configurer) override;
   void StartAssociating(const StartCallback& start_callback) override;
-  void Stop(SyncStopMetadataFate metadata_fate) override;
+  void Stop(SyncStopMetadataFate metadata_fate, StopCallback callback) override;
   State state() const override;
   void ActivateDataType(ModelTypeConfigurer* configurer) override;
   void DeactivateDataType(ModelTypeConfigurer* configurer) override;