[threadpool] Fix --disable-best-effort-tasks and add test.

The --disable-best-effort-tasks switch was only read when a task
execution fence was created. This CL ensure that it is read even if
there is no task execution fence. It also adds a test to verify the
behavior and avoid future regressions.

The TabCapturePerformanceTest.Performance test used
--disable-best-effort-tasks to ensure that the test can pass without
BEST_EFFORT tasks. Prior to this CL, this flag only prevented
BEST_EFFORT tasks from running in the browser process. With this CL, it
also prevents BEST_EFFORT tasks from running in renderers and utility
processes. Since the test needs BEST_EFFORT tasks to run in utility
processes for tracing, this CL changes --disable-best-effort-tasks
to ScopedBestEffortExecutionFence. The ScopedBestEffortExecutionFence
only prevents BEST_EFFORT tasks from running in the process where it
is used (keeps the test's existing behavior).

Bug: 839110
Change-Id: I4ef104ccabb81840cbb48d309e1ed3baaa20feda
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1872654Reviewed-by: Yuri Wiitala <miu@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Commit-Queue: François Doray <fdoray@chromium.org>
Auto-Submit: François Doray <fdoray@chromium.org>
Cr-Commit-Position: refs/heads/master@{#711676}

base/task/thread_pool/thread_pool_impl.cc

@@ -163,6 +163,9 @@ void ThreadPoolImpl::Start(const ThreadPoolInstance::InitParams& init_params,
   task_tracker_->set_io_thread_task_runner(service_thread_->task_runner());
 #endif  // defined(OS_POSIX) && !defined(OS_NACL_SFI)
 
+  // Update the CanRunPolicy based on |has_disable_best_effort_switch_|.
+  UpdateCanRunPolicy();
+
   // Needs to happen after starting the service thread to get its task_runner().
   scoped_refptr<TaskRunner> service_thread_task_runner =
       service_thread_->task_runner();

base/task/thread_pool/thread_pool_impl_unittest.cc

@@ -11,6 +11,7 @@
 #include <utility>
 #include <vector>
 
+#include "base/base_switches.h"
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/callback.h"
@@ -542,6 +543,54 @@ TEST_P(ThreadPoolImplTestAllTraitsExecutionModes, FlushAsyncForTestingSimple) {
   flush_event.Wait();
 }
 
+// Verifies that BEST_EFFORT tasks don't run when the
+// --disable-best-effort-tasks command-line switch is specified.
+//
+// Not using the same fixture as other tests because we want to append a command
+// line switch before creating the pool.
+TEST(ThreadPoolImplTest_Switch, DisableBestEffortTasksSwitch) {
+  CommandLine::ForCurrentProcess()->AppendSwitch(
+      switches::kDisableBestEffortTasks);
+
+  ThreadPoolImpl thread_pool("Test");
+  ThreadPoolInstance::InitParams init_params(kMaxNumForegroundThreads);
+  thread_pool.Start(init_params, nullptr);
+
+  AtomicFlag best_effort_can_run;
+  WaitableEvent best_effort_did_run;
+  thread_pool.PostDelayedTask(FROM_HERE,
+                              {ThreadPool(), TaskPriority::BEST_EFFORT,
+                               TaskShutdownBehavior::BLOCK_SHUTDOWN},
+                              BindLambdaForTesting([&]() {
+                                EXPECT_TRUE(best_effort_can_run.IsSet());
+                                best_effort_did_run.Signal();
+                              }),
+                              TimeDelta());
+
+  WaitableEvent user_blocking_did_run;
+  thread_pool.PostDelayedTask(
+      FROM_HERE, {ThreadPool(), TaskPriority::USER_BLOCKING},
+      BindLambdaForTesting([&]() { user_blocking_did_run.Signal(); }),
+      TimeDelta());
+
+  // The USER_BLOCKING task should run.
+  user_blocking_did_run.Wait();
+
+  PlatformThread::Sleep(TestTimeouts::tiny_timeout());
+
+  // The BEST_EFFORT task should not run when a BEST_EFFORT fence is deleted.
+  thread_pool.SetHasBestEffortFence(true);
+  thread_pool.SetHasBestEffortFence(false);
+
+  PlatformThread::Sleep(TestTimeouts::tiny_timeout());
+
+  // The BEST_EFFORT task should only run during shutdown.
+  best_effort_can_run.Set();
+  thread_pool.Shutdown();
+  EXPECT_TRUE(best_effort_did_run.IsSignaled());
+  thread_pool.JoinForTesting();
+}
+
 // Verifies that tasks only run when allowed by SetHasFence().
 TEST_P(ThreadPoolImplTestAllTraitsExecutionModes, SetHasFence) {
   StartThreadPool();

chrome/browser/extensions/api/tab_capture/tab_capture_performance_test_base.cc

@@ -64,6 +64,8 @@ void TabCapturePerformanceTestBase::SetUp() {
 void TabCapturePerformanceTestBase::SetUpOnMainThread() {
   InProcessBrowserTest::SetUpOnMainThread();
 
+  best_effort_fence_.emplace();
+
   host_resolver()->AddRule("*", "127.0.0.1");
   embedded_test_server()->RegisterRequestHandler(base::BindRepeating(
       &TabCapturePerformanceTestBase::HandleRequest, base::Unretained(this)));
@@ -75,12 +77,6 @@ void TabCapturePerformanceTestBase::SetUpCommandLine(
     base::CommandLine* command_line) {
   is_full_performance_run_ = command_line->HasSwitch(kFullPerformanceRunSwitch);
 
-  // In the spirit of the NoBestEffortTasksTests, it's important to add this
-  // flag to make sure best-effort tasks are not required for the success of
-  // these tests. In a performance test run, this also removes sources of
-  // variance.
-  command_line->AppendSwitch(switches::kDisableBestEffortTasks);
-
   // Note: The naming "kUseGpuInTests" is very misleading. It actually means
   // "don't use a software OpenGL implementation." Subclasses will either call
   // UseSoftwareCompositing() to use Chrome's software compositor, or else they

chrome/browser/extensions/api/tab_capture/tab_capture_performance_test_base.h

@@ -10,7 +10,9 @@
 #include <vector>
 
 #include "base/macros.h"
+#include "base/optional.h"
 #include "base/strings/string_piece.h"
+#include "base/task/thread_pool/thread_pool_instance.h"
 #include "base/test/scoped_feature_list.h"
 #include "base/test/trace_event_analyzer.h"
 #include "chrome/test/base/in_process_browser_test.h"
@@ -123,6 +125,16 @@ class TabCapturePerformanceTestBase : public InProcessBrowserTest {
   static const char kExtensionId[];
 
  private:
+  // In the spirit of NoBestEffortTasksTests, use a fence to make sure that
+  // BEST_EFFORT tasks in the browser process are not required for the success
+  // of these tests. In a performance test run, this also removes sources of
+  // variance. Do not use the --disable-best-effort-tasks command line switch as
+  // that would also preempt BEST_EFFORT tasks in utility processes, and
+  // TabCapturePerformanceTest.Performance relies on BEST_EFFORT tasks in
+  // utility process for tracing.
+  base::Optional<base::ThreadPoolInstance::ScopedBestEffortExecutionFence>
+      best_effort_fence_;
+
   bool is_full_performance_run_ = false;
 
   // Set to the test page that should be served by the next call to