From 49ed221659c48e3eaf26214ebf071773c01dd10a Mon Sep 17 00:00:00 2001 From: Claudio Costa Date: Tue, 7 Mar 2023 18:23:21 -0600 Subject: [PATCH] Fix race condition on switching calls (#2591) --- src/main/windows/callsWidgetWindow.test.js | 52 +++++++++++++++------- src/main/windows/callsWidgetWindow.ts | 11 ++++- src/main/windows/windowManager.test.js | 29 +++++++++--- src/main/windows/windowManager.ts | 7 ++- 4 files changed, 73 insertions(+), 26 deletions(-) diff --git a/src/main/windows/callsWidgetWindow.test.js b/src/main/windows/callsWidgetWindow.test.js index 35f75fc5..86bb2288 100644 --- a/src/main/windows/callsWidgetWindow.test.js +++ b/src/main/windows/callsWidgetWindow.test.js @@ -75,13 +75,15 @@ describe('main/windows/callsWidgetWindow', () => { baseWindow.setAlwaysOnTop = jest.fn(); baseWindow.setBackgroundColor = jest.fn(); baseWindow.setMenuBarVisibility = jest.fn(); - baseWindow.setBounds = jest.fn(); baseWindow.webContents = { setWindowOpenHandler: jest.fn(), on: jest.fn(), }; + baseWindow.isDestroyed = jest.fn(() => false); beforeEach(() => { + baseWindow.setBounds = jest.fn(); + mainWindow.getBounds.mockImplementation(() => { return { x: 0, @@ -100,6 +102,14 @@ describe('main/windows/callsWidgetWindow', () => { }; }); + baseWindow.show = jest.fn(() => { + baseWindow.emit('show'); + }); + + baseWindow.close = jest.fn(() => { + baseWindow.emit('closed'); + }); + baseWindow.loadURL.mockImplementation(() => ({ catch: jest.fn(), })); @@ -108,6 +118,8 @@ describe('main/windows/callsWidgetWindow', () => { afterEach(() => { jest.resetAllMocks(); + baseWindow.removeAllListeners('show'); + baseWindow.removeAllListeners('ready-to-show'); }); it('verify initial configuration', () => { @@ -175,14 +187,24 @@ describe('main/windows/callsWidgetWindow', () => { expect(widgetWindow.win.webContents.openDevTools).toHaveBeenCalled(); }); - it('closing window', () => { - baseWindow.close = jest.fn(() => { - baseWindow.emit('closed'); - }); + it('closing window', async () => { + const widgetWindow = new CallsWidgetWindow(mainWindow, mainView, widgetConfig); + + await widgetWindow.close(); + + expect(widgetWindow.win.close).toHaveBeenCalled(); + expect(widgetWindow.win.isDestroyed).toHaveBeenCalled(); + }); + + it('closing window - already closed', async () => { + baseWindow.isDestroyed = jest.fn().mockReturnValue(true); const widgetWindow = new CallsWidgetWindow(mainWindow, mainView, widgetConfig); - widgetWindow.close(); - expect(widgetWindow.win.close).toHaveBeenCalled(); + + await widgetWindow.close(); + + expect(widgetWindow.win.isDestroyed).toHaveBeenCalled(); + expect(widgetWindow.win.close).not.toHaveBeenCalled(); }); it('resize', () => { @@ -191,10 +213,6 @@ describe('main/windows/callsWidgetWindow', () => { id: 'windowID', }; - baseWindow.show = jest.fn(() => { - baseWindow.emit('show'); - }); - let winBounds = { x: 0, y: 0, @@ -209,12 +227,18 @@ describe('main/windows/callsWidgetWindow', () => { winBounds = bounds; }); + baseWindow.show = jest.fn(() => { + baseWindow.emit('show'); + }); + + expect(baseWindow.setBounds).not.toHaveBeenCalled(); + baseWindow.webContents.getZoomFactor = jest.fn(() => 1.0); const widgetWindow = new CallsWidgetWindow(mainWindow, mainView, widgetConfig); widgetWindow.win.emit('ready-to-show'); - expect(baseWindow.setBounds).toHaveBeenCalledTimes(2); + expect(baseWindow.setBounds).toHaveBeenCalledTimes(1); expect(baseWindow.setBounds).toHaveBeenCalledWith({ x: 12, @@ -260,10 +284,6 @@ describe('main/windows/callsWidgetWindow', () => { id: 'windowID', }; - baseWindow.show = jest.fn(() => { - baseWindow.emit('show'); - }); - let winBounds = { x: 0, y: 0, diff --git a/src/main/windows/callsWidgetWindow.ts b/src/main/windows/callsWidgetWindow.ts index 1206d705..822bc5aa 100644 --- a/src/main/windows/callsWidgetWindow.ts +++ b/src/main/windows/callsWidgetWindow.ts @@ -89,9 +89,16 @@ export default class CallsWidgetWindow extends EventEmitter { this.load(); } - public close() { + public async close() { log.debug('CallsWidgetWindow.close'); - this.win.close(); + return new Promise((resolve) => { + if (this.win.isDestroyed()) { + resolve(); + return; + } + this.once('closed', resolve); + this.win.close(); + }); } public getServerName() { diff --git a/src/main/windows/windowManager.test.js b/src/main/windows/windowManager.test.js index 889e1406..e0b01ac6 100644 --- a/src/main/windows/windowManager.test.js +++ b/src/main/windows/windowManager.test.js @@ -1032,6 +1032,23 @@ describe('main/windows/windowManager', () => { }, }, }; + + beforeEach(() => { + CallsWidgetWindow.mockImplementation(() => { + return { + win: { + isDestroyed: jest.fn(() => true), + }, + on: jest.fn(), + close: jest.fn(), + }; + }); + }); + + afterEach(() => { + jest.resetAllMocks(); + }); + const windowManager = new WindowManager(); windowManager.viewManager = { views: new Map([ @@ -1039,25 +1056,25 @@ describe('main/windows/windowManager', () => { ]), }; - it('should create calls widget window', () => { + it('should create calls widget window', async () => { expect(windowManager.callsWidgetWindow).toBeUndefined(); - windowManager.createCallsWidgetWindow('server-1_tab-messaging', {callID: 'test'}); + await windowManager.createCallsWidgetWindow('server-1_tab-messaging', {callID: 'test'}); expect(windowManager.callsWidgetWindow).toBeDefined(); }); - it('should not create a new window if call is the same', () => { + it('should not create a new window if call is the same', async () => { const widgetWindow = windowManager.callsWidgetWindow; expect(widgetWindow).toBeDefined(); widgetWindow.getCallID = jest.fn(() => 'test'); - windowManager.createCallsWidgetWindow('server-1_tab-messaging', {callID: 'test'}); + await windowManager.createCallsWidgetWindow('server-1_tab-messaging', {callID: 'test'}); expect(windowManager.callsWidgetWindow).toEqual(widgetWindow); }); - it('should create a new window if switching calls', () => { + it('should create a new window if switching calls', async () => { const widgetWindow = windowManager.callsWidgetWindow; expect(widgetWindow).toBeDefined(); widgetWindow.getCallID = jest.fn(() => 'test'); - windowManager.createCallsWidgetWindow('server-1_tab-messaging', {callID: 'test2'}); + await windowManager.createCallsWidgetWindow('server-1_tab-messaging', {callID: 'test2'}); expect(windowManager.callsWidgetWindow).not.toEqual(widgetWindow); }); }); diff --git a/src/main/windows/windowManager.ts b/src/main/windows/windowManager.ts index c8a044e4..27ed4d4d 100644 --- a/src/main/windows/windowManager.ts +++ b/src/main/windows/windowManager.ts @@ -127,14 +127,17 @@ export class WindowManager { }; } - createCallsWidgetWindow = (viewName: string, msg: CallsJoinCallMessage) => { + createCallsWidgetWindow = async (viewName: string, msg: CallsJoinCallMessage) => { log.debug('WindowManager.createCallsWidgetWindow'); if (this.callsWidgetWindow) { // trying to join again the call we are already in should not be allowed. if (this.callsWidgetWindow.getCallID() === msg.callID) { return; } - this.callsWidgetWindow.close(); + + // to switch from one call to another we need to wait for the existing + // window to be fully closed. + await this.callsWidgetWindow.close(); } const currentView = this.viewManager?.views.get(viewName); if (!currentView) {