Skip to content

GitLab

T
tangled
Commit d6c771f5 authored by Ben Pastene's avatar Ben Pastene Committed by Commit Bot
Browse files
Options

Revert "ThreadPool: Do not run BEST_EFFORT tasks during startup." 

This reverts commit 2b4b4ee8.

Reason for revert: CrOS OOBE/login-screen failures

See crbug.com/968554
eg: https://ci.chromium.org/p/chromium/builders/ci/chromeos-kevin-rel/9026

Original change's description:
> 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}

TBR=avi@chromium.org,gab@chromium.org,fdoray@chromium.org,carlscab@google.com

CQ_INCLUDE_TRYBOTS=luci.chromium.try:chromeos-kevin-rel
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 887407, 968554

Change-Id: I205b250267fd629b5da5db646d3b93c522642949
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1637005Reviewed-by: default avatarBen Pastene <bpastene@chromium.org>
Commit-Queue: Ben Pastene <bpastene@chromium.org>
Cr-Commit-Position: refs/heads/master@{#664826}
parent f4fb6aa2
Hide whitespace changes
Inline Side-by-side
Showing with 8 additions and 93 deletions
chrome/browser/no_best_effort_tasks_during_startup_browsertest.cc deleted 100644 → 0
View file @ f4fb6aa2
// 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
View file @ d6c771f5
......@@ -847,7 +847,6 @@ 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
View file @ d6c771f5
......@@ -529,14 +529,7 @@ 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))
#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
{
scoped_execution_fence_(std::move(scoped_execution_fence)) {
DCHECK(!g_current_browser_main_loop);
DCHECK(scoped_execution_fence_)
<< "ThreadPool must be halted before kicking off content.";
......@@ -959,20 +952,12 @@ int BrowserMainLoop::CreateThreads() {
// TODO(https://crbug.com/863341): Replace with a better API
GetContentClient()->browser()->PostAfterStartupTask(
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)));
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();
}));
created_threads_ = true;
return result_code_;
......
content/browser/browser_main_loop.h
View file @ d6c771f5
......@@ -86,7 +86,7 @@ class CompositingModeReporterImpl;
class FrameSinkManagerImpl;
class HostFrameSinkManager;
class ServerSharedBitmapManager;
} // namespace viz
}
namespace content {
class BrowserMainParts;
......@@ -305,14 +305,6 @@ 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()| -----------------------
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment