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: Ensure context from previous run is used for native main crashes #683

Merged
merged 7 commits into from
Aug 2, 2023
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
90 changes: 62 additions & 28 deletions src/main/integrations/sentry-minidump/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import { basename, logger, SentryError } from '@sentry/utils';
import { app, crashReporter } from 'electron';

import { mergeEvents } from '../../../common';
import { getEventDefaults } from '../../context';
import { getDefaultEnvironment, getDefaultReleaseName, getEventDefaults } from '../../context';
import { EXIT_REASONS, onChildProcessGone, onRendererProcessGone } from '../../electron-normalize';
import { sentryCachePath } from '../../fs';
import { getRendererProperties, trackRendererProperties } from '../../renderers';
Expand All @@ -14,6 +14,11 @@ import { checkPreviousSession, sessionCrashed } from '../../sessions';
import { BufferedWriteStore } from '../../store';
import { deleteMinidump, getMinidumpLoader, MinidumpLoader } from './minidump-loader';

interface PreviousRun {
scope: Scope;
event?: Event;
}

/** Sends minidumps via the Sentry uploader */
export class SentryMinidump implements Integration {
/** @inheritDoc */
Expand All @@ -23,10 +28,10 @@ export class SentryMinidump implements Integration {
public name: string = SentryMinidump.id;

/** Store to persist context information beyond application crashes. */
private _scopeStore?: BufferedWriteStore<Scope>;
private _scopeStore?: BufferedWriteStore<PreviousRun>;

/** Temp store for the scope of last run */
private _scopeLastRun?: Promise<Scope>;
private _scopeLastRun?: Promise<PreviousRun>;

private _minidumpLoader?: MinidumpLoader;

Expand All @@ -41,15 +46,22 @@ export class SentryMinidump implements Integration {

this._startCrashReporter();

this._scopeStore = new BufferedWriteStore<Scope>(sentryCachePath, 'scope_v2', new Scope());
this._scopeStore = new BufferedWriteStore<PreviousRun>(sentryCachePath, 'scope_v3', {
scope: new Scope(),
});

// We need to store the scope in a variable here so it can be attached to minidumps
this._scopeLastRun = this._scopeStore.get();

this._setupScopeListener();

const client = getCurrentHub().getClient<NodeClient>();
const hub = getCurrentHub();
const client = hub.getClient<NodeClient>();
const options = client?.getOptions() as ElectronMainOptions;

const currentRelease = options?.release || getDefaultReleaseName();
const currentEnvironment = options?.environment || getDefaultEnvironment();

this._setupScopeListener(currentRelease, currentEnvironment);

if (!options?.dsn) {
throw new SentryError('Attempted to enable Electron native crash reporter but no DSN was supplied');
}
Expand Down Expand Up @@ -169,22 +181,31 @@ export class SentryMinidump implements Integration {
/**
* Adds a scope listener to persist changes to disk.
*/
private _setupScopeListener(): void {
const hubScope = getCurrentHub().getScope();
if (hubScope) {
hubScope.addScopeListener((updatedScope) => {
const scope = Scope.clone(updatedScope);
// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
(scope as any)._eventProcessors = [];
// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
(scope as any)._scopeListeners = [];

// Since the initial scope read is async, we need to ensure that any writes do not beat that
// https://github.com/getsentry/sentry-electron/issues/585
setImmediate(() => {
void this._scopeStore?.set(scope);
private _setupScopeListener(currentRelease: string, currentEnvironment: string): void {
const scopeChanged = (updatedScope: Scope): void => {
const scope = Scope.clone(updatedScope);
// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
(scope as any)._eventProcessors = [];
// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
(scope as any)._scopeListeners = [];

// Since the initial scope read is async, we need to ensure that any writes do not beat that
// https://github.com/getsentry/sentry-electron/issues/585
setImmediate(async () => {
const event = await getEventDefaults(currentRelease, currentEnvironment);
void this._scopeStore?.set({
scope,
event,
});
});
};

const scope = getCurrentHub().getScope();

if (scope) {
scope.addScopeListener(scopeChanged);
// Ensure at least one event is written to disk
scopeChanged(scope);
}
}

Expand All @@ -193,7 +214,7 @@ export class SentryMinidump implements Integration {
*
* Returns true if one or more minidumps were found
*/
private async _sendNativeCrashes(event: Event): Promise<boolean> {
private async _sendNativeCrashes(eventIn: Event): Promise<boolean> {
// Whenever we are called, assume that the crashes we are going to load down
// below have occurred recently. This means, we can use the same event data
// for all minidumps that we load now. There are two conditions:
Expand All @@ -206,6 +227,8 @@ export class SentryMinidump implements Integration {
// about it. Just use the breadcrumbs and context information we have
// right now and hope that the delay was not too long.

let event: Event | null = eventIn;

if (this._minidumpLoader === undefined) {
throw new SentryError('Invariant violation: Native crashes not enabled');
}
Expand All @@ -224,27 +247,38 @@ export class SentryMinidump implements Integration {
const enabled = client.getOptions().enabled;

// If the SDK is not enabled, we delete the minidump files so they
// dont accumulate and/or get sent later
// don't accumulate and/or get sent later
if (enabled === false) {
minidumps.forEach(deleteMinidump);
return false;
}

const storedScope = Scope.clone(await this._scopeLastRun);
let newEvent = await storedScope.applyToEvent(event);
// If this is a native main process crash, we need to apply the scope and context from the previous run
if (event?.tags?.['event.process'] === 'browser') {
const previousRun = await this._scopeLastRun;

const storedScope = Scope.clone(previousRun?.scope);
event = await storedScope.applyToEvent(event);

if (event && previousRun) {
event.release = previousRun.event?.release || event.release;
event.environment = previousRun.event?.environment || event.environment;
event.contexts = previousRun.event?.contexts || event.contexts;
}
}

const hubScope = hub.getScope();
newEvent = hubScope ? await hubScope.applyToEvent(event) : event;
event = hubScope && event ? await hubScope.applyToEvent(event) : event;

if (!newEvent) {
if (!event) {
return false;
}

for (const minidump of minidumps) {
const data = await minidump.load();

if (data) {
captureEvent(newEvent, {
captureEvent(event, {
attachments: [
{
attachmentType: 'event.minidump',
Expand Down
5 changes: 1 addition & 4 deletions src/main/store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -117,8 +117,6 @@ export class Store<T> {
* Extends Store to throttle writes.
*/
export class BufferedWriteStore<T> extends Store<T> {
/** The minimum time between writes */
private readonly _throttleTime?: number;
/** A write that hasn't been written to disk yet */
private _pendingWrite: { data: T; timeout: NodeJS.Timeout } | undefined;

Expand All @@ -130,9 +128,8 @@ export class BufferedWriteStore<T> extends Store<T> {
* @param initial An initial value to initialize data with.
* @param throttleTime The minimum time between writes
*/
public constructor(path: string, id: string, initial: T, throttleTime: number = 500) {
public constructor(path: string, id: string, initial: T, private readonly _throttleTime: number = 500) {
super(path, id, initial);
this._throttleTime = throttleTime;
}

/** @inheritdoc */
Expand Down
72 changes: 72 additions & 0 deletions test/e2e/test-apps/native-sentry/main-update/event.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
{
"method": "envelope",
"sentryKey": "37f8a2ee37c0409d8970bc7559c7c7e4",
"appId": "277345",
"data": {
"sdk": {
"name": "sentry.javascript.electron",
"packages": [
{
"name": "npm:@sentry/electron",
"version": "{{version}}"
}
],
"version": "{{version}}"
},
"contexts": {
"app": {
"app_name": "native-sentry-main-update",
"app_version": "1.0.0",
"app_start_time": "{{time}}"
},
"browser": {
"name": "Chrome"
},
"chrome": {
"name": "Chrome",
"type": "runtime",
"version": "{{version}}"
},
"device": {
"arch": "{{arch}}",
"family": "Desktop",
"memory_size": 0,
"free_memory": 0,
"processor_count": 0,
"processor_frequency": 0,
"cpu_description": "{{cpu}}",
"screen_resolution":"{{screen}}",
"screen_density": 1,
"language": "{{language}}"
},
"node": {
"name": "Node",
"type": "runtime",
"version": "{{version}}"
},
"os": {
"name": "{{platform}}",
"version": "{{version}}"
},
"runtime": {
"name": "Electron",
"version": "{{version}}"
}
},
"release": "[email protected]",
"environment": "development",
"user": {
"ip_address": "{{auto}}"
},
"event_id": "{{id}}",
"timestamp": 0,
"breadcrumbs": [],
"tags": {
"event.environment": "native",
"event.origin": "electron",
"event.process": "browser",
"event_type": "native"
}
},
"attachments": [ { "attachment_type": "event.minidump" } ]
}
8 changes: 8 additions & 0 deletions test/e2e/test-apps/native-sentry/main-update/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
{
"name": "native-sentry-main-update",
"version": "1.0.0",
"main": "src/main.js",
"dependencies": {
"@sentry/electron": "3.0.0"
}
}
4 changes: 4 additions & 0 deletions test/e2e/test-apps/native-sentry/main-update/recipe.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
description: Native Main Crash (after update)
category: Native (Sentry Uploader)
command: yarn
runTwice: true
15 changes: 15 additions & 0 deletions test/e2e/test-apps/native-sentry/main-update/src/index.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
<!DOCTYPE html>
<html>
<head>
<meta charset="UTF-8" />
</head>
<body>
<script>
const { init } = require('@sentry/electron');

init({
debug: true,
});
</script>
</body>
</html>
32 changes: 32 additions & 0 deletions test/e2e/test-apps/native-sentry/main-update/src/main.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
const path = require('path');

const { app, BrowserWindow } = require('electron');
const { init } = require('@sentry/electron');

init({
dsn: '__DSN__',
release: process.env.APP_FIRST_RUN ? '[email protected]' : '[email protected]',
debug: true,
autoSessionTracking: false,
onFatalError: () => {},
});

app.on('ready', () => {
const mainWindow = new BrowserWindow({
show: false,
webPreferences: {
nodeIntegration: true,
contextIsolation: false,
},
});

mainWindow.loadFile(path.join(__dirname, 'index.html'));

// We only crash on the first run
// The second run is where the crash is uploaded
if (process.env.APP_FIRST_RUN) {
setTimeout(() => {
process.crash();
}, 1000);
}
});
2 changes: 1 addition & 1 deletion test/e2e/test-apps/native-sentry/main/src/main.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,6 @@ app.on('ready', () => {
if (process.env.APP_FIRST_RUN) {
setTimeout(() => {
process.crash();
}, 500);
}, 1000);
}
});
Loading