Reland "Followup cleanup for ProfileSyncServiceUsesTaskScheduler experiment code."

This is a reland of ded13f7d

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}

Bug: 1014464
Change-Id: I8d9a6d0fcf055f466d7e4a3b30a3c1e4e92042bf

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

Change-Id: I8d9a6d0fcf055f466d7e4a3b30a3c1e4e92042bf
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1904309
Auto-Submit: Oliver Li <olivierli@chromium.org>
Commit-Queue: François Doray <fdoray@chromium.org>
Reviewed-by: François Doray <fdoray@chromium.org>
Cr-Commit-Position: refs/heads/master@{#713533}

base/threading/thread_restrictions.h

@@ -211,6 +211,9 @@ class PrinterQuery;
 namespace rlz_lib {
 class FinancialPing;
 }
+namespace syncer {
+class GetLocalChangesRequest;
+}
 namespace ui {
 class CommandBufferClientImpl;
 class CommandBufferLocal;
@@ -418,6 +421,7 @@ 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,6 +11,7 @@
 #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"
@@ -74,9 +75,11 @@ 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 SyncTest {
+class EnableDisableSingleClientTest : public FeatureToggler, public SyncTest {
  public:
-  EnableDisableSingleClientTest() : SyncTest(SINGLE_CLIENT) {}
+  EnableDisableSingleClientTest()
+      : FeatureToggler(switches::kProfileSyncServiceUsesThreadPool),
+        SyncTest(SINGLE_CLIENT) {}
   ~EnableDisableSingleClientTest() override {}
 
   // Don't use self-notifications as they can trigger additional sync cycles.
@@ -167,7 +170,7 @@ class EnableDisableSingleClientTest : public SyncTest {
   DISALLOW_COPY_AND_ASSIGN(EnableDisableSingleClientTest);
 };
 
-IN_PROC_BROWSER_TEST_F(EnableDisableSingleClientTest, EnableOneAtATime) {
+IN_PROC_BROWSER_TEST_P(EnableDisableSingleClientTest, EnableOneAtATime) {
   // Setup sync with no enabled types.
   SetupTest(/*all_types_enabled=*/false);
 
@@ -202,7 +205,7 @@ IN_PROC_BROWSER_TEST_F(EnableDisableSingleClientTest, EnableOneAtATime) {
   }
 }
 
-IN_PROC_BROWSER_TEST_F(EnableDisableSingleClientTest, DisableOneAtATime) {
+IN_PROC_BROWSER_TEST_P(EnableDisableSingleClientTest, DisableOneAtATime) {
   // Setup sync with no disabled types.
   SetupTest(/*all_types_enabled=*/true);
 
@@ -238,7 +241,7 @@ IN_PROC_BROWSER_TEST_F(EnableDisableSingleClientTest, DisableOneAtATime) {
   }
 }
 
-IN_PROC_BROWSER_TEST_F(EnableDisableSingleClientTest,
+IN_PROC_BROWSER_TEST_P(EnableDisableSingleClientTest,
                        FastEnableDisableOneAtATime) {
   // Setup sync with no enabled types.
   SetupTest(/*all_types_enabled=*/false);
@@ -270,7 +273,7 @@ IN_PROC_BROWSER_TEST_F(EnableDisableSingleClientTest,
   }
 }
 
-IN_PROC_BROWSER_TEST_F(EnableDisableSingleClientTest,
+IN_PROC_BROWSER_TEST_P(EnableDisableSingleClientTest,
                        FastDisableEnableOneAtATime) {
   // Setup sync with no disabled types.
   SetupTest(/*all_types_enabled=*/true);
@@ -294,7 +297,7 @@ IN_PROC_BROWSER_TEST_F(EnableDisableSingleClientTest,
   }
 }
 
-IN_PROC_BROWSER_TEST_F(EnableDisableSingleClientTest,
+IN_PROC_BROWSER_TEST_P(EnableDisableSingleClientTest,
                        FastEnableDisableEnableOneAtATime) {
   // Setup sync with no enabled types.
   SetupTest(/*all_types_enabled=*/false);
@@ -320,7 +323,7 @@ IN_PROC_BROWSER_TEST_F(EnableDisableSingleClientTest,
   }
 }
 
-IN_PROC_BROWSER_TEST_F(EnableDisableSingleClientTest, EnableDisable) {
+IN_PROC_BROWSER_TEST_P(EnableDisableSingleClientTest, EnableDisable) {
   SetupTest(/*all_types_enabled=*/false);
 
   // Enable all, and then disable immediately afterwards, before datatypes
@@ -337,11 +340,11 @@ IN_PROC_BROWSER_TEST_F(EnableDisableSingleClientTest, EnableDisable) {
   }
 }
 
-IN_PROC_BROWSER_TEST_F(EnableDisableSingleClientTest, PRE_EnableAndRestart) {
+IN_PROC_BROWSER_TEST_P(EnableDisableSingleClientTest, PRE_EnableAndRestart) {
   SetupTest(/*all_types_enabled=*/true);
 }
 
-IN_PROC_BROWSER_TEST_F(EnableDisableSingleClientTest, EnableAndRestart) {
+IN_PROC_BROWSER_TEST_P(EnableDisableSingleClientTest, EnableAndRestart) {
   ASSERT_TRUE(SetupClients());
 
   EXPECT_TRUE(GetClient(0)->AwaitEngineInitialization());
@@ -354,7 +357,7 @@ IN_PROC_BROWSER_TEST_F(EnableDisableSingleClientTest, EnableAndRestart) {
   }
 }
 
-IN_PROC_BROWSER_TEST_F(EnableDisableSingleClientTest, FastEnableDisableEnable) {
+IN_PROC_BROWSER_TEST_P(EnableDisableSingleClientTest, FastEnableDisableEnable) {
   SetupTest(/*all_types_enabled=*/false);
 
   // Enable all, and then disable+reenable immediately afterwards, before
@@ -376,7 +379,7 @@ IN_PROC_BROWSER_TEST_F(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_F(EnableDisableSingleClientTest,
+IN_PROC_BROWSER_TEST_P(EnableDisableSingleClientTest,
                        RedownloadsAfterClearData) {
   ASSERT_TRUE(SetupClients());
   ASSERT_FALSE(bookmarks_helper::GetBookmarkModel(0)->IsBookmarked(
@@ -406,7 +409,7 @@ IN_PROC_BROWSER_TEST_F(EnableDisableSingleClientTest,
   EXPECT_EQ(GetNumUpdatesDownloadedInLastCycle(), initial_updates_downloaded);
 }
 
-IN_PROC_BROWSER_TEST_F(EnableDisableSingleClientTest,
+IN_PROC_BROWSER_TEST_P(EnableDisableSingleClientTest,
                        DoesNotRedownloadAfterKeepData) {
   ASSERT_TRUE(SetupClients());
   ASSERT_FALSE(bookmarks_helper::GetBookmarkModel(0)->IsBookmarked(
@@ -448,7 +451,7 @@ IN_PROC_BROWSER_TEST_F(EnableDisableSingleClientTest,
                                          /*REMOTE_INITIAL_UPDATE=*/5));
 }
 
-IN_PROC_BROWSER_TEST_F(EnableDisableSingleClientTest, ClearsPrefsIfClearData) {
+IN_PROC_BROWSER_TEST_P(EnableDisableSingleClientTest, ClearsPrefsIfClearData) {
   SetupTest(/*all_types_enabled=*/true);
 
   SyncPrefs prefs(GetProfile(0)->GetPrefs());
@@ -458,7 +461,7 @@ IN_PROC_BROWSER_TEST_F(EnableDisableSingleClientTest, ClearsPrefsIfClearData) {
   EXPECT_EQ("", prefs.GetCacheGuid());
 }
 
-IN_PROC_BROWSER_TEST_F(EnableDisableSingleClientTest,
+IN_PROC_BROWSER_TEST_P(EnableDisableSingleClientTest,
                        DoesNotClearPrefsWithKeepData) {
   SetupTest(/*all_types_enabled=*/true);
 
@@ -486,7 +489,7 @@ class EnableDisableSingleClientSelfNotifyTest
   }
 };
 
-IN_PROC_BROWSER_TEST_F(EnableDisableSingleClientSelfNotifyTest,
+IN_PROC_BROWSER_TEST_P(EnableDisableSingleClientSelfNotifyTest,
                        PRE_ResendsBagOfChips) {
   sync_pb::ChipBag bag_of_chips;
   bag_of_chips.set_server_chips(kTestServerChips);
@@ -503,7 +506,7 @@ IN_PROC_BROWSER_TEST_F(EnableDisableSingleClientSelfNotifyTest,
   EXPECT_EQ(kTestServerChips, message.bag_of_chips().server_chips());
 }
 
-IN_PROC_BROWSER_TEST_F(EnableDisableSingleClientSelfNotifyTest,
+IN_PROC_BROWSER_TEST_P(EnableDisableSingleClientSelfNotifyTest,
                        ResendsBagOfChips) {
   ASSERT_TRUE(SetupClients());
   SyncPrefs prefs(GetProfile(0)->GetPrefs());
@@ -515,4 +518,8 @@ IN_PROC_BROWSER_TEST_F(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,6 +17,7 @@
 #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"
@@ -51,9 +52,6 @@ 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
@@ -435,18 +433,28 @@ void ProfileSyncService::InitializeBackendTaskRunnerIfNeeded() {
     return;
   }
 
-  if (base::FeatureList::IsEnabled(kProfileSyncServiceUsesTaskScheduler)) {
+  if (base::FeatureList::IsEnabled(
+          switches::kProfileSyncServiceUsesThreadPool)) {
     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");
+    // 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");
     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();
 
-    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));
   }
 }
 
@@ -536,8 +544,9 @@ void ProfileSyncService::Shutdown() {
 
   auth_manager_.reset();
 
-  if (sync_thread_)
-    sync_thread_->Stop();
+  if (sync_thread_stopper_) {
+    std::move(sync_thread_stopper_).Run();
+  }
 }
 
 void ProfileSyncService::ShutdownImpl(ShutdownReason reason) {

components/sync/driver/profile_sync_service.h

@@ -400,13 +400,10 @@ class ProfileSyncService : public SyncService,
   // A utility object containing logic and state relating to encryption.
   SyncServiceCrypto crypto_;
 
-  // 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.
+  // Owns the sync thread and takes care of its destruction.
   // TODO(https://crbug.com/1014464): Remove once we have switched to
   // Threadpool.
-  std::unique_ptr<base::Thread> sync_thread_;
+  base::OnceClosure sync_thread_stopper_;
 
   // 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,4 +76,8 @@ 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,6 +35,7 @@ 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,6 +18,7 @@
 #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"
@@ -789,7 +790,12 @@ void GetLocalChangesRequest::WaitForResponse() {
   if (!cancelation_signal_->TryRegisterHandler(this)) {
     return;
   }
-  response_accepted_.Wait();
+
+  {
+    base::ScopedAllowBaseSyncPrimitives allow_wait;
+    response_accepted_.Wait();
+  }
+
   cancelation_signal_->UnregisterHandler(this);
 }
 

components/sync/test/fake_server/fake_server_http_post_provider.cc

@@ -8,6 +8,7 @@
 
 #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"
@@ -107,7 +108,10 @@ bool FakeServerHttpPostProvider::MakeSynchronousPost(int* net_error_code,
     return false;
   }
 
-  synchronous_post_completion_.Wait();
+  {
+    base::ScopedAllowBaseSyncPrimitivesForTesting allow_wait;
+    synchronous_post_completion_.Wait();
+  }
 
   if (aborted_) {
     *net_error_code = net::ERR_ABORTED;