From 03d499bf34d664866f30b280f55b72290b538653 Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Sat, 12 Jan 2019 15:50:33 +0900 Subject: [PATCH] fix: detach webview instead of destroying it Chromium no longer cleans up everything when a guest webcontents is destroyed, we have to force detaching it and let Chromium destroy everything. --- BUILD.gn | 1 - atom/browser/api/atom_api_web_contents.cc | 5 +++-- atom/browser/api/atom_api_web_contents.h | 1 + ...ontents_osr.cc => atom_api_web_contents_impl.cc} | 13 ++++++++++++- filenames.gni | 1 + lib/browser/guest-view-manager.js | 4 ++-- .../chromium/disable_detach_webview_frame.patch | 2 ++ 7 files changed, 21 insertions(+), 6 deletions(-) rename atom/browser/api/{atom_api_web_contents_osr.cc => atom_api_web_contents_impl.cc} (79%) diff --git a/BUILD.gn b/BUILD.gn index 9776bb37f41f..8d8000dc28a9 100644 --- a/BUILD.gn +++ b/BUILD.gn @@ -421,7 +421,6 @@ static_library("electron_lib") { if (enable_osr) { sources += [ - "atom/browser/api/atom_api_web_contents_osr.cc", "atom/browser/osr/osr_output_device.cc", "atom/browser/osr/osr_output_device.h", "atom/browser/osr/osr_render_widget_host_view.cc", diff --git a/atom/browser/api/atom_api_web_contents.cc b/atom/browser/api/atom_api_web_contents.cc index c46b8b7e577c..c0fec936e961 100644 --- a/atom/browser/api/atom_api_web_contents.cc +++ b/atom/browser/api/atom_api_web_contents.cc @@ -470,9 +470,9 @@ WebContents::~WebContents() { RenderViewDeleted(web_contents()->GetRenderViewHost()); if (type_ == WEB_VIEW) { + DCHECK(!web_contents()->GetOuterWebContents()) + << "Should never manually destroy an attached webview"; // For webview simply destroy the WebContents immediately. - // TODO(zcbenz): Add an internal API for webview instead of using - // destroy(), so we don't have to add a special branch here. DestroyWebContents(false /* async */); } else if (type_ == BROWSER_WINDOW && owner_window()) { // For BrowserWindow we should close the window and clean up everything @@ -2150,6 +2150,7 @@ void WebContents::BuildPrototype(v8::Isolate* isolate, .SetMethod("startDrag", &WebContents::StartDrag) .SetMethod("isGuest", &WebContents::IsGuest) .SetMethod("attachToIframe", &WebContents::AttachToIframe) + .SetMethod("detachFromOuterFrame", &WebContents::DetachFromOuterFrame) .SetMethod("isOffscreen", &WebContents::IsOffScreen) #if BUILDFLAG(ENABLE_OSR) .SetMethod("startPainting", &WebContents::StartPainting) diff --git a/atom/browser/api/atom_api_web_contents.h b/atom/browser/api/atom_api_web_contents.h index 59067e3dd7d1..ee40928b3947 100644 --- a/atom/browser/api/atom_api_web_contents.h +++ b/atom/browser/api/atom_api_web_contents.h @@ -242,6 +242,7 @@ class WebContents : public mate::TrackableObject, bool IsGuest() const; void AttachToIframe(content::WebContents* embedder_web_contents, int embedder_frame_id); + void DetachFromOuterFrame(); // Methods for offscreen rendering bool IsOffScreen() const; diff --git a/atom/browser/api/atom_api_web_contents_osr.cc b/atom/browser/api/atom_api_web_contents_impl.cc similarity index 79% rename from atom/browser/api/atom_api_web_contents_osr.cc rename to atom/browser/api/atom_api_web_contents_impl.cc index 7c2aadacfd4d..7f76e75e3283 100644 --- a/atom/browser/api/atom_api_web_contents_osr.cc +++ b/atom/browser/api/atom_api_web_contents_impl.cc @@ -4,9 +4,12 @@ #include "atom/browser/api/atom_api_web_contents.h" +#include "content/browser/web_contents/web_contents_impl.h" + +#if BUILDFLAG(ENABLE_OSR) #include "atom/browser/osr/osr_render_widget_host_view.h" #include "atom/browser/osr/osr_web_contents_view.h" -#include "content/browser/web_contents/web_contents_impl.h" +#endif // Including both web_contents_impl.h and node.h would introduce a error, we // have to isolate the usage of WebContentsImpl into a clean file to fix it: @@ -16,6 +19,13 @@ namespace atom { namespace api { +void WebContents::DetachFromOuterFrame() { + // See detach_webview_frame.patch on how to detach. + auto* impl = static_cast(web_contents()); + impl->GetRenderManagerForTesting()->RemoveOuterDelegateFrame(); +} + +#if BUILDFLAG(ENABLE_OSR) OffScreenWebContentsView* WebContents::GetOffScreenWebContentsView() const { if (IsOffScreen()) { const auto* impl = @@ -35,6 +45,7 @@ OffScreenRenderWidgetHostView* WebContents::GetOffScreenRenderWidgetHostView() return nullptr; } } +#endif } // namespace api diff --git a/filenames.gni b/filenames.gni index 255ecc2c0703..d913ad232fd1 100644 --- a/filenames.gni +++ b/filenames.gni @@ -177,6 +177,7 @@ filenames = { "atom/browser/api/atom_api_view.h", "atom/browser/api/atom_api_web_contents.cc", "atom/browser/api/atom_api_web_contents.h", + "atom/browser/api/atom_api_web_contents_impl.cc", "atom/browser/api/atom_api_web_contents_mac.mm", "atom/browser/api/atom_api_web_contents_view.cc", "atom/browser/api/atom_api_web_contents_view.h", diff --git a/lib/browser/guest-view-manager.js b/lib/browser/guest-view-manager.js index 378f07500dde..5c0c40024830 100644 --- a/lib/browser/guest-view-manager.js +++ b/lib/browser/guest-view-manager.js @@ -181,7 +181,7 @@ const attachGuest = function (event, embedderFrameId, elementInstanceId, guestIn const oldGuestInstance = guestInstances[oldGuestInstanceId] if (oldGuestInstance) { - oldGuestInstance.guest.destroy() + oldGuestInstance.guest.detachFromOuterFrame() } } @@ -360,7 +360,7 @@ handleMessage('ELECTRON_GUEST_VIEW_MANAGER_CREATE_GUEST_SYNC', function (event, handleMessage('ELECTRON_GUEST_VIEW_MANAGER_DESTROY_GUEST', function (event, guestInstanceId) { try { const guest = getGuestForWebContents(guestInstanceId, event.sender) - guest.destroy() + guest.detachFromOuterFrame() } catch (error) { console.error(`Guest destroy failed: ${error}`) } diff --git a/patches/common/chromium/disable_detach_webview_frame.patch b/patches/common/chromium/disable_detach_webview_frame.patch index 858cd03f2dac..998f864bea36 100644 --- a/patches/common/chromium/disable_detach_webview_frame.patch +++ b/patches/common/chromium/disable_detach_webview_frame.patch @@ -9,6 +9,8 @@ This is part of the fixes for https://github.com/electron/electron/issues/14211. We should revisit this bug after upgrading to newer versions of Chrome, this patch was introduced in Chrome 66. +Update(zcbenz): The bug is still in Chrome 72. + diff --git a/content/browser/frame_host/render_frame_proxy_host.cc b/content/browser/frame_host/render_frame_proxy_host.cc index cfa3fd15714c3743cb9d2900b35570c94545fa87..80785902890c57ffae2fa841831d3bd60a8fa11a 100644 --- a/content/browser/frame_host/render_frame_proxy_host.cc