From 574d4dcedce9ff4f4bce8a96df336d11c34d9633 Mon Sep 17 00:00:00 2001 From: Andrei Kurushin Date: Mon, 20 Jul 2020 14:24:16 -0400 Subject: [PATCH] Fix crash when closing an OSR browser (fixes issue #2919) Release the Compositor before the WebContents is destroyed to match the behavior in non-OSR code. --- .../osr/browser_platform_delegate_osr.cc | 14 ++++ .../osr/browser_platform_delegate_osr.h | 1 + .../osr/render_widget_host_view_osr.cc | 75 ++++++++++++------- .../browser/osr/render_widget_host_view_osr.h | 2 + 4 files changed, 66 insertions(+), 26 deletions(-) diff --git a/libcef/browser/osr/browser_platform_delegate_osr.cc b/libcef/browser/osr/browser_platform_delegate_osr.cc index 7e2730b41..87e4d015f 100644 --- a/libcef/browser/osr/browser_platform_delegate_osr.cc +++ b/libcef/browser/osr/browser_platform_delegate_osr.cc @@ -71,6 +71,20 @@ void CefBrowserPlatformDelegateOsr::BrowserCreated( } } +void CefBrowserPlatformDelegateOsr::NotifyBrowserDestroyed() { + content::WebContents* web_contents = browser_->web_contents(); + content::RenderViewHost* host = web_contents->GetRenderViewHost(); + if (host) { + CefRenderWidgetHostViewOSR* view = + static_cast(host->GetWidget()->GetView()); + if (view) { + view->ReleaseCompositor(); + } + } + + CefBrowserPlatformDelegateAlloy::NotifyBrowserDestroyed(); +} + void CefBrowserPlatformDelegateOsr::BrowserDestroyed( CefBrowserHostImpl* browser) { CefBrowserPlatformDelegateAlloy::BrowserDestroyed(browser); diff --git a/libcef/browser/osr/browser_platform_delegate_osr.h b/libcef/browser/osr/browser_platform_delegate_osr.h index 6537915b4..98eaa1c39 100644 --- a/libcef/browser/osr/browser_platform_delegate_osr.h +++ b/libcef/browser/osr/browser_platform_delegate_osr.h @@ -27,6 +27,7 @@ class CefBrowserPlatformDelegateOsr void WebContentsCreated(content::WebContents* web_contents, bool owned) override; void BrowserCreated(CefBrowserHostImpl* browser) override; + void NotifyBrowserDestroyed() override; void BrowserDestroyed(CefBrowserHostImpl* browser) override; SkColor GetBackgroundColor() const override; void WasResized() override; diff --git a/libcef/browser/osr/render_widget_host_view_osr.cc b/libcef/browser/osr/render_widget_host_view_osr.cc index 16d1edc06..596da991b 100644 --- a/libcef/browser/osr/render_widget_host_view_osr.cc +++ b/libcef/browser/osr/render_widget_host_view_osr.cc @@ -25,6 +25,7 @@ #include "components/viz/common/frame_sinks/begin_frame_args.h" #include "components/viz/common/frame_sinks/copy_output_request.h" #include "components/viz/common/frame_sinks/delay_based_time_source.h" +#include "components/viz/common/surfaces/frame_sink_id_allocator.h" #include "components/viz/common/switches.h" #include "content/browser/bad_message.h" #include "content/browser/gpu/gpu_data_manager_impl.h" @@ -291,6 +292,23 @@ CefRenderWidgetHostViewOSR::CefRenderWidgetHostViewOSR( } CefRenderWidgetHostViewOSR::~CefRenderWidgetHostViewOSR() { + ReleaseCompositor(); + root_layer_.reset(nullptr); + + DCHECK(!parent_host_view_); + DCHECK(!popup_host_view_); + DCHECK(!child_host_view_); + DCHECK(guest_host_views_.empty()); + + if (text_input_manager_) + text_input_manager_->RemoveObserver(this); +} + +void CefRenderWidgetHostViewOSR::ReleaseCompositor() { + if (!compositor_) { + return; // already released + } + // Marking the DelegatedFrameHost as removed from the window hierarchy is // necessary to remove all connections to its old ui::Compositor. if (is_showing_) { @@ -301,15 +319,6 @@ CefRenderWidgetHostViewOSR::~CefRenderWidgetHostViewOSR() { delegated_frame_host_.reset(nullptr); compositor_.reset(nullptr); - root_layer_.reset(nullptr); - - DCHECK(!parent_host_view_); - DCHECK(!popup_host_view_); - DCHECK(!child_host_view_); - DCHECK(guest_host_views_.empty()); - - if (text_input_manager_) - text_input_manager_->RemoveObserver(this); } // Called for full-screen widgets. @@ -351,7 +360,9 @@ bool CefRenderWidgetHostViewOSR::HasFocus() { } bool CefRenderWidgetHostViewOSR::IsSurfaceAvailableForCopy() { - return delegated_frame_host_->CanCopyFromCompositingSurface(); + return delegated_frame_host_ + ? delegated_frame_host_->CanCopyFromCompositingSurface() + : false; } void CefRenderWidgetHostViewOSR::Show() { @@ -380,10 +391,12 @@ void CefRenderWidgetHostViewOSR::Show() { base::nullopt /* record_tab_switch_time_request */); } - delegated_frame_host_->AttachToCompositor(compositor_.get()); - delegated_frame_host_->WasShown( - GetLocalSurfaceIdAllocation().local_surface_id(), GetViewBounds().size(), - base::nullopt); + if (delegated_frame_host_) { + delegated_frame_host_->AttachToCompositor(compositor_.get()); + delegated_frame_host_->WasShown( + GetLocalSurfaceIdAllocation().local_surface_id(), + GetViewBounds().size(), base::nullopt); + } if (!content::GpuDataManagerImpl::GetInstance()->IsGpuCompositingDisabled()) { // Start generating frames when we're visible and at the correct size. @@ -418,9 +431,11 @@ void CefRenderWidgetHostViewOSR::Hide() { if (render_widget_host_) render_widget_host_->WasHidden(); - delegated_frame_host_->WasHidden( - content::DelegatedFrameHost::HiddenCause::kOther); - delegated_frame_host_->DetachFromCompositor(); + if (delegated_frame_host_) { + delegated_frame_host_->WasHidden( + content::DelegatedFrameHost::HiddenCause::kOther); + delegated_frame_host_->DetachFromCompositor(); + } } bool CefRenderWidgetHostViewOSR::IsShowing() { @@ -554,7 +569,9 @@ void CefRenderWidgetHostViewOSR::AddDamageRect(uint32_t sequence, } void CefRenderWidgetHostViewOSR::ResetFallbackToFirstNavigationSurface() { - delegated_frame_host_->ResetFallbackToFirstNavigationSurface(); + if (delegated_frame_host_) { + delegated_frame_host_->ResetFallbackToFirstNavigationSurface(); + } } void CefRenderWidgetHostViewOSR::InitAsPopup( @@ -718,8 +735,10 @@ void CefRenderWidgetHostViewOSR::CopyFromSurface( const gfx::Rect& src_rect, const gfx::Size& output_size, base::OnceCallback callback) { - delegated_frame_host_->CopyFromCompositingSurface(src_rect, output_size, - std::move(callback)); + if (delegated_frame_host_) { + delegated_frame_host_->CopyFromCompositingSurface(src_rect, output_size, + std::move(callback)); + } } void CefRenderWidgetHostViewOSR::GetScreenInfo(content::ScreenInfo* results) { @@ -894,11 +913,13 @@ CefRenderWidgetHostViewOSR::GetLocalSurfaceIdAllocation() const { } const viz::FrameSinkId& CefRenderWidgetHostViewOSR::GetFrameSinkId() const { - return delegated_frame_host_->frame_sink_id(); + return delegated_frame_host_ + ? delegated_frame_host_->frame_sink_id() + : viz::FrameSinkIdAllocator::InvalidFrameSinkId(); } viz::FrameSinkId CefRenderWidgetHostViewOSR::GetRootFrameSinkId() { - return compositor_->frame_sink_id(); + return compositor_ ? compositor_->frame_sink_id() : viz::FrameSinkId(); } void CefRenderWidgetHostViewOSR::OnRenderFrameMetadataChangedAfterActivation() { @@ -1077,10 +1098,12 @@ void CefRenderWidgetHostViewOSR::SendExternalBeginFrame() { if (render_widget_host_) render_widget_host_->ProgressFlingIfNeeded(frame_time); - compositor_->IssueExternalBeginFrame( - begin_frame_args, /* force= */ true, - base::BindOnce(&CefRenderWidgetHostViewOSR::OnFrameComplete, - weak_ptr_factory_.GetWeakPtr())); + if (compositor_) { + compositor_->IssueExternalBeginFrame( + begin_frame_args, /* force= */ true, + base::BindOnce(&CefRenderWidgetHostViewOSR::OnFrameComplete, + weak_ptr_factory_.GetWeakPtr())); + } if (!IsPopupWidget() && popup_host_view_) { popup_host_view_->SendExternalBeginFrame(); diff --git a/libcef/browser/osr/render_widget_host_view_osr.h b/libcef/browser/osr/render_widget_host_view_osr.h index 52e9b5abe..68d36c28c 100644 --- a/libcef/browser/osr/render_widget_host_view_osr.h +++ b/libcef/browser/osr/render_widget_host_view_osr.h @@ -264,6 +264,8 @@ class CefRenderWidgetHostViewOSR : public content::RenderWidgetHostViewBase, void OnDidUpdateVisualPropertiesComplete( const cc::RenderFrameMetadata& metadata); + void ReleaseCompositor(); + private: void SetFrameRate(); bool SetDeviceScaleFactor();