ref: Rework XHR wrapping logic to make sure it always triggers

kamilogorek · kamilogorek · commit 973cfde4a774 · 2020-02-19T15:04:57.000+01:00
diff --git a/packages/apm/src/integrations/tracing.ts b/packages/apm/src/integrations/tracing.ts
@@ -83,6 +83,7 @@ interface Activity {
 }
 
 const global = getGlobalObject<Window>();
+const defaultTracingOrigins = ['localhost', /^\//];
 
 /**
  * Tracing Integration
@@ -119,13 +120,14 @@ export class Tracing implements Integration {
 
   private static _debounce: number = 0;
 
+  private readonly _emitOptionsWarning: boolean = false;
+
   /**
    * Constructor for Tracing
    *
    * @param _options TracingOptions
    */
   public constructor(private readonly _options?: Partial<TracingOptions>) {
-    const defaultTracingOrigins = ['localhost', /^\//];
     const defaults = {
       idleTimeout: 500,
       maxTransactionDuration: 600,
@@ -142,11 +144,9 @@ export class Tracing implements Integration {
       tracesSampleRate: 1,
       tracingOrigins: defaultTracingOrigins,
     };
+    // NOTE: Logger doesn't work in contructors, as it's initialized after integrations instances
     if (!_options || !Array.isArray(_options.tracingOrigins) || _options.tracingOrigins.length === 0) {
-      logger.warn(
-        'Sentry: You need to define `tracingOrigins` in the options. Set an array of urls or patterns to trace.',
-      );
-      logger.warn(`Sentry: We added a reasonable default for you: ${defaultTracingOrigins}`);
+      this._emitOptionsWarning = true;
     }
     Tracing.options = this._options = {
       ...defaults,
@@ -160,6 +160,13 @@ export class Tracing implements Integration {
   public setupOnce(addGlobalEventProcessor: (callback: EventProcessor) => void, getCurrentHub: () => Hub): void {
     Tracing._getCurrentHub = getCurrentHub;
 
+    if (this._emitOptionsWarning) {
+      logger.warn(
+        'Sentry: You need to define `tracingOrigins` in the options. Set an array of urls or patterns to trace.',
+      );
+      logger.warn(`Sentry: We added a reasonable default for you: ${defaultTracingOrigins}`);
+    }
+
     if (!Tracing._isEnabled()) {
       return;
     }
diff --git a/packages/browser/src/integrations/trycatch.ts b/packages/browser/src/integrations/trycatch.ts
@@ -3,7 +3,7 @@ import { fill, getFunctionName, getGlobalObject } from '@sentry/utils';
 
 import { wrap } from '../helpers';
 
-type XMLHttpRequestProp = 'onload' | 'onerror' | 'onprogress';
+type XMLHttpRequestProp = 'onload' | 'onerror' | 'onprogress' | 'onreadystatechange';
 
 /** Wrap timer functions and event targets to catch errors and provide better meta data */
 export class TryCatch implements Integration {
@@ -133,12 +133,12 @@ export class TryCatch implements Integration {
   private _wrapXHR(originalSend: () => void): () => void {
     return function(this: XMLHttpRequest, ...args: any[]): void {
       const xhr = this; // tslint:disable-line:no-this-assignment
-      const xmlHttpRequestProps: XMLHttpRequestProp[] = ['onload', 'onerror', 'onprogress'];
+      const xmlHttpRequestProps: XMLHttpRequestProp[] = ['onload', 'onerror', 'onprogress', 'onreadystatechange'];
 
       xmlHttpRequestProps.forEach(prop => {
-        if (prop in this && typeof this[prop] === 'function') {
-          fill(this, prop, original =>
-            wrap(original, {
+        if (prop in xhr && typeof xhr[prop] === 'function') {
+          fill(xhr, prop, function(original: WrappedFunction): Function {
+            const wrapOptions = {
               mechanism: {
                 data: {
                   function: prop,
@@ -147,33 +147,18 @@ export class TryCatch implements Integration {
                 handled: true,
                 type: 'instrument',
               },
-            }),
-          );
-        }
-      });
-
-      if ('onreadystatechange' in xhr && typeof xhr.onreadystatechange === 'function') {
-        fill(xhr, 'onreadystatechange', function(original: WrappedFunction): Function {
-          const wrapOptions = {
-            mechanism: {
-              data: {
-                function: 'onreadystatechange',
-                handler: getFunctionName(original),
-              },
-              handled: true,
-              type: 'instrument',
-            },
-          };
+            };
 
-          // If Instrument integration has been called before TryCatch, get the name of original function
-          if (original.__sentry_original__) {
-            wrapOptions.mechanism.data.handler = getFunctionName(original.__sentry_original__);
-          }
+            // If Instrument integration has been called before TryCatch, get the name of original function
+            if (original.__sentry_original__) {
+              wrapOptions.mechanism.data.handler = getFunctionName(original.__sentry_original__);
+            }
 
-          // Otherwise wrap directly
-          return wrap(original, wrapOptions);
-        });
-      }
+            // Otherwise wrap directly
+            return wrap(original, wrapOptions);
+          });
+        }
+      });
 
       return originalSend.apply(this, args);
     };
diff --git a/packages/browser/test/integration/suites/builtins.js b/packages/browser/test/integration/suites/builtins.js
@@ -118,18 +118,28 @@ describe("wrapped built-ins", function() {
     });
   });
 
-  it("should not call XMLHttpRequest onreadystatechange more than once", function() {
+  it("should not call XMLHttpRequest onreadystatechange more than once per state", function() {
     return runInSandbox(sandbox, { manual: true }, function() {
-      window.calls = 0;
+      window.calls = {};
       var xhr = new XMLHttpRequest();
       xhr.open("GET", "/base/subjects/example.json");
       xhr.onreadystatechange = function wat() {
-        window.finalizeManualTest();
-        window.calls += 1;
+        window.calls[xhr.readyState] = window.calls[xhr.readyState]
+          ? window.calls[xhr.readyState] + 1
+          : 1;
+        if (xhr.readyState === 4) {
+          window.finalizeManualTest();
+        }
       };
       xhr.send();
     }).then(function(summary) {
-      assert.equal(summary.window.calls, 3);
+      for (var state in summary.window.calls) {
+        assert.equal(summary.window.calls[state], 1);
+      }
+      // IE Triggers all states (1-4), including 1 (open), despite it being assigned before
+      // the `onreadystatechange` handler.
+      assert.isAtLeast(Object.keys(summary.window.calls).length, 3);
+      assert.isAtMost(Object.keys(summary.window.calls).length, 4);
       delete summary.window.calls;
     });
   });
diff --git a/packages/utils/src/instrument.ts b/packages/utils/src/instrument.ts
@@ -230,10 +230,7 @@ function instrumentXHR(): void {
         ...commonHandlerData,
       });
 
-      /**
-       * @hidden
-       */
-      function onreadystatechangeHandler(): void {
+      xhr.addEventListener('readystatechange', function() {
         if (xhr.readyState === 4) {
           try {
             // touching statusCode in some platforms throws
@@ -249,20 +246,7 @@ function instrumentXHR(): void {
             endTimestamp: Date.now(),
           });
         }
-      }
-
-      if ('onreadystatechange' in xhr && typeof xhr.onreadystatechange === 'function') {
-        fill(xhr, 'onreadystatechange', function(original: WrappedFunction): Function {
-          return function(...readyStateArgs: any[]): void {
-            onreadystatechangeHandler();
-            return original.apply(xhr, readyStateArgs);
-          };
-        });
-      } else {
-        // if onreadystatechange wasn't actually set by the page on this xhr, we
-        // are free to set our own and capture the breadcrumb
-        xhr.onreadystatechange = onreadystatechangeHandler;
-      }
+      });
 
       return originalSend.apply(this, args);
     };
