fix: set window to null when no window is passed (#18381)

Fixes #18236.

Our options parsing for dialogs correctly set options if no window was passed, but then did not correctly set the window back to null and it was then left as an object equal to options. This caused a crash when passed to native code as there is no valid circumstance where the window would be an object.

lib/browser/api/dialog.js

@@ -47,7 +47,11 @@ const checkAppInitialized = function () {
 const saveDialog = (sync, window, options) => {
   checkAppInitialized()
 
-  if (window && window.constructor !== BrowserWindow) options = window
+  if (window && window.constructor !== BrowserWindow) {
+    options = window
+    window = null
+  }
+
   if (options == null) options = { title: 'Save' }
 
   const {
@@ -74,7 +78,11 @@ const saveDialog = (sync, window, options) => {
 const openDialog = (sync, window, options) => {
   checkAppInitialized()
 
-  if (window && window.constructor !== BrowserWindow) options = window
+  if (window && window.constructor !== BrowserWindow) {
+    options = window
+    window = null
+  }
+
   if (options == null) {
     options = {
       title: 'Open',
@@ -115,7 +123,11 @@ const openDialog = (sync, window, options) => {
 const messageBox = (sync, window, options) => {
   checkAppInitialized()
 
-  if (window && window.constructor !== BrowserWindow) options = window
+  if (window && window.constructor !== BrowserWindow) {
+    options = window
+    window = null
+  }
+
   if (options == null) options = { type: 'none' }
 
   const messageBoxTypes = ['none', 'info', 'warning', 'error', 'question']

spec/api-dialog-spec.js

@@ -1,8 +1,29 @@
 const { expect } = require('chai')
-const { dialog } = require('electron').remote
+const { closeWindow } = require('./window-helpers')
+const { remote } = require('electron')
+const { BrowserWindow, dialog } = remote
+const isCI = remote.getGlobal('isCi')
 
 describe('dialog module', () => {
   describe('showOpenDialog', () => {
+    it('should not throw for valid cases', () => {
+      // Blocks the main process and can't be run in CI
+      if (isCI) return
+
+      let w
+
+      expect(() => {
+        dialog.showOpenDialog({ title: 'i am title' })
+      }).to.not.throw()
+
+      expect(() => {
+        w = new BrowserWindow()
+        dialog.showOpenDialog(w, { title: 'i am title' })
+      }).to.not.throw()
+
+      closeWindow(w).then(() => { w = null })
+    })
+
     it('throws errors when the options are invalid', () => {
       expect(() => {
         dialog.showOpenDialog({ properties: false })
@@ -27,6 +48,24 @@ describe('dialog module', () => {
   })
 
   describe('showSaveDialog', () => {
+    it('should not throw for valid cases', () => {
+      // Blocks the main process and can't be run in CI
+      if (isCI) return
+
+      let w
+
+      expect(() => {
+        dialog.showSaveDialog({ title: 'i am title' })
+      }).to.not.throw()
+
+      expect(() => {
+        w = new BrowserWindow()
+        dialog.showSaveDialog(w, { title: 'i am title' })
+      }).to.not.throw()
+
+      closeWindow(w).then(() => { w = null })
+    })
+
     it('throws errors when the options are invalid', () => {
       expect(() => {
         dialog.showSaveDialog({ title: 300 })
@@ -51,6 +90,24 @@ describe('dialog module', () => {
   })
 
   describe('showMessageBox', () => {
+    it('should not throw for valid cases', () => {
+      // Blocks the main process and can't be run in CI
+      if (isCI) return
+
+      let w
+
+      expect(() => {
+        dialog.showMessageBox({ title: 'i am title' })
+      }).to.not.throw()
+
+      expect(() => {
+        w = new BrowserWindow()
+        dialog.showMessageBox(w, { title: 'i am title' })
+      }).to.not.throw()
+
+      closeWindow(w).then(() => { w = null })
+    })
+
     it('throws errors when the options are invalid', () => {
       expect(() => {
         dialog.showMessageBox(undefined, { type: 'not-a-valid-type' })