Doing this isn't immediately problematic: `~SupportsUserData()` attempts
to work around reentrant calls into itself by swapping out the backing
map during destruction.
However, if code calls `SetUserData()` on the `SupportsUserData` being
destroyed, this means the backing map is no longer empty—and when the
body of the destructor is complete, if the same problematic code calls
`SetUserData()` while `SupportsUserData`'s fields are being destroyed,
bad things will happen, since containers:
- require iterating over the elements during destruction
- are not robust against mutation of the container itself during
iteration
There are no new checks added to `GetUserData()`. While not ideal, it
is not dangerous in the same way that calling `SetUserData()` would be.
`GetUserData()` will always return `nullptr` in this case.
Checks *will* be added to `RemoveUserData()` and `TakeUserData()` in
followups. While they are not actively dangerous in the same way
`SetUserData()` is, it is conceptually incorrect to attempt to mutate
the `SupportsUserData` during destruction. It is also entirely possible
that landing this CL will reveal interesting bugs in the wild, so
landing these checks in separate CLs means that reverts can be limited
in scope.
Bug: 1483288
Change-Id: Ib21626a5ce59480baf231c93630642484b9c3148
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4906190
Reviewed-by: danakj <danakj@chromium.org>
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1204380}
Note that this simply deletes the base::flat_map arm; the experiment was
considered successful, and the default container guidelines are being
updated to recommend absl::flat_hash_map as a default. As such, it does
not seem to make sense to fall back to base::flat_map temporarily, only
to convert it again later.
Bug: 1121345
Change-Id: I627cab7a6ef313308811beb6e680e7a037a5eaa2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4906604
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1204305}
Introduce NavigationEntryScreenshot, which preserves a screenshot on the
current NavigationEntry when the user is navigating away from it. The
stored screenshot will later be used to present the user with a preview,
when the user navigates back to this page.
The caching mechanism is composed of the screenshot, the cache and a
global manager. The global manager lives on the BrowserContext and is
responsible for memory budgeting. The cache lives on the primary
NavigationController which owns the NavigationEntry where the
screenshot is stashed.
This CL contains the impl of caching, retrieving and eviction of the
screenshots with unittests. The cache is not integrated with the navigation thus no browsertest.
The code resides in `//content/browser/renderer_host/` because no need
to access WebContents.
[Updated] DD/Explainer (chromium access): https://docs.google.com/document/d/1-7TatVjSL4n-RNyvN_MWdFoOiR6YJ1BjvJKgvym_ArU
Feature EngDoc: https://docs.google.com/document/d/1H99XZHdAWNfbfboHbGdMB97k81AzAxVtPSizAjO2Poc/
PRD (google access): https://docs.google.com/document/d/1JMWzArv0VR-G4xx3a5ku8N5np8mDeGSuvR95kZc8ynY/
Bug: 1415332, 1413521
Change-Id: Icc3b2aa7b5b36ed460ad2ddd1ff5f645a38d2835
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4245857
Reviewed-by: Kyle Charbonneau <kylechar@chromium.org>
Commit-Queue: William Liu <liuwilliam@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1125127}
For a pointer-sized key to a pointer-sized value, absl::flat_hash_map
should have strictly better memory scaling than both std::map and
std::unordered_map. In addition, the map data is stored inline rather
than in nodes, so CPU scaling should be strictly better as well. An
actual A/B experiment should help confirm this hypothesis (or even
confirm that the impact is negligible—which would still be fine).
Bug: 1121345
Change-Id: I757b0c057d376e0733ab996de287ce5611d91ca3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4136838
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Mirko Bonadei <mbonadei@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1089555}
Allow SupportsUserData to be used by subclasses which can be "reset", by letting
the subclass clear all associated user-data if required.
(e.g. CertVerifyResult has a Reset method, so if it subclass SupportsUserData,
it also needs a way to clear any attached user data when Reset is called.)
Bug: 991247
Change-Id: I3b3edaa58b2e70daac453edd28e4aa37eee17bf2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1738628
Commit-Queue: Matt Mueller <mattm@chromium.org>
Reviewed-by: Wez <wez@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#686528}
Making SupportsUserData movable allows it to be used to allow additional
data to be attached to movable data objects, e.g. we'll use this to allow
CertVerifyResult instances to have debug information attached by
different verifier implementations
Bug: 991247
Change-Id: Iba8c9000750559e7edbafac8e5ebefddf8a38e10
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1739147
Reviewed-by: Wez <wez@chromium.org>
Commit-Queue: Matt Mueller <mattm@chromium.org>
Cr-Commit-Position: refs/heads/master@{#685843}
SupportsUserData takes ownership of the SupportsUserData::Data
passed to SetUserData. Add an overload taking a std::unique_ptr
so that client code can be converted incrementally.
BUG=690937
Review-Url: https://codereview.chromium.org/2680403004
Cr-Commit-Position: refs/heads/master@{#449933}
