fix(browser): Set : as a part of gecko protocol regex group. (#4153)

onurtemizkan · web-flow · commit cb925e38f2f4 · 2022-08-02T08:41:43.000-07:00
Fixes: #4138 The original issue about `file` keyword also happens for everything in that group, such as `http`, `blob` and so on. And the problem originates from : `.*` between that group and `:`. It seems that `:` follows those keywords without anything in between, as far as I have seen from the tests. Removing that from the regex solved the issue without breaking any tests other than `safari-extension` and `safari-web-extension`, which are special-cased in https://github.com/getsentry/sentry-javascript/blob/d2e0cc4d683364dc500a681631b5be7df40ffec6/packages/browser/src/stack-parsers.ts#L160, and adding those two to the matching group also solved the issue of an extra `:` coming from https://github.com/getsentry/sentry-javascript/blob/d2e0cc4d683364dc500a681631b5be7df40ffec6/packages/browser/src/stack-parsers.ts#L165.
diff --git a/packages/browser/src/stack-parsers.ts b/packages/browser/src/stack-parsers.ts
@@ -67,7 +67,7 @@ export const chromeStackLineParser: StackLineParser = [CHROME_PRIORITY, chrome];
 // generates filenames without a prefix like `file://` the filenames in the stacktrace are just 42.js
 // We need this specific case for now because we want no other regex to match.
 const geckoREgex =
-  /^\s*(.*?)(?:\((.*?)\))?(?:^|@)?((?:file|https?|blob|chrome|webpack|resource|moz-extension|capacitor).*?:\/.*?|\[native code\]|[^@]*(?:bundle|\d+\.js)|\/[\w\-. /=]+)(?::(\d+))?(?::(\d+))?\s*$/i;
+  /^\s*(.*?)(?:\((.*?)\))?(?:^|@)?((?:file|https?|blob|chrome|webpack|resource|moz-extension|safari-extension|safari-web-extension|capacitor)?:\/.*?|\[native code\]|[^@]*(?:bundle|\d+\.js)|\/[\w\-. /=]+)(?::(\d+))?(?::(\d+))?\s*$/i;
 const geckoEvalRegex = /(\S+) line (\d+)(?: > eval line \d+)* > eval/i;
 
 const gecko: StackLineParserFn = line => {
diff --git a/packages/browser/test/unit/tracekit/firefox.test.ts b/packages/browser/test/unit/tracekit/firefox.test.ts
@@ -310,4 +310,50 @@ describe('Tracekit - Firefox Tests', () => {
       },
     });
   });
+
+  it('should parse Firefox errors with `file` inside an identifier', () => {
+    const FIREFOX_FILE_IN_IDENTIFIER = {
+      stack:
+        'us@https://www.random_website.com/vendor.d1cae9cfc9917df88de7.js:1:296021\n' +
+        'detectChanges@https://www.random_website.com/vendor.d1cae9cfc9917df88de7.js:1:333807\n' +
+        'handleProfileResult@https://www.random_website.com/main.4a4119c3cdfd10266d84.js:146:1018410\n',
+      fileName: 'https://www.random_website.com/main.4a4119c3cdfd10266d84.js',
+      lineNumber: 5529,
+      columnNumber: 16,
+      message: 'this.props.raw[this.state.dataSource].rows is undefined',
+      name: 'TypeError',
+    };
+
+    const stacktrace = exceptionFromError(parser, FIREFOX_FILE_IN_IDENTIFIER);
+
+    expect(stacktrace).toEqual({
+      stacktrace: {
+        frames: [
+          {
+            colno: 1018410,
+            filename: 'https://www.random_website.com/main.4a4119c3cdfd10266d84.js',
+            function: 'handleProfileResult',
+            in_app: true,
+            lineno: 146,
+          },
+          {
+            colno: 333807,
+            filename: 'https://www.random_website.com/vendor.d1cae9cfc9917df88de7.js',
+            function: 'detectChanges',
+            in_app: true,
+            lineno: 1,
+          },
+          {
+            colno: 296021,
+            filename: 'https://www.random_website.com/vendor.d1cae9cfc9917df88de7.js',
+            function: 'us',
+            in_app: true,
+            lineno: 1,
+          },
+        ],
+      },
+      type: 'TypeError',
+      value: 'this.props.raw[this.state.dataSource].rows is undefined',
+    });
+  });
 });
diff --git a/packages/integration-tests/suites/stacktraces/init.js b/packages/integration-tests/suites/stacktraces/init.js
@@ -0,0 +1,7 @@
+import * as Sentry from '@sentry/browser';
+
+window.Sentry = Sentry;
+
+Sentry.init({
+  dsn: 'https://public@dsn.ingest.sentry.io/1337',
+});
diff --git a/packages/integration-tests/suites/stacktraces/protocol_containing_fn_identifiers/subject.js b/packages/integration-tests/suites/stacktraces/protocol_containing_fn_identifiers/subject.js
@@ -0,0 +1,27 @@
+function httpsCall() {
+  webpackDevServer();
+}
+
+const webpackDevServer = () => {
+  Response.httpCode();
+};
+
+class Response {
+  constructor() {}
+
+  static httpCode(params) {
+    throw new Error('test_err');
+  }
+}
+
+const decodeBlob = function() {
+  (function readFile() {
+    httpsCall();
+  })();
+};
+
+try {
+  decodeBlob();
+} catch (err) {
+  Sentry.captureException(err);
+}
diff --git a/packages/integration-tests/suites/stacktraces/protocol_containing_fn_identifiers/test.ts b/packages/integration-tests/suites/stacktraces/protocol_containing_fn_identifiers/test.ts
@@ -0,0 +1,65 @@
+import { expect } from '@playwright/test';
+import { Event } from '@sentry/types';
+
+import { sentryTest } from '../../../utils/fixtures';
+import { getFirstSentryEnvelopeRequest } from '../../../utils/helpers';
+
+sentryTest(
+  'should parse function identifiers that contain protocol names correctly',
+  async ({ getLocalTestPath, page, runInChromium, runInFirefox, runInWebkit }) => {
+    const url = await getLocalTestPath({ testDir: __dirname });
+
+    const eventData = await getFirstSentryEnvelopeRequest<Event>(page, url);
+    const frames = eventData.exception?.values?.[0].stacktrace?.frames;
+
+    runInChromium(() => {
+      expect(frames).toMatchObject([
+        { function: '?' },
+        { function: '?' },
+        { function: 'decodeBlob' },
+        { function: 'readFile' },
+        { function: 'httpsCall' },
+        { function: 'webpackDevServer' },
+        { function: 'Function.httpCode' },
+      ]);
+    });
+
+    runInFirefox(() => {
+      expect(frames).toMatchObject([
+        { function: '?' },
+        { function: '?' },
+        { function: 'decodeBlob' },
+        { function: 'readFile' },
+        { function: 'httpsCall' },
+        { function: 'webpackDevServer' },
+        { function: 'httpCode' },
+      ]);
+    });
+
+    runInWebkit(() => {
+      expect(frames).toMatchObject([
+        { function: 'global code' },
+        { function: '?' },
+        { function: 'decodeBlob' },
+        { function: 'readFile' },
+        { function: 'httpsCall' },
+        { function: 'webpackDevServer' },
+        { function: 'httpCode' },
+      ]);
+    });
+  },
+);
+
+sentryTest(
+  'should not add any part of the function identifier to beginning of filename',
+  async ({ getLocalTestPath, page }) => {
+    const url = await getLocalTestPath({ testDir: __dirname });
+
+    const eventData = await getFirstSentryEnvelopeRequest<Event>(page, url);
+
+    expect(eventData.exception?.values?.[0].stacktrace?.frames).toMatchObject(
+      // specifically, we're trying to avoid values like `Blob@file://path/to/file` in frames with function names like `makeBlob`
+      Array(7).fill({ filename: expect.stringMatching(/^file:\/?/) }),
+    );
+  },
+);
diff --git a/packages/integration-tests/suites/stacktraces/protocol_fn_identifiers/subject.js b/packages/integration-tests/suites/stacktraces/protocol_fn_identifiers/subject.js
@@ -0,0 +1,27 @@
+function https() {
+  webpack();
+}
+
+const webpack = () => {
+  File.http();
+};
+
+class File {
+  constructor() {}
+
+  static http(params) {
+    throw new Error('test_err');
+  }
+}
+
+const blob = function() {
+  (function file() {
+    https();
+  })();
+};
+
+try {
+  blob();
+} catch (err) {
+  Sentry.captureException(err);
+}
diff --git a/packages/integration-tests/suites/stacktraces/protocol_fn_identifiers/test.ts b/packages/integration-tests/suites/stacktraces/protocol_fn_identifiers/test.ts
@@ -0,0 +1,64 @@
+import { expect } from '@playwright/test';
+import { Event } from '@sentry/types';
+
+import { sentryTest } from '../../../utils/fixtures';
+import { getFirstSentryEnvelopeRequest } from '../../../utils/helpers';
+
+sentryTest(
+  'should parse function identifiers that are protocol names correctly',
+  async ({ getLocalTestPath, page, runInChromium, runInFirefox, runInWebkit }) => {
+    const url = await getLocalTestPath({ testDir: __dirname });
+
+    const eventData = await getFirstSentryEnvelopeRequest<Event>(page, url);
+    const frames = eventData.exception?.values?.[0].stacktrace?.frames;
+
+    runInChromium(() => {
+      expect(frames).toMatchObject([
+        { function: '?' },
+        { function: '?' },
+        { function: 'blob' },
+        { function: 'file' },
+        { function: 'https' },
+        { function: 'webpack' },
+        { function: 'Function.http' },
+      ]);
+    });
+
+    runInFirefox(() => {
+      expect(frames).toMatchObject([
+        { function: '?' },
+        { function: '?' },
+        { function: 'blob' },
+        { function: 'file' },
+        { function: 'https' },
+        { function: 'webpack' },
+        { function: 'http' },
+      ]);
+    });
+
+    runInWebkit(() => {
+      expect(frames).toMatchObject([
+        { function: 'global code' },
+        { function: '?' },
+        { function: 'blob' },
+        { function: 'file' },
+        { function: 'https' },
+        { function: 'webpack' },
+        { function: 'http' },
+      ]);
+    });
+  },
+);
+
+sentryTest(
+  'should not add any part of the function identifier to beginning of filename',
+  async ({ getLocalTestPath, page }) => {
+    const url = await getLocalTestPath({ testDir: __dirname });
+
+    const eventData = await getFirstSentryEnvelopeRequest<Event>(page, url);
+
+    expect(eventData.exception?.values?.[0].stacktrace?.frames).toMatchObject(
+      Array(7).fill({ filename: expect.stringMatching(/^file:\/?/) }),
+    );
+  },
+);
diff --git a/packages/integration-tests/suites/stacktraces/regular_fn_identifiers/subject.js b/packages/integration-tests/suites/stacktraces/regular_fn_identifiers/subject.js
@@ -0,0 +1,29 @@
+function foo() {
+  bar();
+}
+
+const bar = () => {
+  Test.baz();
+};
+
+class Test {
+  constructor() {}
+
+  static baz(params) {
+    throw new Error('test_err');
+  }
+}
+
+const qux = function() {
+  (() => {
+    (function() {
+      foo();
+    })();
+  })();
+};
+
+try {
+  qux();
+} catch (err) {
+  Sentry.captureException(err);
+}
diff --git a/packages/integration-tests/suites/stacktraces/regular_fn_identifiers/test.ts b/packages/integration-tests/suites/stacktraces/regular_fn_identifiers/test.ts
@@ -0,0 +1,68 @@
+import { expect } from '@playwright/test';
+import { Event } from '@sentry/types';
+
+import { sentryTest } from '../../../utils/fixtures';
+import { getFirstSentryEnvelopeRequest } from '../../../utils/helpers';
+
+sentryTest(
+  'should parse function identifiers correctly',
+  async ({ getLocalTestPath, page, runInChromium, runInFirefox, runInWebkit }) => {
+    const url = await getLocalTestPath({ testDir: __dirname });
+
+    const eventData = await getFirstSentryEnvelopeRequest<Event>(page, url);
+    const frames = eventData.exception?.values?.[0].stacktrace?.frames;
+
+    runInChromium(() => {
+      expect(frames).toMatchObject([
+        { function: '?' },
+        { function: '?' },
+        { function: 'qux' },
+        { function: '?' },
+        { function: '?' },
+        { function: 'foo' },
+        { function: 'bar' },
+        { function: 'Function.baz' },
+      ]);
+    });
+
+    runInFirefox(() => {
+      expect(frames).toMatchObject([
+        { function: '?' },
+        { function: '?' },
+        { function: 'qux' },
+        { function: 'qux/<' },
+        { function: 'qux/</<' },
+        { function: 'foo' },
+        { function: 'bar' },
+        { function: 'baz' },
+      ]);
+    });
+
+    runInWebkit(() => {
+      expect(frames).toMatchObject([
+        { function: 'global code' },
+        { function: '?' },
+        { function: 'qux' },
+        { function: '?' },
+        { function: '?' },
+        { function: 'foo' },
+        { function: 'bar' },
+        { function: 'baz' },
+      ]);
+    });
+  },
+);
+
+sentryTest(
+  'should not add any part of the function identifier to beginning of filename',
+  async ({ getLocalTestPath, page }) => {
+    const url = await getLocalTestPath({ testDir: __dirname });
+
+    const eventData = await getFirstSentryEnvelopeRequest<Event>(page, url);
+
+    expect(eventData.exception?.values?.[0].stacktrace?.frames).toMatchObject(
+      // specifically, we're trying to avoid values like `Blob@file://path/to/file` in frames with function names like `makeBlob`
+      Array(8).fill({ filename: expect.stringMatching(/^file:\/?/) }),
+    );
+  },
+);
diff --git a/packages/integration-tests/suites/stacktraces/template.hbs b/packages/integration-tests/suites/stacktraces/template.hbs
@@ -0,0 +1,11 @@
+<!DOCTYPE html>
+<html>
+  <head>
+    <meta charset="utf-8" />
+    <title></title>
+  <script src="{{htmlWebpackPlugin.options.initialization}}"></script>
+  </head>
+  <body>
+    <script src="{{htmlWebpackPlugin.options.subject}}"></script>
+  </body>
+</html>
diff --git a/packages/integration-tests/utils/fixtures.ts b/packages/integration-tests/utils/fixtures.ts

-Original file line number
+Diff line change
@@ @@ -0,0 +1,7 @@ @@
 +import * as Sentry from '@sentry/browser';
++
 +window.Sentry = Sentry;
++
 +Sentry.init({
 +  dsn: 'https://[email protected]/1337',
 +});