From c3525e997d804517bb27fc5276e5fd19346e4b2b Mon Sep 17 00:00:00 2001 From: Joe Haines Date: Thu, 14 Apr 2022 15:12:18 +0100 Subject: [PATCH 1/2] Add the ability to destroy browser windows --- .../src/BrowserWindow.ts | 27 +++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/packages/electron-test-helpers/src/BrowserWindow.ts b/packages/electron-test-helpers/src/BrowserWindow.ts index 1244507ae6..ba7159e8d6 100644 --- a/packages/electron-test-helpers/src/BrowserWindow.ts +++ b/packages/electron-test-helpers/src/BrowserWindow.ts @@ -40,6 +40,7 @@ export interface BrowserWindow { on: (event: BrowserWindowEvent, callback: Function) => void getSize: () => Size getPosition: () => Position + isDestroyed: () => boolean _emit: (event: string, ...args: any[]) => void readonly callbacks: { [event in BrowserWindowEvent]: Function[] } @@ -107,17 +108,43 @@ export function makeBrowserWindow ({ windows = [], focusedWindow = null } = {}): } on (event: BrowserWindowEvent, callback: Function): void { + this._assertNotDestroyed() + this.callbacks[event].push(callback) } getSize (): Size { + this._assertNotDestroyed() + return this.size } getPosition (): Position { + this._assertNotDestroyed() + return this.position } + destroy (): void { + // > Force closing the window, the unload and beforeunload event won't be + // > emitted for the web page, and close event will also not be emitted for + // > this window, but it guarantees the closed event will be emitted. + // > https://www.electronjs.org/docs/latest/api/browser-window#windestroy + this._emit('closed') + + this._isDestroyed = true + } + + isDestroyed (): boolean { + return this._isDestroyed + } + + _assertNotDestroyed (): void { + if (this._isDestroyed) { + throw new TypeError('Object has been destroyed') + } + } + _emit (event: string, ...args: any[]): void { this.callbacks[event as BrowserWindowEvent].forEach(cb => { cb(null, ...args) }) } From 04767f46161e95679c0f03888da59d2149a33277 Mon Sep 17 00:00:00 2001 From: Joe Haines Date: Thu, 14 Apr 2022 15:12:31 +0100 Subject: [PATCH 2/2] Fix a possible crash in app breadcrumbs plugin This can happen when closing a window immediately after moving it (i.e. within the ~250ms debounce timer). We protect against this in two ways: 1. check if the window is destroyed before using it in the 'moved' callback 2. cancel a queued call to the 'moved' callback when the 'closed' event fires, as this is the point where the browser window is destroyed The other event listeners should never be called with a destroyed browser window (other than 'closed') because they are not debounced and so run as soon as the event is emitted --- CHANGELOG.md | 1 + .../src/BrowserWindow.ts | 3 ++- .../app-breadcrumbs.js | 24 ++++++++++++++----- .../test/app-breadcrumbs.test.ts | 21 ++++++++++++++++ 4 files changed, 42 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 65fcd5f521..70d235e111 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ ### Fixed - (react-native) Fix reporting of `RCTFatal()` crashes on iOS. [#1719](https://github.com/bugsnag/bugsnag-js/pull/1719) +- (plugin-electron-app-breadcrumbs) Fix a TypeError caused by using a BrowserWindow object after it is destroyed [#1722](https://github.com/bugsnag/bugsnag-js/pull/1722) ## v7.16.3 (2022-04-05) diff --git a/packages/electron-test-helpers/src/BrowserWindow.ts b/packages/electron-test-helpers/src/BrowserWindow.ts index ba7159e8d6..5f8a9ce885 100644 --- a/packages/electron-test-helpers/src/BrowserWindow.ts +++ b/packages/electron-test-helpers/src/BrowserWindow.ts @@ -41,6 +41,7 @@ export interface BrowserWindow { getSize: () => Size getPosition: () => Position isDestroyed: () => boolean + destroy: () => void _emit: (event: string, ...args: any[]) => void readonly callbacks: { [event in BrowserWindowEvent]: Function[] } @@ -139,7 +140,7 @@ export function makeBrowserWindow ({ windows = [], focusedWindow = null } = {}): return this._isDestroyed } - _assertNotDestroyed (): void { + private _assertNotDestroyed (): void { if (this._isDestroyed) { throw new TypeError('Object has been destroyed') } diff --git a/packages/plugin-electron-app-breadcrumbs/app-breadcrumbs.js b/packages/plugin-electron-app-breadcrumbs/app-breadcrumbs.js index b49c9da083..767b7d5e7a 100644 --- a/packages/plugin-electron-app-breadcrumbs/app-breadcrumbs.js +++ b/packages/plugin-electron-app-breadcrumbs/app-breadcrumbs.js @@ -72,6 +72,20 @@ module.exports = (app, BrowserWindow) => ({ }) function attachBrowserWindowListeners (leaveBreadcrumb, browserWindow) { + // the moved event fires too frequently to add a breadcrumb each time + const onMoved = debounce(() => { + // it's possible for the window to be destroyed at this point because we + // debounce this callback. If we try to use 'getPosition' when the window is + // destroyed then we'll will raise a TypeError, so we bail out instead + if (browserWindow.isDestroyed()) { + return + } + + const [left, top] = browserWindow.getPosition() + + leaveBreadcrumb('was moved', browserWindow, { left, top }) + }, 250, debounceOptions) + // when the 'closed' event fires we aren't allowed to read from the browserWindow, // so we cache the values we care about in the 'close' event instead // we don't use the close event for the breadcrumb as it can be cancelled @@ -83,6 +97,9 @@ function attachBrowserWindowListeners (leaveBreadcrumb, browserWindow) { }) browserWindow.on('closed', () => { + // cancel the onMoved callback in case it's still scheduled to run + onMoved.cancel() + leaveBreadcrumb('closed', lastKnownState) }) @@ -116,12 +133,7 @@ function attachBrowserWindowListeners (leaveBreadcrumb, browserWindow) { leaveBreadcrumb('was resized', browserWindow, { width, height }) }) - // the moved event fires too frequently to add a breadcrumb each time - browserWindow.on('moved', debounce(() => { - const [left, top] = browserWindow.getPosition() - - leaveBreadcrumb('was moved', browserWindow, { left, top }) - }, 250, debounceOptions)) + browserWindow.on('moved', onMoved) browserWindow.on('enter-full-screen', () => { leaveBreadcrumb('went full-screen', browserWindow) diff --git a/packages/plugin-electron-app-breadcrumbs/test/app-breadcrumbs.test.ts b/packages/plugin-electron-app-breadcrumbs/test/app-breadcrumbs.test.ts index de632f731b..84faf92af8 100644 --- a/packages/plugin-electron-app-breadcrumbs/test/app-breadcrumbs.test.ts +++ b/packages/plugin-electron-app-breadcrumbs/test/app-breadcrumbs.test.ts @@ -254,6 +254,27 @@ describe('plugin: electron app breadcrumbs', () => { expect(client._breadcrumbs[0]).toMatchBreadcrumb(breadcrumb) }) + it('moved with a destroyed window', () => { + const app = makeApp() + const BrowserWindow = makeBrowserWindow() + const window = new BrowserWindow(7575, 'bbb', { position: { top: 463, left: 817 } }) + + const client = makeClient({ app, BrowserWindow }) + + // destroy the window before emitting the 'moved' event; this can happen + // when closing the window just after moving it, as we debounce the + // 'moved' event callback + window.destroy() + window._emit('moved') + + // the 'destroy' method will fire the 'closed' event, but the 'moved' + // event should be ignored as this window was destroyed before it settled + const breadcrumb = new Breadcrumb('Browser window 7575 closed', { id: 7575, title: 'bbb' }, 'state') + + expect(client._breadcrumbs).toHaveLength(1) + expect(client._breadcrumbs[0]).toMatchBreadcrumb(breadcrumb) + }) + it('enter-full-screen', () => { const app = makeApp() const BrowserWindow = makeBrowserWindow()