diff --git a/src/app/mainWindow/mainWindow.test.js b/src/app/mainWindow/mainWindow.test.js index b3907383f70..e1600adf35f 100644 --- a/src/app/mainWindow/mainWindow.test.js +++ b/src/app/mainWindow/mainWindow.test.js @@ -586,6 +586,7 @@ describe('main/windows/mainWindow', () => { isVisible: jest.fn(() => mainWindow.win.browserWindow.visible), show: jest.fn(), focus: jest.fn(), + isDestroyed: jest.fn(() => false), }, }; mainWindow.init = jest.fn(); diff --git a/src/app/mainWindow/mainWindow.ts b/src/app/mainWindow/mainWindow.ts index d8568ddf4e4..b9504440e3a 100644 --- a/src/app/mainWindow/mainWindow.ts +++ b/src/app/mainWindow/mainWindow.ts @@ -131,7 +131,10 @@ export class MainWindow extends EventEmitter { } get = () => { - return this.win?.browserWindow; + if (!this.win || this.win.browserWindow.isDestroyed()) { + return undefined; + } + return this.win.browserWindow; }; get window() { @@ -139,20 +142,21 @@ export class MainWindow extends EventEmitter { } show = () => { - if (this.win && this.isReady) { + const browserWindow = this.get(); + if (browserWindow && this.isReady) { // There's a bug on Windows in Electron where if the window is snapped, it will unsnap when you call show() // See here: https://github.com/electron/electron/issues/25359 // So to make sure we always show the window on macOS/Linux (need for workspace switching) // We make an exception here if (process.platform === 'win32') { - if (this.win.browserWindow.isVisible()) { - this.win.browserWindow.focus(); + if (browserWindow.isVisible()) { + browserWindow.focus(); } else { - this.win.browserWindow.show(); + browserWindow.show(); } } else { log.info('showing main window'); - this.win.browserWindow.show(); + browserWindow.show(); } } else { this.init(); @@ -164,7 +168,7 @@ export class MainWindow extends EventEmitter { }; getBounds = () => { - return this.win?.browserWindow.getContentBounds(); + return this.get()?.getContentBounds(); }; private shouldStartFullScreen = () => { @@ -217,6 +221,10 @@ export class MainWindow extends EventEmitter { }; private saveWindowState = (file: string, window: BrowserWindow) => { + if (window.isDestroyed()) { + return; + } + const windowState: SavedWindowState = { ...window.getBounds(), maximized: window.isMaximized(), @@ -263,11 +271,12 @@ export class MainWindow extends EventEmitter { }; private onBlur = () => { - if (!this.win) { + const browserWindow = this.get(); + if (!browserWindow) { return; } - this.emit(MAIN_WINDOW_RESIZED, this.win?.browserWindow.getContentBounds()); + this.emit(MAIN_WINDOW_RESIZED, browserWindow.getContentBounds()); ipcMain.emit(TOGGLE_SECURE_INPUT, null, false); // App should save bounds when a window is closed. @@ -275,7 +284,7 @@ export class MainWindow extends EventEmitter { // because main process is killed in such situations. // 'blur' event was effective in order to avoid this. // Ideally, app should detect that OS is shutting down. - this.saveWindowState(boundsInfoPath, this.win.browserWindow); + this.saveWindowState(boundsInfoPath, browserWindow); }; private onClose = (event: Event) => { diff --git a/src/app/system/tray/tray.ts b/src/app/system/tray/tray.ts index d318d5918af..603b9e1bce0 100644 --- a/src/app/system/tray/tray.ts +++ b/src/app/system/tray/tray.ts @@ -107,7 +107,12 @@ export class TrayIcon { } }; - setMenu = (tMenu: Electron.Menu) => this.tray?.setContextMenu(tMenu); + setMenu = (tMenu: Electron.Menu) => { + if (!this.tray || this.tray.isDestroyed()) { + return; + } + this.tray.setContextMenu(tMenu); + }; private update = (status: string, message: string) => { if (!this.tray || this.tray.isDestroyed()) { diff --git a/src/app/views/MattermostWebContentsView.test.js b/src/app/views/MattermostWebContentsView.test.js index 29490ee9f98..761f64f2e2d 100644 --- a/src/app/views/MattermostWebContentsView.test.js +++ b/src/app/views/MattermostWebContentsView.test.js @@ -124,7 +124,7 @@ const view = new MattermostView(server, ViewType.TAB); describe('main/views/MattermostWebContentsView', () => { describe('load', () => { - const window = {on: jest.fn(), webContents: {send: jest.fn()}}; + const window = {on: jest.fn(), webContents: {send: jest.fn()}, isDestroyed: jest.fn(() => false)}; const mattermostView = new MattermostWebContentsView(view, {}, window); beforeEach(() => { @@ -190,7 +190,7 @@ describe('main/views/MattermostWebContentsView', () => { }); describe('retry', () => { - const window = {on: jest.fn(), webContents: {send: jest.fn()}}; + const window = {on: jest.fn(), webContents: {send: jest.fn()}, isDestroyed: jest.fn(() => false)}; const mattermostView = new MattermostWebContentsView(view, {}, window); const retryInBackgroundFn = jest.fn(); @@ -254,7 +254,7 @@ describe('main/views/MattermostWebContentsView', () => { }); describe('retryInBackground', () => { - const window = {on: jest.fn(), webContents: {send: jest.fn()}}; + const window = {on: jest.fn(), webContents: {send: jest.fn()}, isDestroyed: jest.fn(() => false)}; const mattermostView = new MattermostWebContentsView(view, {}, window); mattermostView.reload = jest.fn(); @@ -272,7 +272,7 @@ describe('main/views/MattermostWebContentsView', () => { }); describe('goToOffset', () => { - const window = {on: jest.fn(), webContents: {send: jest.fn()}}; + const window = {on: jest.fn(), webContents: {send: jest.fn()}, isDestroyed: jest.fn(() => false)}; const mattermostView = new MattermostWebContentsView(view, {}, window); mattermostView.reload = jest.fn(); @@ -302,7 +302,7 @@ describe('main/views/MattermostWebContentsView', () => { }); describe('loadSuccess', () => { - const window = {on: jest.fn(), webContents: {send: jest.fn()}}; + const window = {on: jest.fn(), webContents: {send: jest.fn()}, isDestroyed: jest.fn(() => false)}; const mattermostView = new MattermostWebContentsView(view, {}, window); beforeEach(() => { @@ -331,7 +331,7 @@ describe('main/views/MattermostWebContentsView', () => { }); describe('updateHistoryButton', () => { - const window = {on: jest.fn(), webContents: {send: jest.fn()}}; + const window = {on: jest.fn(), webContents: {send: jest.fn()}, isDestroyed: jest.fn(() => false)}; const mattermostView = new MattermostWebContentsView(view, {}, window); beforeEach(() => { @@ -463,7 +463,7 @@ describe('main/views/MattermostWebContentsView', () => { }); describe('handleUpdateTarget', () => { - const window = {on: jest.fn(), webContents: {send: jest.fn()}}; + const window = {on: jest.fn(), webContents: {send: jest.fn()}, isDestroyed: jest.fn(() => false)}; const mattermostView = new MattermostWebContentsView(view, {}, window); beforeEach(() => { @@ -495,7 +495,7 @@ describe('main/views/MattermostWebContentsView', () => { }); describe('handlePageTitleUpdated', () => { - const window = {on: jest.fn(), webContents: {send: jest.fn()}}; + const window = {on: jest.fn(), webContents: {send: jest.fn()}, isDestroyed: jest.fn(() => false)}; const mattermostView = new MattermostWebContentsView(view, {}, window); beforeEach(() => { @@ -556,7 +556,7 @@ describe('main/views/MattermostWebContentsView', () => { }); describe('useLastPath', () => { - const window = {on: jest.fn(), webContents: {send: jest.fn()}}; + const window = {on: jest.fn(), webContents: {send: jest.fn()}, isDestroyed: jest.fn(() => false)}; let mattermostView; beforeEach(() => { @@ -632,21 +632,21 @@ describe('main/views/MattermostWebContentsView', () => { }); it('should register devtools-focused listener permanently', () => { - const window = {on: jest.fn(), webContents: {send: jest.fn()}}; + const window = {on: jest.fn(), webContents: {send: jest.fn()}, isDestroyed: jest.fn(() => false)}; const mattermostView = new MattermostWebContentsView(view, {}, window); const onCalls = mattermostView.webContentsView.webContents.on.mock.calls; expect(onCalls.some(([e]) => e === 'devtools-focused')).toBe(true); }); it('should register devtools-closed listener permanently', () => { - const window = {on: jest.fn(), webContents: {send: jest.fn()}}; + const window = {on: jest.fn(), webContents: {send: jest.fn()}, isDestroyed: jest.fn(() => false)}; const mattermostView = new MattermostWebContentsView(view, {}, window); const onCalls = mattermostView.webContentsView.webContents.on.mock.calls; expect(onCalls.some(([e]) => e === 'devtools-closed')).toBe(true); }); it('should emit UPDATE_SHORTCUT_MENU when devtools-focused fires', () => { - const window = {on: jest.fn(), webContents: {send: jest.fn()}}; + const window = {on: jest.fn(), webContents: {send: jest.fn()}, isDestroyed: jest.fn(() => false)}; const mattermostView = new MattermostWebContentsView(view, {}, window); const onCall = mattermostView.webContentsView.webContents.on.mock.calls.find(([e]) => e === 'devtools-focused'); expect(onCall).toBeDefined(); @@ -655,7 +655,7 @@ describe('main/views/MattermostWebContentsView', () => { }); it('should emit UPDATE_SHORTCUT_MENU when devtools-closed fires', () => { - const window = {on: jest.fn(), webContents: {send: jest.fn()}}; + const window = {on: jest.fn(), webContents: {send: jest.fn()}, isDestroyed: jest.fn(() => false)}; const mattermostView = new MattermostWebContentsView(view, {}, window); const onCall = mattermostView.webContentsView.webContents.on.mock.calls.find(([e]) => e === 'devtools-closed'); expect(onCall).toBeDefined(); @@ -665,7 +665,7 @@ describe('main/views/MattermostWebContentsView', () => { }); describe('openDevTools', () => { - const window = {on: jest.fn(), webContents: {send: jest.fn()}}; + const window = {on: jest.fn(), webContents: {send: jest.fn()}, isDestroyed: jest.fn(() => false)}; let mattermostView; beforeEach(() => { diff --git a/src/app/views/MattermostWebContentsView.ts b/src/app/views/MattermostWebContentsView.ts index faec923f148..4b5032d4cf6 100644 --- a/src/app/views/MattermostWebContentsView.ts +++ b/src/app/views/MattermostWebContentsView.ts @@ -202,7 +202,7 @@ export class MattermostWebContentsView extends EventEmitter { const loading = this.webContentsView.webContents.loadURL(loadURL, {userAgent: composeUserAgent(DeveloperMode.get('browserOnly'))}); loading.then(this.loadSuccess(loadURL)).catch((err) => { if (err.code && err.code.startsWith('ERR_CERT')) { - this.parentWindow.webContents.send(LOAD_FAILED, this.id, err.toString(), loadURL.toString()); + this.sendToParentWindow(LOAD_FAILED, this.id, err.toString(), loadURL.toString()); this.emit(LOAD_FAILED, this.id, err.toString(), loadURL.toString()); this.log.info(`Invalid certificate, stop retrying until the user decides what to do: ${err}.`); this.status = Status.ERROR; @@ -353,6 +353,12 @@ export class MattermostWebContentsView extends EventEmitter { this.webContents?.send(channel, ...args); }; + private sendToParentWindow = (channel: string, ...args: any[]) => { + if (this.parentWindow && !this.parentWindow.isDestroyed()) { + this.parentWindow.webContents.send(channel, ...args); + } + }; + isDestroyed = () => { return this.webContentsView?.webContents?.isDestroyed() ?? true; }; @@ -382,7 +388,7 @@ export class MattermostWebContentsView extends EventEmitter { if (this.maxRetries-- > 0) { this.loadRetry(loadURL, err); } else { - this.parentWindow.webContents.send(LOAD_FAILED, this.id, err.toString(), loadURL.toString()); + this.sendToParentWindow(LOAD_FAILED, this.id, err.toString(), loadURL.toString()); this.emit(LOAD_FAILED, this.id, err.toString(), loadURL.toString()); this.log.info('Could not establish a connection, will continue to retry in the background', {err}); this.status = Status.ERROR; @@ -426,7 +432,7 @@ export class MattermostWebContentsView extends EventEmitter { return; } this.retryLoad = setTimeout(this.retry(loadURL), RELOAD_INTERVAL); - this.parentWindow.webContents.send(LOAD_RETRY, this.id, Date.now() + RELOAD_INTERVAL, err.toString(), loadURL.toString()); + this.sendToParentWindow(LOAD_RETRY, this.id, Date.now() + RELOAD_INTERVAL, err.toString(), loadURL.toString()); this.log.info(`failed loading URL: ${err}, retrying in ${RELOAD_INTERVAL / SECOND} seconds`); }; @@ -438,7 +444,7 @@ export class MattermostWebContentsView extends EventEmitter { const serverInfo = ServerManager.getRemoteInfo(this.view.serverId); if (!serverInfo?.serverVersion || semver.gte(serverInfo.serverVersion, '9.4.0')) { this.log.verbose('finished loading URL'); - this.parentWindow.webContents.send(LOAD_SUCCESS, this.id); + this.sendToParentWindow(LOAD_SUCCESS, this.id); this.maxRetries = MAX_SERVER_RETRIES; this.status = Status.WAITING_MM; this.removeLoading = setTimeout(this.setInitialized, MAX_LOADING_SCREEN_SECONDS, true); @@ -447,7 +453,7 @@ export class MattermostWebContentsView extends EventEmitter { this.setBounds(getWindowBoundaries(this.parentWindow)); } } else { - this.parentWindow.webContents.send(LOAD_INCOMPATIBLE_SERVER, this.id, loadURL.toString()); + this.sendToParentWindow(LOAD_INCOMPATIBLE_SERVER, this.id, loadURL.toString()); this.emit(LOAD_FAILED, this.id, 'Incompatible server version', loadURL.toString()); this.status = Status.ERROR; } diff --git a/src/main/app/app.test.js b/src/main/app/app.test.js index c00cf5e8282..b7a4250017e 100644 --- a/src/main/app/app.test.js +++ b/src/main/app/app.test.js @@ -6,7 +6,7 @@ import {app, dialog} from 'electron'; import MainWindow from 'app/mainWindow/mainWindow'; import WebContentsManager from 'app/views/webContentsManager'; import ServerManager from 'common/servers/serverManager'; -import {handleAppWillFinishLaunching, handleAppCertificateError, certificateErrorCallbacks} from 'main/app/app'; +import {handleAppActivate, handleAppWillFinishLaunching, handleAppCertificateError, certificateErrorCallbacks} from 'main/app/app'; import {getDeeplinkingURL, openDeepLink} from 'main/app/utils'; import CertificateStore from 'main/security/certificateStore'; @@ -60,6 +60,32 @@ jest.mock('common/views/viewManager', () => ({ })); describe('main/app/app', () => { + describe('handleAppActivate', () => { + afterEach(() => { + jest.resetAllMocks(); + }); + + it('should show the main window if app is ready', () => { + app.isReady.mockReturnValue(true); + handleAppActivate(); + expect(MainWindow.show).toHaveBeenCalled(); + }); + + it('should wait until app is ready to show the main window', () => { + let callback; + app.once.mockImplementation((event, cb) => { + if (event === 'ready') { + callback = cb; + } + }); + app.isReady.mockReturnValue(false); + handleAppActivate(); + expect(MainWindow.show).not.toHaveBeenCalled(); + callback(); + expect(MainWindow.show).toHaveBeenCalled(); + }); + }); + describe('handleAppWillFinishLaunching', () => { const deepLinkURL = 'mattermost://server-1.com'; const testURL = 'http://server-1.com'; diff --git a/src/main/app/app.ts b/src/main/app/app.ts index 96dca862472..161cf0a030c 100644 --- a/src/main/app/app.ts +++ b/src/main/app/app.ts @@ -38,6 +38,15 @@ export function handleAppSecondInstance(event: Event, argv: string[]) { } } +export function handleAppActivate() { + if (app.isReady()) { + MainWindow.show(); + } else { + // On macOS 'activate' can fire before 'ready' (e.g. clicking the dock icon while still launching). + app.once('ready', () => MainWindow.show()); + } +} + export function handleAppWindowAllClosed() { log.debug('handleAppWindowAllClosed'); diff --git a/src/main/app/initialize.ts b/src/main/app/initialize.ts index 7ac09e3a0b8..bbeed7579a0 100644 --- a/src/main/app/initialize.ts +++ b/src/main/app/initialize.ts @@ -65,6 +65,7 @@ import updateNotifier from 'main/updateNotifier'; import UserActivityMonitor from 'main/UserActivityMonitor'; import { + handleAppActivate, handleAppBeforeQuit, handleAppBrowserWindowCreated, handleAppCertificateError, @@ -187,7 +188,7 @@ function initializeAppEventListeners() { app.on('second-instance', handleAppSecondInstance); app.on('window-all-closed', handleAppWindowAllClosed); app.on('browser-window-created', handleAppBrowserWindowCreated); - app.on('activate', () => MainWindow.show()); + app.on('activate', handleAppActivate); app.on('before-quit', handleAppBeforeQuit); app.on('certificate-error', handleAppCertificateError); app.on('child-process-gone', handleChildProcessGone);