From f4d5395c4be69897d5f35a328e21ceb96dafa987 Mon Sep 17 00:00:00 2001 From: Marshall Greenblatt Date: Mon, 29 Nov 2021 14:58:09 -0500 Subject: [PATCH] Skip GetNewBrowserInfo call for PDF renderer processes (see issue #3047) With PdfUnseasoned the PDF file will be loaded in a dedicated renderer process. We identify this process by adding the kPdfRenderer command-line flag (similar to how kExtensionProcess is used to identify an extension renderer process). We then avoid calling GetNewBrowserInfo for the PDF renderer process because we know the request will otherwise time out. --- .../alloy/alloy_content_browser_client.cc | 8 ++++ libcef/browser/browser_info_manager.cc | 8 ++++ .../chrome_content_browser_client_cef.cc | 10 +++++ .../alloy/alloy_content_renderer_client.cc | 12 +----- .../extensions/extensions_renderer_client.cc | 9 ----- .../extensions/extensions_renderer_client.h | 2 - libcef/renderer/render_manager.cc | 39 +++++++++++++------ libcef/renderer/render_manager.h | 6 +++ tests/ceftests/pdf_viewer_unittest.cc | 15 ++----- 9 files changed, 63 insertions(+), 46 deletions(-) diff --git a/libcef/browser/alloy/alloy_content_browser_client.cc b/libcef/browser/alloy/alloy_content_browser_client.cc index fad83b986..64a53173f 100644 --- a/libcef/browser/alloy/alloy_content_browser_client.cc +++ b/libcef/browser/alloy/alloy_content_browser_client.cc @@ -763,6 +763,14 @@ void AlloyContentBrowserClient::AppendExtraCommandLineSwitches( if (extensions::ExtensionsEnabled()) { content::RenderProcessHost* process = content::RenderProcessHost::FromID(child_process_id); +#if !defined(OS_WIN) + // kPdfRenderer will be set for Windows in + // RenderProcessHostImpl::AppendRendererCommandLine. + if (process && process->IsPdf()) { + command_line->AppendSwitch(switches::kPdfRenderer); + } +#endif // !defined(OS_WIN) + auto browser_context = process->GetBrowserContext(); CefBrowserContext* cef_browser_context = process ? CefBrowserContext::FromBrowserContext(browser_context) diff --git a/libcef/browser/browser_info_manager.cc b/libcef/browser/browser_info_manager.cc index 7c45eaa7f..702f7863a 100644 --- a/libcef/browser/browser_info_manager.cc +++ b/libcef/browser/browser_info_manager.cc @@ -531,6 +531,7 @@ void CefBrowserInfoManager::CancelNewBrowserInfoResponse( void CefBrowserInfoManager::TimeoutNewBrowserInfoResponse( const content::GlobalRenderFrameHostId& global_id, int timeout_id) { + CEF_REQUIRE_UIT(); if (!g_info_manager) return; @@ -544,6 +545,13 @@ void CefBrowserInfoManager::TimeoutNewBrowserInfoResponse( if (pending_info->timeout_id != timeout_id) return; +#if DCHECK_IS_ON() + // This method should never be called for a PDF renderer. + content::RenderProcessHost* process = + content::RenderProcessHost::FromID(global_id.child_id); + DCHECK(!process || !process->IsPdf()); +#endif + LOG(ERROR) << "Timeout of new browser info response for frame " << frame_util::GetFrameDebugString(global_id); diff --git a/libcef/browser/chrome/chrome_content_browser_client_cef.cc b/libcef/browser/chrome/chrome_content_browser_client_cef.cc index 6d93f3d27..be40baa6e 100644 --- a/libcef/browser/chrome/chrome_content_browser_client_cef.cc +++ b/libcef/browser/chrome/chrome_content_browser_client_cef.cc @@ -116,6 +116,16 @@ void ChromeContentBrowserClientCef::AppendExtraCommandLineSwitches( }; command_line->CopySwitchesFrom(*browser_cmd, kSwitchNames, base::size(kSwitchNames)); + +#if !defined(OS_WIN) + // kPdfRenderer will be set for Windows in + // RenderProcessHostImpl::AppendRendererCommandLine. + content::RenderProcessHost* process = + content::RenderProcessHost::FromID(child_process_id); + if (process && process->IsPdf()) { + command_line->AppendSwitch(switches::kPdfRenderer); + } +#endif // !defined(OS_WIN) } CefRefPtr app = CefAppManager::Get()->GetApplication(); diff --git a/libcef/renderer/alloy/alloy_content_renderer_client.cc b/libcef/renderer/alloy/alloy_content_renderer_client.cc index 6dce1b4a6..1e9ae6dbb 100644 --- a/libcef/renderer/alloy/alloy_content_renderer_client.cc +++ b/libcef/renderer/alloy/alloy_content_renderer_client.cc @@ -110,16 +110,6 @@ #include "base/strings/sys_string_conversions.h" #endif -namespace { - -bool IsStandaloneExtensionProcess() { - return extensions::ExtensionsEnabled() && - extensions::CefExtensionsRendererClient:: - IsStandaloneExtensionProcess(); -} - -} // namespace - AlloyContentRendererClient::AlloyContentRendererClient() : main_entry_time_(base::TimeTicks::Now()), render_manager_(new CefRenderManager) { @@ -200,7 +190,7 @@ void AlloyContentRendererClient::RenderThreadStarted() { content::RenderThread* thread = content::RenderThread::Get(); - const bool is_extension = IsStandaloneExtensionProcess(); + const bool is_extension = CefRenderManager::IsExtensionProcess(); thread->SetRendererProcessType( is_extension diff --git a/libcef/renderer/extensions/extensions_renderer_client.cc b/libcef/renderer/extensions/extensions_renderer_client.cc index 0aa083cef..03c34822a 100644 --- a/libcef/renderer/extensions/extensions_renderer_client.cc +++ b/libcef/renderer/extensions/extensions_renderer_client.cc @@ -7,19 +7,16 @@ #include "libcef/renderer/alloy/alloy_render_thread_observer.h" #include "libcef/renderer/extensions/extensions_dispatcher_delegate.h" -#include "base/command_line.h" #include "base/stl_util.h" #include "chrome/common/url_constants.h" #include "chrome/renderer/extensions/extension_process_policy.h" #include "chrome/renderer/extensions/resource_request_policy.h" #include "components/guest_view/renderer/guest_view_container_dispatcher.h" #include "content/public/common/content_constants.h" -#include "content/public/common/content_switches.h" #include "content/public/renderer/render_frame.h" #include "content/public/renderer/render_thread.h" #include "extensions/common/constants.h" #include "extensions/common/permissions/permissions_data.h" -#include "extensions/common/switches.h" #include "extensions/renderer/dispatcher.h" #include "extensions/renderer/extension_frame_helper.h" #include "extensions/renderer/extensions_render_frame_observer.h" @@ -149,10 +146,4 @@ void CefExtensionsRendererClient::RunScriptsAtDocumentIdle( extension_dispatcher_->RunScriptsAtDocumentIdle(render_frame); } -// static -bool CefExtensionsRendererClient::IsStandaloneExtensionProcess() { - return base::CommandLine::ForCurrentProcess()->HasSwitch( - extensions::switches::kExtensionProcess); -} - } // namespace extensions diff --git a/libcef/renderer/extensions/extensions_renderer_client.h b/libcef/renderer/extensions/extensions_renderer_client.h index 731309a80..3ac4a39d9 100644 --- a/libcef/renderer/extensions/extensions_renderer_client.h +++ b/libcef/renderer/extensions/extensions_renderer_client.h @@ -78,8 +78,6 @@ class CefExtensionsRendererClient : public ExtensionsRendererClient { void RunScriptsAtDocumentEnd(content::RenderFrame* render_frame); void RunScriptsAtDocumentIdle(content::RenderFrame* render_frame); - static bool IsStandaloneExtensionProcess(); - private: std::unique_ptr extension_dispatcher_; std::unique_ptr diff --git a/libcef/renderer/render_manager.cc b/libcef/renderer/render_manager.cc index a06f8b72b..c98f8fc35 100644 --- a/libcef/renderer/render_manager.cc +++ b/libcef/renderer/render_manager.cc @@ -30,9 +30,11 @@ #include "base/command_line.h" #include "base/strings/string_number_conversions.h" #include "cef/libcef/common/mojom/cef.mojom.h" +#include "content/public/common/content_switches.h" #include "content/public/renderer/render_frame.h" #include "content/public/renderer/render_thread.h" #include "content/public/renderer/render_view.h" +#include "extensions/common/switches.h" #include "mojo/public/cpp/bindings/binder_map.h" #include "services/network/public/mojom/cors_origin_pattern.mojom.h" #include "third_party/blink/public/platform/web_string.h" @@ -181,6 +183,18 @@ CefRenderManager::GetBrowserManager() { return browser_manager_; } +// static +bool CefRenderManager::IsExtensionProcess() { + return base::CommandLine::ForCurrentProcess()->HasSwitch( + extensions::switches::kExtensionProcess); +} + +// static +bool CefRenderManager::IsPdfProcess() { + return base::CommandLine::ForCurrentProcess()->HasSwitch( + switches::kPdfRenderer); +} + void CefRenderManager::BindReceiver( mojo::PendingReceiver receiver) { receivers_.Add(this, std::move(receiver)); @@ -293,25 +307,26 @@ CefRefPtr CefRenderManager::MaybeCreateBrowser( return nullptr; } - const int render_frame_routing_id = render_frame->GetRoutingID(); + const bool is_pdf = IsPdfProcess(); - // Retrieve the browser information synchronously. This will also register - // the routing ids with the browser info object in the browser process. auto params = cef::mojom::NewBrowserInfo::New(); - GetBrowserManager()->GetNewBrowserInfo(render_frame_routing_id, ¶ms); + if (!is_pdf) { + // Retrieve browser information synchronously. + GetBrowserManager()->GetNewBrowserInfo(render_frame->GetRoutingID(), + ¶ms); + if (params->browser_id == 0) { + // The popup may have been canceled during creation. + return nullptr; + } + } if (is_windowless) { *is_windowless = params->is_windowless; } - if (params->browser_id == 0) { - // The popup may have been canceled during creation. - return nullptr; - } - - if (params->is_guest_view || params->browser_id < 0) { - // Don't create a CefBrowser for guest views, or if the new browser info - // response has timed out. + if (is_pdf || params->is_guest_view || params->browser_id < 0) { + // Don't create a CefBrowser for a PDF renderer, guest view, or if the new + // browser info response has timed out. guest_views_.insert(std::make_pair( render_view, std::make_unique(this, render_view, params->is_windowless))); diff --git a/libcef/renderer/render_manager.h b/libcef/renderer/render_manager.h index 5d42c0ed7..789efac59 100644 --- a/libcef/renderer/render_manager.h +++ b/libcef/renderer/render_manager.h @@ -67,6 +67,12 @@ class CefRenderManager : public cef::mojom::RenderManager { // Connects to CefBrowserManager in the browser process. mojo::Remote& GetBrowserManager(); + // Returns true if this renderer process is hosting an extension. + static bool IsExtensionProcess(); + + // Returns true if this renderer process is hosting a PDF. + static bool IsPdfProcess(); + private: friend class CefBrowserImpl; friend class CefGuestView; diff --git a/tests/ceftests/pdf_viewer_unittest.cc b/tests/ceftests/pdf_viewer_unittest.cc index 0a5e7796c..afecfce35 100644 --- a/tests/ceftests/pdf_viewer_unittest.cc +++ b/tests/ceftests/pdf_viewer_unittest.cc @@ -41,15 +41,6 @@ const int64 kPdfLoadDelayMs = 7000; const int64 kPdfLoadDelayMs = 5000; #endif -int64 PdfLoadDelayMs() { - // TODO(chrome): The PDF renderer process created when using - // `--enable-features=PdfUnseasoned` is currently causing a - // "Timeout of new browser info response for frame" error which necessitates - // a longer delay. Reduce this delay once we can properly account for that - // new renderer process (see issue #3047). - return IsChromeRuntimeEnabled() ? kPdfLoadDelayMs * 2 : kPdfLoadDelayMs; -} - // Browser-side test handler. class PdfViewerTestHandler : public TestHandler, public CefContextMenuHandler { public: @@ -91,7 +82,7 @@ class PdfViewerTestHandler : public TestHandler, public CefContextMenuHandler { CreateBrowser(url_, request_context); // Time out the test after a reasonable period of time. - SetTestTimeout(5000 + PdfLoadDelayMs()); + SetTestTimeout(5000 + kPdfLoadDelayMs); } CefRefPtr GetResourceHandler( @@ -142,14 +133,14 @@ class PdfViewerTestHandler : public TestHandler, public CefContextMenuHandler { // After context menu display. Destroy the test. CefPostDelayedTask( TID_UI, base::BindOnce(&PdfViewerTestHandler::DestroyTest, this), - PdfLoadDelayMs()); + kPdfLoadDelayMs); } else { // Trigger the context menu. CefPostDelayedTask( TID_UI, base::BindOnce(&PdfViewerTestHandler::TriggerContextMenu, this, frame->GetBrowser()), - PdfLoadDelayMs()); + kPdfLoadDelayMs); } } }