Revert "Followup cleanup for ProfileSyncServiceUsesTaskScheduler experiment code."

This reverts commit ded13f7d.

Reason for revert: Build Failure (Tree Closer)

Original change's description:
> Followup cleanup for ProfileSyncServiceUsesTaskScheduler experiment code.
> 
> 1) Remove sync_thread_ from the ProfileSyncService interface to avoid
> accidental use.
> 2) Enable experiment in tests to provide coverage.
> 3) Add some ScopedAllowBaseSyncPrimitives in code that needed it.
> 
> Bug: 1014464
> Change-Id: I79393edac048a2960b4f2bc7593be326f5d27419
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1895797
> Commit-Queue: Oliver Li <olivierli@chromium.org>
> Reviewed-by: François Doray <fdoray@chromium.org>
> Reviewed-by: Mikel Astiz <mastiz@chromium.org>
> Auto-Submit: Oliver Li <olivierli@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#713459}

TBR=fdoray@chromium.org,mastiz@chromium.org,olivierli@chromium.org

Change-Id: I40c10c06af767261b7e8173db77d8a5b6224e297
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 1014464
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1903833Reviewed-by: Hongchan Choi <hongchan@chromium.org>
Commit-Queue: Hongchan Choi <hongchan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#713465}

base/threading/thread_restrictions.h

@@ -211,9 +211,6 @@ class PrinterQuery;
 namespace rlz_lib {
 class FinancialPing;
 }
-namespace syncer {
-class GetLocalChangesRequest;
-}
 namespace ui {
 class CommandBufferClientImpl;
 class CommandBufferLocal;
@@ -421,7 +418,6 @@ class BASE_EXPORT ScopedAllowBaseSyncPrimitives {
   friend class rlz_lib::FinancialPing;
   friend class shell_integration_linux::
       LaunchXdgUtilityScopedAllowBaseSyncPrimitives;
-  friend class syncer::GetLocalChangesRequest;
   friend class webrtc::DesktopConfigurationMonitor;
 
   // Usage that should be fixed:

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

@@ -11,7 +11,6 @@
 #include "base/test/metrics/histogram_tester.h"
 #include "base/values.h"
 #include "chrome/browser/sync/test/integration/bookmarks_helper.h"
-#include "chrome/browser/sync/test/integration/feature_toggler.h"
 #include "chrome/browser/sync/test/integration/profile_sync_service_harness.h"
 #include "chrome/browser/sync/test/integration/sync_test.h"
 #include "chrome/browser/sync/test/integration/updated_progress_marker_checker.h"
@@ -75,11 +74,9 @@ ModelTypeSet MultiGroupTypes(const ModelTypeSet& registered_types) {
 
 // This test enables and disables types and verifies the type is sufficiently
 // affected by checking for existence of a root node.
-class EnableDisableSingleClientTest : public FeatureToggler, public SyncTest {
+class EnableDisableSingleClientTest : public SyncTest {
  public:
-  EnableDisableSingleClientTest()
-      : FeatureToggler(switches::kProfileSyncServiceUsesThreadPool),
-        SyncTest(SINGLE_CLIENT) {}
+  EnableDisableSingleClientTest() : SyncTest(SINGLE_CLIENT) {}
   ~EnableDisableSingleClientTest() override {}
 
   // Don't use self-notifications as they can trigger additional sync cycles.
@@ -170,7 +167,7 @@ class EnableDisableSingleClientTest : public FeatureToggler, public SyncTest {
   DISALLOW_COPY_AND_ASSIGN(EnableDisableSingleClientTest);
 };
 
-IN_PROC_BROWSER_TEST_P(EnableDisableSingleClientTest, EnableOneAtATime) {
+IN_PROC_BROWSER_TEST_F(EnableDisableSingleClientTest, EnableOneAtATime) {
   // Setup sync with no enabled types.
   SetupTest(/*all_types_enabled=*/false);
 
@@ -205,7 +202,7 @@ IN_PROC_BROWSER_TEST_P(EnableDisableSingleClientTest, EnableOneAtATime) {
   }
 }
 
-IN_PROC_BROWSER_TEST_P(EnableDisableSingleClientTest, DisableOneAtATime) {
+IN_PROC_BROWSER_TEST_F(EnableDisableSingleClientTest, DisableOneAtATime) {
   // Setup sync with no disabled types.
   SetupTest(/*all_types_enabled=*/true);
 
@@ -241,7 +238,7 @@ IN_PROC_BROWSER_TEST_P(EnableDisableSingleClientTest, DisableOneAtATime) {
   }
 }
 
-IN_PROC_BROWSER_TEST_P(EnableDisableSingleClientTest,
+IN_PROC_BROWSER_TEST_F(EnableDisableSingleClientTest,
                        FastEnableDisableOneAtATime) {
   // Setup sync with no enabled types.
   SetupTest(/*all_types_enabled=*/false);
@@ -273,7 +270,7 @@ IN_PROC_BROWSER_TEST_P(EnableDisableSingleClientTest,
   }
 }
 
-IN_PROC_BROWSER_TEST_P(EnableDisableSingleClientTest,
+IN_PROC_BROWSER_TEST_F(EnableDisableSingleClientTest,
                        FastDisableEnableOneAtATime) {
   // Setup sync with no disabled types.
   SetupTest(/*all_types_enabled=*/true);
@@ -297,7 +294,7 @@ IN_PROC_BROWSER_TEST_P(EnableDisableSingleClientTest,
   }
 }
 
-IN_PROC_BROWSER_TEST_P(EnableDisableSingleClientTest,
+IN_PROC_BROWSER_TEST_F(EnableDisableSingleClientTest,
                        FastEnableDisableEnableOneAtATime) {
   // Setup sync with no enabled types.
   SetupTest(/*all_types_enabled=*/false);
@@ -323,7 +320,7 @@ IN_PROC_BROWSER_TEST_P(EnableDisableSingleClientTest,
   }
 }
 
-IN_PROC_BROWSER_TEST_P(EnableDisableSingleClientTest, EnableDisable) {
+IN_PROC_BROWSER_TEST_F(EnableDisableSingleClientTest, EnableDisable) {
   SetupTest(/*all_types_enabled=*/false);
 
   // Enable all, and then disable immediately afterwards, before datatypes
@@ -340,11 +337,11 @@ IN_PROC_BROWSER_TEST_P(EnableDisableSingleClientTest, EnableDisable) {
   }
 }
 
-IN_PROC_BROWSER_TEST_P(EnableDisableSingleClientTest, PRE_EnableAndRestart) {
+IN_PROC_BROWSER_TEST_F(EnableDisableSingleClientTest, PRE_EnableAndRestart) {
   SetupTest(/*all_types_enabled=*/true);
 }
 
-IN_PROC_BROWSER_TEST_P(EnableDisableSingleClientTest, EnableAndRestart) {
+IN_PROC_BROWSER_TEST_F(EnableDisableSingleClientTest, EnableAndRestart) {
   ASSERT_TRUE(SetupClients());
 
   EXPECT_TRUE(GetClient(0)->AwaitEngineInitialization());
@@ -357,7 +354,7 @@ IN_PROC_BROWSER_TEST_P(EnableDisableSingleClientTest, EnableAndRestart) {
   }
 }
 
-IN_PROC_BROWSER_TEST_P(EnableDisableSingleClientTest, FastEnableDisableEnable) {
+IN_PROC_BROWSER_TEST_F(EnableDisableSingleClientTest, FastEnableDisableEnable) {
   SetupTest(/*all_types_enabled=*/false);
 
   // Enable all, and then disable+reenable immediately afterwards, before
@@ -379,7 +376,7 @@ IN_PROC_BROWSER_TEST_P(EnableDisableSingleClientTest, FastEnableDisableEnable) {
 // redownloaded when Sync is started again. This does not actually verify that
 // the data is gone from disk (which seems infeasible); it's mostly here as a
 // baseline for the following tests.
-IN_PROC_BROWSER_TEST_P(EnableDisableSingleClientTest,
+IN_PROC_BROWSER_TEST_F(EnableDisableSingleClientTest,
                        RedownloadsAfterClearData) {
   ASSERT_TRUE(SetupClients());
   ASSERT_FALSE(bookmarks_helper::GetBookmarkModel(0)->IsBookmarked(
@@ -409,7 +406,7 @@ IN_PROC_BROWSER_TEST_P(EnableDisableSingleClientTest,
   EXPECT_EQ(GetNumUpdatesDownloadedInLastCycle(), initial_updates_downloaded);
 }
 
-IN_PROC_BROWSER_TEST_P(EnableDisableSingleClientTest,
+IN_PROC_BROWSER_TEST_F(EnableDisableSingleClientTest,
                        DoesNotRedownloadAfterKeepData) {
   ASSERT_TRUE(SetupClients());
   ASSERT_FALSE(bookmarks_helper::GetBookmarkModel(0)->IsBookmarked(
@@ -451,7 +448,7 @@ IN_PROC_BROWSER_TEST_P(EnableDisableSingleClientTest,
                                          /*REMOTE_INITIAL_UPDATE=*/5));
 }
 
-IN_PROC_BROWSER_TEST_P(EnableDisableSingleClientTest, ClearsPrefsIfClearData) {
+IN_PROC_BROWSER_TEST_F(EnableDisableSingleClientTest, ClearsPrefsIfClearData) {
   SetupTest(/*all_types_enabled=*/true);
 
   SyncPrefs prefs(GetProfile(0)->GetPrefs());
@@ -461,7 +458,7 @@ IN_PROC_BROWSER_TEST_P(EnableDisableSingleClientTest, ClearsPrefsIfClearData) {
   EXPECT_EQ("", prefs.GetCacheGuid());
 }
 
-IN_PROC_BROWSER_TEST_P(EnableDisableSingleClientTest,
+IN_PROC_BROWSER_TEST_F(EnableDisableSingleClientTest,
                        DoesNotClearPrefsWithKeepData) {
   SetupTest(/*all_types_enabled=*/true);
 
@@ -489,7 +486,7 @@ class EnableDisableSingleClientSelfNotifyTest
   }
 };
 
-IN_PROC_BROWSER_TEST_P(EnableDisableSingleClientSelfNotifyTest,
+IN_PROC_BROWSER_TEST_F(EnableDisableSingleClientSelfNotifyTest,
                        PRE_ResendsBagOfChips) {
   sync_pb::ChipBag bag_of_chips;
   bag_of_chips.set_server_chips(kTestServerChips);
@@ -506,7 +503,7 @@ IN_PROC_BROWSER_TEST_P(EnableDisableSingleClientSelfNotifyTest,
   EXPECT_EQ(kTestServerChips, message.bag_of_chips().server_chips());
 }
 
-IN_PROC_BROWSER_TEST_P(EnableDisableSingleClientSelfNotifyTest,
+IN_PROC_BROWSER_TEST_F(EnableDisableSingleClientSelfNotifyTest,
                        ResendsBagOfChips) {
   ASSERT_TRUE(SetupClients());
   SyncPrefs prefs(GetProfile(0)->GetPrefs());
@@ -518,8 +515,4 @@ IN_PROC_BROWSER_TEST_P(EnableDisableSingleClientSelfNotifyTest,
   EXPECT_EQ(kTestServerChips, message.bag_of_chips().server_chips());
 }
 
-INSTANTIATE_TEST_SUITE_P(,
-                         EnableDisableSingleClientSelfNotifyTest,
-                         ::testing::Values(false, true));
-
 }  // namespace

components/sync/driver/profile_sync_service.cc

@@ -17,7 +17,6 @@
 #include "base/run_loop.h"
 #include "base/strings/strcat.h"
 #include "base/task/post_task.h"
-#include "base/threading/thread.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"
@@ -52,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
@@ -433,28 +435,18 @@ void ProfileSyncService::InitializeBackendTaskRunnerIfNeeded() {
     return;
   }
 
-  if (base::FeatureList::IsEnabled(
-          switches::kProfileSyncServiceUsesThreadPool)) {
+  if (base::FeatureList::IsEnabled(kProfileSyncServiceUsesTaskScheduler)) {
     backend_task_runner_ = base::CreateSequencedTaskRunner(
         {base::ThreadPool(), base::MayBlock(), base::TaskPriority::USER_VISIBLE,
          base::TaskShutdownBehavior::BLOCK_SHUTDOWN});
   } else {
-    // The thread where all the sync operations happen. This thread is kept
-    // alive 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.
-    auto sync_thread = std::make_unique<base::Thread>("Chrome_SyncThread");
+    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);
+    bool success = sync_thread_->StartWithOptions(options);
     DCHECK(success);
-    backend_task_runner_ = sync_thread->task_runner();
 
-    // Transfer ownership of the thread to the stopper closure that gets
-    // executed at shutdown.
-    sync_thread_stopper_ =
-        base::BindOnce(&base::Thread::Stop, std::move(sync_thread));
+    backend_task_runner_ = sync_thread_->task_runner();
   }
 }
 
@@ -544,9 +536,8 @@ void ProfileSyncService::Shutdown() {
 
   auth_manager_.reset();
 
-  if (sync_thread_stopper_) {
-    std::move(sync_thread_stopper_).Run();
-  }
+  if (sync_thread_)
+    sync_thread_->Stop();
 }
 
 void ProfileSyncService::ShutdownImpl(ShutdownReason reason) {

components/sync/driver/profile_sync_service.h

@@ -400,10 +400,13 @@ class ProfileSyncService : public SyncService,
   // A utility object containing logic and state relating to encryption.
   SyncServiceCrypto crypto_;
 
-  // Owns the sync thread and takes care of its destruction.
+  // The thread where all the sync operations happen. This thread is kept alive
+  // 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.
-  base::OnceClosure sync_thread_stopper_;
+  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_;

components/sync/driver/sync_driver_switches.cc

@@ -76,8 +76,4 @@ const base::Feature kMergeBookmarksUsingGUIDs{
 const base::Feature kSyncDeviceInfoInTransportMode{
     "SyncDeviceInfoInTransportMode", base::FEATURE_DISABLED_BY_DEFAULT};
 
-// Enables the running of backend ProfileSyncService tasks on the ThreadPool.
-const base::Feature kProfileSyncServiceUsesThreadPool{
-    "ProfileSyncServiceUsesThreadPool", base::FEATURE_DISABLED_BY_DEFAULT};
-
 }  // namespace switches

components/sync/driver/sync_driver_switches.h

@@ -35,7 +35,6 @@ extern const base::Feature kSyncWifiConfigurations;
 extern const base::Feature kUpdateBookmarkGUIDWithNodeReplacement;
 extern const base::Feature kMergeBookmarksUsingGUIDs;
 extern const base::Feature kSyncDeviceInfoInTransportMode;
-extern const base::Feature kProfileSyncServiceUsesThreadPool;
 
 }  // namespace switches
 

components/sync/engine_impl/model_type_worker.cc

@@ -18,7 +18,6 @@
 #include "base/metrics/histogram_functions.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/strings/stringprintf.h"
-#include "base/threading/thread_restrictions.h"
 #include "base/trace_event/memory_usage_estimator.h"
 #include "components/sync/base/cancelation_signal.h"
 #include "components/sync/base/client_tag_hash.h"
@@ -790,12 +789,7 @@ void GetLocalChangesRequest::WaitForResponse() {
   if (!cancelation_signal_->TryRegisterHandler(this)) {
     return;
   }
-
-  {
-    base::ScopedAllowBaseSyncPrimitives allow_wait;
   response_accepted_.Wait();
-  }
-
   cancelation_signal_->UnregisterHandler(this);
 }
 

components/sync/test/fake_server/fake_server_http_post_provider.cc

@@ -8,7 +8,6 @@
 
 #include "base/bind.h"
 #include "base/location.h"
-#include "base/threading/thread_restrictions.h"
 #include "base/time/time.h"
 #include "components/sync/test/fake_server/fake_server.h"
 #include "net/base/net_errors.h"
@@ -108,10 +107,7 @@ bool FakeServerHttpPostProvider::MakeSynchronousPost(int* net_error_code,
     return false;
   }
 
-  {
-    base::ScopedAllowBaseSyncPrimitivesForTesting allow_wait;
   synchronous_post_completion_.Wait();
-  }
 
   if (aborted_) {
     *net_error_code = net::ERR_ABORTED;