From 1abe4bb58d5300b156394caadbe7fc11e38b5fb5 Mon Sep 17 00:00:00 2001 From: Nicolas Ouellet-Payeur Date: Wed, 9 Feb 2022 22:31:39 +0000 Subject: [PATCH] M98 merge: [Extensions] Fix a null dereference in CrxInstaller This class holds a ScopedProfileKeepAlive, which points to a Profile. This SPKA can be destroy too late, if Profile is destroyed before it. This can only happen if Chrome is shutting down entirely, but we need to perform some cleanup in that case. (cherry picked from commit fa1f6c3b5e699836a8f26a7ea454899fa454f289) Bug: 1291767 Change-Id: Id6c411edcf4fe8d7d0bd0d941de9180e95b08e4e Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3424799 Commit-Queue: Nicolas Ouellet-Payeur Auto-Submit: Nicolas Ouellet-Payeur Reviewed-by: Devlin Cronin Commit-Queue: Devlin Cronin Cr-Original-Commit-Position: refs/heads/main@{#964665} Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3450858 Cr-Commit-Position: refs/branch-heads/4758@{#1124} Cr-Branched-From: 4a2cf4baf90326df19c3ee70ff987960d59a386e-refs/heads/main@{#950365} --- chrome/browser/extensions/crx_installer.cc | 8 ++++++++ chrome/browser/extensions/crx_installer.h | 10 +++++++++- 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/chrome/browser/extensions/crx_installer.cc b/chrome/browser/extensions/crx_installer.cc index 38dac2056e39dc..c00f1151cfe6d2 100644 --- a/chrome/browser/extensions/crx_installer.cc +++ b/chrome/browser/extensions/crx_installer.cc @@ -129,6 +129,8 @@ CrxInstaller::CrxInstaller(base::WeakPtr service_weak, shared_file_task_runner_(GetExtensionFileTaskRunner()), update_from_settings_page_(false), install_flags_(kInstallFlagNone) { + profile_observation_.Observe(profile_); + if (!approval) return; @@ -601,6 +603,12 @@ void CrxInstaller::OnStageChanged(InstallationStage stage) { ReportInstallationStage(stage); } +void CrxInstaller::OnProfileWillBeDestroyed(Profile* profile) { + DCHECK_EQ(profile, profile_); + profile_keep_alive_.reset(); + profile_observation_.Reset(); +} + void CrxInstaller::CheckInstall() { DCHECK_CURRENTLY_ON(BrowserThread::UI); ExtensionService* service = service_weak_.get(); diff --git a/chrome/browser/extensions/crx_installer.h b/chrome/browser/extensions/crx_installer.h index 089ab148b0c962..608e0d9f973690 100644 --- a/chrome/browser/extensions/crx_installer.h +++ b/chrome/browser/extensions/crx_installer.h @@ -14,10 +14,12 @@ #include "base/memory/raw_ptr.h" #include "base/memory/ref_counted.h" #include "base/memory/weak_ptr.h" +#include "base/scoped_observation.h" #include "base/version.h" #include "chrome/browser/extensions/extension_install_prompt.h" #include "chrome/browser/extensions/extension_service.h" #include "chrome/browser/extensions/webstore_installer.h" +#include "chrome/browser/profiles/profile_observer.h" #include "chrome/common/extensions/extension_constants.h" #include "components/sync/model/string_ordinal.h" #include "extensions/browser/api/declarative_net_request/ruleset_install_pref.h" @@ -72,7 +74,7 @@ class PreloadCheckGroup; // terminating during the install. We can't listen for the app termination // notification here in this class because it can be destroyed on any thread // and won't safely be able to clean up UI thread notification listeners. -class CrxInstaller : public SandboxedUnpackerClient { +class CrxInstaller : public SandboxedUnpackerClient, public ProfileObserver { public: // A callback to be executed when the install finishes. using InstallerResultCallback = ExtensionSystem::InstallUpdateCallback; @@ -302,6 +304,9 @@ class CrxInstaller : public SandboxedUnpackerClient { ruleset_install_prefs) override; void OnStageChanged(InstallationStage stage) override; + // ProfileObserver + void OnProfileWillBeDestroyed(Profile* profile) override; + // Called on the UI thread to start the requirements, policy and blocklist // checks on the extension. void CheckInstall(); @@ -371,6 +376,9 @@ class CrxInstaller : public SandboxedUnpackerClient { // Prevent Profile destruction until the CrxInstaller is done. std::unique_ptr profile_keep_alive_; + // ... but |profile_| could still get destroyed early, if Chrome shuts down + // completely. We need to perform some cleanup if that happens. + base::ScopedObservation profile_observation_{this}; // The extension being installed. scoped_refptr extension_;