0

Preventing a dangling ExtensionRegistrar::extension_system_.

When `KeyedService`s are getting destroyed, their dependencies might
already be gone (and pointers to such dependencies might be dangling).
As documented in //components/keyed_service/core/keyed_service.h
"two-phase shutdown allows keyed services to have a first pass shutdown
phase where they drop references".  This was discussed in the following
(Google-internal, sorry) email thread:
https://groups.google.com/a/google.com/g/chrome-memory-safety/c/kLaT9TsZyS8/m/9t5KGRnvBQAJ

Bug: 1444204
Change-Id: If69aaddafccd3be331045686c71fd40ae884d1a2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4550894
Auto-Submit: Łukasz Anforowicz <lukasza@chromium.org>
Reviewed-by: Arthur Sonzogni <arthursonzogni@chromium.org>
Commit-Queue: David Bertoni <dbertoni@chromium.org>
Reviewed-by: David Bertoni <dbertoni@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1147993}
This commit is contained in:
Lukasz Anforowicz
2023-05-23 17:16:08 +00:00
committed by Chromium LUCI CQ
parent d87e587be4
commit d6d7ab9c74
4 changed files with 20 additions and 1 deletions

@ -480,6 +480,7 @@ void ExtensionService::Shutdown() {
this);
external_install_manager_->Shutdown();
corrupted_extension_reinstaller_.Shutdown();
extension_registrar_.Shutdown();
}
void ExtensionService::Init() {

@ -73,6 +73,15 @@ appear before unowned members (`raw_ptr<>`), and to make the unowned members
appear last in the class, since the unowned members often refer to resources
owned by the owning members or the class itself.
One sub-category of destruction order issues is related to `KeyedService`s which
need to correctly
[declare their dependencies](https://source.chromium.org/chromium/chromium/src/+/main:components/keyed_service/core/keyed_service_base_factory.h;l=60-62;drc=8ba1bad80dc22235693a0dd41fe55c0fd2dbdabd)
and
[are expected to drop references](https://source.chromium.org/chromium/chromium/src/+/main:components/keyed_service/core/keyed_service.h;l=12-13;drc=8ba1bad80dc22235693a0dd41fe55c0fd2dbdabd)
to their dependencies in their
[`Shutdown`](https://source.chromium.org/chromium/chromium/src/+/main:components/keyed_service/core/keyed_service.h;l=36-39;drc=8ba1bad80dc22235693a0dd41fe55c0fd2dbdabd) method
(i.e. before their destructor runs).
#### Observer callback
This represents ~4% of the dangling pointers.

@ -54,6 +54,12 @@ ExtensionRegistrar::ExtensionRegistrar(content::BrowserContext* browser_context,
ExtensionRegistrar::~ExtensionRegistrar() = default;
void ExtensionRegistrar::Shutdown() {
// Setting to `nullptr`, because this raw pointer may become dangling once
// the `ExtensionSystem` keyed service is destroyed.
extension_system_ = nullptr;
}
void ExtensionRegistrar::AddExtension(
scoped_refptr<const Extension> extension) {
DCHECK_CURRENTLY_ON(content::BrowserThread::UI);

@ -97,6 +97,9 @@ class ExtensionRegistrar : public ProcessManagerObserver {
~ExtensionRegistrar() override;
// Called when the associated Profile is going to be destroyed.
void Shutdown();
// Adds the extension to the ExtensionRegistry. The extension will be added to
// the enabled, disabled, blocklisted or blocked set. If the extension is
// added as enabled, it will be activated.
@ -186,7 +189,7 @@ class ExtensionRegistrar : public ProcessManagerObserver {
const raw_ptr<Delegate> delegate_;
// Keyed services we depend on. Cached here for repeated access.
const raw_ptr<ExtensionSystem, DanglingUntriaged> extension_system_;
raw_ptr<ExtensionSystem> extension_system_;
const raw_ptr<ExtensionPrefs> extension_prefs_;
const raw_ptr<ExtensionRegistry> registry_;
const raw_ptr<RendererStartupHelper> renderer_helper_;