From 3d9b9e54834c5102d39fa138a48c2d5ddd66e504 Mon Sep 17 00:00:00 2001 From: samuelmaddock Date: Mon, 25 Apr 2022 15:59:57 -0400 Subject: [PATCH 1/4] fix: crash when creating interface for speculative frame --- shell/browser/api/electron_api_web_frame_main.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/shell/browser/api/electron_api_web_frame_main.cc b/shell/browser/api/electron_api_web_frame_main.cc index e7ba32a33f782..36dc7439b6a1f 100644 --- a/shell/browser/api/electron_api_web_frame_main.cc +++ b/shell/browser/api/electron_api_web_frame_main.cc @@ -316,7 +316,7 @@ std::vector WebFrameMain::FramesInSubtree() const { } void WebFrameMain::Connect() { - if (pending_receiver_) { + if (pending_receiver_ && render_frame_->IsRenderFrameCreated()) { render_frame_->GetRemoteInterfaces()->GetInterface( std::move(pending_receiver_)); } From 8de49350aa2376fee3aa8621d9c9c6b2ea9a8034 Mon Sep 17 00:00:00 2001 From: samuelmaddock Date: Thu, 12 May 2022 21:08:44 -0400 Subject: [PATCH 2/4] fix: (attempt 2) always try to connect when using renderer api --- .../browser/api/electron_api_web_contents.cc | 2 +- .../api/electron_api_web_frame_main.cc | 23 +++++++++---------- .../browser/api/electron_api_web_frame_main.h | 4 ++-- 3 files changed, 14 insertions(+), 15 deletions(-) diff --git a/shell/browser/api/electron_api_web_contents.cc b/shell/browser/api/electron_api_web_contents.cc index caf9cdb41b3ea..5db6744bd7ce1 100644 --- a/shell/browser/api/electron_api_web_contents.cc +++ b/shell/browser/api/electron_api_web_contents.cc @@ -1510,7 +1510,7 @@ void WebContents::HandleNewRenderFrame( auto* web_frame = WebFrameMain::FromRenderFrameHost(render_frame_host); if (web_frame) - web_frame->Connect(); + web_frame->MaybeSetupMojoConnection(); } void WebContents::OnBackgroundColorChanged() { diff --git a/shell/browser/api/electron_api_web_frame_main.cc b/shell/browser/api/electron_api_web_frame_main.cc index 36dc7439b6a1f..de3af5d4df4cd 100644 --- a/shell/browser/api/electron_api_web_frame_main.cc +++ b/shell/browser/api/electron_api_web_frame_main.cc @@ -103,6 +103,7 @@ void WebFrameMain::UpdateRenderFrameHost(content::RenderFrameHost* rfh) { render_frame_disposed_ = false; render_frame_ = rfh; renderer_api_.reset(); + MaybeSetupMojoConnection(); } bool WebFrameMain::CheckRenderFrame() const { @@ -182,16 +183,21 @@ void WebFrameMain::Send(v8::Isolate* isolate, } const mojo::Remote& WebFrameMain::GetRendererApi() { + MaybeSetupMojoConnection(); + return renderer_api_; +} + +void WebFrameMain::MaybeSetupMojoConnection() { if (!renderer_api_) { pending_receiver_ = renderer_api_.BindNewPipeAndPassReceiver(); - if (render_frame_->IsRenderFrameCreated()) { - render_frame_->GetRemoteInterfaces()->GetInterface( - std::move(pending_receiver_)); - } renderer_api_.set_disconnect_handler(base::BindOnce( &WebFrameMain::OnRendererConnectionError, weak_factory_.GetWeakPtr())); } - return renderer_api_; + // Wait for RenderFrame to be created in renderer before accessing remote. + if (pending_receiver_ && render_frame_->IsRenderFrameCreated()) { + render_frame_->GetRemoteInterfaces()->GetInterface( + std::move(pending_receiver_)); + } } void WebFrameMain::OnRendererConnectionError() { @@ -315,13 +321,6 @@ std::vector WebFrameMain::FramesInSubtree() const { return frame_hosts; } -void WebFrameMain::Connect() { - if (pending_receiver_ && render_frame_->IsRenderFrameCreated()) { - render_frame_->GetRemoteInterfaces()->GetInterface( - std::move(pending_receiver_)); - } -} - void WebFrameMain::DOMContentLoaded() { Emit("dom-ready"); } diff --git a/shell/browser/api/electron_api_web_frame_main.h b/shell/browser/api/electron_api_web_frame_main.h index 23a3b657fd45c..8017cc0284eeb 100644 --- a/shell/browser/api/electron_api_web_frame_main.h +++ b/shell/browser/api/electron_api_web_frame_main.h @@ -82,6 +82,8 @@ class WebFrameMain : public gin::Wrappable, void UpdateRenderFrameHost(content::RenderFrameHost* rfh); const mojo::Remote& GetRendererApi(); + void MaybeSetupMojoConnection(); + void OnRendererConnectionError(); // WebFrameMain can outlive its RenderFrameHost pointer so we need to check // whether its been disposed of prior to accessing it. @@ -112,8 +114,6 @@ class WebFrameMain : public gin::Wrappable, std::vector Frames() const; std::vector FramesInSubtree() const; - void OnRendererConnectionError(); - void Connect(); void DOMContentLoaded(); mojo::Remote renderer_api_; From 98c3d1c84d9f65037357393a5c3597a2653eefc1 Mon Sep 17 00:00:00 2001 From: samuelmaddock Date: Tue, 17 May 2022 12:26:27 -0400 Subject: [PATCH 3/4] fix: potential crash when rfh is disposed --- shell/browser/api/electron_api_web_frame_main.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/shell/browser/api/electron_api_web_frame_main.cc b/shell/browser/api/electron_api_web_frame_main.cc index de3af5d4df4cd..4d924f3774bac 100644 --- a/shell/browser/api/electron_api_web_frame_main.cc +++ b/shell/browser/api/electron_api_web_frame_main.cc @@ -103,6 +103,7 @@ void WebFrameMain::UpdateRenderFrameHost(content::RenderFrameHost* rfh) { render_frame_disposed_ = false; render_frame_ = rfh; renderer_api_.reset(); + pending_receiver_.reset(); MaybeSetupMojoConnection(); } @@ -194,7 +195,7 @@ void WebFrameMain::MaybeSetupMojoConnection() { &WebFrameMain::OnRendererConnectionError, weak_factory_.GetWeakPtr())); } // Wait for RenderFrame to be created in renderer before accessing remote. - if (pending_receiver_ && render_frame_->IsRenderFrameCreated()) { + if (pending_receiver_ && !render_frame_disposed_ && render_frame_->IsRenderFrameCreated()) { render_frame_->GetRemoteInterfaces()->GetInterface( std::move(pending_receiver_)); } From 57aa7835bec10e456cb38d2072e6ec7513e45ca8 Mon Sep 17 00:00:00 2001 From: samuelmaddock Date: Tue, 17 May 2022 12:34:59 -0400 Subject: [PATCH 4/4] refactor: always teardown mojo connection This should eliminate an entire class of potential errors from appearing in the future. --- shell/browser/api/electron_api_web_frame_main.cc | 13 +++++++++---- shell/browser/api/electron_api_web_frame_main.h | 1 + 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/shell/browser/api/electron_api_web_frame_main.cc b/shell/browser/api/electron_api_web_frame_main.cc index 4d924f3774bac..70ae9210aaa85 100644 --- a/shell/browser/api/electron_api_web_frame_main.cc +++ b/shell/browser/api/electron_api_web_frame_main.cc @@ -96,14 +96,14 @@ void WebFrameMain::Destroyed() { void WebFrameMain::MarkRenderFrameDisposed() { render_frame_ = nullptr; render_frame_disposed_ = true; + TeardownMojoConnection(); } void WebFrameMain::UpdateRenderFrameHost(content::RenderFrameHost* rfh) { // Should only be called when swapping frames. render_frame_disposed_ = false; render_frame_ = rfh; - renderer_api_.reset(); - pending_receiver_.reset(); + TeardownMojoConnection(); MaybeSetupMojoConnection(); } @@ -195,14 +195,19 @@ void WebFrameMain::MaybeSetupMojoConnection() { &WebFrameMain::OnRendererConnectionError, weak_factory_.GetWeakPtr())); } // Wait for RenderFrame to be created in renderer before accessing remote. - if (pending_receiver_ && !render_frame_disposed_ && render_frame_->IsRenderFrameCreated()) { + if (pending_receiver_ && render_frame_->IsRenderFrameCreated()) { render_frame_->GetRemoteInterfaces()->GetInterface( std::move(pending_receiver_)); } } -void WebFrameMain::OnRendererConnectionError() { +void WebFrameMain::TeardownMojoConnection() { renderer_api_.reset(); + pending_receiver_.reset(); +} + +void WebFrameMain::OnRendererConnectionError() { + TeardownMojoConnection(); } void WebFrameMain::PostMessage(v8::Isolate* isolate, diff --git a/shell/browser/api/electron_api_web_frame_main.h b/shell/browser/api/electron_api_web_frame_main.h index 8017cc0284eeb..b45264db59627 100644 --- a/shell/browser/api/electron_api_web_frame_main.h +++ b/shell/browser/api/electron_api_web_frame_main.h @@ -83,6 +83,7 @@ class WebFrameMain : public gin::Wrappable, const mojo::Remote& GetRendererApi(); void MaybeSetupMojoConnection(); + void TeardownMojoConnection(); void OnRendererConnectionError(); // WebFrameMain can outlive its RenderFrameHost pointer so we need to check