From 8b35a4ae828d718e01e2c8a30443eb0e4d2e9c01 Mon Sep 17 00:00:00 2001 From: Shelley Vohr Date: Sun, 13 Dec 2020 20:25:15 -0800 Subject: [PATCH 1/3] fix: handle BrowserView reparenting --- shell/browser/api/electron_api_base_window.cc | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/shell/browser/api/electron_api_base_window.cc b/shell/browser/api/electron_api_base_window.cc index 1314fd2bf1d49..54b532798b283 100644 --- a/shell/browser/api/electron_api_base_window.cc +++ b/shell/browser/api/electron_api_base_window.cc @@ -759,6 +759,14 @@ void BaseWindow::AddBrowserView(v8::Local value) { auto get_that_view = browser_views_.find(browser_view->ID()); if (get_that_view == browser_views_.end()) { if (browser_view->web_contents()) { + // If we're reparenting a BrowserView, ensure that it's detached from + // its previous owner window. + auto* owner_window = browser_view->web_contents()->owner_window(); + if (owner_window && owner_window != window_.get()) { + owner_window->RemoveBrowserView(browser_view->view()); + browser_view->web_contents()->SetOwnerWindow(nullptr); + } + window_->AddBrowserView(browser_view->view()); browser_view->web_contents()->SetOwnerWindow(window_.get()); } @@ -1071,9 +1079,12 @@ void BaseWindow::ResetBrowserViews() { v8::Local::New(isolate(), item.second), &browser_view) && !browser_view.IsEmpty()) { - if (browser_view->web_contents()) { - window_->RemoveBrowserView(browser_view->view()); + // There's a chance that the BrowserView may have been reparented - only + // reset if the owner window is *this* window. + auto* owner_window = browser_view->web_contents()->owner_window(); + if (browser_view->web_contents() && owner_window == window_.get()) { browser_view->web_contents()->SetOwnerWindow(nullptr); + owner_window->RemoveBrowserView(browser_view->view()); } } From a89cf08765d16eccc0b75d3579e4428210e94a8c Mon Sep 17 00:00:00 2001 From: Shelley Vohr Date: Tue, 15 Dec 2020 12:35:43 -0800 Subject: [PATCH 2/3] Fix case where webContents is destroyed --- shell/browser/api/electron_api_base_window.cc | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/shell/browser/api/electron_api_base_window.cc b/shell/browser/api/electron_api_base_window.cc index 54b532798b283..adbaab0d8b1d3 100644 --- a/shell/browser/api/electron_api_base_window.cc +++ b/shell/browser/api/electron_api_base_window.cc @@ -1081,10 +1081,12 @@ void BaseWindow::ResetBrowserViews() { !browser_view.IsEmpty()) { // There's a chance that the BrowserView may have been reparented - only // reset if the owner window is *this* window. - auto* owner_window = browser_view->web_contents()->owner_window(); - if (browser_view->web_contents() && owner_window == window_.get()) { - browser_view->web_contents()->SetOwnerWindow(nullptr); - owner_window->RemoveBrowserView(browser_view->view()); + if (browser_view->web_contents()) { + auto* owner_window = browser_view->web_contents()->owner_window(); + if (owner_window == window_.get()) { + browser_view->web_contents()->SetOwnerWindow(nullptr); + owner_window->RemoveBrowserView(browser_view->view()); + } } } From 81fd419d9e0bbe583f17cb55770f0123aadf36c9 Mon Sep 17 00:00:00 2001 From: Shelley Vohr Date: Tue, 15 Dec 2020 12:56:18 -0800 Subject: [PATCH 3/3] Add a reparenting test --- spec-main/api-browser-view-spec.ts | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/spec-main/api-browser-view-spec.ts b/spec-main/api-browser-view-spec.ts index e6813e31cefac..8680dd3e0406c 100644 --- a/spec-main/api-browser-view-spec.ts +++ b/spec-main/api-browser-view-spec.ts @@ -149,6 +149,27 @@ describe('BrowserView module', () => { w.addBrowserView(view1); }).to.not.throw(); }); + + it('can handle BrowserView reparenting', async () => { + view = new BrowserView(); + + w.addBrowserView(view); + view.webContents.loadURL('about:blank'); + await emittedOnce(view.webContents, 'did-finish-load'); + + const w2 = new BrowserWindow({ show: false }); + w2.addBrowserView(view); + + w.close(); + + view.webContents.loadURL(`file://${fixtures}/pages/blank.html`); + await emittedOnce(view.webContents, 'did-finish-load'); + + // Clean up - the afterEach hook assumes the webContents on w is still alive. + w = new BrowserWindow({ show: false }); + w2.close(); + w2.destroy(); + }); }); describe('BrowserWindow.removeBrowserView()', () => {