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

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;
     }

packages/browser/package.json

@@ -77,7 +77,7 @@
     "test:integration": "test/integration/run.js",
     "test:integration:watch": "test/integration/run.js --watch",
     "test:integration:checkbrowsers": "node scripts/checkbrowsers.js",
-    "test:manual": "node test/manual/npm-build.js && rm test/manual/tmp.js",
+    "test:package": "node test/package/npm-build.js && rm test/package/tmp.js",
     "size:check": "run-p size:check:es5 size:check:es6",
     "size:check:es5": "cat build/bundle.min.js | gzip -9 | wc -c | awk '{$1=$1/1024; print \"ES5: \",$1,\"kB\";}'",
     "size:check:es6": "cat build/bundle.es6.min.js | gzip -9 | wc -c | awk '{$1=$1/1024; print \"ES6: \",$1,\"kB\";}'",

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);
     };

packages/browser/test/integration/browsers.js

@@ -4,73 +4,81 @@
 module.exports = {
   bs_android_4: {
     base: "BrowserStack",
-    browser: "Android Browser",
-    device: "Google Nexus 5",
+    browser: "Android",
+    device: "Samsung Galaxy Note 4",
     os: "android",
     os_version: "4.4",
     real_mobile: true,
     browser_version: null,
   },
   bs_android_5: {
     base: "BrowserStack",
-    browser: "Android Browser",
-    device: "Google Nexus 9",
+    browser: "Android",
+    device: "Samsung Galaxy S6",
     os: "android",
-    os_version: "5.1",
+    os_version: "5.0",
     real_mobile: true,
     browser_version: null,
   },
   bs_android_6: {
     base: "BrowserStack",
-    browser: "Android Browser",
-    device: "Samsung Galaxy S7",
+    browser: "Android",
+    device: "Samsung Galaxy Note 4",
     os: "android",
     os_version: "6.0",
     real_mobile: true,
     browser_version: null,
   },
   bs_android_7: {
     base: "BrowserStack",
-    browser: "Android Browser",
-    device: "Samsung Galaxy S8",
+    browser: "Android",
+    device: "Samsung Galaxy Note 8",
     os: "android",
-    os_version: "7.0",
+    os_version: "7.1",
     real_mobile: true,
     browser_version: null,
   },
   bs_android_8: {
     base: "BrowserStack",
-    browser: "Android Browser",
-    device: "Samsung Galaxy S9",
+    browser: "Android",
+    device: "Samsung Galaxy Note 9",
     os: "android",
-    os_version: "8.0",
+    os_version: "8.1",
     real_mobile: true,
     browser_version: null,
   },
   bs_android_9: {
     base: "BrowserStack",
-    browser: "Android Browser",
-    device: "Samsung Galaxy S9 Plus",
+    browser: "Android",
+    device: "Samsung Galaxy Note 10 Plus",
     os: "android",
     os_version: "9.0",
     real_mobile: true,
     browser_version: null,
   },
-  bs_ios_11: {
+  bs_android_10: {
+    base: "BrowserStack",
+    browser: "Android",
+    device: "Google Pixel 4 XL",
+    os: "android",
+    os_version: "10.0",
+    real_mobile: true,
+  },
+  bs_ios_12: {
     base: "BrowserStack",
     browser: "Mobile Safari",
-    device: "iPhone 6",
+    device: "iPhone XS",
     os: "ios",
-    os_version: "11.4",
+    os_version: "12",
     real_mobile: true,
     browser_version: null,
   },
-  bs_ios_12: {
+  bs_ios_13: {
     base: "BrowserStack",
     browser: "Mobile Safari",
-    device: "iPhone 8",
+    device: "iPhone 11",
     os: "ios",
-    os_version: "12.1",
+    os_version: "13",
     real_mobile: true,
     browser_version: null,
   },
@@ -97,9 +105,7 @@ module.exports = {
     browser: "Safari",
     browser_version: "latest",
     os: "OS X",
-    os_version: "Mojave",
-    device: null,
-    real_mobile: null,
+    os_version: "Catalina",
   },
   bs_edge: {
     base: "BrowserStack",

packages/browser/test/integration/karma.conf.js

@@ -8,6 +8,16 @@ const isLocalRun =
 const customLaunchers = isLocalRun ? {} : require("./browsers.js");
 const browsers = isLocalRun ? ["ChromeHeadless"] : Object.keys(customLaunchers);
 
+// NOTE: It "should" work as a global `build` config option, but it doesn't, so setting it up
+// for each browser here, so that we have a nice distinction of when the tests were run exactly.
+if (!isLocalRun) {
+  for (const browser in customLaunchers) {
+    customLaunchers[browser].build = process.env.TRAVIS_BUILD_NUMBER
+      ? `Travis: ${process.env.TRAVIS_BUILD_NUMBER}`
+      : `Manual: ${new Date().toLocaleString()}`;
+  }
+}
+
 const plugins = [
   "karma-mocha",
   "karma-chai",
@@ -76,7 +86,6 @@ module.exports = config => {
         ui: "bdd",
       },
     },
-    build: process.env.TRAVIS_BUILD_NUMBER || Date.now(),
     concurrency: isLocalRun ? 1 : 2,
     retryLimit: 5,
     browserDisconnectTolerance: 5,

packages/browser/test/integration/suites/breadcrumbs.js

@@ -24,6 +24,39 @@ describe("breadcrumbs", function() {
     }
   );
 
+  it(
+    optional(
+      "should record an XMLHttpRequest with a handler attached after send was called",
+      IS_LOADER
+    ),
+    function() {
+      return runInSandbox(sandbox, { manual: true }, function() {
+        var xhr = new XMLHttpRequest();
+        xhr.open("GET", "/base/subjects/example.json");
+        xhr.send();
+        xhr.onreadystatechange = function() {
+          window.handlerCalled = true;
+        };
+        waitForXHR(xhr, function() {
+          Sentry.captureMessage("test");
+          window.finalizeManualTest();
+        });
+      }).then(function(summary) {
+        // The async loader doesn't wrap XHR
+        if (IS_LOADER) {
+          return;
+        }
+        assert.equal(summary.breadcrumbs.length, 1);
+        assert.equal(summary.breadcrumbs[0].type, "http");
+        assert.equal(summary.breadcrumbs[0].category, "xhr");
+        assert.equal(summary.breadcrumbs[0].data.method, "GET");
+        assert.typeOf(summary.breadcrumbs[0].timestamp, "number");
+        assert.isTrue(summary.window.handlerCalled);
+        delete summary.window.handlerCalled;
+      });
+    }
+  );
+
   it(
     optional(
       "should record an XMLHttpRequest without any handlers set",

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;
     });
   });

packages/utils/src/instrument.ts

@@ -230,10 +230,7 @@ function instrumentXHR(): void {
         ...commonHandlerData,
       });
 
-      /**
-       * @hidden
-       */
-      function onreadystatechangeHandler(): void {
+      xhr.addEventListener('readystatechange', function(): void {
         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);
     };

scripts/browser-integration.sh

@@ -6,5 +6,5 @@ yarn
 yarn build
 cd packages/browser
 yarn test:integration
-yarn test:manual
+yarn test:package