diff --git a/dev-packages/browser-integration-tests/suites/public-api/startSpan/thenable-with-extra-methods/subject.js b/dev-packages/browser-integration-tests/suites/public-api/startSpan/thenable-with-extra-methods/subject.js new file mode 100644 index 000000000000..6a0a6c4cae5e --- /dev/null +++ b/dev-packages/browser-integration-tests/suites/public-api/startSpan/thenable-with-extra-methods/subject.js @@ -0,0 +1,79 @@ +/** + * Test that verifies thenable objects with extra methods (like jQuery's jqXHR) + * preserve those methods when returned from Sentry.startSpan(). + * + * Example case: + * const jqXHR = Sentry.startSpan({ name: "test" }, () => $.ajax(...)); + * jqXHR.abort(); // Should work and not throw an error because of missing abort() method + */ + +// Load jQuery from CDN +const script = document.createElement('script'); +script.src = 'https://code.jquery.com/jquery-3.7.1.min.js'; +script.integrity = 'sha256-/JqT3SQfawRcv/BIHPThkBvs0OEvtFFmqPF/lYI/Cxo='; +script.crossOrigin = 'anonymous'; + +script.onload = function () { + runTest(); +}; + +script.onerror = function () { + window.jqXHRTestError = 'Failed to load jQuery'; + window.jqXHRMethodsPreserved = false; +}; + +document.head.appendChild(script); + +async function runTest() { + window.jqXHRAbortCalled = false; + window.jqXHRAbortResult = null; + window.jqXHRTestError = null; + + try { + if (!window.jQuery) { + throw new Error('jQuery not loaded'); + } + + const result = Sentry.startSpan({ name: 'test-jqxhr', op: 'http.client' }, () => { + // Make a real AJAX request with jQuery + return window.jQuery.ajax({ + url: 'https://httpbin.org/status/200', + method: 'GET', + timeout: 5000, + }); + }); + + const hasAbort = typeof result.abort === 'function'; + const hasReadyState = 'readyState' in result; + + if (hasAbort && hasReadyState) { + try { + result.abort(); + window.jqXHRAbortCalled = true; + window.jqXHRAbortResult = 'abort-successful'; + window.jqXHRMethodsPreserved = true; + } catch (e) { + console.log('abort() threw an error:', e); + window.jqXHRTestError = `abort() failed: ${e.message}`; + window.jqXHRMethodsPreserved = false; + } + } else { + window.jqXHRMethodsPreserved = false; + window.jqXHRTestError = 'jqXHR methods not preserved'; + } + + // Since we aborted the request, it should be rejected + try { + await result; + window.jqXHRPromiseResolved = true; // Unexpected + } catch (err) { + // Expected: aborted request rejects + window.jqXHRPromiseResolved = false; + window.jqXHRPromiseRejected = true; + } + } catch (error) { + console.error('Test error:', error); + window.jqXHRTestError = error.message; + window.jqXHRMethodsPreserved = false; + } +} diff --git a/dev-packages/browser-integration-tests/suites/public-api/startSpan/thenable-with-extra-methods/test.ts b/dev-packages/browser-integration-tests/suites/public-api/startSpan/thenable-with-extra-methods/test.ts new file mode 100644 index 000000000000..44008a93a3e9 --- /dev/null +++ b/dev-packages/browser-integration-tests/suites/public-api/startSpan/thenable-with-extra-methods/test.ts @@ -0,0 +1,51 @@ +import { expect } from '@playwright/test'; +import { sentryTest } from '../../../../utils/fixtures'; +import { envelopeRequestParser, shouldSkipTracingTest, waitForTransactionRequest } from '../../../../utils/helpers'; + +sentryTest('preserves extra methods on real jQuery jqXHR objects', async ({ getLocalTestUrl, page }) => { + if (shouldSkipTracingTest()) { + sentryTest.skip(); + } + + const url = await getLocalTestUrl({ testDir: __dirname }); + const transactionPromise = waitForTransactionRequest(page); + + await page.goto(url); + + // Wait for jQuery to load + await page.waitForTimeout(1000); + + const methodsPreserved = await page.evaluate(() => (window as any).jqXHRMethodsPreserved); + expect(methodsPreserved).toBe(true); + + const abortCalled = await page.evaluate(() => (window as any).jqXHRAbortCalled); + expect(abortCalled).toBe(true); + + const abortReturnValue = await page.evaluate(() => (window as any).jqXHRAbortResult); + expect(abortReturnValue).toBe('abort-successful'); + + const testError = await page.evaluate(() => (window as any).jqXHRTestError); + expect(testError).toBeNull(); + + const transaction = envelopeRequestParser(await transactionPromise); + expect(transaction.transaction).toBe('test-jqxhr'); + expect(transaction.spans).toBeDefined(); +}); + +sentryTest('aborted request rejects promise correctly', async ({ getLocalTestUrl, page }) => { + if (shouldSkipTracingTest()) { + sentryTest.skip(); + } + + const url = await getLocalTestUrl({ testDir: __dirname }); + await page.goto(url); + + // Wait for jQuery to load + await page.waitForTimeout(1000); + + const promiseRejected = await page.evaluate(() => (window as any).jqXHRPromiseRejected); + expect(promiseRejected).toBe(true); + + const promiseResolved = await page.evaluate(() => (window as any).jqXHRPromiseResolved); + expect(promiseResolved).toBe(false); +}); diff --git a/packages/core/src/asyncContext/stackStrategy.ts b/packages/core/src/asyncContext/stackStrategy.ts index 87dc534fc636..36c1d2127530 100644 --- a/packages/core/src/asyncContext/stackStrategy.ts +++ b/packages/core/src/asyncContext/stackStrategy.ts @@ -1,6 +1,7 @@ import type { Client } from '../client'; import { getDefaultCurrentScope, getDefaultIsolationScope } from '../defaultScopes'; import { Scope } from '../scope'; +import { chainAndCopyPromiseLike } from '../utils/chain-and-copy-promiselike'; import { isThenable } from '../utils/is'; import { getMainCarrier, getSentryCarrier } from './../carrier'; import type { AsyncContextStrategy } from './types'; @@ -52,17 +53,11 @@ export class AsyncContextStack { } if (isThenable(maybePromiseResult)) { - // @ts-expect-error - isThenable returns the wrong type - return maybePromiseResult.then( - res => { - this._popScope(); - return res; - }, - e => { - this._popScope(); - throw e; - }, - ); + return chainAndCopyPromiseLike( + maybePromiseResult as PromiseLike> & Record, + () => this._popScope(), + () => this._popScope(), + ) as T; } this._popScope(); diff --git a/packages/core/src/utils/chain-and-copy-promiselike.ts b/packages/core/src/utils/chain-and-copy-promiselike.ts new file mode 100644 index 000000000000..d0cc130d4f3e --- /dev/null +++ b/packages/core/src/utils/chain-and-copy-promiselike.ts @@ -0,0 +1,43 @@ +/** + * Copy the properties from a decorated promiselike object onto its chained + * actual promise. + */ +export const chainAndCopyPromiseLike = & Record>( + original: T, + onSuccess: (value: V) => void, + onError: (e: unknown) => void, +): T => { + return copyProps( + original, + original.then( + value => { + onSuccess(value); + return value; + }, + err => { + onError(err); + throw err; + }, + ), + ) as T; +}; + +// eslint-disable-next-line @typescript-eslint/no-explicit-any +const copyProps = >(original: T, chained: T): T => { + for (const key in original) { + if (key in chained) continue; + const value = original[key]; + if (typeof value === 'function') { + Object.defineProperty(chained, key, { + value: (...args: unknown[]) => value.apply(original, args), + enumerable: true, + configurable: true, + writable: true, + }); + } else { + (chained as Record)[key] = value; + } + } + + return chained; +}; diff --git a/packages/core/src/utils/handleCallbackErrors.ts b/packages/core/src/utils/handleCallbackErrors.ts index 1a09e23a40aa..72fe475f7e4f 100644 --- a/packages/core/src/utils/handleCallbackErrors.ts +++ b/packages/core/src/utils/handleCallbackErrors.ts @@ -1,4 +1,5 @@ import { isThenable } from '../utils/is'; +import { chainAndCopyPromiseLike } from '../utils/chain-and-copy-promiselike'; /* eslint-disable */ // Vendor "Awaited" in to be TS 3.8 compatible @@ -62,7 +63,12 @@ export function handleCallbackErrors< * Maybe handle a promise rejection. * This expects to be given a value that _may_ be a promise, or any other value. * If it is a promise, and it rejects, it will call the `onError` callback. - * Other than this, it will generally return the given value as-is. + * + * For thenable objects with extra methods (like jQuery's jqXHR), + * this function preserves those methods by wrapping the original thenable in a Proxy + * that intercepts .then() calls to apply error handling while forwarding all other + * properties to the original object. + * This allows code like `startSpan(() => $.ajax(...)).abort()` to work correctly. */ function maybeHandlePromiseRejection( value: MaybePromise, @@ -71,22 +77,21 @@ function maybeHandlePromiseRejection( onSuccess: (result: MaybePromise | AwaitedPromise) => void, ): MaybePromise { if (isThenable(value)) { - // @ts-expect-error - the isThenable check returns the "wrong" type here - return value.then( - res => { + return chainAndCopyPromiseLike( + value as MaybePromise & PromiseLike> & Record, + (result) => { onFinally(); - onSuccess(res); - return res; + onSuccess(result as Awaited); }, - e => { - onError(e); - onFinally(); - throw e; + (err) => { + onError(err) + onFinally() }, - ); + ) as MaybePromise; + } else { + // Non-thenable value - call callbacks immediately and return as-is + onFinally(); + onSuccess(value); } - - onFinally(); - onSuccess(value); return value; } diff --git a/packages/core/test/lib/tracing/trace.test.ts b/packages/core/test/lib/tracing/trace.test.ts index 271cd669d56c..e7ea8bc8fa7e 100644 --- a/packages/core/test/lib/tracing/trace.test.ts +++ b/packages/core/test/lib/tracing/trace.test.ts @@ -210,6 +210,32 @@ describe('startSpan', () => { }); }); + describe('AsyncContext withScope promise integrity behavior', () => { + it('returns the original promise instance', async () => { + const original = Promise.resolve(42); + const result = startSpan({}, () => original); + expect(result).toBe(original); // New behavior + }); + + it('returns same instance on multiple calls', () => { + const p = Promise.resolve(1); + const result1 = startSpan({}, () => p); + const result2 = startSpan({}, () => p); + expect(result1).toBe(result2); + }); + + it('preserves custom thenable methods', async () => { + const jqXHR = { + then: Promise.resolve(1).then.bind(Promise.resolve(1)), + abort: vi.fn(), + }; + const result = startSpan({}, () => jqXHR); + expect(typeof result.abort).toBe('function'); + result.abort(); + expect(jqXHR.abort).toHaveBeenCalled(); + }); + }); + it('returns a non recording span if tracing is disabled', () => { const options = getDefaultTestClientOptions({}); client = new TestClient(options);