fix: crash when `setWindowOpenHandler` callback throws (#34627)

fix: crash when `setWindowOpenHandler` callback throws (#34523)

Co-authored-by: Shelley Vohr <[email protected]>

lib/browser/api/web-contents.ts

@@ -500,6 +500,7 @@ WebContents.prototype._callWindowOpenHandler = function (event: Electron.Event,
   if (!this._windowOpenHandler) {
     return null;
   }
+
   const response = this._windowOpenHandler(details);
 
   if (typeof response !== 'object') {
@@ -656,7 +657,15 @@ WebContents.prototype._init = function () {
         postBody,
         disposition
       };
-      const options = this._callWindowOpenHandler(event, details);
+
+      let options: ReturnType<typeof this._callWindowOpenHandler>;
+      try {
+        options = this._callWindowOpenHandler(event, details);
+      } catch (err) {
+        event.preventDefault();
+        throw err;
+      }
+
       if (!event.defaultPrevented) {
         openGuestWindow({
           event,
@@ -684,7 +693,16 @@ WebContents.prototype._init = function () {
         referrer,
         postBody
       };
-      windowOpenOverriddenOptions = this._callWindowOpenHandler(event, details);
+
+      let result: ReturnType<typeof this._callWindowOpenHandler>;
+      try {
+        result = this._callWindowOpenHandler(event, details);
+      } catch (err) {
+        event.preventDefault();
+        throw err;
+      }
+
+      windowOpenOverriddenOptions = result;
       if (!event.defaultPrevented) {
         const secureOverrideWebPreferences = windowOpenOverriddenOptions ? {
           // Allow setting of backgroundColor as a webPreference even though

spec-main/guest-window-manager-spec.ts

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