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}
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}
This CL moves GTK and Aura "Parts" out of the primary BrowserMainParts tree and into orthogonal parts that can be added independently. This was done in a way that will facilitate adding additional parts if (when) we need them.
The motivation for this was to
a) eliminate the existing typedef in chrome_browser_main_chromeos.h
b) reduce the number of #ifdefs required in the setup / parts code
For an outline of the new parts see:
https://docs.google.com/drawings/d/1-gIMl-81c4SvcMrT1xaxnDGibDe7VQfMkFT1bMnIvrg/edit?hl=en_US
Please consider this a proposal; I am entirely open to feedback.
BUG=none
TEST=Chrome compiles and passes tests on all platfroms.
Review URL: http://codereview.chromium.org/8302016
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@107383 0039d316-1c4b-4281-b951-d872f2087c98
Puts CHECKs in RenderProcessHost's constructor to make sure the main MessageLoop hasn't stopped running yet.
BUG=91398
TEST=none
Review URL: http://codereview.chromium.org/8210004
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@104849 0039d316-1c4b-4281-b951-d872f2087c98
