From 262a93b2f7eddf8c038bbbd8cdbfa4ee4e9cb3c6 Mon Sep 17 00:00:00 2001 From: Marshall Greenblatt Date: Mon, 11 Dec 2023 15:36:56 -0500 Subject: [PATCH] Split UI thread shutdown into before and after stages (see #3609) Existing UI thread shutdown tasks need to be executed before the UI thread RunLoop is terminated. Those tasks have now been moved to CefMainRunner::StartShutdownOnUIThread and FinishShutdownOnUIThread is now called after the RunLoop is terminated. This fixes a shutdown crash in ChromeProcessSingleton::DeleteInstance. DeleteInstance needs to be called on the UI thread near the end of the shutdown process (after ChromeProcessSingleton::Cleanup is called via PostMainMessageLoopRun). --- libcef/browser/main_runner.cc | 100 +++++++++--------- libcef/browser/main_runner.h | 12 +-- .../alloy/alloy_main_runner_delegate.cc | 7 +- .../common/alloy/alloy_main_runner_delegate.h | 1 + .../chrome/chrome_main_runner_delegate.cc | 13 ++- .../chrome/chrome_main_runner_delegate.h | 3 + libcef/common/main_runner_delegate.h | 1 + 7 files changed, 77 insertions(+), 60 deletions(-) diff --git a/libcef/browser/main_runner.cc b/libcef/browser/main_runner.cc index c3eb99bbd..3dbf759ab 100644 --- a/libcef/browser/main_runner.cc +++ b/libcef/browser/main_runner.cc @@ -162,6 +162,10 @@ class CefUIThread : public base::PlatformThread::Delegate { CHECK_EQ(exit_code, -1); } + void set_shutdown_callback(base::OnceClosure shutdown_callback) { + shutdown_callback_ = std::move(shutdown_callback); + } + protected: void ThreadMain() override { base::PlatformThread::SetName("CefUIThread"); @@ -182,6 +186,8 @@ class CefUIThread : public base::PlatformThread::Delegate { content::BrowserTaskExecutor::Shutdown(); + std::move(shutdown_callback_).Run(); + // Run exit callbacks on the UI thread to avoid sequence check failures. base::AtExitManager::ProcessCallbacksNow(); @@ -194,6 +200,7 @@ class CefUIThread : public base::PlatformThread::Delegate { CefMainRunner* const runner_; base::OnceClosure setup_callback_; + base::OnceClosure shutdown_callback_; std::unique_ptr browser_runner_; @@ -252,29 +259,45 @@ bool CefMainRunner::Initialize(CefSettings* settings, void CefMainRunner::Shutdown(base::OnceClosure shutdown_on_ui_thread, base::OnceClosure finalize_shutdown) { if (multi_threaded_message_loop_) { - // Events that will be used to signal when shutdown is complete. Start in - // non-signaled mode so that the event will block. - base::WaitableEvent uithread_shutdown_event( - base::WaitableEvent::ResetPolicy::AUTOMATIC, - base::WaitableEvent::InitialState::NOT_SIGNALED); + // Start shutdown on the UI thread. This is guaranteed to run before the + // thread RunLoop has stopped. + CEF_POST_TASK(CEF_UIT, + base::BindOnce(&CefMainRunner::StartShutdownOnUIThread, + base::Unretained(this), + std::move(shutdown_on_ui_thread))); - // Finish shutdown on the UI thread. - CEF_POST_TASK( - CEF_UIT, - base::BindOnce(&CefMainRunner::FinishShutdownOnUIThread, - base::Unretained(this), std::move(shutdown_on_ui_thread), - &uithread_shutdown_event)); + // Finish shutdown on the UI thread after the thread RunLoop has stopped and + // before running exit callbacks. + ui_thread_->set_shutdown_callback(base::BindOnce( + &CefMainRunner::FinishShutdownOnUIThread, base::Unretained(this))); - /// Block until UI thread shutdown is complete. - uithread_shutdown_event.Wait(); - - FinalizeShutdown(std::move(finalize_shutdown)); - } else { - // Finish shutdown on the current thread, which should be the UI thread. - FinishShutdownOnUIThread(std::move(shutdown_on_ui_thread), nullptr); - - FinalizeShutdown(std::move(finalize_shutdown)); + // Blocks until the thread has stopped. + ui_thread_->Stop(); + ui_thread_.reset(); } + + main_delegate_->BeforeMainThreadShutdown(); + + if (!multi_threaded_message_loop_) { + // Main thread and UI thread are the same. + StartShutdownOnUIThread(std::move(shutdown_on_ui_thread)); + + browser_runner_->Shutdown(); + browser_runner_.reset(); + + FinishShutdownOnUIThread(); + } + + // Shut down the content runner. + content::ContentMainShutdown(main_runner_.get()); + + main_runner_.reset(); + + std::move(finalize_shutdown).Run(); + main_delegate_->AfterMainThreadShutdown(); + + main_delegate_.reset(); + g_runtime_type = RuntimeType::UNINITIALIZED; } void CefMainRunner::RunMessageLoop() { @@ -493,9 +516,8 @@ void CefMainRunner::OnContextInitialized( std::move(context_initialized).Run(); } -void CefMainRunner::FinishShutdownOnUIThread( - base::OnceClosure shutdown_on_ui_thread, - base::WaitableEvent* uithread_shutdown_event) { +void CefMainRunner::StartShutdownOnUIThread( + base::OnceClosure shutdown_on_ui_thread) { CEF_REQUIRE_UIT(); // Execute all pending tasks now before proceeding with shutdown. Otherwise, @@ -512,37 +534,11 @@ void CefMainRunner::FinishShutdownOnUIThread( ->ShutdownOnUIThread(); std::move(shutdown_on_ui_thread).Run(); - main_delegate_->AfterUIThreadShutdown(); - - if (uithread_shutdown_event) { - uithread_shutdown_event->Signal(); - } + main_delegate_->BeforeUIThreadShutdown(); } -void CefMainRunner::FinalizeShutdown(base::OnceClosure finalize_shutdown) { - main_delegate_->BeforeMainThreadShutdown(); - - if (browser_runner_.get()) { - browser_runner_->Shutdown(); - browser_runner_.reset(); - } - - if (ui_thread_.get()) { - // Blocks until the thread has stopped. - ui_thread_->Stop(); - ui_thread_.reset(); - } - - // Shut down the content runner. - content::ContentMainShutdown(main_runner_.get()); - - main_runner_.reset(); - - std::move(finalize_shutdown).Run(); - main_delegate_->AfterMainThreadShutdown(); - - main_delegate_.reset(); - g_runtime_type = RuntimeType::UNINITIALIZED; +void CefMainRunner::FinishShutdownOnUIThread() { + main_delegate_->AfterUIThreadShutdown(); } // From libcef/features/runtime.h: diff --git a/libcef/browser/main_runner.h b/libcef/browser/main_runner.h index 56e6b7fd7..f9252a6ab 100644 --- a/libcef/browser/main_runner.h +++ b/libcef/browser/main_runner.h @@ -71,13 +71,13 @@ class CefMainRunner : public CefMainRunnerHandler { // Called on the UI thread after the context is initialized. void OnContextInitialized(base::OnceClosure context_initialized); - // Performs shutdown actions that need to occur on the UI thread before any - // threads are destroyed. - void FinishShutdownOnUIThread(base::OnceClosure shutdown_on_ui_thread, - base::WaitableEvent* uithread_shutdown_event); + // Performs shutdown actions that need to occur on the UI thread before the + // thread RunLoop has stopped. + void StartShutdownOnUIThread(base::OnceClosure shutdown_on_ui_thread); - // Destroys the runtime and related objects. - void FinalizeShutdown(base::OnceClosure finalize_shutdown); + // Performs shutdown actions that need to occur on the UI thread after the + // thread RunLoop has stopped and before running exit callbacks. + void FinishShutdownOnUIThread(); const bool multi_threaded_message_loop_; const bool external_message_pump_; diff --git a/libcef/common/alloy/alloy_main_runner_delegate.cc b/libcef/common/alloy/alloy_main_runner_delegate.cc index 883a2eaf3..52e604084 100644 --- a/libcef/common/alloy/alloy_main_runner_delegate.cc +++ b/libcef/common/alloy/alloy_main_runner_delegate.cc @@ -9,6 +9,7 @@ #include "libcef/common/alloy/alloy_main_delegate.h" #include "libcef/renderer/alloy/alloy_content_renderer_client.h" +#include "chrome/browser/chrome_process_singleton.h" #include "content/public/browser/render_process_host.h" #include "ui/base/resource/resource_bundle.h" @@ -42,13 +43,17 @@ void AlloyMainRunnerDelegate::AfterUIThreadInitialize() { ->OnContextInitialized(); } -void AlloyMainRunnerDelegate::AfterUIThreadShutdown() { +void AlloyMainRunnerDelegate::BeforeUIThreadShutdown() { static_cast(g_browser_process) ->CleanupOnUIThread(); ui::ResourceBundle::GetSharedInstance().CleanupOnUIThread(); } +void AlloyMainRunnerDelegate::AfterUIThreadShutdown() { + ChromeProcessSingleton::DeleteInstance(); +} + void AlloyMainRunnerDelegate::BeforeMainThreadShutdown() { if (content::RenderProcessHost::run_renderer_in_process()) { // Blocks until RenderProcess cleanup is complete. diff --git a/libcef/common/alloy/alloy_main_runner_delegate.h b/libcef/common/alloy/alloy_main_runner_delegate.h index a256f9600..c0be48fe9 100644 --- a/libcef/common/alloy/alloy_main_runner_delegate.h +++ b/libcef/common/alloy/alloy_main_runner_delegate.h @@ -33,6 +33,7 @@ class AlloyMainRunnerDelegate : public CefMainRunnerDelegate { void BeforeMainThreadInitialize(const CefMainArgs& args) override; void BeforeMainThreadRun(bool multi_threaded_message_loop) override; void AfterUIThreadInitialize() override; + void BeforeUIThreadShutdown() override; void AfterUIThreadShutdown() override; void BeforeMainThreadShutdown() override; void AfterMainThreadShutdown() override; diff --git a/libcef/common/chrome/chrome_main_runner_delegate.cc b/libcef/common/chrome/chrome_main_runner_delegate.cc index 004f98998..ce44e62e6 100644 --- a/libcef/common/chrome/chrome_main_runner_delegate.cc +++ b/libcef/common/chrome/chrome_main_runner_delegate.cc @@ -12,6 +12,7 @@ #include "base/run_loop.h" #include "chrome/browser/browser_process_impl.h" #include "chrome/browser/chrome_content_browser_client.h" +#include "chrome/browser/chrome_process_singleton.h" #include "chrome/common/profiler/main_thread_stack_sampling_profiler.h" #include "components/keep_alive_registry/keep_alive_types.h" #include "components/keep_alive_registry/scoped_keep_alive.h" @@ -46,6 +47,8 @@ void ChromeMainRunnerDelegate::BeforeMainThreadInitialize( void ChromeMainRunnerDelegate::BeforeMainThreadRun( bool multi_threaded_message_loop) { if (multi_threaded_message_loop) { + multi_threaded_message_loop_ = true; + // Detach from the main thread so that these objects can be attached and // modified from the UI thread going forward. metrics::GlobalPersistentSystemProfile::GetInstance() @@ -83,7 +86,7 @@ void ChromeMainRunnerDelegate::BeforeUIThreadInitialize() { sampling_profiler_ = std::make_unique(); } -void ChromeMainRunnerDelegate::AfterUIThreadShutdown() { +void ChromeMainRunnerDelegate::BeforeUIThreadShutdown() { static_cast( CefAppManager::Get()->GetContentClient()->browser()) ->CleanupOnUIThread(); @@ -92,6 +95,14 @@ void ChromeMainRunnerDelegate::AfterUIThreadShutdown() { sampling_profiler_.reset(); } +void ChromeMainRunnerDelegate::AfterUIThreadShutdown() { + if (multi_threaded_message_loop_) { + // Don't wait for this to be called in ChromeMainDelegate::ProcessExiting. + // It is safe to call multiple times. + ChromeProcessSingleton::DeleteInstance(); + } +} + void ChromeMainRunnerDelegate::BeforeExecuteProcess(const CefMainArgs& args) { BeforeMainThreadInitialize(args); } diff --git a/libcef/common/chrome/chrome_main_runner_delegate.h b/libcef/common/chrome/chrome_main_runner_delegate.h index 13216b207..3e93ecfcb 100644 --- a/libcef/common/chrome/chrome_main_runner_delegate.h +++ b/libcef/common/chrome/chrome_main_runner_delegate.h @@ -37,6 +37,7 @@ class ChromeMainRunnerDelegate : public CefMainRunnerDelegate { void BeforeMainMessageLoopRun(base::RunLoop* run_loop) override; bool HandleMainMessageLoopQuit() override; void BeforeUIThreadInitialize() override; + void BeforeUIThreadShutdown() override; void AfterUIThreadShutdown() override; void BeforeExecuteProcess(const CefMainArgs& args) override; void AfterExecuteProcess() override; @@ -50,6 +51,8 @@ class ChromeMainRunnerDelegate : public CefMainRunnerDelegate { CefMainRunnerHandler* const runner_; CefSettings* const settings_; CefRefPtr application_; + + bool multi_threaded_message_loop_ = false; }; #endif // CEF_LIBCEF_COMMON_CHROME_CHROME_MAIN_RUNNER_DELEGATE_CEF_ diff --git a/libcef/common/main_runner_delegate.h b/libcef/common/main_runner_delegate.h index daf40807b..2a433394c 100644 --- a/libcef/common/main_runner_delegate.h +++ b/libcef/common/main_runner_delegate.h @@ -26,6 +26,7 @@ class CefMainRunnerDelegate { virtual bool HandleMainMessageLoopQuit() { return false; } virtual void BeforeUIThreadInitialize() {} virtual void AfterUIThreadInitialize() {} + virtual void BeforeUIThreadShutdown() {} virtual void AfterUIThreadShutdown() {} virtual void BeforeMainThreadShutdown() {} virtual void AfterMainThreadShutdown() {}