From c8a0b2b71d8622bbb848e364a842ea12bd5966fb Mon Sep 17 00:00:00 2001 From: Shelley Vohr Date: Tue, 25 Aug 2020 20:04:13 -0700 Subject: [PATCH] fix: prevent crash if BrowserView webContents was destroyed (#25112) --- docs/api/browser-view.md | 10 ---------- shell/browser/api/electron_api_base_window.cc | 10 ++++++---- .../browser/api/electron_api_web_contents.cc | 3 ++- shell/browser/native_window_mac.mm | 20 +++++++++++-------- shell/browser/native_window_views.cc | 11 +++++----- spec-main/api-browser-view-spec.ts | 19 ++++++++++++++---- 6 files changed, 41 insertions(+), 32 deletions(-) diff --git a/docs/api/browser-view.md b/docs/api/browser-view.md index a9dd89a33fc..0b7c1ef1ec8 100644 --- a/docs/api/browser-view.md +++ b/docs/api/browser-view.md @@ -40,16 +40,6 @@ A [`WebContents`](web-contents.md) object owned by this view. Objects created with `new BrowserView` have the following instance methods: -#### `view.destroy()` - -Force closing the view, the `unload` and `beforeunload` events won't be emitted -for the web page. After you're done with a view, call this function in order to -free memory and other resources as soon as possible. - -#### `view.isDestroyed()` - -Returns `Boolean` - Whether the view is destroyed. - #### `view.setAutoResize(options)` _Experimental_ * `options` Object diff --git a/shell/browser/api/electron_api_base_window.cc b/shell/browser/api/electron_api_base_window.cc index e2b1422675c..cad0598faed 100644 --- a/shell/browser/api/electron_api_base_window.cc +++ b/shell/browser/api/electron_api_base_window.cc @@ -745,7 +745,8 @@ void BaseWindow::AddBrowserView(v8::Local value) { auto get_that_view = browser_views_.find(browser_view->ID()); if (get_that_view == browser_views_.end()) { window_->AddBrowserView(browser_view->view()); - browser_view->web_contents()->SetOwnerWindow(window_.get()); + if (browser_view->web_contents()) + browser_view->web_contents()->SetOwnerWindow(window_.get()); browser_views_[browser_view->ID()].Reset(isolate(), value); } } @@ -758,8 +759,8 @@ void BaseWindow::RemoveBrowserView(v8::Local value) { auto get_that_view = browser_views_.find(browser_view->ID()); if (get_that_view != browser_views_.end()) { window_->RemoveBrowserView(browser_view->view()); - browser_view->web_contents()->SetOwnerWindow(nullptr); - + if (browser_view->web_contents()) + browser_view->web_contents()->SetOwnerWindow(nullptr); (*get_that_view).second.Reset(isolate(), value); browser_views_.erase(get_that_view); } @@ -1055,7 +1056,8 @@ void BaseWindow::ResetBrowserViews() { &browser_view) && !browser_view.IsEmpty()) { window_->RemoveBrowserView(browser_view->view()); - browser_view->web_contents()->SetOwnerWindow(nullptr); + if (browser_view->web_contents()) + browser_view->web_contents()->SetOwnerWindow(nullptr); } item.second.Reset(); diff --git a/shell/browser/api/electron_api_web_contents.cc b/shell/browser/api/electron_api_web_contents.cc index fee2dcf38cf..d387d8b9a8f 100644 --- a/shell/browser/api/electron_api_web_contents.cc +++ b/shell/browser/api/electron_api_web_contents.cc @@ -658,7 +658,8 @@ WebContents::~WebContents() { } else { // Destroy WebContents asynchronously unless app is shutting down, // because destroy() might be called inside WebContents's event handler. - DestroyWebContents(!IsGuest() /* async */); + bool is_browser_view = type_ == Type::BROWSER_VIEW; + DestroyWebContents(!(IsGuest() || is_browser_view) /* async */); // The WebContentsDestroyed will not be called automatically because we // destroy the webContents in the next tick. So we have to manually // call it here to make sure "destroyed" event is emitted. diff --git a/shell/browser/native_window_mac.mm b/shell/browser/native_window_mac.mm index 6d30196840f..60398a514e8 100644 --- a/shell/browser/native_window_mac.mm +++ b/shell/browser/native_window_mac.mm @@ -1275,12 +1275,15 @@ void NativeWindowMac::AddBrowserView(NativeBrowserView* view) { } add_browser_view(view); - auto* native_view = - view->GetInspectableWebContentsView()->GetNativeView().GetNativeNSView(); - [[window_ contentView] addSubview:native_view - positioned:NSWindowAbove - relativeTo:nil]; - native_view.hidden = NO; + if (view->GetInspectableWebContentsView()) { + auto* native_view = view->GetInspectableWebContentsView() + ->GetNativeView() + .GetNativeNSView(); + [[window_ contentView] addSubview:native_view + positioned:NSWindowAbove + relativeTo:nil]; + native_view.hidden = NO; + } [CATransaction commit]; } @@ -1294,8 +1297,9 @@ void NativeWindowMac::RemoveBrowserView(NativeBrowserView* view) { return; } - [view->GetInspectableWebContentsView()->GetNativeView().GetNativeNSView() - removeFromSuperview]; + if (view->GetInspectableWebContentsView()) + [view->GetInspectableWebContentsView()->GetNativeView().GetNativeNSView() + removeFromSuperview]; remove_browser_view(view); [CATransaction commit]; diff --git a/shell/browser/native_window_views.cc b/shell/browser/native_window_views.cc index df88eaa1ed3..3a7984ae086 100644 --- a/shell/browser/native_window_views.cc +++ b/shell/browser/native_window_views.cc @@ -1072,9 +1072,9 @@ void NativeWindowViews::AddBrowserView(NativeBrowserView* view) { } add_browser_view(view); - - content_view()->AddChildView( - view->GetInspectableWebContentsView()->GetView()); + if (view->GetInspectableWebContentsView()) + content_view()->AddChildView( + view->GetInspectableWebContentsView()->GetView()); } void NativeWindowViews::RemoveBrowserView(NativeBrowserView* view) { @@ -1085,8 +1085,9 @@ void NativeWindowViews::RemoveBrowserView(NativeBrowserView* view) { return; } - content_view()->RemoveChildView( - view->GetInspectableWebContentsView()->GetView()); + if (view->GetInspectableWebContentsView()) + content_view()->RemoveChildView( + view->GetInspectableWebContentsView()->GetView()); remove_browser_view(view); } diff --git a/spec-main/api-browser-view-spec.ts b/spec-main/api-browser-view-spec.ts index 366b18154b8..15709cb031e 100644 --- a/spec-main/api-browser-view-spec.ts +++ b/spec-main/api-browser-view-spec.ts @@ -134,20 +134,31 @@ describe('BrowserView module', () => { w.addBrowserView(view2); defer(() => w.removeBrowserView(view2)); }); + it('does not throw if called multiple times with same view', () => { view = new BrowserView(); w.addBrowserView(view); w.addBrowserView(view); w.addBrowserView(view); }); + + it('does not crash if the BrowserView webContents are destroyed prior to window removal', () => { + expect(() => { + const view1 = new BrowserView(); + (view1.webContents as any).destroy(); + w.addBrowserView(view1); + }).to.not.throw(); + }); }); describe('BrowserWindow.removeBrowserView()', () => { it('does not throw if called multiple times with same view', () => { - view = new BrowserView(); - w.addBrowserView(view); - w.removeBrowserView(view); - w.removeBrowserView(view); + expect(() => { + view = new BrowserView(); + w.addBrowserView(view); + w.removeBrowserView(view); + w.removeBrowserView(view); + }).to.not.throw(); }); });