ThreadPool: Do not run BEST_EFFORT tasks during startup.

With this CL, a task posted with:

base::PostTaskWithTraits(
    FROM_HERE,
    {base::TaskPriority::BEST_EFFORT, ...},
    base::BindOnce(...));

Will not be allowed to run before startup is complete.

The signal used to know that "startup is complete" is the same as
AfterStartupTaskUtils. It is not perfect. It will be improved in a
separate CL.

This CL aims to be the smallest change possible to allow seamless
migration of BrowserThread::PostAfterStartupTask() to
base::PostTaskWithTraits().

Bug: 887407
Change-Id: I83ef387b82496ece3ed10c0c2466f42a6a5b19b3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1631668
Commit-Queue: François Doray <fdoray@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: Carlos Caballero <carlscab@google.com>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#664488}

chrome/browser/no_best_effort_tasks_during_startup_browsertest.cc

+// Copyright 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.
+
+#include "base/barrier_closure.h"
+#include "base/run_loop.h"
+#include "base/task/post_task.h"
+#include "base/test/bind_test_util.h"
+#include "chrome/browser/after_startup_task_utils.h"
+#include "chrome/test/base/in_process_browser_test.h"
+#include "content/public/browser/browser_task_traits.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace {
+
+class NoBestEffortTasksDuringStartupTest : public InProcessBrowserTest {
+ public:
+  // InProcessBrowserTest:
+  void PreRunTestOnMainThread() override {
+    // This test must run before PreRunTestOnMainThread() sets startup as
+    // complete.
+    TestNoBestEffortTasksDuringStartup();
+
+    InProcessBrowserTest::PreRunTestOnMainThread();
+  }
+
+  void TestNoBestEffortTasksDuringStartup() {
+    EXPECT_FALSE(AfterStartupTaskUtils::IsBrowserStartupComplete());
+
+    base::RunLoop run_loop;
+    auto barrier = base::BarrierClosure(2, run_loop.QuitClosure());
+
+    // Thread pool task.
+    base::PostTaskWithTraits(
+        FROM_HERE, {base::TaskPriority::BEST_EFFORT},
+        base::BindLambdaForTesting([&]() {
+          EXPECT_TRUE(AfterStartupTaskUtils::IsBrowserStartupComplete());
+          barrier.Run();
+        }));
+
+    // UI thread task.
+    base::PostTaskWithTraits(
+        FROM_HERE,
+        {content::BrowserThread::UI, base::TaskPriority::BEST_EFFORT},
+        base::BindLambdaForTesting([&]() {
+          EXPECT_TRUE(AfterStartupTaskUtils::IsBrowserStartupComplete());
+          barrier.Run();
+        }));
+
+    run_loop.Run();
+  }
+};
+
+}  // namespace
+
+// Verify that BEST_EFFORT tasks don't run until startup is complete.
+IN_PROC_BROWSER_TEST_F(NoBestEffortTasksDuringStartupTest,
+                       NoBestEffortTasksDuringStartup) {
+  // The body of the test is in the TestNoBestEffortTasksDuringStartup() method
+  // called from PreRunTestOnMainThread().
+}

chrome/test/BUILD.gn

@@ -847,6 +847,7 @@ if (!is_android) {
       "../browser/net/variations_http_headers_browsertest.cc",
       "../browser/net/websocket_browsertest.cc",
       "../browser/no_best_effort_tasks_browsertest.cc",
+      "../browser/no_best_effort_tasks_during_startup_browsertest.cc",
       "../browser/ntp_snippets/content_suggestions_service_factory_browsertest.cc",
       "../browser/ntp_tiles/ntp_tiles_browsertest.cc",
       "../browser/page_load_metrics/observers/ad_metrics/ads_page_load_metrics_observer_browsertest.cc",

content/browser/browser_main_loop.cc

@@ -529,7 +529,14 @@ BrowserMainLoop::BrowserMainLoop(
       parsed_command_line_(parameters.command_line),
       result_code_(service_manager::RESULT_CODE_NORMAL_EXIT),
       created_threads_(false),
-      scoped_execution_fence_(std::move(scoped_execution_fence)) {
+      scoped_execution_fence_(std::move(scoped_execution_fence))
+#if !defined(OS_ANDROID)
+      ,
+      // TODO(fdoray): Create the fence on Android too. Not enabled yet because
+      // tests timeout. https://crbug.com/887407
+      scoped_best_effort_execution_fence_(base::in_place_t())
+#endif
+{
   DCHECK(!g_current_browser_main_loop);
   DCHECK(scoped_execution_fence_)
       << "ThreadPool must be halted before kicking off content.";
@@ -951,12 +958,20 @@ int BrowserMainLoop::CreateThreads() {
 
   // TODO(https://crbug.com/863341): Replace with a better API
   GetContentClient()->browser()->PostAfterStartupTask(
-      FROM_HERE, base::SequencedTaskRunnerHandle::Get(), base::BindOnce([]() {
-        // Non best effort queues will already have been enabled
-        // This will enable all queues on all browser threads, so we need to do
-        // this after the threads have been created, i.e. here.
-        content::BrowserTaskExecutor::EnableAllQueues();
-      }));
+      FROM_HERE, base::SequencedTaskRunnerHandle::Get(),
+      base::BindOnce(
+          [](BrowserMainLoop* browser_main_loop) {
+            // Enable main thread and thread pool best effort queues. Non-best
+            // effort queues will already have been enabled. This will enable
+            // all queues on all browser threads, so we need to do this after
+            // the threads have been created, i.e. here.
+            content::BrowserTaskExecutor::EnableAllQueues();
+            browser_main_loop->scoped_best_effort_execution_fence_.reset();
+          },
+          // Main thread tasks can't run after BrowserMainLoop destruction.
+          // Accessing an Unretained pointer to BrowserMainLoop from a main
+          // thread task is therefore safe.
+          base::Unretained(this)));
 
   created_threads_ = true;
   return result_code_;

content/browser/browser_main_loop.h

@@ -86,7 +86,7 @@ class CompositingModeReporterImpl;
 class FrameSinkManagerImpl;
 class HostFrameSinkManager;
 class ServerSharedBitmapManager;
-}
+}  // namespace viz
 
 namespace content {
 class BrowserMainParts;
@@ -305,6 +305,14 @@ class CONTENT_EXPORT BrowserMainLoop {
   std::unique_ptr<base::ThreadPoolInstance::ScopedExecutionFence>
       scoped_execution_fence_;
 
+  // BEST_EFFORT tasks are not allowed to run between //content initialization
+  // and startup completion.
+  //
+  // TODO(fdoray): Move this to a more elaborate class that prevents BEST_EFFORT
+  // tasks from running when resources are needed to respond to user actions.
+  base::Optional<base::ThreadPoolInstance::ScopedBestEffortExecutionFence>
+      scoped_best_effort_execution_fence_;
+
   // Members initialized in |MainMessageLoopStart()| ---------------------------
 
   // Members initialized in |PostMainMessageLoopStart()| -----------------------