From cb2e7f130b66a58edba518605b371a35bd2761ee Mon Sep 17 00:00:00 2001 From: Shelley Vohr Date: Mon, 8 Jul 2024 12:13:53 +0200 Subject: [PATCH] fix: potentially closed webContents in BrowserView (#42633) --- lib/browser/api/browser-view.ts | 34 +++++++++-- lib/browser/api/browser-window.ts | 8 ++- shell/browser/api/electron_api_view.cc | 8 ++- spec/api-browser-view-spec.ts | 78 ++++++++++++++++++++++---- 4 files changed, 107 insertions(+), 21 deletions(-) diff --git a/lib/browser/api/browser-view.ts b/lib/browser/api/browser-view.ts index 2c633f735a38..a1659a88ee8d 100644 --- a/lib/browser/api/browser-view.ts +++ b/lib/browser/api/browser-view.ts @@ -4,6 +4,9 @@ const v8Util = process._linkedBinding('electron_common_v8_util'); export default class BrowserView { #webContentsView: WebContentsView; + #ownerWindow: BrowserWindow | null = null; + + #destroyListener: ((e: any) => void) | null = null; // AutoResize state #resizeListener: ((...args: any[]) => void) | null = null; @@ -17,6 +20,9 @@ export default class BrowserView { } webPreferences.type = 'browserView'; this.#webContentsView = new WebContentsView({ webPreferences }); + + this.#destroyListener = this.#onDestroy.bind(this); + this.#webContentsView.webContents.once('destroyed', this.#destroyListener); } get webContents () { @@ -55,23 +61,39 @@ export default class BrowserView { // Internal methods get ownerWindow (): BrowserWindow | null { - return !this.webContents.isDestroyed() ? this.webContents.getOwnerBrowserWindow() : null; + return this.#ownerWindow; } + // We can't rely solely on the webContents' owner window because + // a webContents can be closed by the user while the BrowserView + // remains alive and attached to a BrowserWindow. set ownerWindow (w: BrowserWindow | null) { - if (this.webContents.isDestroyed()) return; - const oldWindow = this.webContents.getOwnerBrowserWindow(); - if (oldWindow && this.#resizeListener) { - oldWindow.off('resize', this.#resizeListener); + if (this.#ownerWindow && this.#resizeListener) { + this.#ownerWindow.off('resize', this.#resizeListener); this.#resizeListener = null; } - this.webContents._setOwnerWindow(w); + + if (this.webContents && !this.webContents.isDestroyed()) { + this.webContents._setOwnerWindow(w); + } + + this.#ownerWindow = w; if (w) { this.#lastWindowSize = w.getBounds(); w.on('resize', this.#resizeListener = this.#autoResize.bind(this)); + w.on('closed', () => { + this.#ownerWindow = null; + this.#destroyListener = null; + }); } } + #onDestroy () { + // Ensure that if #webContentsView's webContents is destroyed, + // the WebContentsView is removed from the view hierarchy. + this.#ownerWindow?.contentView.removeChildView(this.webContentsView); + } + #autoHorizontalProportion: {width: number, left: number} | null = null; #autoVerticalProportion: {height: number, top: number} | null = null; #autoResize () { diff --git a/lib/browser/api/browser-window.ts b/lib/browser/api/browser-window.ts index 0eaaf5684565..adffaf8fa424 100644 --- a/lib/browser/api/browser-window.ts +++ b/lib/browser/api/browser-window.ts @@ -222,7 +222,9 @@ BrowserWindow.prototype.removeBrowserView = function (browserView: BrowserView) }; BrowserWindow.prototype.getBrowserView = function () { - if (this._browserViews.length > 1) { throw new Error('This BrowserWindow has multiple BrowserViews, use getBrowserViews() instead'); } + if (this._browserViews.length > 1) { + throw new Error('This BrowserWindow has multiple BrowserViews - use getBrowserViews() instead'); + } return this._browserViews[0] ?? null; }; @@ -231,7 +233,9 @@ BrowserWindow.prototype.getBrowserViews = function () { }; BrowserWindow.prototype.setTopBrowserView = function (browserView: BrowserView) { - if (browserView.ownerWindow !== this) { throw new Error('Given BrowserView is not attached to the window'); } + if (browserView.ownerWindow !== this) { + throw new Error('Given BrowserView is not attached to the window'); + } const idx = this._browserViews.indexOf(browserView); if (idx >= 0) { this.contentView.addChildView(browserView.webContentsView); diff --git a/shell/browser/api/electron_api_view.cc b/shell/browser/api/electron_api_view.cc index 16630a1ff0aa..4650dc0e2bb8 100644 --- a/shell/browser/api/electron_api_view.cc +++ b/shell/browser/api/electron_api_view.cc @@ -249,14 +249,16 @@ void View::AddChildViewAt(gin::Handle child, void View::RemoveChildView(gin::Handle child) { if (!view_) return; - if (!child->view()) - return; + const auto it = base::ranges::find(child_views_, child.ToV8()); if (it != child_views_.end()) { #if BUILDFLAG(IS_MAC) ScopedCAActionDisabler disable_animations; #endif - view_->RemoveChildView(child->view()); + // It's possible for the child's view to be invalid here + // if the child's webContents was closed or destroyed. + if (child->view()) + view_->RemoveChildView(child->view()); child_views_.erase(it); } } diff --git a/spec/api-browser-view-spec.ts b/spec/api-browser-view-spec.ts index e8cbbd1deeac..62927d1c8804 100644 --- a/spec/api-browser-view-spec.ts +++ b/spec/api-browser-view-spec.ts @@ -370,6 +370,19 @@ describe('BrowserView module', () => { const view = w.getBrowserView(); expect(view).to.be.null('view'); }); + + it('throws if multiple BrowserViews are attached', () => { + view = new BrowserView(); + w.setBrowserView(view); + const view2 = new BrowserView(); + defer(() => view2.webContents.destroy()); + w.addBrowserView(view2); + defer(() => w.removeBrowserView(view2)); + + expect(() => { + w.getBrowserView(); + }).to.throw(/has multiple BrowserViews/); + }); }); describe('BrowserWindow.addBrowserView()', () => { @@ -391,14 +404,6 @@ describe('BrowserView module', () => { w.addBrowserView(view); }); - it('does not crash if the BrowserView webContents are destroyed prior to window addition', () => { - expect(() => { - const view1 = new BrowserView(); - view1.webContents.destroy(); - w.addBrowserView(view1); - }).to.not.throw(); - }); - it('does not crash if the webContents is destroyed after a URL is loaded', () => { view = new BrowserView(); expect(async () => { @@ -411,13 +416,19 @@ describe('BrowserView module', () => { it('can handle BrowserView reparenting', async () => { view = new BrowserView(); + expect(view.ownerWindow).to.be.null('ownerWindow'); + w.addBrowserView(view); view.webContents.loadURL('about:blank'); await once(view.webContents, 'did-finish-load'); + expect(view.ownerWindow).to.equal(w); + const w2 = new BrowserWindow({ show: false }); w2.addBrowserView(view); + expect(view.ownerWindow).to.equal(w2); + w.close(); view.webContents.loadURL(`file://${fixtures}/pages/blank.html`); @@ -429,15 +440,31 @@ describe('BrowserView module', () => { w2.destroy(); }); - it('does not cause a crash when used for view with destroyed web contents', async () => { + it('allows attaching a BrowserView with a previously-closed webContents', async () => { const w2 = new BrowserWindow({ show: false }); const view = new BrowserView(); + + expect(view.ownerWindow).to.be.null('ownerWindow'); view.webContents.close(); w2.addBrowserView(view); + expect(view.ownerWindow).to.equal(w2); + w2.webContents.loadURL('about:blank'); await once(w2.webContents, 'did-finish-load'); w2.close(); }); + + it('allows attaching a BrowserView with a previously-destroyed webContents', async () => { + const view = new BrowserView(); + + expect(view.ownerWindow).to.be.null('ownerWindow'); + view.webContents.destroy(); + w.addBrowserView(view); + expect(view.ownerWindow).to.equal(w); + + w.webContents.loadURL('about:blank'); + await once(w.webContents, 'did-finish-load'); + }); }); describe('BrowserWindow.removeBrowserView()', () => { @@ -535,16 +562,47 @@ describe('BrowserView module', () => { }); }); - describe('BrowserView.webContents.getOwnerBrowserWindow()', () => { + describe('BrowserView owning window', () => { it('points to owning window', () => { view = new BrowserView(); expect(view.webContents.getOwnerBrowserWindow()).to.be.null('owner browser window'); + expect(view.ownerWindow).to.be.null('ownerWindow'); w.setBrowserView(view); expect(view.webContents.getOwnerBrowserWindow()).to.equal(w); + expect(view.ownerWindow).to.equal(w); w.setBrowserView(null); expect(view.webContents.getOwnerBrowserWindow()).to.be.null('owner browser window'); + expect(view.ownerWindow).to.be.null('ownerWindow'); + }); + + it('works correctly when the webContents is destroyed', async () => { + view = new BrowserView(); + w.setBrowserView(view); + + expect(view.webContents.getOwnerBrowserWindow()).to.equal(w); + expect(view.ownerWindow).to.equal(w); + + const destroyed = once(view.webContents, 'destroyed'); + view.webContents.close(); + await destroyed; + + expect(view.ownerWindow).to.equal(w); + }); + + it('works correctly when owner window is closed', async () => { + view = new BrowserView(); + w.setBrowserView(view); + + expect(view.webContents.getOwnerBrowserWindow()).to.equal(w); + expect(view.ownerWindow).to.equal(w); + + const destroyed = once(w, 'closed'); + w.close(); + await destroyed; + + expect(view.ownerWindow).to.equal(null); }); });