fix(tracing): ignore the xhr/fetch response if its request is not being tracked (#4428)

Author: datbth

packages/tracing/src/browser/request.ts

@@ -141,8 +141,11 @@ export function fetchCallback(
     return;
   }
 
-  if (handlerData.endTimestamp && handlerData.fetchData.__span) {
-    const span = spans[handlerData.fetchData.__span];
+  if (handlerData.endTimestamp) {
+    const spanId = handlerData.fetchData.__span;
+    if (!spanId) return;
+
+    const span = spans[spanId];
     if (span) {
       if (handlerData.response) {
         // TODO (kmclb) remove this once types PR goes through
@@ -154,7 +157,7 @@ export function fetchCallback(
       span.finish();
 
       // eslint-disable-next-line @typescript-eslint/no-dynamic-delete
-      delete spans[handlerData.fetchData.__span];
+      delete spans[spanId];
     }
     return;
   }
@@ -216,14 +219,17 @@ export function xhrCallback(
   const xhr = handlerData.xhr.__sentry_xhr__;
 
   // check first if the request has finished and is tracked by an existing span which should now end
-  if (handlerData.endTimestamp && handlerData.xhr.__sentry_xhr_span_id__) {
-    const span = spans[handlerData.xhr.__sentry_xhr_span_id__];
+  if (handlerData.endTimestamp) {
+    const spanId = handlerData.xhr.__sentry_xhr_span_id__;
+    if (!spanId) return;
+
+    const span = spans[spanId];
     if (span) {
       span.setHttpStatus(xhr.status_code);
       span.finish();
 
       // eslint-disable-next-line @typescript-eslint/no-dynamic-delete
-      delete spans[handlerData.xhr.__sentry_xhr_span_id__];
+      delete spans[spanId];
     }
     return;
   }

packages/tracing/test/browser/request.test.ts

@@ -48,10 +48,12 @@ describe('callbacks', () => {
   let transaction: Transaction;
   const alwaysCreateSpan = () => true;
   const neverCreateSpan = () => false;
+  const startTimestamp = 1356996072000;
+  const endTimestamp = 1356996072000;
   const fetchHandlerData: FetchData = {
     args: ['http://dogs.are.great/', {}],
     fetchData: { url: 'http://dogs.are.great/', method: 'GET' },
-    startTimestamp: 1356996072000,
+    startTimestamp,
   };
   const xhrHandlerData: XHRData = {
     xhr: {
@@ -66,9 +68,8 @@ describe('callbacks', () => {
       // setRequestHeader: XMLHttpRequest.prototype.setRequestHeader,
       setRequestHeader,
     },
-    startTimestamp: 1353501072000,
+    startTimestamp,
   };
-  const endTimestamp = 1356996072000;
 
   beforeAll(() => {
     hub = new Hub(new BrowserClient({ tracesSampleRate: 1 }));
@@ -173,6 +174,23 @@ describe('callbacks', () => {
       expect(newSpan!.status).toBe(spanStatusfromHttpCode(404));
     });
 
+    it('ignores response with no associated span', () => {
+      // the request might be missed somehow. E.g. if it was sent before tracing gets enabled.
+
+      const postRequestFetchHandlerData = {
+        ...fetchHandlerData,
+        endTimestamp,
+        response: { status: 404 } as Response,
+      };
+
+      // in that case, the response coming back will be ignored
+      fetchCallback(postRequestFetchHandlerData, alwaysCreateSpan, {});
+
+      const newSpan = transaction.spanRecorder?.spans[1];
+
+      expect(newSpan).toBeUndefined();
+    });
+
     it('adds sentry-trace header to fetch requests', () => {
       // TODO
     });
@@ -263,5 +281,26 @@ describe('callbacks', () => {
 
       expect(newSpan!.status).toBe(spanStatusfromHttpCode(404));
     });
+
+    it('ignores response with no associated span', () => {
+      // the request might be missed somehow. E.g. if it was sent before tracing gets enabled.
+
+      const postRequestXHRHandlerData = {
+        ...{
+          xhr: {
+            __sentry_xhr__: xhrHandlerData.xhr.__sentry_xhr__,
+          },
+        },
+        startTimestamp,
+        endTimestamp,
+      };
+
+      // in that case, the response coming back will be ignored
+      xhrCallback(postRequestXHRHandlerData, alwaysCreateSpan, {});
+
+      const newSpan = transaction.spanRecorder?.spans[1];
+
+      expect(newSpan).toBeUndefined();
+    });
   });
 });

packages/tracing/test/hub.test.ts

@@ -359,7 +359,7 @@ describe('Hub', () => {
       // TODO the way we dig out the headers to test them doesn't work on Node < 10
       testOnlyIfNodeVersionAtLeast(10)(
         'should propagate positive sampling decision to child transactions in XHR header',
-        () => {
+        async () => {
           const hub = new Hub(
             new BrowserClient({
               dsn: 'https://[email protected]/1121',
@@ -375,12 +375,12 @@ describe('Hub', () => {
           });
 
           const request = new XMLHttpRequest();
-          request.open('GET', '/chase-partners');
-
-          // mock a response having been received successfully (we have to do it in this roundabout way because readyState
-          // is readonly and changing it doesn't trigger a readystatechange event)
-          Object.defineProperty(request, 'readyState', { value: 4 });
-          request.dispatchEvent(new Event('readystatechange'));
+          await new Promise(resolve => {
+            request.timeout = 1;
+            request.onloadend = request.ontimeout = resolve;
+            request.open('GET', '/chase-partners');
+            request.send('');
+          });
 
           // this looks weird, it's true, but it's really just `request.impl.flag.requestHeaders` - it's just that the
           // `impl` key is a symbol rather than a string, and therefore needs to be referred to by reference rather than
@@ -401,7 +401,7 @@ describe('Hub', () => {
       // TODO the way we dig out the headers to test them doesn't work on Node < 10
       testOnlyIfNodeVersionAtLeast(10)(
         'should propagate negative sampling decision to child transactions in XHR header',
-        () => {
+        async () => {
           const hub = new Hub(
             new BrowserClient({
               dsn: 'https://[email protected]/1121',
@@ -417,12 +417,12 @@ describe('Hub', () => {
           });
 
           const request = new XMLHttpRequest();
-          request.open('GET', '/chase-partners');
-
-          // mock a response having been received successfully (we have to do it in this roundabout way because readyState
-          // is readonly and changing it doesn't trigger a readystatechange event)
-          Object.defineProperty(request, 'readyState', { value: 4 });
-          request.dispatchEvent(new Event('readystatechange'));
+          await new Promise(resolve => {
+            request.timeout = 1;
+            request.onloadend = request.ontimeout = resolve;
+            request.open('GET', '/chase-partners');
+            request.send('');
+          });
 
           // this looks weird, it's true, but it's really just `request.impl.flag.requestHeaders` - it's just that the
           // `impl` key is a symbol rather than a string, and therefore needs to be referred to by reference rather than