Add more temporary CHECKs to investigate crasher

After a long debugging session, we haven't yet found an explanation as
per why the processor enters an inconsistent state, where apparently
OnSyncStarting() is received twice without OnSyncStopping() in between.

As a last resort to continue the investigation, we add even more CHECKs
to understand which layer is violating the API contracts.

Bug: 876490,885019
Change-Id: Ia15fa29d8bd128730bd79b4760ea233f09d9862e
Reviewed-on: https://chromium-review.googlesource.com/1230733Reviewed-by: Marc Treib <treib@chromium.org>
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#592041}

components/sync/driver/model_type_controller.cc

@@ -76,12 +76,13 @@ void ModelTypeController::LoadModels(
     const ConfigureContext& configure_context,
     const ModelLoadCallback& model_load_callback) {
   DCHECK(CalledOnValidThread());
-  DCHECK(!model_load_callback.is_null());
-  DCHECK_EQ(NOT_RUNNING, state_);
+  CHECK(!model_load_callback.is_null());
+  CHECK_EQ(NOT_RUNNING, state_);
 
   auto it = delegate_map_.find(configure_context.storage_option);
-  DCHECK(it != delegate_map_.end());
+  CHECK(it != delegate_map_.end());
   delegate_ = it->second.get();
+  CHECK(delegate_);
 
   DVLOG(1) << "Sync starting for " << ModelTypeToString(type());
   state_ = MODEL_STARTING;
@@ -95,8 +96,8 @@ void ModelTypeController::LoadModels(
   request.authenticated_account_id = configure_context.authenticated_account_id;
   request.cache_guid = configure_context.cache_guid;
 
-  DCHECK(!request.authenticated_account_id.empty());
-  DCHECK(!request.cache_guid.empty());
+  CHECK(!request.authenticated_account_id.empty());
+  CHECK(!request.cache_guid.empty());
 
   // Ask the delegate to actually start the datatype.
   delegate_->OnSyncStarting(
@@ -109,7 +110,7 @@ void ModelTypeController::BeforeLoadModels(ModelTypeConfigurer* configurer) {}
 void ModelTypeController::LoadModelsDone(ConfigureResult result,
                                          const SyncError& error) {
   DCHECK(CalledOnValidThread());
-  DCHECK_NE(NOT_RUNNING, state_);
+  CHECK_NE(NOT_RUNNING, state_);
 
   if (state_ == STOPPING) {
     DCHECK(!model_stop_callbacks_.empty());
@@ -136,7 +137,7 @@ void ModelTypeController::LoadModelsDone(ConfigureResult result,
   }
 
   if (IsSuccessfulResult(result)) {
-    DCHECK_EQ(MODEL_STARTING, state_);
+    CHECK_EQ(MODEL_STARTING, state_);
     state_ = MODEL_LOADED;
     DVLOG(1) << "Sync start completed for " << ModelTypeToString(type());
   } else {
@@ -167,7 +168,7 @@ void ModelTypeController::RegisterWithBackend(
     return;
   DCHECK(configurer);
   DCHECK(activation_response_);
-  DCHECK_EQ(MODEL_LOADED, state_);
+  CHECK_EQ(MODEL_LOADED, state_);
   // Inform the DataTypeManager whether our initial download is complete.
   set_downloaded.Run(
       activation_response_->model_type_state.initial_sync_done());
@@ -182,7 +183,7 @@ void ModelTypeController::StartAssociating(
     const StartCallback& start_callback) {
   DCHECK(CalledOnValidThread());
   DCHECK(!start_callback.is_null());
-  DCHECK_EQ(MODEL_LOADED, state_);
+  CHECK_EQ(MODEL_LOADED, state_);
 
   state_ = RUNNING;
   DVLOG(1) << "Sync running for " << ModelTypeToString(type());
@@ -195,11 +196,11 @@ void ModelTypeController::StartAssociating(
 void ModelTypeController::ActivateDataType(ModelTypeConfigurer* configurer) {
   DCHECK(CalledOnValidThread());
   DCHECK(configurer);
-  DCHECK_EQ(RUNNING, state_);
+  CHECK_EQ(RUNNING, state_);
   // In contrast with directory datatypes, non-blocking data types should be
   // activated in RegisterWithBackend. activation_response_ should be
   // passed to backend before call to ActivateDataType.
-  DCHECK(!activation_response_);
+  CHECK(!activation_response_);
 }
 
 void ModelTypeController::DeactivateDataType(ModelTypeConfigurer* configurer) {
@@ -229,15 +230,15 @@ void ModelTypeController::Stop(SyncStopMetadataFate metadata_fate,
       return;
 
     case STOPPING:
-      DCHECK(!model_stop_callbacks_.empty());
+      CHECK(!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:
-      DCHECK(!model_load_callback_.is_null());
-      DCHECK(model_stop_callbacks_.empty());
+      CHECK(!model_load_callback_.is_null());
+      CHECK(model_stop_callbacks_.empty());
       DLOG(WARNING) << "Deferring stop for " << ModelTypeToString(type())
                     << " because it's still starting";
       model_stop_metadata_fate_ = metadata_fate;

components/sync/model_impl/client_tag_based_model_type_processor.cc

@@ -87,13 +87,15 @@ void ClientTagBasedModelTypeProcessor::OnSyncStarting(
     const DataTypeActivationRequest& request,
     StartCallback start_callback) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  DVLOG(1) << "Sync is starting for " << ModelTypeToString(type_);
   // TODO(crbug.com/876490): Here and elsewhere in this file, CHECKs have been
   // introduced to investigate some crashes in the wild. Let's downgrade all
   // CHECKs to DCHECKs as soon as the investigation is completed.
-  CHECK(!IsConnected());
+  CHECK(this);
   CHECK(request.error_handler);
   CHECK(start_callback);
-  DVLOG(1) << "Sync is starting for " << ModelTypeToString(type_);
+  CHECK(!start_callback_);
+  CHECK(!IsConnected());
 
   start_callback_ = std::move(start_callback);
   activation_request_ = request;
@@ -218,6 +220,7 @@ void ClientTagBasedModelTypeProcessor::OnSyncStopping(
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   // Disabling sync for a type never happens before the model is ready to sync.
   CHECK(model_ready_to_sync_);
+  CHECK(!start_callback_);
 
   switch (metadata_fate) {
     case KEEP_METADATA: {

components/sync/model_impl/proxy_model_type_controller_delegate.cc

@@ -17,6 +17,7 @@ void OnSyncStartingHelperOnModelThread(
     const DataTypeActivationRequest& request,
     ModelTypeControllerDelegate::StartCallback callback_bound_to_ui_thread,
     base::WeakPtr<ModelTypeControllerDelegate> delegate) {
+  CHECK(delegate);
   delegate->OnSyncStarting(request, std::move(callback_bound_to_ui_thread));
 }
 
@@ -24,6 +25,7 @@ void GetAllNodesForDebuggingHelperOnModelThread(
     ProxyModelTypeControllerDelegate::AllNodesCallback
         callback_bound_to_ui_thread,
     base::WeakPtr<ModelTypeControllerDelegate> delegate) {
+  CHECK(delegate);
   delegate->GetAllNodesForDebugging(std::move(callback_bound_to_ui_thread));
 }
 
@@ -31,6 +33,7 @@ void GetStatusCountersForDebuggingHelperOnModelThread(
     ProxyModelTypeControllerDelegate::StatusCountersCallback
         callback_bound_to_ui_thread,
     base::WeakPtr<ModelTypeControllerDelegate> delegate) {
+  CHECK(delegate);
   delegate->GetStatusCountersForDebugging(
       std::move(callback_bound_to_ui_thread));
 }
@@ -38,11 +41,13 @@ void GetStatusCountersForDebuggingHelperOnModelThread(
 void StopSyncHelperOnModelThread(
     SyncStopMetadataFate metadata_fate,
     base::WeakPtr<ModelTypeControllerDelegate> delegate) {
+  CHECK(delegate);
   delegate->OnSyncStopping(metadata_fate);
 }
 
 void RecordMemoryUsageAndCountsHistogramsHelperOnModelThread(
     base::WeakPtr<ModelTypeControllerDelegate> delegate) {
+  CHECK(delegate);
   delegate->RecordMemoryUsageAndCountsHistograms();
 }