Names for Blink internal objects are helpful to have when debugging
certain leak or OOM scenarios, but the steps to enable them aren't
written down anywhere I could find. Document them for future reference.
Change-Id: Ibe4092cdd255b962dea45f3c841d48b4799f5702
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5032492
Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Commit-Queue: Kevin Babbitt <kbabbitt@microsoft.com>
Cr-Commit-Position: refs/heads/main@{#1225537}
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}
Sadly dskiba and mariakhomenko are moving on to work on something else.
We are scraping really hard to fill the gaps. Updating the docs to the
new harsh realities.
Bug: None
Change-Id: I2e8899ad975ba1279a95ce4f702d20756981ed47
Reviewed-on: https://chromium-review.googlesource.com/1106617
Commit-Queue: Egor Pasko <pasko@chromium.org>
Reviewed-by: Maria Khomenko <mariakhomenko@chromium.org>
Reviewed-by: Siddhartha S <ssid@chromium.org>
Cr-Commit-Position: refs/heads/master@{#568787}
This CL has no intended behavior change.
In addition to moving the code, this CL:
* Updates the names of classes/files to remove the Memlog prefix.
* Updates the name of the service to be "Heap Profiling Service" from
"Profiling Service".
* profiling_browsertest was removed as it wasn't testing anything useful.
There is a large suite of end-to-end browser tests in memlog_browsertest.cc
[untouched by this CL].
Bug: 827545
Change-Id: I7ef0947d7de4070d1863c509e2d280cefd4fec2d
Reviewed-on: https://chromium-review.googlesource.com/995641
Commit-Queue: Erik Chen <erikchen@chromium.org>
Reviewed-by: Nico Weber <thakis@chromium.org>
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Reviewed-by: Ken Rockot <rockot@chromium.org>
Reviewed-by: Cait Phillips <caitkp@chromium.org>
Cr-Commit-Position: refs/heads/master@{#548510}
This overlaps some of the documentation in docs/memory-infra.
The objective of this documentation is to provide a higher-level summary
of the features with a description about the types of memory questions
that they can answer as opposed to a detailed explanation of all the
attributes.
Bug: 801006
Change-Id: Idbbc2815b1be8721c7a0af195e32e0792fa31215
Reviewed-on: https://chromium-review.googlesource.com/869854
Reviewed-by: Maria Khomenko <mariakhomenko@chromium.org>
Reviewed-by: Erik Chen <erikchen@chromium.org>
Commit-Queue: Albert J. Wong <ajwong@chromium.org>
Cr-Commit-Position: refs/heads/master@{#529940}
First in a set of changes to the memory directory. The idea is to consolidate
some overview documentation for techniques and tools used, even if the
documentation does nothing more than name the tool or provide keywords for
a dev to start doing searches on.
The idea is to get some of the tribal knowledge down on paper.
Overall, the docs will focus on helping answer problems as opposed to
describing how things work.
Bug: 801006
Change-Id: I93c72cabfafdbd103eda64aa94791a23885663ae
Reviewed-on: https://chromium-review.googlesource.com/861197
Reviewed-by: Erik Chen <erikchen@chromium.org>
Commit-Queue: Albert J. Wong <ajwong@chromium.org>
Cr-Commit-Position: refs/heads/master@{#528691}
Cuz wez@ asked for it.
Careful what you wish for.
TBR=wez
Bug: I ain't creating a bug for this.
Change-Id: Ifc7d4275383417fe30b9c65c5c0571f144ddf0d0
Reviewed-on: https://chromium-review.googlesource.com/536155
Reviewed-by: Albert J. Wong <ajwong@chromium.org>
Cr-Commit-Position: refs/heads/master@{#479543}
