From d87e3f41f66a7f422ec49d7f2b9af4e05de02524 Mon Sep 17 00:00:00 2001 From: Marshall Greenblatt Date: Fri, 28 Jan 2022 16:16:29 -0500 Subject: [PATCH] alloy: Move ExtensionsBrowserClient ownership to BrowserProcess (fixes issue #3247) Fixes a shutdown crash due to `ExtensionsBrowserClient::Set(nullptr)` being called too early. Some code that may occasionally be triggered via `content::ContentMainShutdown()` is expecting the extensions objects to still be valid. This new ownership pattern matches the code in chrome/. --- libcef/browser/alloy/alloy_browser_main.cc | 17 +++-------------- libcef/browser/alloy/alloy_browser_main.h | 9 --------- .../alloy/chrome_browser_process_alloy.cc | 18 ++++++++++++++++++ .../alloy/chrome_browser_process_alloy.h | 9 +++++++++ 4 files changed, 30 insertions(+), 23 deletions(-) diff --git a/libcef/browser/alloy/alloy_browser_main.cc b/libcef/browser/alloy/alloy_browser_main.cc index 7154ce4ce..301cfbe99 100644 --- a/libcef/browser/alloy/alloy_browser_main.cc +++ b/libcef/browser/alloy/alloy_browser_main.cc @@ -13,12 +13,10 @@ #include "libcef/browser/context.h" #include "libcef/browser/devtools/devtools_manager_delegate.h" #include "libcef/browser/extensions/extension_system_factory.h" -#include "libcef/browser/extensions/extensions_browser_client.h" #include "libcef/browser/net/chrome_scheme_handler.h" #include "libcef/browser/printing/constrained_window_views_client.h" #include "libcef/browser/thread_util.h" #include "libcef/common/app_manager.h" -#include "libcef/common/extensions/extensions_client.h" #include "libcef/common/extensions/extensions_util.h" #include "libcef/common/net/net_resource_provider.h" @@ -35,7 +33,7 @@ #include "content/public/browser/gpu_data_manager.h" #include "content/public/browser/network_service_instance.h" #include "content/public/common/result_codes.h" -#include "extensions/browser/extension_system.h" +#include "extensions/browser/extensions_browser_client.h" #include "extensions/common/constants.h" #include "net/base/net_module.h" #include "third_party/widevine/cdm/buildflags.h" @@ -175,14 +173,10 @@ int AlloyBrowserMainParts::PreMainMessageLoopRun() { #endif if (extensions::ExtensionsEnabled()) { + // This should be set in ChromeBrowserProcessAlloy::Initialize. + DCHECK(extensions::ExtensionsBrowserClient::Get()); // Initialize extension global objects before creating the global // BrowserContext. - extensions_client_.reset(new extensions::CefExtensionsClient()); - extensions::ExtensionsClient::Set(extensions_client_.get()); - extensions_browser_client_.reset( - new extensions::CefExtensionsBrowserClient); - extensions::ExtensionsBrowserClient::Set(extensions_browser_client_.get()); - extensions::CefExtensionSystemFactory::GetInstance(); } @@ -253,11 +247,6 @@ void AlloyBrowserMainParts::PostMainMessageLoopRun() { } void AlloyBrowserMainParts::PostDestroyThreads() { - if (extensions::ExtensionsEnabled()) { - extensions::ExtensionsBrowserClient::Set(nullptr); - extensions_browser_client_.reset(); - } - #if defined(TOOLKIT_VIEWS) views_delegate_.reset(); #if defined(OS_MAC) diff --git a/libcef/browser/alloy/alloy_browser_main.h b/libcef/browser/alloy/alloy_browser_main.h index 9ed3161f3..ff3645b9d 100644 --- a/libcef/browser/alloy/alloy_browser_main.h +++ b/libcef/browser/alloy/alloy_browser_main.h @@ -15,11 +15,6 @@ #include "content/public/browser/browser_main_parts.h" #include "content/public/common/main_function_params.h" -namespace extensions { -class ExtensionsBrowserClient; -class ExtensionsClient; -} // namespace extensions - #if defined(USE_AURA) namespace display { class Screen; @@ -84,10 +79,6 @@ class AlloyBrowserMainParts : public content::BrowserMainParts { CefRefPtr global_request_context_; CefDevToolsDelegate* devtools_delegate_ = nullptr; // Deletes itself. - std::unique_ptr extensions_client_; - std::unique_ptr - extensions_browser_client_; - // Blocking task runners exposed via CefTaskRunner. For consistency with // previous named thread behavior always execute all pending tasks before // shutdown (e.g. to make sure critical data is saved to disk). diff --git a/libcef/browser/alloy/chrome_browser_process_alloy.cc b/libcef/browser/alloy/chrome_browser_process_alloy.cc index 40de08878..5f5cbfb99 100644 --- a/libcef/browser/alloy/chrome_browser_process_alloy.cc +++ b/libcef/browser/alloy/chrome_browser_process_alloy.cc @@ -8,9 +8,12 @@ #include "libcef/browser/alloy/chrome_profile_manager_alloy.h" #include "libcef/browser/browser_context.h" #include "libcef/browser/context.h" +#include "libcef/browser/extensions/extensions_browser_client.h" #include "libcef/browser/prefs/browser_prefs.h" #include "libcef/browser/thread_util.h" #include "libcef/common/cef_switches.h" +#include "libcef/common/extensions/extensions_client.h" +#include "libcef/common/extensions/extensions_util.h" #include "base/command_line.h" #include "chrome/browser/component_updater/chrome_component_updater_configurator.h" @@ -38,6 +41,11 @@ ChromeBrowserProcessAlloy::ChromeBrowserProcessAlloy() ChromeBrowserProcessAlloy::~ChromeBrowserProcessAlloy() { DCHECK((!initialized_ && !context_initialized_) || shutdown_); + + if (extensions::ExtensionsEnabled()) { + extensions::ExtensionsBrowserClient::Set(nullptr); + extensions_browser_client_.reset(); + } } void ChromeBrowserProcessAlloy::Initialize() { @@ -49,6 +57,16 @@ void ChromeBrowserProcessAlloy::Initialize() { // Initialize this early before any code tries to check feature flags. field_trial_list_ = content::SetUpFieldTrialsAndFeatureList(); + if (extensions::ExtensionsEnabled()) { + // Initialize extension global objects before creating the global + // BrowserContext. + extensions_client_.reset(new extensions::CefExtensionsClient()); + extensions::ExtensionsClient::Set(extensions_client_.get()); + extensions_browser_client_.reset( + new extensions::CefExtensionsBrowserClient); + extensions::ExtensionsBrowserClient::Set(extensions_browser_client_.get()); + } + initialized_ = true; } diff --git a/libcef/browser/alloy/chrome_browser_process_alloy.h b/libcef/browser/alloy/chrome_browser_process_alloy.h index 2b9ea1a20..19af6137e 100644 --- a/libcef/browser/alloy/chrome_browser_process_alloy.h +++ b/libcef/browser/alloy/chrome_browser_process_alloy.h @@ -18,6 +18,11 @@ #include "chrome/browser/extensions/event_router_forwarder.h" #include "media/media_buildflags.h" +namespace extensions { +class ExtensionsBrowserClient; +class ExtensionsClient; +} // namespace extensions + class ChromeProfileManagerAlloy; class BackgroundModeManager { @@ -112,6 +117,10 @@ class ChromeBrowserProcessAlloy : public BrowserProcess { bool context_initialized_; bool shutdown_; + std::unique_ptr extensions_client_; + std::unique_ptr + extensions_browser_client_; + std::string locale_; std::unique_ptr print_job_manager_; std::unique_ptr profile_manager_;