Merge pull request #1722 from bugsnag/fix-electron-app-breadcrumb-crash

Fix electron app breadcrumb exception

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)
 

packages/electron-test-helpers/src/BrowserWindow.ts

@@ -40,6 +40,8 @@ export interface BrowserWindow {
   on: (event: BrowserWindowEvent, callback: Function) => void
   getSize: () => Size
   getPosition: () => Position
+  isDestroyed: () => boolean
+  destroy: () => void
 
   _emit: (event: string, ...args: any[]) => void
   readonly callbacks: { [event in BrowserWindowEvent]: Function[] }
@@ -107,17 +109,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
+    }
+
+    private _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) })
     }

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)

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()