Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: crash when setWindowOpenHandler callback throws #34546

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
21 changes: 19 additions & 2 deletions lib/browser/api/web-contents.ts
Expand Up @@ -504,6 +504,7 @@ WebContents.prototype._callWindowOpenHandler = function (event: Electron.Event,
if (!this._windowOpenHandler) {
return defaultResponse;
}

const response = this._windowOpenHandler(details);

if (typeof response !== 'object') {
Expand Down Expand Up @@ -666,7 +667,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({
Expand Down Expand Up @@ -697,7 +706,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) {
Expand Down
64 changes: 58 additions & 6 deletions spec-main/guest-window-manager-spec.ts
Expand Up @@ -79,6 +79,58 @@ describe('webContents.setWindowOpenHandler', () => {

afterEach(closeAllWindows);

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', () => {
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");

browserWindow.webContents.setWindowOpenHandler(() => {
throw 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(() => {
Expand All @@ -87,11 +139,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");
Expand All @@ -107,11 +159,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,<a target="_blank" href="http://example.com" style="display: block; width: 100%; height: 100%; position: fixed; left: 0; top: 0;">link</a>');
Expand All @@ -129,11 +181,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,<a href="http://example.com" style="display: block; width: 100%; height: 100%; position: fixed; left: 0; top: 0;">link</a>');
Expand Down