Create experiment to have ProfileSyncService's backend task runner use the ThreadPool.

This change is the last mile of of the Chrome-wide effort to prefer Sequences to
Physical Threads (https://chromium.googlesource.com/chromium/src/+/master/docs/threading_and_tasks.md#Prefer-Sequences-to-Physical-Threads)

This change is intended to be a no-op from sync's point of view.

Change-Id: Ie771c3a681b0483711ec1232544b92f2766caf47

Bug: 1014464
Change-Id: Ie771c3a681b0483711ec1232544b92f2766caf47
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1856922
Commit-Queue: Oliver Li <olivierli@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#709898}

chrome/browser/sync/test/integration/single_client_directory_sync_test.cc

-// Copyright (c) 2014 The Chromium Authors. All rights reserved.
+// Copyright (c) 2019 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
@@ -8,10 +8,7 @@
 #include "base/location.h"
 #include "base/macros.h"
 #include "base/run_loop.h"
-#include "base/single_thread_task_runner.h"
 #include "base/strings/string_number_conversions.h"
-#include "base/threading/thread_restrictions.h"
-#include "base/threading/thread_task_runner_handle.h"
 #include "chrome/browser/sync/test/integration/bookmarks_helper.h"
 #include "chrome/browser/sync/test/integration/single_client_status_change_checker.h"
 #include "chrome/browser/sync/test/integration/sync_test.h"
@@ -49,16 +46,6 @@ class SingleClientDirectorySyncTest : public SyncTest {
   DISALLOW_COPY_AND_ASSIGN(SingleClientDirectorySyncTest);
 };
 
-void WaitForExistingTasksOnTaskRunner(
-    scoped_refptr<base::SingleThreadTaskRunner> task_runner) {
-  base::RunLoop run_loop;
-  // Post a task to |loop| that will, in turn, post a task back to the current
-  // sequenced task runner to quit the nested loop.
-  task_runner->PostTaskAndReply(FROM_HERE, base::DoNothing(),
-                                run_loop.QuitClosure());
-  run_loop.Run();
-}
-
 // A status change checker that waits for an unrecoverable sync error to occur.
 class SyncUnrecoverableErrorChecker : public SingleClientStatusChangeChecker {
  public:
@@ -86,8 +73,7 @@ IN_PROC_BROWSER_TEST_F(SingleClientDirectorySyncTest,
                                                 run_loop.QuitClosure());
   run_loop.Run();
   // Wait for the directory deletion to finish.
-  WaitForExistingTasksOnTaskRunner(
-      sync_service->GetSyncThreadTaskRunnerForTest());
+  sync_service->FlushBackendTaskRunnerForTest();
   EXPECT_FALSE(FolderContainsFiles(directory_path));
 }
 
@@ -105,9 +91,7 @@ IN_PROC_BROWSER_TEST_F(SingleClientDirectorySyncTest,
   // completes.
   syncer::ProfileSyncService* sync_service = GetSyncService(0);
   sync_service->FlushDirectory();
-  scoped_refptr<base::SingleThreadTaskRunner> sync_thread_task_runner =
-      sync_service->GetSyncThreadTaskRunnerForTest();
-  WaitForExistingTasksOnTaskRunner(sync_thread_task_runner);
+  sync_service->FlushBackendTaskRunnerForTest();
 
   // Now corrupt the database.
   FilePath directory_path = sync_service->GetSyncClientForTest()
@@ -141,7 +125,7 @@ IN_PROC_BROWSER_TEST_F(SingleClientDirectorySyncTest,
 
   // Wait until the sync loop has processed any existing tasks and see that the
   // directory no longer exists.
-  WaitForExistingTasksOnTaskRunner(sync_thread_task_runner);
+  sync_service->FlushBackendTaskRunnerForTest();
   ASSERT_FALSE(FolderContainsFiles(directory_path));
 }
 

components/sync/driver/profile_sync_service.cc

@@ -14,7 +14,9 @@
 #include "base/files/file_path.h"
 #include "base/logging.h"
 #include "base/metrics/histogram_macros.h"
+#include "base/run_loop.h"
 #include "base/strings/strcat.h"
+#include "base/task/post_task.h"
 #include "components/invalidation/public/invalidation_service.h"
 #include "components/signin/public/base/signin_metrics.h"
 #include "components/signin/public/identity_manager/account_info.h"
@@ -49,6 +51,9 @@ namespace syncer {
 
 namespace {
 
+const base::Feature kProfileSyncServiceUsesTaskScheduler{
+    "ProfileSyncServiceUsesThreadPool", base::FEATURE_DISABLED_BY_DEFAULT};
+
 // The initial state of sync, for the Sync.InitialState histogram. Even if
 // this value is CAN_START, sync startup might fail for reasons that we may
 // want to consider logging in the future, such as a passphrase needed for
@@ -424,17 +429,25 @@ void ProfileSyncService::OnDataTypeRequestsSyncStartup(ModelType type) {
   startup_controller_->OnDataTypeRequestsSyncStartup(type);
 }
 
-void ProfileSyncService::StartSyncThreadIfNeeded() {
-  if (sync_thread_) {
+void ProfileSyncService::InitializeBackendTaskRunnerIfNeeded() {
+  if (backend_task_runner_) {
     // Already started.
     return;
   }
 
+  if (base::FeatureList::IsEnabled(kProfileSyncServiceUsesTaskScheduler)) {
+    backend_task_runner_ = base::CreateSequencedTaskRunner(
+        {base::ThreadPool(), base::MayBlock(), base::TaskPriority::USER_VISIBLE,
+         base::TaskShutdownBehavior::BLOCK_SHUTDOWN});
+  } else {
     sync_thread_ = std::make_unique<base::Thread>("Chrome_SyncThread");
     base::Thread::Options options;
     options.timer_slack = base::TIMER_SLACK_MAXIMUM;
     bool success = sync_thread_->StartWithOptions(options);
     DCHECK(success);
+
+    backend_task_runner_ = sync_thread_->task_runner();
+  }
 }
 
 void ProfileSyncService::StartUpSlowEngineComponents() {
@@ -449,10 +462,10 @@ void ProfileSyncService::StartUpSlowEngineComponents() {
     last_actionable_error_ = SyncProtocolError();
   }
 
-  StartSyncThreadIfNeeded();
+  InitializeBackendTaskRunnerIfNeeded();
 
   SyncEngine::InitParams params;
-  params.sync_task_runner = sync_thread_->task_runner();
+  params.sync_task_runner = backend_task_runner_;
   params.host = this;
   params.registrar = std::make_unique<SyncBackendRegistrar>(
       debug_identifier_,
@@ -533,16 +546,16 @@ void ProfileSyncService::ShutdownImpl(ShutdownReason reason) {
     // happens, the data directory needs to be cleaned up here.
     if (reason == ShutdownReason::DISABLE_SYNC) {
       // Clearing the Directory via Directory::DeleteDirectoryFiles() requires
-      // the |sync_thread_| initialized. It also means there's IO involved which
-      // may we considerable overhead if triggered consistently upon browser
-      // startup (which is the case for certain codepaths such as the user being
-      // signed out). To avoid that, SyncPrefs is used to determine whether it's
-      // worth.
+      // the |backend_task_runner_| initialized. It also means there's IO
+      // involved which may we considerable overhead if triggered consistently
+      // upon browser startup (which is the case for certain codepaths such as
+      // the user being signed out). To avoid that, SyncPrefs is used to
+      // determine whether it's worth it.
       if (!sync_prefs_.GetCacheGuid().empty()) {
-        StartSyncThreadIfNeeded();
+        InitializeBackendTaskRunnerIfNeeded();
       }
-      if (sync_thread_) {
-        sync_thread_->task_runner()->PostTask(
+      if (backend_task_runner_) {
+        backend_task_runner_->PostTask(
             FROM_HERE,
             base::BindOnce(&syncable::Directory::DeleteDirectoryFiles,
                            sync_client_->GetSyncDataPath()));
@@ -1837,10 +1850,12 @@ void ProfileSyncService::SetPassphrasePrompted(bool prompted) {
   sync_prefs_.SetPassphrasePrompted(prompted);
 }
 
-scoped_refptr<base::SingleThreadTaskRunner>
-ProfileSyncService::GetSyncThreadTaskRunnerForTest() const {
+void ProfileSyncService::FlushBackendTaskRunnerForTest() const {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  return sync_thread_ ? sync_thread_->task_runner() : nullptr;
+  base::RunLoop run_loop;
+
+  backend_task_runner_->PostTask(FROM_HERE, run_loop.QuitClosure());
+  run_loop.Run();
 }
 
 SyncEncryptionHandler::Observer*

components/sync/driver/profile_sync_service.h

@@ -16,7 +16,7 @@
 #include "base/memory/weak_ptr.h"
 #include "base/observer_list.h"
 #include "base/sequence_checker.h"
-#include "base/single_thread_task_runner.h"
+#include "base/sequenced_task_runner.h"
 #include "base/threading/thread.h"
 #include "base/time/time.h"
 #include "components/invalidation/public/identity_provider.h"
@@ -252,9 +252,8 @@ class ProfileSyncService : public SyncService,
 
   bool IsDataTypeControllerRunningForTest(ModelType type) const;
 
-  // Sometimes we need to wait for tasks on the sync thread in tests.
-  scoped_refptr<base::SingleThreadTaskRunner> GetSyncThreadTaskRunnerForTest()
-      const;
+  // Sometimes we need to wait for tasks on the |backend_task_runner_| in tests.
+  void FlushBackendTaskRunnerForTest() const;
 
   // Some tests rely on injecting calls to the encryption observer.
   SyncEncryptionHandler::Observer* GetEncryptionObserverForTest();
@@ -339,8 +338,9 @@ class ProfileSyncService : public SyncService,
 
   void ClearUnrecoverableError();
 
-  // Initializes and starts |sync_thread_|.
-  void StartSyncThreadIfNeeded();
+  // Initializes |backend_task_runner_| which is backed by |sync_thread_| or the
+  // ThreadPool depending on the ProfileSyncServiceUsesThreadPool experiment.
+  void InitializeBackendTaskRunnerIfNeeded();
 
   // Kicks off asynchronous initialization of the SyncEngine.
   void StartUpSlowEngineComponents();
@@ -404,8 +404,13 @@ class ProfileSyncService : public SyncService,
   // until browser shutdown and reused if sync is turned off and on again. It is
   // joined during the shutdown process, but there is an abort mechanism in
   // place to prevent slow HTTP requests from blocking browser shutdown.
+  // TODO(https://crbug.com/1014464): Remove once we have switched to
+  // Threadpool.
   std::unique_ptr<base::Thread> sync_thread_;
 
+  // TODO(crbug.com/923287): Move out of this class. Possibly to SyncEngineImpl.
+  scoped_refptr<base::SequencedTaskRunner> backend_task_runner_;
+
   // Our asynchronous engine to communicate with sync components living on
   // other threads.
   std::unique_ptr<SyncEngine> engine_;