Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Correctly parse mixed chrome/node renderer stack traces #509

Merged
merged 3 commits into from
Jul 9, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

## Unreleased

- fix: Correctly parse mixed Chrome/node stack traces in the renderer (#509)

## 4.0.0-beta.1

Updating the underlying Sentry JavaScript SDK's to v7 forces a major version bump due to minor breaking changes in user
Expand Down
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@
"chai-subset": "^1.6.0",
"cross-env": "^7.0.3",
"eslint": "7.27.0",
"electron": "15.3.0",
"electron": "19.0.8",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Intentional?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this is the version used for unit tests. If we don't keep this updated we can get dependabot security alerts.

This does mean we only run the unit tests in one Electron version but the e2e tests should keep on top of any breakages between Electron versions.

"@electron/get": "^1.13.0",
"electron-mocha": "^11.0.0",
"electron-latest-versions": "^0.1.7",
Expand Down
5 changes: 5 additions & 0 deletions src/renderer/sdk.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import { logger } from '@sentry/utils';

import { ensureProcess } from '../common';
import { EventToMain, ScopeToMain } from './integrations';
import { electronRendererStackParser } from './stack-parse';

export const defaultIntegrations = [...defaultBrowserIntegrations, new ScopeToMain(), new EventToMain()];

Expand Down Expand Up @@ -40,6 +41,10 @@ If init has been called in the preload and contextIsolation is disabled, is not
options.defaultIntegrations = defaultIntegrations;
}

if (options.stackParser === undefined) {
options.stackParser = electronRendererStackParser;
}

if (options.dsn === undefined) {
// Events are sent via the main process but browser SDK wont start without dsn
options.dsn = 'https://12345@dummy.dsn/12345';
Expand Down
27 changes: 27 additions & 0 deletions src/renderer/stack-parse.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
import { chromeStackLineParser } from '@sentry/browser';
import { StackFrame, StackParser } from '@sentry/types';
import { dropUndefinedKeys, nodeStackLineParser, stripSentryFramesAndReverse } from '@sentry/utils';

const [, chrome] = chromeStackLineParser;
const [, node] = nodeStackLineParser();

/**
* A stack parser than combines Chrome and node.js parsers to give the best results even when nodeIntegration = true
*/
export const electronRendererStackParser: StackParser = (stack: string, skipFirst: number = 0): StackFrame[] => {
const frames: StackFrame[] = [];

for (const line of stack.split('\n').slice(skipFirst)) {
const chromeFrame = chrome(line);
const nodeFrame = node(line);

// We favour the chrome parser unless in_app == false
if (chromeFrame && nodeFrame?.in_app !== false) {
frames.push(chromeFrame);
} else if (nodeFrame) {
frames.push(dropUndefinedKeys(nodeFrame));
}
}

return stripSentryFramesAndReverse(frames);
};
4 changes: 2 additions & 2 deletions test/e2e/download.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { download as electronDownload } from '@electron/get';
import { downloadArtifact as electronDownload } from '@electron/get';
import electronExtract = require('extract-zip');
import { existsSync } from 'fs';
import { join } from 'path';
Expand Down Expand Up @@ -33,7 +33,7 @@ export async function downloadElectron(version: string): Promise<string> {
const dir = join(cacheDir, `${version}-x64`);

if (!existsSync(dir)) {
const zipPath = await electronDownload(version, { cacheRoot: cacheDir, downloadOptions: { arch: 'x64' } });
const zipPath = await electronDownload({ version, arch: 'x64', artifactName: 'electron' });
await electronExtract(zipPath, { dir });
}

Expand Down
147 changes: 147 additions & 0 deletions test/unit/stack-parse.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,147 @@
import { expect, should, use } from 'chai';
import chaiAsPromised = require('chai-as-promised');

import { electronRendererStackParser } from '../../src/renderer/stack-parse';

should();
use(chaiAsPromised);

describe('Parse mixed renderer stack traces', () => {
it('Electron v2', () => {
const stack = `Error: ENOENT: no such file or directory, open '/does-not-exist'
at Object.fs.openSync (fs.js:646:18)
at Object.module.(anonymous function) [as openSync] (ELECTRON_ASAR.js:166:20)
at fs.readFileSync (fs.js:551:33)
at fs.readFileSync (ELECTRON_ASAR.js:538:29)
at two (file:///Users/tim/Documents/Repositories/sentry-electron/test/e2e/dist/javascript-renderer/src/index.html:17:11)
at one (file:///Users/tim/Documents/Repositories/sentry-electron/test/e2e/dist/javascript-renderer/src/index.html:24:9)
at global.setTimeout (file:///Users/tim/Documents/Repositories/sentry-electron/test/e2e/dist/javascript-renderer/src/index.html:28:9)
at sentryWrapped (/Users/tim/Documents/Repositories/sentry-electron/test/e2e/dist/javascript-renderer/node_modules/@sentry/browser/cjs/helpers.js:87:17)`;

const frames = electronRendererStackParser(stack);

expect(frames).to.eql([
{
filename:
'file:///Users/tim/Documents/Repositories/sentry-electron/test/e2e/dist/javascript-renderer/src/index.html',
function: 'global.setTimeout',
in_app: true,
lineno: 28,
colno: 9,
},
{
filename:
'file:///Users/tim/Documents/Repositories/sentry-electron/test/e2e/dist/javascript-renderer/src/index.html',
function: 'one',
in_app: true,
lineno: 24,
colno: 9,
},
{
filename:
'file:///Users/tim/Documents/Repositories/sentry-electron/test/e2e/dist/javascript-renderer/src/index.html',
function: 'two',
in_app: true,
lineno: 17,
colno: 11,
},
{
filename: 'ELECTRON_ASAR.js',
function: 'fs.readFileSync',
lineno: 538,
colno: 29,
in_app: false,
},
{
filename: 'fs.js',
function: 'fs.readFileSync',
lineno: 551,
colno: 33,
in_app: false,
},
{
filename: 'ELECTRON_ASAR.js',
function: 'Object.module.(anonymous function) [as openSync]',
lineno: 166,
colno: 20,
in_app: false,
},
{
filename: 'fs.js',
function: 'Object.fs.openSync',
lineno: 646,
colno: 18,
in_app: false,
},
]);
});

it('Electron v19', () => {
const stack = `Error: ENOENT: no such file or directory, open '/does-not-exist'
at Object.openSync (node:fs:585:3)
at Object.func [as openSync] (node:electron/js2c/asar_bundle:5:1812)
at readFileSync (node:fs:453:35)
at e.readFileSync (node:electron/js2c/asar_bundle:5:9160)
at two (file:///Users/tim/Documents/Repositories/sentry-electron/test/e2e/dist/javascript-renderer/src/index.html:17:11)
at one (file:///Users/tim/Documents/Repositories/sentry-electron/test/e2e/dist/javascript-renderer/src/index.html:24:9)
at file:///Users/tim/Documents/Repositories/sentry-electron/test/e2e/dist/javascript-renderer/src/index.html:28:9
at sentryWrapped (/Users/tim/Documents/Repositories/sentry-electron/test/e2e/dist/javascript-renderer/node_modules/@sentry/browser/cjs/helpers.js:87:17)`;

const frames = electronRendererStackParser(stack);

expect(frames).to.eql([
{
filename:
'file:///Users/tim/Documents/Repositories/sentry-electron/test/e2e/dist/javascript-renderer/src/index.html',
function: '?',
in_app: true,
lineno: 28,
colno: 9,
},
{
filename:
'file:///Users/tim/Documents/Repositories/sentry-electron/test/e2e/dist/javascript-renderer/src/index.html',
function: 'one',
in_app: true,
lineno: 24,
colno: 9,
},
{
filename:
'file:///Users/tim/Documents/Repositories/sentry-electron/test/e2e/dist/javascript-renderer/src/index.html',
function: 'two',
in_app: true,
lineno: 17,
colno: 11,
},
{
filename: 'node:electron/js2c/asar_bundle',
function: 'e.readFileSync',
lineno: 5,
colno: 9160,
in_app: false,
},
{
filename: 'node:fs',
function: 'readFileSync',
lineno: 453,
colno: 35,
in_app: false,
},
{
filename: 'node:electron/js2c/asar_bundle',
function: 'Object.func [as openSync]',
lineno: 5,
colno: 1812,
in_app: false,
},
{
filename: 'node:fs',
function: 'Object.openSync',
lineno: 585,
colno: 3,
in_app: false,
},
]);
});
});