fix: network plugin cleanup

Author: pauldambra

.changeset/olive-drinks-itch.md

@@ -0,0 +1,5 @@
+---
+'posthog-js': patch
+---
+
+fix: properly cleanup in network plugin

packages/browser/src/__tests__/extensions/replay/external/network-plugin.test.ts

@@ -14,6 +14,70 @@ class MockRequest {
 // Replace global Request with our mock
 global.Request = MockRequest as any
 
+// Mock window with performance API
+function createMockWindow() {
+    const performanceEntries: PerformanceEntry[] = []
+    const observerCallbacks: Array<(entries: PerformanceObserverEntryList) => void> = []
+
+    const mockWindow = {
+        performance: {
+            now: () => Date.now(),
+            getEntries: () => performanceEntries,
+            getEntriesByName: (name: string) => performanceEntries.filter((e: any) => e.name === name),
+            clearResourceTimings: () => {
+                performanceEntries.length = 0
+            },
+        },
+        PerformanceObserver: class {
+            callback: (entries: PerformanceObserverEntryList) => void
+            constructor(callback: (entries: PerformanceObserverEntryList) => void) {
+                this.callback = callback
+            }
+            observe() {
+                observerCallbacks.push(this.callback)
+            }
+            disconnect() {
+                const index = observerCallbacks.indexOf(this.callback)
+                if (index > -1) observerCallbacks.splice(index, 1)
+            }
+        } as any,
+        XMLHttpRequest: class {
+            listeners: Map<string, Array<(e: any) => void>> = new Map()
+            readyState = 0
+            DONE = 4
+            status = 200
+            response = ''
+            responseText = ''
+
+            open() {}
+            send() {}
+            setRequestHeader() {}
+            getAllResponseHeaders() {
+                return ''
+            }
+            addEventListener(event: string, listener: (e: any) => void) {
+                if (!this.listeners.has(event)) this.listeners.set(event, [])
+                this.listeners.get(event)!.push(listener)
+            }
+            removeEventListener(event: string, listener: (e: any) => void) {
+                const listeners = this.listeners.get(event)
+                if (listeners) {
+                    const index = listeners.indexOf(listener)
+                    if (index > -1) listeners.splice(index, 1)
+                }
+            }
+            getListenerCount(event: string) {
+                return this.listeners.get(event)?.length || 0
+            }
+        } as any,
+        fetch: async () => new Response(),
+    } as any
+
+    mockWindow.PerformanceObserver.supportedEntryTypes = ['navigation', 'resource']
+
+    return { mockWindow, performanceEntries, observerCallbacks }
+}
+
 const blobUrlTestCases = [
     { url: 'blob:https://example.com/123', expected: false },
     { url: new URL('blob:https://example.com/123'), expected: false },
@@ -180,4 +244,141 @@ describe('network plugin', () => {
             })
         })
     })
+
+    describe('network observer lifecycle', () => {
+        describe('singleton initialization and cleanup', () => {
+            it('should initialize successfully on first call', () => {
+                jest.isolateModules(() => {
+                    // eslint-disable-next-line @typescript-eslint/no-require-imports
+                    const { getRecordNetworkPlugin } = require('../../../../extensions/replay/external/network-plugin')
+                    const { mockWindow, observerCallbacks } = createMockWindow()
+                    global.PerformanceObserver = mockWindow.PerformanceObserver
+
+                    const plugin = getRecordNetworkPlugin()
+                    const cleanup = plugin.observer(() => {}, mockWindow, {})
+
+                    expect(typeof cleanup).toBe('function')
+                    expect(observerCallbacks.length).toBe(1)
+                })
+            })
+
+            it('should allow re-initialization after cleanup', () => {
+                jest.isolateModules(() => {
+                    // eslint-disable-next-line @typescript-eslint/no-require-imports
+                    const { getRecordNetworkPlugin } = require('../../../../extensions/replay/external/network-plugin')
+                    const { mockWindow, observerCallbacks } = createMockWindow()
+                    global.PerformanceObserver = mockWindow.PerformanceObserver
+
+                    const plugin1 = getRecordNetworkPlugin()
+                    const cleanup1 = plugin1.observer(() => {}, mockWindow, {})
+                    expect(observerCallbacks.length).toBe(1)
+
+                    cleanup1()
+                    expect(observerCallbacks.length).toBe(0)
+
+                    const plugin2 = getRecordNetworkPlugin()
+                    const cleanup2 = plugin2.observer(() => {}, mockWindow, {})
+                    expect(observerCallbacks.length).toBe(1)
+
+                    cleanup2()
+                })
+            })
+
+            it('should handle multiple cleanup calls safely', () => {
+                jest.isolateModules(() => {
+                    // eslint-disable-next-line @typescript-eslint/no-require-imports
+                    const { getRecordNetworkPlugin } = require('../../../../extensions/replay/external/network-plugin')
+                    const { mockWindow, observerCallbacks } = createMockWindow()
+                    global.PerformanceObserver = mockWindow.PerformanceObserver
+
+                    const plugin = getRecordNetworkPlugin()
+                    const cleanup = plugin.observer(() => {}, mockWindow, {})
+
+                    expect(() => {
+                        cleanup()
+                        cleanup()
+                        cleanup()
+                    }).not.toThrow()
+
+                    expect(observerCallbacks.length).toBe(0)
+                })
+            })
+        })
+
+        describe('XHR listener cleanup', () => {
+            let mockWindow: any
+            let xhr: any
+
+            beforeEach(() => {
+                jest.isolateModules(() => {
+                    // eslint-disable-next-line @typescript-eslint/no-require-imports
+                    const { getRecordNetworkPlugin } = require('../../../../extensions/replay/external/network-plugin')
+                    const mock = createMockWindow()
+                    mockWindow = mock.mockWindow
+
+                    global.PerformanceObserver = mockWindow.PerformanceObserver
+
+                    const plugin = getRecordNetworkPlugin({ recordBody: true })
+                    plugin.observer(() => {}, mockWindow, { recordBody: true })
+
+                    xhr = new mockWindow.XMLHttpRequest()
+                })
+            })
+
+            it('should remove readystatechange listener on successful request', () => {
+                xhr.open('GET', 'https://example.com')
+                xhr.send()
+
+                expect(xhr.getListenerCount('readystatechange')).toBeGreaterThan(0)
+
+                xhr.readyState = xhr.DONE
+                xhr.status = 200
+                const listeners = xhr.listeners.get('readystatechange') || []
+                listeners.forEach((listener: any) => listener())
+
+                expect(xhr.getListenerCount('readystatechange')).toBe(0)
+            })
+
+            const failureEvents = [
+                { event: 'error', payload: new Error('Network error') },
+                { event: 'abort', payload: { type: 'abort' } },
+                { event: 'timeout', payload: { type: 'timeout' } },
+            ]
+
+            failureEvents.forEach(({ event, payload }) => {
+                it(`should remove all listeners when XHR ${event}s`, () => {
+                    xhr.open('GET', 'https://example.com')
+                    xhr.send()
+
+                    const listeners = xhr.listeners.get(event) || []
+                    listeners.forEach((listener: any) => listener(payload))
+
+                    expect(xhr.getListenerCount('readystatechange')).toBe(0)
+                    expect(xhr.getListenerCount('error')).toBe(0)
+                    expect(xhr.getListenerCount('abort')).toBe(0)
+                    expect(xhr.getListenerCount('timeout')).toBe(0)
+                })
+            })
+
+            it('should not leak memory with multiple failed requests', () => {
+                const xhrInstances = Array.from({ length: 10 }, (_, i) => {
+                    const testXhr = new mockWindow.XMLHttpRequest()
+                    testXhr.open('GET', `https://example.com/${i}`)
+                    testXhr.send()
+
+                    const errorListeners = testXhr.listeners.get('error') || []
+                    errorListeners.forEach((listener: any) => listener(new Error('Network error')))
+
+                    return testXhr
+                })
+
+                xhrInstances.forEach((testXhr) => {
+                    expect(testXhr.getListenerCount('readystatechange')).toBe(0)
+                    expect(testXhr.getListenerCount('error')).toBe(0)
+                    expect(testXhr.getListenerCount('abort')).toBe(0)
+                    expect(testXhr.getListenerCount('timeout')).toBe(0)
+                })
+            })
+        })
+    })
 })

packages/browser/src/extensions/replay/external/network-plugin.ts

@@ -292,13 +292,21 @@ function initXhrObserver(cb: networkCallback, win: IWindow, options: Required<Ne
                     return originalSend(body)
                 }
 
+                // Cleanup function to remove all event listeners and prevent memory leaks
+                const cleanup = () => {
+                    xhr.removeEventListener('readystatechange', readyStateListener)
+                    xhr.removeEventListener('error', cleanup)
+                    xhr.removeEventListener('abort', cleanup)
+                    xhr.removeEventListener('timeout', cleanup)
+                }
+
                 const readyStateListener = () => {
                     if (xhr.readyState !== xhr.DONE) {
                         return
                     }
 
-                    // Clean up the listener immediately when done to prevent memory leaks
-                    xhr.removeEventListener('readystatechange', readyStateListener)
+                    // Clean up all listeners immediately when done to prevent memory leaks
+                    cleanup()
 
                     end = win.performance.now()
                     const responseHeaders: Headers = {}
@@ -348,6 +356,13 @@ function initXhrObserver(cb: networkCallback, win: IWindow, options: Required<Ne
                 // so let's ignore the rule here.
                 // eslint-disable-next-line posthog-js/no-add-event-listener
                 xhr.addEventListener('readystatechange', readyStateListener)
+                // Also clean up on error, abort, and timeout to prevent memory leaks
+                // eslint-disable-next-line posthog-js/no-add-event-listener
+                xhr.addEventListener('error', cleanup)
+                // eslint-disable-next-line posthog-js/no-add-event-listener
+                xhr.addEventListener('abort', cleanup)
+                // eslint-disable-next-line posthog-js/no-add-event-listener
+                xhr.addEventListener('timeout', cleanup)
 
                 originalOpen.call(xhr, method, url.toString(), async, username, password)
             }
@@ -672,6 +687,7 @@ function initNetworkObserver(
         performanceObserver()
         xhrObserver()
         fetchObserver()
+        initialisedHandler = null
     }
     return initialisedHandler
 }