[Sync] Pass a TaskRunner into SBHI/SBHC, not a thread or message loop.

components/sync/driver/glue/sync_backend_host_core.cc

Index: components/sync/driver/glue/sync_backend_host_core.cc
diff --git a/components/sync/driver/glue/sync_backend_host_core.cc b/components/sync/driver/glue/sync_backend_host_core.cc
index 9771e18f4ec2414733b61668e877c274dc93b1e2..c9c465971a418361f40ccbabacd8b12c795e884d 100644
--- a/components/sync/driver/glue/sync_backend_host_core.cc
+++ b/components/sync/driver/glue/sync_backend_host_core.cc
@@ -55,7 +55,7 @@ namespace syncer {
 class EngineComponentsFactory;
 
 DoInitializeOptions::DoInitializeOptions(
-    base::MessageLoop* sync_loop,
+    scoped_refptr<base::SingleThreadTaskRunner> sync_task_runner,
     SyncBackendRegistrar* registrar,
     const std::vector<scoped_refptr<ModelSafeWorker>>& workers,
     const scoped_refptr<ExtensionsActivity>& extensions_activity,
@@ -76,7 +76,7 @@ DoInitializeOptions::DoInitializeOptions(
     const base::Closure& report_unrecoverable_error_function,
     std::unique_ptr<SyncEncryptionHandler::NigoriState> saved_nigori_state,
     const std::map<ModelType, int64_t>& invalidation_versions)
-    : sync_loop(sync_loop),
+    : sync_task_runner(std::move(sync_task_runner)),
       registrar(registrar),
       workers(workers),
       extensions_activity(extensions_activity),
@@ -116,7 +116,6 @@ SyncBackendHostCore::SyncBackendHostCore(
     : name_(name),
       sync_data_folder_path_(sync_data_folder_path),
       host_(backend),
-      sync_loop_(nullptr),
       registrar_(nullptr),
       has_sync_setup_completed_(has_sync_setup_completed),
       forward_protocol_events_(false),
@@ -132,7 +131,7 @@ SyncBackendHostCore::~SyncBackendHostCore() {
 bool SyncBackendHostCore::OnMemoryDump(
     const base::trace_event::MemoryDumpArgs& args,
     base::trace_event::ProcessMemoryDump* pmd) {
-  DCHECK(sync_loop_->task_runner()->BelongsToCurrentThread());
+  DCHECK(sync_task_runner_->BelongsToCurrentThread());
   if (!sync_manager_)
     return false;
   sync_manager_->OnMemoryDump(pmd);
@@ -141,9 +140,7 @@ bool SyncBackendHostCore::OnMemoryDump(
 
 void SyncBackendHostCore::OnSyncCycleCompleted(
     const SyncCycleSnapshot& snapshot) {
-  if (!sync_loop_)

[skym, 2016/11/30 18:45:23]

What makes us think that removing all these if (!sync_loop_) return; checks is
safe?

[maxbogue, 2016/11/30 20:05:06]

It was never nulled out and it was set in DoInitialize (the first thing that's
ever called on this class).

-    return;
-  DCHECK(sync_loop_->task_runner()->BelongsToCurrentThread());
+  DCHECK(sync_task_runner_->BelongsToCurrentThread());
 
   host_.Call(FROM_HERE,
              &SyncBackendHostImpl::HandleSyncCycleCompletedOnFrontendLoop,
@@ -151,7 +148,7 @@ void SyncBackendHostCore::OnSyncCycleCompleted(
 }
 
 void SyncBackendHostCore::DoRefreshTypes(ModelTypeSet types) {
-  DCHECK(sync_loop_->task_runner()->BelongsToCurrentThread());
+  DCHECK(sync_task_runner_->BelongsToCurrentThread());
   sync_manager_->RefreshTypes(types);
 }
 
@@ -160,7 +157,7 @@ void SyncBackendHostCore::OnInitializationComplete(
     const WeakHandle<DataTypeDebugInfoListener>& debug_info_listener,
     bool success,
     const ModelTypeSet restored_types) {
-  DCHECK(sync_loop_->task_runner()->BelongsToCurrentThread());
+  DCHECK(sync_task_runner_->BelongsToCurrentThread());
 
   if (!success) {
     DoDestroySyncManager(STOP_SYNC);
@@ -177,7 +174,7 @@ void SyncBackendHostCore::OnInitializationComplete(
 
   // Sync manager initialization is complete, so we can schedule recurring
   // SaveChanges.
-  sync_loop_->task_runner()->PostTask(
+  sync_task_runner_->PostTask(
       FROM_HERE, base::Bind(&SyncBackendHostCore::StartSavingChanges,
                             weak_ptr_factory_.GetWeakPtr()));
 
@@ -217,9 +214,7 @@ void SyncBackendHostCore::OnInitializationComplete(
 }
 
 void SyncBackendHostCore::OnConnectionStatusChange(ConnectionStatus status) {
-  if (!sync_loop_)
-    return;
-  DCHECK(sync_loop_->task_runner()->BelongsToCurrentThread());
+  DCHECK(sync_task_runner_->BelongsToCurrentThread());
   host_.Call(FROM_HERE,
              &SyncBackendHostImpl::HandleConnectionStatusChangeOnFrontendLoop,
              status);
@@ -228,44 +223,34 @@ void SyncBackendHostCore::OnConnectionStatusChange(ConnectionStatus status) {
 void SyncBackendHostCore::OnPassphraseRequired(
     PassphraseRequiredReason reason,
     const sync_pb::EncryptedData& pending_keys) {
-  if (!sync_loop_)
-    return;
-  DCHECK(sync_loop_->task_runner()->BelongsToCurrentThread());
+  DCHECK(sync_task_runner_->BelongsToCurrentThread());
   host_.Call(FROM_HERE, &SyncBackendHostImpl::NotifyPassphraseRequired, reason,
              pending_keys);
 }
 
 void SyncBackendHostCore::OnPassphraseAccepted() {
-  if (!sync_loop_)
-    return;
-  DCHECK(sync_loop_->task_runner()->BelongsToCurrentThread());
+  DCHECK(sync_task_runner_->BelongsToCurrentThread());
   host_.Call(FROM_HERE, &SyncBackendHostImpl::NotifyPassphraseAccepted);
 }
 
 void SyncBackendHostCore::OnBootstrapTokenUpdated(
     const std::string& bootstrap_token,
     BootstrapTokenType type) {
-  if (!sync_loop_)
-    return;
-  DCHECK(sync_loop_->task_runner()->BelongsToCurrentThread());
+  DCHECK(sync_task_runner_->BelongsToCurrentThread());
   host_.Call(FROM_HERE, &SyncBackendHostImpl::PersistEncryptionBootstrapToken,
              bootstrap_token, type);
 }
 
 void SyncBackendHostCore::OnEncryptedTypesChanged(ModelTypeSet encrypted_types,
                                                   bool encrypt_everything) {
-  if (!sync_loop_)
-    return;
-  DCHECK(sync_loop_->task_runner()->BelongsToCurrentThread());
+  DCHECK(sync_task_runner_->BelongsToCurrentThread());
   // NOTE: We're in a transaction.
   host_.Call(FROM_HERE, &SyncBackendHostImpl::NotifyEncryptedTypesChanged,
              encrypted_types, encrypt_everything);
 }
 
 void SyncBackendHostCore::OnEncryptionComplete() {
-  if (!sync_loop_)
-    return;
-  DCHECK(sync_loop_->task_runner()->BelongsToCurrentThread());
+  DCHECK(sync_task_runner_->BelongsToCurrentThread());
   // NOTE: We're in a transaction.
   host_.Call(FROM_HERE, &SyncBackendHostImpl::NotifyEncryptionComplete);
 }
@@ -319,16 +304,14 @@ void SyncBackendHostCore::OnStatusCountersUpdated(
 
 void SyncBackendHostCore::OnActionableError(
     const SyncProtocolError& sync_error) {
-  if (!sync_loop_)
-    return;
-  DCHECK(sync_loop_->task_runner()->BelongsToCurrentThread());
+  DCHECK(sync_task_runner_->BelongsToCurrentThread());
   host_.Call(FROM_HERE,
              &SyncBackendHostImpl::HandleActionableErrorEventOnFrontendLoop,
              sync_error);
 }
 
 void SyncBackendHostCore::OnMigrationRequested(ModelTypeSet types) {
-  DCHECK(sync_loop_->task_runner()->BelongsToCurrentThread());
+  DCHECK(sync_task_runner_->BelongsToCurrentThread());
   host_.Call(FROM_HERE,
              &SyncBackendHostImpl::HandleMigrationRequestedOnFrontendLoop,
              types);
@@ -344,13 +327,13 @@ void SyncBackendHostCore::OnProtocolEvent(const ProtocolEvent& event) {
 }
 
 void SyncBackendHostCore::DoOnInvalidatorStateChange(InvalidatorState state) {
-  DCHECK(sync_loop_->task_runner()->BelongsToCurrentThread());
+  DCHECK(sync_task_runner_->BelongsToCurrentThread());
   sync_manager_->SetInvalidatorEnabled(state == INVALIDATIONS_ENABLED);
 }
 
 void SyncBackendHostCore::DoOnIncomingInvalidation(
     const ObjectIdInvalidationMap& invalidation_map) {
-  DCHECK(sync_loop_->task_runner()->BelongsToCurrentThread());
+  DCHECK(sync_task_runner_->BelongsToCurrentThread());
 
   ObjectIdSet ids = invalidation_map.GetObjectIds();
   for (const invalidation::ObjectId& object_id : ids) {
@@ -387,9 +370,7 @@ void SyncBackendHostCore::DoOnIncomingInvalidation(
 
 void SyncBackendHostCore::DoInitialize(
     std::unique_ptr<DoInitializeOptions> options) {
-  DCHECK(!sync_loop_);
-  sync_loop_ = options->sync_loop;
-  DCHECK(sync_loop_);
+  sync_task_runner_ = std::move(options->sync_task_runner);
 
   // Finish initializing the HttpBridgeFactory.  We do this here because
   // building the user agent may block on some platforms.
@@ -443,12 +424,12 @@ void SyncBackendHostCore::DoInitialize(
   args.saved_nigori_state = std::move(options->saved_nigori_state);
   sync_manager_->Init(&args);
   base::trace_event::MemoryDumpManager::GetInstance()->RegisterDumpProvider(
-      this, "SyncDirectory", sync_loop_->task_runner());
+      this, "SyncDirectory", sync_task_runner_);
 }
 
 void SyncBackendHostCore::DoUpdateCredentials(
     const SyncCredentials& credentials) {
-  DCHECK(sync_loop_->task_runner()->BelongsToCurrentThread());
+  DCHECK(sync_task_runner_->BelongsToCurrentThread());

[skym, 2016/11/30 18:45:23]

Seems like this class could get away with a ThreadCheckers and
SequencedTaskRunnerHandle::Get() instead. We talked offline, you said that you
wanted the interfaces to look the same. Okay, but lets just throw away the
SingleThreadTaskRunner.

[maxbogue, 2016/11/30 20:05:06]

Done. Except I used ThreadTaskRunnerHandle::Get() because it's important that
the sync thread stuff is a thread and not just a sequence.

   // UpdateCredentials can be called during backend initialization, possibly
   // when backend initialization has failed but hasn't notified the UI thread
   // yet. In that case, the sync manager may have been destroyed on the sync
@@ -461,20 +442,20 @@ void SyncBackendHostCore::DoUpdateCredentials(
 void SyncBackendHostCore::DoStartSyncing(
     const ModelSafeRoutingInfo& routing_info,
     base::Time last_poll_time) {
-  DCHECK(sync_loop_->task_runner()->BelongsToCurrentThread());
+  DCHECK(sync_task_runner_->BelongsToCurrentThread());
   sync_manager_->StartSyncingNormally(routing_info, last_poll_time);
 }
 
 void SyncBackendHostCore::DoSetEncryptionPassphrase(
     const std::string& passphrase,
     bool is_explicit) {
-  DCHECK(sync_loop_->task_runner()->BelongsToCurrentThread());
+  DCHECK(sync_task_runner_->BelongsToCurrentThread());
   sync_manager_->GetEncryptionHandler()->SetEncryptionPassphrase(passphrase,
                                                                  is_explicit);
 }
 
 void SyncBackendHostCore::DoInitialProcessControlTypes() {
-  DCHECK(sync_loop_->task_runner()->BelongsToCurrentThread());
+  DCHECK(sync_task_runner_->BelongsToCurrentThread());
 
   DVLOG(1) << "Initilalizing Control Types";
 
@@ -511,12 +492,12 @@ void SyncBackendHostCore::DoInitialProcessControlTypes() {
 
 void SyncBackendHostCore::DoSetDecryptionPassphrase(
     const std::string& passphrase) {
-  DCHECK(sync_loop_->task_runner()->BelongsToCurrentThread());
+  DCHECK(sync_task_runner_->BelongsToCurrentThread());
   sync_manager_->GetEncryptionHandler()->SetDecryptionPassphrase(passphrase);
 }
 
 void SyncBackendHostCore::DoEnableEncryptEverything() {
-  DCHECK(sync_loop_->task_runner()->BelongsToCurrentThread());
+  DCHECK(sync_task_runner_->BelongsToCurrentThread());
   sync_manager_->GetEncryptionHandler()->EnableEncryptEverything();
 }
 
@@ -542,7 +523,7 @@ void SyncBackendHostCore::ShutdownOnUIThread() {
 }
 
 void SyncBackendHostCore::DoShutdown(ShutdownReason reason) {
-  DCHECK(sync_loop_->task_runner()->BelongsToCurrentThread());
+  DCHECK(sync_task_runner_->BelongsToCurrentThread());
 
   DoDestroySyncManager(reason);
 
@@ -556,7 +537,7 @@ void SyncBackendHostCore::DoShutdown(ShutdownReason reason) {
 }
 
 void SyncBackendHostCore::DoDestroySyncManager(ShutdownReason reason) {
-  DCHECK(sync_loop_->task_runner()->BelongsToCurrentThread());
+  DCHECK(sync_task_runner_->BelongsToCurrentThread());
   base::trace_event::MemoryDumpManager::GetInstance()->UnregisterDumpProvider(
       this);
   if (sync_manager_) {
@@ -574,7 +555,7 @@ void SyncBackendHostCore::DoConfigureSyncer(
     const ModelSafeRoutingInfo routing_info,
     const base::Callback<void(ModelTypeSet, ModelTypeSet)>& ready_task,
     const base::Closure& retry_callback) {
-  DCHECK(sync_loop_->task_runner()->BelongsToCurrentThread());
+  DCHECK(sync_task_runner_->BelongsToCurrentThread());
   DCHECK(!ready_task.is_null());
   DCHECK(!retry_callback.is_null());
   base::Closure chained_ready_task(base::Bind(
@@ -592,7 +573,7 @@ void SyncBackendHostCore::DoConfigureSyncer(
 void SyncBackendHostCore::DoFinishConfigureDataTypes(
     ModelTypeSet types_to_config,
     const base::Callback<void(ModelTypeSet, ModelTypeSet)>& ready_task) {
-  DCHECK(sync_loop_->task_runner()->BelongsToCurrentThread());
+  DCHECK(sync_task_runner_->BelongsToCurrentThread());
 
   // Update the enabled types for the bridge and sync manager.
   ModelSafeRoutingInfo routing_info;
@@ -612,13 +593,13 @@ void SyncBackendHostCore::DoFinishConfigureDataTypes(
 
 void SyncBackendHostCore::DoRetryConfiguration(
     const base::Closure& retry_callback) {
-  DCHECK(sync_loop_->task_runner()->BelongsToCurrentThread());
+  DCHECK(sync_task_runner_->BelongsToCurrentThread());
   host_.Call(FROM_HERE, &SyncBackendHostImpl::RetryConfigurationOnFrontendLoop,
              retry_callback);
 }
 
 void SyncBackendHostCore::SendBufferedProtocolEventsAndEnableForwarding() {
-  DCHECK(sync_loop_->task_runner()->BelongsToCurrentThread());
+  DCHECK(sync_task_runner_->BelongsToCurrentThread());
   forward_protocol_events_ = true;
 
   if (sync_manager_) {
@@ -663,7 +644,7 @@ void SyncBackendHostCore::DisableDirectoryTypeDebugInfoForwarding() {
 }
 
 void SyncBackendHostCore::DeleteSyncDataFolder() {
-  DCHECK(sync_loop_->task_runner()->BelongsToCurrentThread());
+  DCHECK(sync_task_runner_->BelongsToCurrentThread());
   if (base::DirectoryExists(sync_data_folder_path_)) {
     if (!base::DeleteFile(sync_data_folder_path_, true))
       SLOG(DFATAL) << "Could not delete the Sync Data folder.";
@@ -671,10 +652,7 @@ void SyncBackendHostCore::DeleteSyncDataFolder() {
 }
 
 void SyncBackendHostCore::StartSavingChanges() {
-  // We may already be shut down.
-  if (!sync_loop_)
-    return;
-  DCHECK(sync_loop_->task_runner()->BelongsToCurrentThread());
+  DCHECK(sync_task_runner_->BelongsToCurrentThread());
   DCHECK(!save_changes_timer_.get());
   save_changes_timer_ = base::MakeUnique<base::RepeatingTimer>();
   save_changes_timer_->Start(
@@ -683,13 +661,13 @@ void SyncBackendHostCore::StartSavingChanges() {
 }
 
 void SyncBackendHostCore::SaveChanges() {
-  DCHECK(sync_loop_->task_runner()->BelongsToCurrentThread());
+  DCHECK(sync_task_runner_->BelongsToCurrentThread());
   sync_manager_->SaveChanges();
 }
 
 void SyncBackendHostCore::DoClearServerData(
     const SyncManager::ClearServerDataCallback& frontend_callback) {
-  DCHECK(sync_loop_->task_runner()->BelongsToCurrentThread());
+  DCHECK(sync_task_runner_->BelongsToCurrentThread());
   const SyncManager::ClearServerDataCallback callback =
       base::Bind(&SyncBackendHostCore::ClearServerDataDone,
                  weak_ptr_factory_.GetWeakPtr(), frontend_callback);
@@ -698,13 +676,13 @@ void SyncBackendHostCore::DoClearServerData(
 
 void SyncBackendHostCore::DoOnCookieJarChanged(bool account_mismatch,
                                                bool empty_jar) {
-  DCHECK(sync_loop_->task_runner()->BelongsToCurrentThread());
+  DCHECK(sync_task_runner_->BelongsToCurrentThread());
   sync_manager_->OnCookieJarChanged(account_mismatch, empty_jar);
 }
 
 void SyncBackendHostCore::ClearServerDataDone(
     const base::Closure& frontend_callback) {
-  DCHECK(sync_loop_->task_runner()->BelongsToCurrentThread());
+  DCHECK(sync_task_runner_->BelongsToCurrentThread());
   host_.Call(FROM_HERE, &SyncBackendHostImpl::ClearServerDataDoneOnFrontendLoop,
              frontend_callback);
 }