From 9c3482cdec81143c702ece5c239c29f36f0cd3dd Mon Sep 17 00:00:00 2001 From: Shelley Vohr Date: Mon, 13 Jun 2022 10:33:51 +0200 Subject: [PATCH 1/2] fix: crash when setWindowOpenHandler throws --- lib/browser/api/web-contents.ts | 10 ++++- spec-main/guest-window-manager-spec.ts | 54 +++++++++++++++++++++++--- 2 files changed, 57 insertions(+), 7 deletions(-) diff --git a/lib/browser/api/web-contents.ts b/lib/browser/api/web-contents.ts index f54b6369a4447..3b08ee92c5330 100644 --- a/lib/browser/api/web-contents.ts +++ b/lib/browser/api/web-contents.ts @@ -482,7 +482,15 @@ WebContents.prototype._callWindowOpenHandler = function (event: Electron.Event, if (!this._windowOpenHandler) { return defaultResponse; } - const response = this._windowOpenHandler(details); + + let response; + try { + response = this._windowOpenHandler(details); + } catch (e: any) { + event.preventDefault(); + console.log(`Error in windowOpenHandler: ${e.message}`); + return defaultResponse; + } if (typeof response !== 'object') { event.preventDefault(); diff --git a/spec-main/guest-window-manager-spec.ts b/spec-main/guest-window-manager-spec.ts index 7d802709c66f1..a46b4145c1e91 100644 --- a/spec-main/guest-window-manager-spec.ts +++ b/spec-main/guest-window-manager-spec.ts @@ -79,6 +79,48 @@ describe('webContents.setWindowOpenHandler', () => { afterEach(closeAllWindows); + it('does not fire window creation events if the handler callback throws an error', async () => { + const error = new Promise((resolve) => { + browserWindow.webContents.setWindowOpenHandler(() => { + setTimeout(resolve); + throw new Error('oh no'); + }); + }); + + browserWindow.webContents.on('new-window', () => { + assert.fail('new-window should not be called with an overridden window.open'); + }); + + browserWindow.webContents.on('did-create-window', () => { + assert.fail('did-create-window should not be called with an overridden window.open'); + }); + + browserWindow.webContents.executeJavaScript("window.open('about:blank', '', 'show=no') && true"); + + await error; + }); + + it('does not fire window creation events if the handler callback returns a bad result', async () => { + const bad = new Promise((resolve) => { + browserWindow.webContents.setWindowOpenHandler(() => { + setTimeout(resolve); + return [1, 2, 3] as any; + }); + }); + + browserWindow.webContents.on('new-window', () => { + assert.fail('new-window should not be called with an overridden window.open'); + }); + + browserWindow.webContents.on('did-create-window', () => { + assert.fail('did-create-window should not be called with an overridden window.open'); + }); + + browserWindow.webContents.executeJavaScript("window.open('about:blank', '', 'show=no') && true"); + + await bad; + }); + it('does not fire window creation events if an override returns action: deny', async () => { const denied = new Promise((resolve) => { browserWindow.webContents.setWindowOpenHandler(() => { @@ -87,11 +129,11 @@ describe('webContents.setWindowOpenHandler', () => { }); }); browserWindow.webContents.on('new-window', () => { - assert.fail('new-window should not to be called with an overridden window.open'); + assert.fail('new-window should not be called with an overridden window.open'); }); browserWindow.webContents.on('did-create-window', () => { - assert.fail('did-create-window should not to be called with an overridden window.open'); + assert.fail('did-create-window should not be called with an overridden window.open'); }); browserWindow.webContents.executeJavaScript("window.open('about:blank', '', 'show=no') && true"); @@ -107,11 +149,11 @@ describe('webContents.setWindowOpenHandler', () => { }); }); browserWindow.webContents.on('new-window', () => { - assert.fail('new-window should not to be called with an overridden window.open'); + assert.fail('new-window should not be called with an overridden window.open'); }); browserWindow.webContents.on('did-create-window', () => { - assert.fail('did-create-window should not to be called with an overridden window.open'); + assert.fail('did-create-window should not be called with an overridden window.open'); }); await browserWindow.webContents.loadURL('data:text/html,link'); @@ -129,11 +171,11 @@ describe('webContents.setWindowOpenHandler', () => { }); }); browserWindow.webContents.on('new-window', () => { - assert.fail('new-window should not to be called with an overridden window.open'); + assert.fail('new-window should not be called with an overridden window.open'); }); browserWindow.webContents.on('did-create-window', () => { - assert.fail('did-create-window should not to be called with an overridden window.open'); + assert.fail('did-create-window should not be called with an overridden window.open'); }); await browserWindow.webContents.loadURL('data:text/html,link'); From 972a98952e3b25151629eaab13bdf5a59793ed3a Mon Sep 17 00:00:00 2001 From: Shelley Vohr Date: Mon, 13 Jun 2022 11:29:06 +0200 Subject: [PATCH 2/2] refactor: throw as process uncaughtException event --- lib/browser/api/web-contents.ts | 29 +++++++++++++++++--------- spec-main/guest-window-manager-spec.ts | 24 ++++++++++++++------- 2 files changed, 36 insertions(+), 17 deletions(-) diff --git a/lib/browser/api/web-contents.ts b/lib/browser/api/web-contents.ts index 3b08ee92c5330..f493d49801a8f 100644 --- a/lib/browser/api/web-contents.ts +++ b/lib/browser/api/web-contents.ts @@ -483,14 +483,7 @@ WebContents.prototype._callWindowOpenHandler = function (event: Electron.Event, return defaultResponse; } - let response; - try { - response = this._windowOpenHandler(details); - } catch (e: any) { - event.preventDefault(); - console.log(`Error in windowOpenHandler: ${e.message}`); - return defaultResponse; - } + const response = this._windowOpenHandler(details); if (typeof response !== 'object') { event.preventDefault(); @@ -652,7 +645,15 @@ WebContents.prototype._init = function () { postBody, disposition }; - const result = this._callWindowOpenHandler(event, details); + + let result; + try { + result = this._callWindowOpenHandler(event, details); + } catch (err) { + event.preventDefault(); + throw err; + } + const options = result.browserWindowConstructorOptions; if (!event.defaultPrevented) { openGuestWindow({ @@ -683,7 +684,15 @@ WebContents.prototype._init = function () { referrer, postBody }; - const result = this._callWindowOpenHandler(event, details); + + let result; + try { + result = this._callWindowOpenHandler(event, details); + } catch (err) { + event.preventDefault(); + throw err; + } + windowOpenOutlivesOpenerOption = result.outlivesOpener; windowOpenOverriddenOptions = result.browserWindowConstructorOptions; if (!event.defaultPrevented) { diff --git a/spec-main/guest-window-manager-spec.ts b/spec-main/guest-window-manager-spec.ts index a46b4145c1e91..974e8fce452c2 100644 --- a/spec-main/guest-window-manager-spec.ts +++ b/spec-main/guest-window-manager-spec.ts @@ -79,12 +79,20 @@ describe('webContents.setWindowOpenHandler', () => { afterEach(closeAllWindows); - it('does not fire window creation events if the handler callback throws an error', async () => { - const error = new Promise((resolve) => { - browserWindow.webContents.setWindowOpenHandler(() => { - setTimeout(resolve); - throw new Error('oh no'); - }); + it('does not fire window creation events if the handler callback throws an error', (done) => { + const error = new Error('oh no'); + const listeners = process.listeners('uncaughtException'); + process.removeAllListeners('uncaughtException'); + process.on('uncaughtException', (thrown) => { + try { + expect(thrown).to.equal(error); + done(); + } catch (e) { + done(e); + } finally { + process.removeAllListeners('uncaughtException'); + listeners.forEach((listener) => process.on('uncaughtException', listener)); + } }); browserWindow.webContents.on('new-window', () => { @@ -97,7 +105,9 @@ describe('webContents.setWindowOpenHandler', () => { browserWindow.webContents.executeJavaScript("window.open('about:blank', '', 'show=no') && true"); - await error; + browserWindow.webContents.setWindowOpenHandler(() => { + throw error; + }); }); it('does not fire window creation events if the handler callback returns a bad result', async () => {