From fe52193e5c398da205ca4e86da3504415c26b366 Mon Sep 17 00:00:00 2001 From: Yousif Ahmed Date: Wed, 13 Dec 2023 17:47:55 +0000 Subject: [PATCH] Track patched XHR requests for network breadcrumbs --- .../network-breadcrumbs.js | 56 ++++---- .../test/network-breadcrumbs.test.ts | 124 +++++++++++++----- .../script/xhr_failure.html | 2 +- .../script/xhr_success.html | 2 +- 4 files changed, 121 insertions(+), 63 deletions(-) diff --git a/packages/plugin-network-breadcrumbs/network-breadcrumbs.js b/packages/plugin-network-breadcrumbs/network-breadcrumbs.js index adc907391b..63bff65eca 100644 --- a/packages/plugin-network-breadcrumbs/network-breadcrumbs.js +++ b/packages/plugin-network-breadcrumbs/network-breadcrumbs.js @@ -21,45 +21,45 @@ module.exports = (_ignoredUrls = [], win = window) => { // XMLHttpRequest monkey patch function monkeyPatchXMLHttpRequest () { - if (!('addEventListener' in win.XMLHttpRequest.prototype)) return - const nativeOpen = win.XMLHttpRequest.prototype.open + if (!('addEventListener' in win.XMLHttpRequest.prototype) || !('WeakMap' in win)) return - // override native open() - win.XMLHttpRequest.prototype.open = function open (method, url) { - let requestSetupKey = false - - const error = () => handleXHRError(method, url, getDuration(requestStart)) - const load = () => handleXHRLoad(method, url, this.status, getDuration(requestStart)) - - // if we have already setup listeners, it means open() was called twice, we need to remove - // the listeners and recreate them - if (requestSetupKey) { - this.removeEventListener('load', load) - this.removeEventListener('error', error) - } + const trackedRequests = new WeakMap() + const requestHandlers = new WeakMap() - // attach load event listener - this.addEventListener('load', load) - // attach error event listener - this.addEventListener('error', error) + const originalOpen = win.XMLHttpRequest.prototype.open + win.XMLHttpRequest.prototype.open = function open (method, url) { + trackedRequests.set(this, { method, url }) + originalOpen.apply(this, arguments) + } - requestSetupKey = true + const originalSend = win.XMLHttpRequest.prototype.send + win.XMLHttpRequest.prototype.send = function send (body) { + const requestData = trackedRequests.get(this) + if (requestData) { + // if we have already setup listeners then this request instance is being reused, + // so we need to remove the listeners from the previous send + const listeners = requestHandlers.get(this) + if (listeners) { + this.removeEventListener('load', listeners.load) + this.removeEventListener('error', listeners.error) + } - const oldSend = this.send - let requestStart + const requestStart = new Date() + const error = () => handleXHRError(requestData.method, requestData.url, getDuration(requestStart)) + const load = () => handleXHRLoad(requestData.method, requestData.url, this.status, getDuration(requestStart)) - // override send() for this XMLHttpRequest instance - this.send = function send () { - requestStart = new Date() - oldSend.apply(this, arguments) + this.addEventListener('load', load) + this.addEventListener('error', error) + requestHandlers.set(this, { load, error }) } - nativeOpen.apply(this, arguments) + originalSend.apply(this, arguments) } if (process.env.NODE_ENV !== 'production') { restoreFunctions.push(() => { - win.XMLHttpRequest.prototype.open = nativeOpen + win.XMLHttpRequest.prototype.open = originalOpen + win.XMLHttpRequest.prototype.send = originalSend }) } } diff --git a/packages/plugin-network-breadcrumbs/test/network-breadcrumbs.test.ts b/packages/plugin-network-breadcrumbs/test/network-breadcrumbs.test.ts index 5818f75c38..dd6b0816f9 100644 --- a/packages/plugin-network-breadcrumbs/test/network-breadcrumbs.test.ts +++ b/packages/plugin-network-breadcrumbs/test/network-breadcrumbs.test.ts @@ -5,11 +5,11 @@ import Client from '@bugsnag/core/client' import { Config } from '@bugsnag/core' class XMLHttpRequest { - _listeners: { load: () => void, error: () => void } + _listeners: { load: Array<() => void>, error: Array<() => void> } status: number | null; constructor () { - this._listeners = { load: () => {}, error: () => {} } + this._listeners = { load: [], error: [] } this.status = null } @@ -18,20 +18,22 @@ class XMLHttpRequest { send (fail: boolean, status: number | null = null) { if (fail) { - this._listeners.error.call(this) + this._listeners.error.map(fn => fn()) } else { this.status = status - this._listeners.load.call(this) + this._listeners.load.map(fn => fn()) } } addEventListener (evt: 'load'| 'error', listener: () => void) { - this._listeners[evt] = listener + this._listeners[evt].push(listener) } removeEventListener (evt: 'load'| 'error', listener: () => void) { - // eslint-disable-next-line @typescript-eslint/no-dynamic-delete - if (listener === this._listeners[evt]) delete this._listeners[evt] + for (let i = this._listeners[evt].length - 1; i >= 0; i--) { + // eslint-disable-next-line @typescript-eslint/no-dynamic-delete + if (listener.name === this._listeners[evt][i].name) delete this._listeners[evt][i] + } } } @@ -65,7 +67,7 @@ describe('plugin: network breadcrumbs', () => { }) it('should leave a breadcrumb when an XMLHTTPRequest resolves', () => { - const window = { XMLHttpRequest } as unknown as Window & typeof globalThis + const window = { XMLHttpRequest, WeakMap } as unknown as Window & typeof globalThis p = plugin([], window) const client = new Client({ apiKey: 'aaaa-aaaa-aaaa-aaaa', plugins: [p] }) @@ -88,21 +90,77 @@ describe('plugin: network breadcrumbs', () => { })) }) - it('should not leave duplicate breadcrumbs if open() is called twice', () => { - const window = { XMLHttpRequest } as unknown as Window & typeof globalThis + it('should not leave duplicate breadcrumbs if open() is called twice (open -> open -> send)', () => { + const window = { XMLHttpRequest, WeakMap } as unknown as Window & typeof globalThis p = plugin([], window) const client = new Client({ apiKey: 'aaaa-aaaa-aaaa-aaaa', plugins: [p] }) const request = new window.XMLHttpRequest() as unknown as XMLHttpRequest request.open('GET', '/') - request.open('GET', '/') + request.open('POST', 'https://example.com') request.send(false, 200) expect(client._breadcrumbs.length).toBe(1) + expect(client._breadcrumbs[0]).toEqual(expect.objectContaining({ + type: 'request', + message: 'XMLHttpRequest succeeded', + metadata: { + status: 200, + method: 'POST', + url: 'https://example.com', + duration: expect.any(Number) + } + })) + }) + + it('should not leave duplicate breadcrumbs if send() is called twice (open -> send -> open -> send)', () => { + const window = { XMLHttpRequest, WeakMap } as unknown as Window & typeof globalThis + + p = plugin([], window) + const client = new Client({ apiKey: 'aaaa-aaaa-aaaa-aaaa', plugins: [p] }) + + const request = new window.XMLHttpRequest() as unknown as XMLHttpRequest + jest.spyOn(request, 'addEventListener') + jest.spyOn(request, 'removeEventListener') + + request.open('GET', '/') + request.send(false, 200) + + expect(request.addEventListener).toHaveBeenCalledTimes(2) + expect(request.removeEventListener).not.toHaveBeenCalled() + + request.open('POST', 'https://example.com') + request.send(false, 200) + + expect(request.removeEventListener).toHaveBeenCalledTimes(2) + expect(request.addEventListener).toHaveBeenCalledTimes(4) + + expect(client._breadcrumbs.length).toBe(2) + expect(client._breadcrumbs[0]).toEqual(expect.objectContaining({ + type: 'request', + message: 'XMLHttpRequest succeeded', + metadata: { + status: 200, + method: 'GET', + url: '/', + duration: expect.any(Number) + } + })) + + expect(client._breadcrumbs[1]).toEqual(expect.objectContaining({ + type: 'request', + message: 'XMLHttpRequest succeeded', + metadata: { + status: 200, + method: 'POST', + url: 'https://example.com', + duration: expect.any(Number) + } + })) }) it('should leave a breadcrumb when an XMLHTTPRequest has a failed response', () => { - const window = { XMLHttpRequest } as unknown as Window & typeof globalThis + const window = { XMLHttpRequest, WeakMap } as unknown as Window & typeof globalThis p = plugin([], window) const client = new Client({ apiKey: 'aaaa-aaaa-aaaa-aaaa', plugins: [p] }) @@ -125,7 +183,7 @@ describe('plugin: network breadcrumbs', () => { }) it('should leave a breadcrumb when an XMLHTTPRequest has a network error', () => { - const window = { XMLHttpRequest } as unknown as Window & typeof globalThis + const window = { XMLHttpRequest, WeakMap } as unknown as Window & typeof globalThis p = plugin([], window) const client = new Client({ apiKey: 'aaaa-aaaa-aaaa-aaaa', plugins: [p] }) @@ -148,7 +206,7 @@ describe('plugin: network breadcrumbs', () => { }) it('should not leave a breadcrumb for request to bugsnag notify endpoint', () => { - const window = { XMLHttpRequest } as unknown as Window & typeof globalThis + const window = { XMLHttpRequest, WeakMap } as unknown as Window & typeof globalThis p = plugin([], window) const client = new Client({ apiKey: 'aaaa-aaaa-aaaa-aaaa', plugins: [p] } as unknown as Config) @@ -161,7 +219,7 @@ describe('plugin: network breadcrumbs', () => { }) it('should not leave a breadcrumb for session tracking requests', () => { - const window = { XMLHttpRequest } as unknown as Window & typeof globalThis + const window = { XMLHttpRequest, WeakMap } as unknown as Window & typeof globalThis p = plugin([], window) const client = new Client({ apiKey: 'aaaa-aaaa-aaaa-aaaa', plugins: [p] } as unknown as Config) @@ -173,7 +231,7 @@ describe('plugin: network breadcrumbs', () => { }) it('should leave a breadcrumb when the request URL is not a string', () => { - const window = { XMLHttpRequest } as unknown as Window & typeof globalThis + const window = { XMLHttpRequest, WeakMap } as unknown as Window & typeof globalThis const logger = { debug: jest.fn(), @@ -203,7 +261,7 @@ describe('plugin: network breadcrumbs', () => { }) it('should leave a breadcrumb when the request URL is not a string for a request that errors', () => { - const window = { XMLHttpRequest } as unknown as Window & typeof globalThis + const window = { XMLHttpRequest, WeakMap } as unknown as Window & typeof globalThis const logger = { debug: jest.fn(), @@ -232,7 +290,7 @@ describe('plugin: network breadcrumbs', () => { }) it('should leave a breadcrumb when a fetch() resolves', (done) => { - const window = { XMLHttpRequest, fetch } as unknown as Window & typeof globalThis + const window = { XMLHttpRequest, WeakMap, fetch } as unknown as Window & typeof globalThis p = plugin([], window) const client = new Client({ apiKey: 'aaaa-aaaa-aaaa-aaaa', plugins: [p] }) @@ -255,7 +313,7 @@ describe('plugin: network breadcrumbs', () => { }) it('should handle a fetch(url, { method: null })', (done) => { - const window = { XMLHttpRequest, fetch } as unknown as Window & typeof globalThis + const window = { XMLHttpRequest, WeakMap, fetch } as unknown as Window & typeof globalThis p = plugin([], window) const client = new Client({ apiKey: 'aaaa-aaaa-aaaa-aaaa', plugins: [p] }) @@ -278,7 +336,7 @@ describe('plugin: network breadcrumbs', () => { }) it('should handle a fetch() request supplied with a Request object', (done) => { - const window = { XMLHttpRequest, fetch } as unknown as Window & typeof globalThis + const window = { XMLHttpRequest, WeakMap, fetch } as unknown as Window & typeof globalThis p = plugin([], window) const client = new Client({ apiKey: 'aaaa-aaaa-aaaa-aaaa', plugins: [p] }) @@ -303,7 +361,7 @@ describe('plugin: network breadcrumbs', () => { }) it('should handle a fetch() request supplied with a Request object that has a method specified', (done) => { - const window = { XMLHttpRequest, fetch } as unknown as Window & typeof globalThis + const window = { XMLHttpRequest, WeakMap, fetch } as unknown as Window & typeof globalThis p = plugin([], window) const client = new Client({ apiKey: 'aaaa-aaaa-aaaa-aaaa', plugins: [p] }) @@ -328,7 +386,7 @@ describe('plugin: network breadcrumbs', () => { }) it('should handle fetch(null)', (done) => { - const window = { XMLHttpRequest, fetch } as unknown as Window & typeof globalThis + const window = { XMLHttpRequest, WeakMap, fetch } as unknown as Window & typeof globalThis p = plugin([], window) const client = new Client({ apiKey: 'aaaa-aaaa-aaaa-aaaa', plugins: [p] }) @@ -351,7 +409,7 @@ describe('plugin: network breadcrumbs', () => { }) it('should handle fetch(url, null)', (done) => { - const window = { XMLHttpRequest, fetch } as unknown as Window & typeof globalThis + const window = { XMLHttpRequest, WeakMap, fetch } as unknown as Window & typeof globalThis p = plugin([], window) const client = new Client({ apiKey: 'aaaa-aaaa-aaaa-aaaa', plugins: [p] }) @@ -374,7 +432,7 @@ describe('plugin: network breadcrumbs', () => { }) it('should handle fetch(undefined)', (done) => { - const window = { XMLHttpRequest, fetch } as unknown as Window & typeof globalThis + const window = { XMLHttpRequest, WeakMap, fetch } as unknown as Window & typeof globalThis p = plugin([], window) const client = new Client({ apiKey: 'aaaa-aaaa-aaaa-aaaa', plugins: [p] }) @@ -397,7 +455,7 @@ describe('plugin: network breadcrumbs', () => { }) it('should handle a fetch(request, { method })', (done) => { - const window = { XMLHttpRequest, fetch } as unknown as Window & typeof globalThis + const window = { XMLHttpRequest, WeakMap, fetch } as unknown as Window & typeof globalThis p = plugin([], window) const client = new Client({ apiKey: 'aaaa-aaaa-aaaa-aaaa', plugins: [p] }) @@ -420,7 +478,7 @@ describe('plugin: network breadcrumbs', () => { }) it('should handle a fetch(request, { method: null })', (done) => { - const window = { XMLHttpRequest, fetch } as unknown as Window & typeof globalThis + const window = { XMLHttpRequest, WeakMap, fetch } as unknown as Window & typeof globalThis p = plugin([], window) const client = new Client({ apiKey: 'aaaa-aaaa-aaaa-aaaa', plugins: [p] }) @@ -443,7 +501,7 @@ describe('plugin: network breadcrumbs', () => { }) it('should handle a fetch(request, { method: undefined })', (done) => { - const window = { XMLHttpRequest, fetch } as unknown as Window & typeof globalThis + const window = { XMLHttpRequest, WeakMap, fetch } as unknown as Window & typeof globalThis p = plugin([], window) const client = new Client({ apiKey: 'aaaa-aaaa-aaaa-aaaa', plugins: [p] }) @@ -466,7 +524,7 @@ describe('plugin: network breadcrumbs', () => { }) it('should leave a breadcrumb when a fetch() has a failed response', (done) => { - const window = { XMLHttpRequest, fetch } as unknown as Window & typeof globalThis + const window = { XMLHttpRequest, WeakMap, fetch } as unknown as Window & typeof globalThis p = plugin([], window) const client = new Client({ apiKey: 'aaaa-aaaa-aaaa-aaaa', plugins: [p] }) @@ -489,7 +547,7 @@ describe('plugin: network breadcrumbs', () => { }) it('should leave a breadcrumb when a fetch() has a network error', (done) => { - const window = { XMLHttpRequest, fetch } as unknown as Window & typeof globalThis + const window = { XMLHttpRequest, WeakMap, fetch } as unknown as Window & typeof globalThis p = plugin([], window) const client = new Client({ apiKey: 'aaaa-aaaa-aaaa-aaaa', plugins: [p] }) @@ -511,7 +569,7 @@ describe('plugin: network breadcrumbs', () => { }) it('should not be enabled when enabledBreadcrumbTypes=[]', () => { - const window = { XMLHttpRequest } as unknown as Window & typeof globalThis + const window = { XMLHttpRequest, WeakMap } as unknown as Window & typeof globalThis p = plugin([], window) const client = new Client({ apiKey: 'aaaa-aaaa-aaaa-aaaa', enabledBreadcrumbTypes: [], plugins: [p] }) @@ -524,7 +582,7 @@ describe('plugin: network breadcrumbs', () => { }) it('should be enabled when enabledBreadcrumbTypes=["request"]', () => { - const window = { XMLHttpRequest } as unknown as Window & typeof globalThis + const window = { XMLHttpRequest, WeakMap } as unknown as Window & typeof globalThis p = plugin([], window) const client = new Client({ apiKey: 'aaaa-aaaa-aaaa-aaaa', enabledBreadcrumbTypes: ['request'], plugins: [p] }) @@ -537,7 +595,7 @@ describe('plugin: network breadcrumbs', () => { }) it('should be enabled when enabledBreadcrumbTypes=null', () => { - const window = { XMLHttpRequest } as unknown as Window & typeof globalThis + const window = { XMLHttpRequest, WeakMap } as unknown as Window & typeof globalThis p = plugin([], window) const client = new Client({ apiKey: 'aaaa-aaaa-aaaa-aaaa', enabledBreadcrumbTypes: null, plugins: [p] }) @@ -550,7 +608,7 @@ describe('plugin: network breadcrumbs', () => { }) it('should strip query string data before checking a url is ignored', () => { - const window = { XMLHttpRequest } as unknown as Window & typeof globalThis + const window = { XMLHttpRequest, WeakMap } as unknown as Window & typeof globalThis p = plugin(['/ignoreme'], window) const client = new Client({ apiKey: 'aaaa-aaaa-aaaa-aaaa', plugins: [p] }) diff --git a/test/browser/features/fixtures/network_breadcrumbs/script/xhr_failure.html b/test/browser/features/fixtures/network_breadcrumbs/script/xhr_failure.html index 6c02f4632b..92351225a4 100644 --- a/test/browser/features/fixtures/network_breadcrumbs/script/xhr_failure.html +++ b/test/browser/features/fixtures/network_breadcrumbs/script/xhr_failure.html @@ -32,7 +32,7 @@ 
PENDING
diff --git a/test/browser/features/fixtures/network_breadcrumbs/script/xhr_success.html b/test/browser/features/fixtures/network_breadcrumbs/script/xhr_success.html index 64c4cb93e8..d4efa11860 100644 --- a/test/browser/features/fixtures/network_breadcrumbs/script/xhr_success.html +++ b/test/browser/features/fixtures/network_breadcrumbs/script/xhr_success.html @@ -31,7 +31,7 @@ 
PENDING