diff --git a/packages/browser-integration-tests/suites/public-api/instrumentation/setTimeout/subject.js b/packages/browser-integration-tests/suites/public-api/instrumentation/setTimeout/subject.js new file mode 100644 index 000000000000..cf5df02445bd --- /dev/null +++ b/packages/browser-integration-tests/suites/public-api/instrumentation/setTimeout/subject.js @@ -0,0 +1,5 @@ +function callback() { + throw new Error('setTimeout_error'); +} + +setTimeout(callback, 0); diff --git a/packages/browser-integration-tests/suites/public-api/instrumentation/setTimeout/test.ts b/packages/browser-integration-tests/suites/public-api/instrumentation/setTimeout/test.ts new file mode 100644 index 000000000000..808e15285cb1 --- /dev/null +++ b/packages/browser-integration-tests/suites/public-api/instrumentation/setTimeout/test.ts @@ -0,0 +1,24 @@ +import { expect } from '@playwright/test'; +import type { Event } from '@sentry/types'; + +import { sentryTest } from '../../../../utils/fixtures'; +import { getFirstSentryEnvelopeRequest } from '../../../../utils/helpers'; + +sentryTest('Instrumentation should capture errors in setTimeout', async ({ getLocalTestPath, page }) => { + const url = await getLocalTestPath({ testDir: __dirname }); + + const eventData = await getFirstSentryEnvelopeRequest(page, url); + + expect(eventData.exception?.values).toHaveLength(1); + expect(eventData.exception?.values?.[0]).toMatchObject({ + type: 'Error', + value: 'setTimeout_error', + mechanism: { + type: 'instrument', + handled: false, + }, + stacktrace: { + frames: expect.any(Array), + }, + }); +}); diff --git a/packages/browser-integration-tests/suites/public-api/instrumentation/setTimeoutFrozen/init.js b/packages/browser-integration-tests/suites/public-api/instrumentation/setTimeoutFrozen/init.js new file mode 100644 index 000000000000..573e4fdcb621 --- /dev/null +++ b/packages/browser-integration-tests/suites/public-api/instrumentation/setTimeoutFrozen/init.js @@ -0,0 +1,8 @@ +import * as Sentry from '@sentry/browser'; + +window.Sentry = Sentry; + +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + debug: true, +}); diff --git a/packages/browser-integration-tests/suites/public-api/instrumentation/setTimeoutFrozen/subject.js b/packages/browser-integration-tests/suites/public-api/instrumentation/setTimeoutFrozen/subject.js new file mode 100644 index 000000000000..29c9ca1f7594 --- /dev/null +++ b/packages/browser-integration-tests/suites/public-api/instrumentation/setTimeoutFrozen/subject.js @@ -0,0 +1,5 @@ +function callback() { + throw new Error('setTimeout_error'); +} + +setTimeout(Object.freeze(callback), 0); diff --git a/packages/browser-integration-tests/suites/public-api/instrumentation/setTimeoutFrozen/test.ts b/packages/browser-integration-tests/suites/public-api/instrumentation/setTimeoutFrozen/test.ts new file mode 100644 index 000000000000..91e82f8b1dcd --- /dev/null +++ b/packages/browser-integration-tests/suites/public-api/instrumentation/setTimeoutFrozen/test.ts @@ -0,0 +1,48 @@ +import { expect } from '@playwright/test'; +import type { Event } from '@sentry/types'; + +import { sentryTest } from '../../../../utils/fixtures'; +import { getFirstSentryEnvelopeRequest } from '../../../../utils/helpers'; + +sentryTest( + 'Instrumentation does not fail when using frozen callback for setTimeout', + async ({ getLocalTestPath, page }) => { + const bundleKey = process.env.PW_BUNDLE || ''; + const hasDebug = !bundleKey.includes('_min'); + + const url = await getLocalTestPath({ testDir: __dirname }); + + const logMessages: string[] = []; + + page.on('console', msg => { + logMessages.push(msg.text()); + }); + + const eventData = await getFirstSentryEnvelopeRequest(page, url); + + // It still captures the error + expect(eventData.exception?.values).toHaveLength(1); + expect(eventData.exception?.values?.[0]).toMatchObject({ + type: 'Error', + value: 'setTimeout_error', + mechanism: { + type: 'instrument', + handled: false, + }, + stacktrace: { + frames: expect.any(Array), + }, + }); + + // We only care about the message when debug is enabled + if (hasDebug) { + expect(logMessages).toEqual( + expect.arrayContaining([ + expect.stringContaining( + 'Sentry Logger [log]: Failed to add non-enumerable property "__sentry_wrapped__" to object function callback()', + ), + ]), + ); + } + }, +); diff --git a/packages/browser/test/unit/index.test.ts b/packages/browser/test/unit/index.test.ts index 95985a2a69d2..0b0df3c4300f 100644 --- a/packages/browser/test/unit/index.test.ts +++ b/packages/browser/test/unit/index.test.ts @@ -1,4 +1,5 @@ import { getReportDialogEndpoint, SDK_VERSION } from '@sentry/core'; +import type { WrappedFunction } from '@sentry/types'; import * as utils from '@sentry/utils'; import type { Event } from '../../src'; @@ -391,4 +392,14 @@ describe('wrap()', () => { expect(result2).toBe(42); }); + + it('should ignore frozen functions', () => { + const func = Object.freeze(() => 42); + + // eslint-disable-next-line deprecation/deprecation + wrap(func); + + expect(func()).toBe(42); + expect((func as WrappedFunction).__sentry_wrapped__).toBeUndefined(); + }); }); diff --git a/packages/utils/src/object.ts b/packages/utils/src/object.ts index 3a22e2aa49a3..e705214f950d 100644 --- a/packages/utils/src/object.ts +++ b/packages/utils/src/object.ts @@ -4,6 +4,7 @@ import type { WrappedFunction } from '@sentry/types'; import { htmlTreeAsString } from './browser'; import { isElement, isError, isEvent, isInstanceOf, isPlainObject, isPrimitive } from './is'; +import { logger } from './logger'; import { truncate } from './string'; /** @@ -28,12 +29,7 @@ export function fill(source: { [key: string]: any }, name: string, replacementFa // Make sure it's a function first, as we need to attach an empty prototype for `defineProperties` to work // otherwise it'll throw "TypeError: Object.defineProperties called on non-object" if (typeof wrapped === 'function') { - try { - markFunctionWrapped(wrapped, original); - } catch (_Oo) { - // This can throw if multiple fill happens on a global object like XMLHttpRequest - // Fixes https://github.com/getsentry/sentry-javascript/issues/2043 - } + markFunctionWrapped(wrapped, original); } source[name] = wrapped; @@ -47,12 +43,16 @@ export function fill(source: { [key: string]: any }, name: string, replacementFa * @param value The value to which to set the property */ export function addNonEnumerableProperty(obj: { [key: string]: unknown }, name: string, value: unknown): void { - Object.defineProperty(obj, name, { - // enumerable: false, // the default, so we can save on bundle size by not explicitly setting it - value: value, - writable: true, - configurable: true, - }); + try { + Object.defineProperty(obj, name, { + // enumerable: false, // the default, so we can save on bundle size by not explicitly setting it + value: value, + writable: true, + configurable: true, + }); + } catch (o_O) { + __DEBUG_BUILD__ && logger.log(`Failed to add non-enumerable property "${name}" to object`, obj); + } } /** @@ -63,9 +63,11 @@ export function addNonEnumerableProperty(obj: { [key: string]: unknown }, name: * @param original the original function that gets wrapped */ export function markFunctionWrapped(wrapped: WrappedFunction, original: WrappedFunction): void { - const proto = original.prototype || {}; - wrapped.prototype = original.prototype = proto; - addNonEnumerableProperty(wrapped, '__sentry_original__', original); + try { + const proto = original.prototype || {}; + wrapped.prototype = original.prototype = proto; + addNonEnumerableProperty(wrapped, '__sentry_original__', original); + } catch (o_O) {} // eslint-disable-line no-empty } /** diff --git a/packages/utils/test/object.test.ts b/packages/utils/test/object.test.ts index 3dad523ae4ce..5deac39e8cbc 100644 --- a/packages/utils/test/object.test.ts +++ b/packages/utils/test/object.test.ts @@ -2,7 +2,17 @@ * @jest-environment jsdom */ -import { dropUndefinedKeys, extractExceptionKeysForMessage, fill, objectify, urlEncode } from '../src/object'; +import type { WrappedFunction } from '@sentry/types'; + +import { + addNonEnumerableProperty, + dropUndefinedKeys, + extractExceptionKeysForMessage, + fill, + markFunctionWrapped, + objectify, + urlEncode, +} from '../src/object'; import { testOnlyIfNodeVersionAtLeast } from './testutils'; describe('fill()', () => { @@ -315,3 +325,95 @@ describe('objectify()', () => { expect(objectifiedNonPrimtive).toBe(notAPrimitive); }); }); + +describe('addNonEnumerableProperty', () => { + it('works with a plain object', () => { + const obj: { foo?: string } = {}; + addNonEnumerableProperty(obj, 'foo', 'bar'); + expect(obj.foo).toBe('bar'); + }); + + it('works with a class', () => { + class MyClass { + public foo?: string; + } + const obj = new MyClass(); + addNonEnumerableProperty(obj as any, 'foo', 'bar'); + expect(obj.foo).toBe('bar'); + }); + + it('works with a function', () => { + const func = jest.fn(); + addNonEnumerableProperty(func as any, 'foo', 'bar'); + expect((func as any).foo).toBe('bar'); + func(); + expect(func).toHaveBeenCalledTimes(1); + }); + + it('works with an existing property object', () => { + const obj = { foo: 'before' }; + addNonEnumerableProperty(obj, 'foo', 'bar'); + expect(obj.foo).toBe('bar'); + }); + + it('works with an existing readonly property object', () => { + const obj = { foo: 'before' }; + + Object.defineProperty(obj, 'foo', { + value: 'defined', + writable: false, + }); + + addNonEnumerableProperty(obj, 'foo', 'bar'); + expect(obj.foo).toBe('bar'); + }); + + it('does not error with a frozen object', () => { + const obj = Object.freeze({ foo: 'before' }); + + addNonEnumerableProperty(obj, 'foo', 'bar'); + expect(obj.foo).toBe('before'); + }); +}); + +describe('markFunctionWrapped', () => { + it('works with a function', () => { + const originalFunc = jest.fn(); + const wrappedFunc = jest.fn(); + markFunctionWrapped(wrappedFunc, originalFunc); + + expect((wrappedFunc as WrappedFunction).__sentry_original__).toBe(originalFunc); + + wrappedFunc(); + + expect(wrappedFunc).toHaveBeenCalledTimes(1); + expect(originalFunc).not.toHaveBeenCalled(); + }); + + it('works with a frozen original function', () => { + const originalFunc = Object.freeze(jest.fn()); + const wrappedFunc = jest.fn(); + markFunctionWrapped(wrappedFunc, originalFunc); + + expect((wrappedFunc as WrappedFunction).__sentry_original__).toBe(originalFunc); + + wrappedFunc(); + + expect(wrappedFunc).toHaveBeenCalledTimes(1); + expect(originalFunc).not.toHaveBeenCalled(); + }); + + it('works with a frozen wrapped function', () => { + const originalFunc = Object.freeze(jest.fn()); + const wrappedFunc = Object.freeze(jest.fn()); + markFunctionWrapped(wrappedFunc, originalFunc); + + // Skips adding the property, but also doesn't error + expect((wrappedFunc as WrappedFunction).__sentry_original__).toBe(undefined); + + wrappedFunc(); + + expect(wrappedFunc).toHaveBeenCalledTimes(1); + expect(originalFunc).not.toHaveBeenCalled(); + }); +});