This is taking care of a long-standing TODO to move these OnceClosure
holders rather than copy them around with their OnceClosure* members.
This is a precursor to
https://chromium-review.googlesource.com/c/chromium/src/+/3187153/35#message-fcc92e9f85e73f0e5ba6c03610a95cda8736f1f9
which highlighted a problem where some tests see a non-null
MainFunctionParams::ui_task but running the closure results in a UAF.
Logs show that the test hitting the UAF is not the one setting this
field. This CL makes that impossible and fixes the issue in the
follow-up CL.
This CL is intended to be a logical no-op.
This CL touches a lot of files and must happen all at once.
The core change is that ContentMainParams and MainFunctionParams's
moveable fields (ui_task, created_main_parts_closure, and startup_data)
are now held by moveable types rather than raw pointers.
This trickles in the following chain:
main() (in various *_main.cc)
(or SetUp() in !OS_ANDROID browser_test_base.cc)
-> ContentMain()
-> ContentMainRunnerImpl::Initialize()
(forwards arg into MainFunctionParams)
-> RunBrowser()
-> BrowserMain()
-> BrowserMainRunnerImpl::Initialize()
-> BrowserMainLoop (stores MainFunctionParams)
-> BrowserMainLoop::Init
-> ContentBrowserClient::CreateBrowserMainParts()
-> (Embedder)ContentBrowserClient::CreateBrowserMainParts()
-> (Embedder)BrowserMainParts(Platform)
-> (Embedder)BrowserMainParts
-> RunOtherNamedProcessTypeMain()
-> (Embedder)ContentMainDelegate::RunProcess()
(or)
-> FooMain() (kMainFunctions)
(or)
-> RunZygote()
(creates its own MainFunctionParams)
-> (Embedder)ContentMainDelegate::RunProcess()
(on OS_ANDROID, browser_test_base.cc calls directly into
ContentMainDelegate::RunProcess())
Few of these needed the params after passing them down so a move-only
model was simple to adapt (even if invasive). The few exceptions like
BrowserMainRunnerImpl::Initialize consuming |created_main_parts_closure|
are better off in the new model (where they take the OnceClosure before
passing down the params) because that prevents others down the chain
from having access to a OnceClosure they shouldn't invoke anyways.
Noteworthy:
- ContentMainDelegate::RunProcess():
Returned an exit_code >= 0 to indicate the embedder elected to handle
the run request given these params. With move-only semantics it is
necessary to return the params back when the embedder declines
handling this run request. An absl::variant return value is used
to satisfy this requirement.
- content/public/test/test_launcher.h : GetContentMainParams():
Becomes CopyContentMainParams() and only exposes a copy of copyable
params. Uses new ContentMainParams::ShallowCopyForTesting() which
verifies that moveable fields are still null by that time as should be
the case in the order browser tests are initialized.
- MainFunctionParams::command_line being const& violated the style-guide
rule to "avoid defining functions that require a const reference
parameter to outlive the call". This also prevented moving. The type
was hence switched to a const CommandLine*.
- BUILD.gn changes for nacl_helper_win_64 which requires static linking
of its minimal //content deps (was previously missing a dep but was
getting away with it because MainFunctionParams was .h only; required
now with .cc). This was already done for static_switches and this CL
adds static_main_function_params, reusing a similar static_features
target that already existed but was no longer required in
/c/nacl/broker, cleaning that up by replacing rather than copying that
target's definition in this CL.
- ContentMainParams::minimal_browser_mode was weirdly passed as a
parameter to ContentMainRunner::Run(bool start_minimal_browser) but
that method also has access to the ContentMainParams originally passed
via ContentMainRunner::Init(). Passing the param again from Run()
would be a use-after-move in content_main.cc, instead
content_main_runner_impl.cc was updated to use the param it already
has in store.
Bug: 1175074
Change-Id: I3af90505525e426383c59107a3903d645d455682
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3244976
Commit-Queue: Gabriel Charette <gab@chromium.org>
Auto-Submit: Gabriel Charette <gab@chromium.org>
Reviewed-by: Alexander Timin <altimin@chromium.org>
Reviewed-by: Brad Nelson <bradnelson@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Clark DuVall <cduvall@chromium.org>
Owners-Override: Alexander Timin <altimin@chromium.org>
Cr-Commit-Position: refs/heads/main@{#940478}
This replaces DISALLOW_COPY_AND_ASSIGN with explicit constructor deletes
where a local script is able to detect its insertion place (~Foo() is
public => insert before this line).
This is incomplete as not all classes have a public ~Foo() declared, so
not all DISALLOW_COPY_AND_ASSIGN occurrences are replaced.
IWYU cleanup is left as a separate pass that is easier when these macros
go away.
Bug: 1010217
Change-Id: Iea478401b7580682c7b9f195f7af9cbbdb6ce315
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3167292
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Owners-Override: Daniel Cheng <dcheng@chromium.org>
Commit-Queue: Peter Boström <pbos@chromium.org>
Cr-Commit-Position: refs/heads/main@{#923194}
In order to leave the base::ThreadPool symbol available for
an upcoming "destination" task trait.
Also moved ThreadPoolImpl to be the implementation of TaskExecutor,
not ThreadPoolInstance. It was a mistake that base::ThreadPool was
implementing TaskExecutor, its users shouldn't have access to
PostTask*() and Create*TaskRunner*().
TBR=fdoray@chromium.org
(bypass owners for side-effects beyond //base)
Bug: 968047
Change-Id: I0607fba6d7f30d202bf7f61a9f461b1256e87467
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1634851
Reviewed-by: Gabriel Charette <gab@chromium.org>
Reviewed-by: François Doray <fdoray@chromium.org>
Commit-Queue: Gabriel Charette <gab@chromium.org>
Auto-Submit: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#664300}
Reason: with the advent of other scheduling primitives in //base
(i.e. SequenceManager), TaskScheduler was no longer the only component
responsible for scheduling tasks. We will from now on refer to the
whole of //base/task as the "task scheduling infrastructure".
There are other types named "TaskScheduler" outside of base:: so
s/TaskScheduler/ThreadPool/ across the codebase wasn't possible.
Instead, this CL did:
1) base/task/task_scheduler => base/task/thread_pool
(catches all files with includes)
1.1) Careful manual search to add files without includes
(e.g. missing IWYU, docs, etc.)
2) TaskScheduler => ThreadPool in all files affected by (1)
3) task_scheduler => thread_pool in all files affected by (1)
4) "task scheduler" => "thread pool" in all files affected by (1)
4) Move task_scheduler_util like headers in
//content //components and //ios
Also:
* Renamed UMA metrics from TaskScheduler.* to ThreadPool.*
and dropped "Pool" from worker pool name suffixes.
* Renamed TaskScheduler*Worker thread names to ThreadPool*Worker
* In base/android: NativeTaskScheduler => NativeScheduler as it
was referring to the whole of base/task.
TaskSchedulerTest.java => NativePostTaskTest.java (former DNE)
* Intentionally ignoring IWYU violations in this already too large
CL.
In follow-up:
* Rename other types as well:
SchedulerWorker => WorkerThread
SchedulerWorkerPool* => WorkerThreadGroup*
Bug: 951388
Change-Id: I5bc2688b593c7682ef7e56d6b228539970ba107e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1561552
Commit-Queue: Gabriel Charette <gab@chromium.org>
Auto-Submit: Gabriel Charette <gab@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: François Doray <fdoray@chromium.org>
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Reviewed-by: Richard Coles <torne@chromium.org>
Reviewed-by: Joe Mason <joenotcharles@google.com>
Reviewed-by: Etienne Pierre-Doray <etiennep@chromium.org>
Cr-Commit-Position: refs/heads/master@{#650997}
This is a follow up CL for: https://crrev.com/c/1115783.
In this CL, we use ScopedExecutionFence to prevents tasks running between the
peorid from BrowserMainRunner is instantiated to
BrowserMainLoop::CreateThreads() is called.
Bug: 846846
Change-Id: I9dec8415e45f901c5883c196ab867c0ea4928594
Reviewed-on: https://chromium-review.googlesource.com/c/1174955
Commit-Queue: Xi Han <hanxi@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#597255}
The service manager thread and (TODO) ServiceManager might be created
before the full browser starts, and we want to reuse them when starting
the full browser. Therefore, we add a pointer of BrowserStartupData in
MainFunctionParams.
Particularly, in this CL, ContentMainRunnerImpl creates and owns a
BrowserStartupData object. It passes a pointer of the BrowserStartupData
through the main function parameter to BrowserMainLoop.
The BrowserStartupData interface was introduced in:
https://crrev.com/c/1117471.
Bug: 846846, 853308
Change-Id: Ie11063227a670cd8d72935131e854ee2b5c46e4e
Reviewed-on: https://chromium-review.googlesource.com/1108178
Commit-Queue: Xi Han <hanxi@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#574054}
This is a reland of I23ef57eb52bfb1eb363682dadf98c571c12afcd1.
It was reverted in Ia548067acbf640010f4c8fbed29a0012a274af05. Reason for revert:
Findit (https://goo.gl/kROfz5) identified this CL at revision 557680 as the culprit
for introducing flakiness in the tests as shown on:
https://findit-for-me.appspot.com/waterfall/flake/flake-culprit?key=ag9zfmZpbmRpdC1mb3ItbWVyQwsSDEZsYWtlQ3VscHJpdCIxY2hyb21pdW0vZmM5NTUwMWZiMjllM2U3NDg3ZmZmOThjMTdlNmFhYmExMDQ2YjMzMAw
The data race has been fixed in:
https://chromium-review.googlesource.com/c/chromium/src/+/1064450.
Original change's description:
> Create BrowserThread::IO thread before browser main loop to start
> ServiceManager.
>
> We need a thread to post/execute tasks when starting the
> ServiceManager. This thread needs to be created before the browser
> main loop is initialized, and will be registered as the
> BrowserThread::IO thread which is currently used by ServiceManager
> connections.
>
> The creation of such a thread is moved to service_manager::main via
> MainDelegate::CreateIOThreadAndGetTaskRunner(). Since it requires no
> thread created before calling fork() on posix, we also move the setup
> of sandbox before creating the IO thread.
>
> Bug: 740677, 729596
> Change-Id: I23ef57eb52bfb1eb363682dadf98c571c12afcd1
> Reviewed-on: https://chromium-review.googlesource.com/969098
> Commit-Queue: Xi Han <hanxi@chromium.org>
> Reviewed-by: John Abd-El-Malek <jam@chromium.org>
> Reviewed-by: Gabriel Charette <gab@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#557680}
Bug: 740677, 729596
Change-Id: I9afb0cdc0f11a1d437c2e9bd09c374503c3d5a4b
Reviewed-on: https://chromium-review.googlesource.com/1059949
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Commit-Queue: Xi Han <hanxi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#560724}
ServiceManager.
We need a thread to post/execute tasks when starting the
ServiceManager. This thread needs to be created before the browser
main loop is initialized, and will be registered as the
BrowserThread::IO thread which is currently used by ServiceManager
connections.
The creation of such a thread is moved to service_manager::main via
MainDelegate::CreateIOThreadAndGetTaskRunner(). Since it requires no
thread created before calling fork() on posix, we also move the setup
of sandbox before creating the IO thread.
Bug: 740677, 729596
Change-Id: I23ef57eb52bfb1eb363682dadf98c571c12afcd1
Reviewed-on: https://chromium-review.googlesource.com/969098
Commit-Queue: Xi Han <hanxi@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#557680}
