From 1a2ab11c90e44bf46bf69f72dcbee2e384116a34 Mon Sep 17 00:00:00 2001
From: Shelley Vohr <shelley.vohr@gmail.com>
Date: Tue, 21 May 2019 07:08:22 -0700
Subject: [PATCH] fix: set window to null when no window is passed (#18240)

* fix: set window to null when no window is passed

* add new specs for dialog

* fix process blocking for showMessageBox
---
 lib/browser/api/dialog.js | 18 ++++++++++--
 spec/api-dialog-spec.js   | 59 ++++++++++++++++++++++++++++++++++++++-
 2 files changed, 73 insertions(+), 4 deletions(-)

diff --git a/lib/browser/api/dialog.js b/lib/browser/api/dialog.js
index 43d93040b29d..24c29c568426 100644
--- a/lib/browser/api/dialog.js
+++ b/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']
diff --git a/spec/api-dialog-spec.js b/spec/api-dialog-spec.js
index 2d9cf71e0f47..4e5525520bac 100644
--- a/spec/api-dialog-spec.js
+++ b/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' })