From c30e52c62084b37525ec4d9fecc6fdcc39797ba9 Mon Sep 17 00:00:00 2001 From: Marshall Greenblatt Date: Wed, 9 Mar 2022 14:45:39 -0500 Subject: [PATCH] chrome: Fix shutdown crashes with multi-threaded-message-loop (fixes issue #3277) --- .../chrome/chrome_main_runner_delegate.cc | 8 ++ .../chrome/chrome_main_runner_delegate.h | 1 + patch/patches/chrome_runtime.patch | 83 +++++++++++++++++-- patch/patches/content_main_654986.patch | 5 +- 4 files changed, 89 insertions(+), 8 deletions(-) diff --git a/libcef/common/chrome/chrome_main_runner_delegate.cc b/libcef/common/chrome/chrome_main_runner_delegate.cc index 14b5ce5ce..1b6f1fca0 100644 --- a/libcef/common/chrome/chrome_main_runner_delegate.cc +++ b/libcef/common/chrome/chrome_main_runner_delegate.cc @@ -5,11 +5,13 @@ #include "libcef/common/chrome/chrome_main_runner_delegate.h" +#include "libcef/common/app_manager.h" #include "libcef/common/chrome/chrome_main_delegate_cef.h" #include "base/command_line.h" #include "base/run_loop.h" #include "chrome/browser/browser_process_impl.h" +#include "chrome/browser/chrome_content_browser_client.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" @@ -68,6 +70,12 @@ bool ChromeMainRunnerDelegate::HandleMainMessageLoopQuit() { return true; } +void ChromeMainRunnerDelegate::AfterUIThreadShutdown() { + static_cast( + CefAppManager::Get()->GetContentClient()->browser()) + ->CleanupOnUIThread(); +} + void ChromeMainRunnerDelegate::AfterMainThreadShutdown() { sampling_profiler_.reset(); } diff --git a/libcef/common/chrome/chrome_main_runner_delegate.h b/libcef/common/chrome/chrome_main_runner_delegate.h index 06b09c9d9..a153d7814 100644 --- a/libcef/common/chrome/chrome_main_runner_delegate.h +++ b/libcef/common/chrome/chrome_main_runner_delegate.h @@ -35,6 +35,7 @@ class ChromeMainRunnerDelegate : public CefMainRunnerDelegate { void BeforeMainThreadInitialize(const CefMainArgs& args) override; void BeforeMainMessageLoopRun(base::RunLoop* run_loop) override; bool HandleMainMessageLoopQuit() override; + void AfterUIThreadShutdown() override; void AfterMainThreadShutdown() override; void BeforeExecuteProcess(const CefMainArgs& args) override; void AfterExecuteProcess() override; diff --git a/patch/patches/chrome_runtime.patch b/patch/patches/chrome_runtime.patch index e4f41a483..47df67072 100644 --- a/patch/patches/chrome_runtime.patch +++ b/patch/patches/chrome_runtime.patch @@ -170,7 +170,7 @@ index 831d7173873d1..594aee58331a7 100644 +#endif } diff --git chrome/browser/chrome_content_browser_client.cc chrome/browser/chrome_content_browser_client.cc -index 2d3d64f06e330..a936eba6c8349 100644 +index d590b2d42f416..0e8067890c203 100644 --- chrome/browser/chrome_content_browser_client.cc +++ chrome/browser/chrome_content_browser_client.cc @@ -28,6 +28,7 @@ @@ -181,7 +181,28 @@ index 2d3d64f06e330..a936eba6c8349 100644 #include "chrome/browser/accessibility/accessibility_labels_service.h" #include "chrome/browser/accessibility/accessibility_labels_service_factory.h" #include "chrome/browser/after_startup_task_utils.h" -@@ -3718,9 +3719,11 @@ void ChromeContentBrowserClient::BrowserURLHandlerCreated( +@@ -1252,6 +1253,8 @@ bool IsTopChromeWebUIURL(const GURL& url) { + } // namespace + + ChromeContentBrowserClient::ChromeContentBrowserClient() { ++ keepalive_timer_.reset(new base::OneShotTimer()); ++ + #if BUILDFLAG(ENABLE_PLUGINS) + extra_parts_.push_back(new ChromeContentBrowserClientPluginsPart); + #endif +@@ -1277,6 +1280,11 @@ ChromeContentBrowserClient::~ChromeContentBrowserClient() { + extra_parts_.clear(); + } + ++void ChromeContentBrowserClient::CleanupOnUIThread() { ++ DCHECK_CURRENTLY_ON(content::BrowserThread::UI); ++ keepalive_timer_.reset(); ++} ++ + // static + void ChromeContentBrowserClient::RegisterLocalStatePrefs( + PrefRegistrySimple* registry) { +@@ -3705,9 +3713,11 @@ void ChromeContentBrowserClient::BrowserURLHandlerCreated( &search::HandleNewTabURLReverseRewrite); #endif // BUILDFLAG(IS_ANDROID) @@ -193,7 +214,7 @@ index 2d3d64f06e330..a936eba6c8349 100644 } base::FilePath ChromeContentBrowserClient::GetDefaultDownloadDirectory() { -@@ -5353,7 +5356,7 @@ void ChromeContentBrowserClient::OnNetworkServiceCreated( +@@ -5340,7 +5350,7 @@ void ChromeContentBrowserClient::OnNetworkServiceCreated( network_service); } @@ -202,7 +223,7 @@ index 2d3d64f06e330..a936eba6c8349 100644 content::BrowserContext* context, bool in_memory, const base::FilePath& relative_partition_path, -@@ -5371,6 +5374,8 @@ void ChromeContentBrowserClient::ConfigureNetworkContextParams( +@@ -5358,6 +5368,8 @@ void ChromeContentBrowserClient::ConfigureNetworkContextParams( network_context_params->user_agent = GetUserAgentBasedOnPolicy(context); network_context_params->accept_language = GetApplicationLocale(); } @@ -211,11 +232,52 @@ index 2d3d64f06e330..a936eba6c8349 100644 } std::vector +@@ -6202,10 +6214,10 @@ void ChromeContentBrowserClient::OnKeepaliveRequestStarted( + const auto now = base::TimeTicks::Now(); + const auto timeout = GetKeepaliveTimerTimeout(context); + keepalive_deadline_ = std::max(keepalive_deadline_, now + timeout); +- if (keepalive_deadline_ > now && !keepalive_timer_.IsRunning()) { ++ if (keepalive_deadline_ > now && !keepalive_timer_->IsRunning()) { + DVLOG(1) << "Starting a keepalive timer(" << timeout.InSecondsF() + << " seconds)"; +- keepalive_timer_.Start( ++ keepalive_timer_->Start( + FROM_HERE, keepalive_deadline_ - now, + base::BindOnce( + &ChromeContentBrowserClient::OnKeepaliveTimerFired, +@@ -6224,7 +6236,8 @@ void ChromeContentBrowserClient::OnKeepaliveRequestFinished() { + --num_keepalive_requests_; + if (num_keepalive_requests_ == 0) { + DVLOG(1) << "Stopping the keepalive timer"; +- keepalive_timer_.Stop(); ++ if (keepalive_timer_) ++ keepalive_timer_->Stop(); + // This deletes the keep alive handle attached to the timer function and + // unblock the shutdown sequence. + } +@@ -6333,7 +6346,7 @@ void ChromeContentBrowserClient::OnKeepaliveTimerFired( + const auto now = base::TimeTicks::Now(); + const auto then = keepalive_deadline_; + if (now < then) { +- keepalive_timer_.Start( ++ keepalive_timer_->Start( + FROM_HERE, then - now, + base::BindOnce(&ChromeContentBrowserClient::OnKeepaliveTimerFired, + weak_factory_.GetWeakPtr(), diff --git chrome/browser/chrome_content_browser_client.h chrome/browser/chrome_content_browser_client.h -index 79b637d77174d..15d5483eeea7a 100644 +index f2a7fdf291652..d086be4c88f56 100644 --- chrome/browser/chrome_content_browser_client.h +++ chrome/browser/chrome_content_browser_client.h -@@ -561,7 +561,7 @@ class ChromeContentBrowserClient : public content::ContentBrowserClient { +@@ -121,6 +121,8 @@ class ChromeContentBrowserClient : public content::ContentBrowserClient { + + ~ChromeContentBrowserClient() override; + ++ virtual void CleanupOnUIThread(); ++ + // TODO(https://crbug.com/787567): This file is about calls from content/ out + // to chrome/ to get values or notify about events, but both of these + // functions are from chrome/ to chrome/ and don't involve content/ at all. +@@ -557,7 +559,7 @@ class ChromeContentBrowserClient : public content::ContentBrowserClient { override; void OnNetworkServiceCreated( network::mojom::NetworkService* network_service) override; @@ -224,6 +286,15 @@ index 79b637d77174d..15d5483eeea7a 100644 content::BrowserContext* context, bool in_memory, const base::FilePath& relative_partition_path, +@@ -909,7 +911,7 @@ class ChromeContentBrowserClient : public content::ContentBrowserClient { + + #if !BUILDFLAG(IS_ANDROID) + uint64_t num_keepalive_requests_ = 0; +- base::OneShotTimer keepalive_timer_; ++ std::unique_ptr keepalive_timer_; + base::TimeTicks keepalive_deadline_; + #endif + diff --git chrome/browser/prefs/browser_prefs.cc chrome/browser/prefs/browser_prefs.cc index 94cf3615137ad..369983be86323 100644 --- chrome/browser/prefs/browser_prefs.cc diff --git a/patch/patches/content_main_654986.patch b/patch/patches/content_main_654986.patch index 9d7013260..b53f882cb 100644 --- a/patch/patches/content_main_654986.patch +++ b/patch/patches/content_main_654986.patch @@ -100,7 +100,7 @@ index 332b7d026e036..0d63ad05fde4e 100644 } diff --git content/app/content_main_runner_impl.cc content/app/content_main_runner_impl.cc -index 9d22238878e32..e0833ed4572e9 100644 +index 9d22238878e32..dd231b0d55422 100644 --- content/app/content_main_runner_impl.cc +++ content/app/content_main_runner_impl.cc @@ -44,6 +44,7 @@ @@ -111,12 +111,13 @@ index 9d22238878e32..e0833ed4572e9 100644 #include "base/time/time.h" #include "base/trace_event/trace_event.h" #include "build/build_config.h" -@@ -1205,6 +1206,11 @@ void ContentMainRunnerImpl::Shutdown() { +@@ -1205,6 +1206,12 @@ void ContentMainRunnerImpl::Shutdown() { is_shutdown_ = true; } +void ContentMainRunnerImpl::ShutdownOnUIThread() { + base::ScopedAllowBaseSyncPrimitivesForTesting allow_wait; ++ unregister_thread_closure_.RunAndReset(); + discardable_shared_memory_manager_.reset(); +} +