From eb146cd7d730daf76c89e886662aa1ae9bfeb7e0 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Tue, 6 Jun 2023 12:08:11 +0200 Subject: [PATCH 1/4] fix: Ensure correct release used for native main crashes --- .../integrations/sentry-minidump/index.ts | 49 +++++++++---- src/main/store.ts | 18 +++-- .../native-sentry/main-update/event.json | 72 +++++++++++++++++++ .../native-sentry/main-update/package.json | 8 +++ .../native-sentry/main-update/recipe.yml | 4 ++ .../native-sentry/main-update/src/index.html | 15 ++++ .../native-sentry/main-update/src/main.js | 32 +++++++++ 7 files changed, 182 insertions(+), 16 deletions(-) create mode 100644 test/e2e/test-apps/native-sentry/main-update/event.json create mode 100644 test/e2e/test-apps/native-sentry/main-update/package.json create mode 100644 test/e2e/test-apps/native-sentry/main-update/recipe.yml create mode 100644 test/e2e/test-apps/native-sentry/main-update/src/index.html create mode 100644 test/e2e/test-apps/native-sentry/main-update/src/main.js diff --git a/src/main/integrations/sentry-minidump/index.ts b/src/main/integrations/sentry-minidump/index.ts index ca4367c8..8ccb4162 100644 --- a/src/main/integrations/sentry-minidump/index.ts +++ b/src/main/integrations/sentry-minidump/index.ts @@ -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'; @@ -14,6 +14,12 @@ import { checkPreviousSession, sessionCrashed } from '../../sessions'; import { BufferedWriteStore } from '../../store'; import { deleteMinidump, getMinidumpLoader, MinidumpLoader } from './minidump-loader'; +interface PreviousRun { + scope: Scope; + release: string; + environment: string; +} + /** Sends minidumps via the Sentry uploader */ export class SentryMinidump implements Integration { /** @inheritDoc */ @@ -23,10 +29,10 @@ export class SentryMinidump implements Integration { public name: string = SentryMinidump.id; /** Store to persist context information beyond application crashes. */ - private _scopeStore?: BufferedWriteStore; + private _scopeStore?: BufferedWriteStore; /** Temp store for the scope of last run */ - private _scopeLastRun?: Promise; + private _scopeLastRun?: Promise; private _minidumpLoader?: MinidumpLoader; @@ -41,14 +47,22 @@ export class SentryMinidump implements Integration { this._startCrashReporter(); - this._scopeStore = new BufferedWriteStore(sentryCachePath, 'scope_v2', new Scope()); + const client = getCurrentHub().getClient(); + const options = client?.getOptions() as ElectronMainOptions; + + const currentRelease = options?.release || getDefaultReleaseName(); + const currentEnvironment = options?.environment || getDefaultEnvironment(); + + this._scopeStore = new BufferedWriteStore(sentryCachePath, 'scope_v3', { + scope: new Scope(), + release: currentRelease, + environment: currentEnvironment, + }); + // 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(); - const options = client?.getOptions() as ElectronMainOptions; + this._setupScopeListener(currentRelease, currentEnvironment); if (!options?.dsn) { throw new SentryError('Attempted to enable Electron native crash reporter but no DSN was supplied'); @@ -169,7 +183,7 @@ export class SentryMinidump implements Integration { /** * Adds a scope listener to persist changes to disk. */ - private _setupScopeListener(): void { + private _setupScopeListener(currentRelease: string, currentEnvironment: string): void { const hubScope = getCurrentHub().getScope(); if (hubScope) { hubScope.addScopeListener((updatedScope) => { @@ -182,7 +196,11 @@ export class SentryMinidump implements Integration { // 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); + void this._scopeStore?.set({ + scope, + release: currentRelease, + environment: currentEnvironment, + }); }); }); } @@ -224,13 +242,15 @@ 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); + const previousRun = await this._scopeLastRun; + + const storedScope = Scope.clone(previousRun?.scope); let newEvent = await storedScope.applyToEvent(event); const hubScope = hub.getScope(); @@ -240,6 +260,11 @@ export class SentryMinidump implements Integration { return false; } + if (previousRun) { + newEvent.release = previousRun.release; + newEvent.environment = previousRun.environment; + } + for (const minidump of minidumps) { const data = await minidump.load(); diff --git a/src/main/store.ts b/src/main/store.ts index 291de428..4195ad03 100644 --- a/src/main/store.ts +++ b/src/main/store.ts @@ -117,8 +117,6 @@ export class Store { * Extends Store to throttle writes. */ export class BufferedWriteStore extends Store { - /** 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; @@ -129,16 +127,28 @@ export class BufferedWriteStore extends Store { * @param id A unique filename to store this data. * @param initial An initial value to initialize data with. * @param throttleTime The minimum time between writes + * @param immediateFirstWrite Whether to write the first set immediately */ - public constructor(path: string, id: string, initial: T, throttleTime: number = 500) { + public constructor( + path: string, + id: string, + initial: T, + private readonly _throttleTime: number = 500, + private _immediateFirstWrite = true, + ) { super(path, id, initial); - this._throttleTime = throttleTime; } /** @inheritdoc */ public override async set(data: T): Promise { this._data = data; + // If this is the first write, we write immediately + if (this._immediateFirstWrite) { + this._immediateFirstWrite = false; + return super.set(data); + } + this._pendingWrite = { // We overwrite the data for the pending write so that the latest data is written in the next flush data, diff --git a/test/e2e/test-apps/native-sentry/main-update/event.json b/test/e2e/test-apps/native-sentry/main-update/event.json new file mode 100644 index 00000000..203cd235 --- /dev/null +++ b/test/e2e/test-apps/native-sentry/main-update/event.json @@ -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": "native-sentry-main-update@1.0.0", + "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" } ] +} diff --git a/test/e2e/test-apps/native-sentry/main-update/package.json b/test/e2e/test-apps/native-sentry/main-update/package.json new file mode 100644 index 00000000..150ecdb8 --- /dev/null +++ b/test/e2e/test-apps/native-sentry/main-update/package.json @@ -0,0 +1,8 @@ +{ + "name": "native-sentry-main-update", + "version": "1.0.0", + "main": "src/main.js", + "dependencies": { + "@sentry/electron": "3.0.0" + } +} diff --git a/test/e2e/test-apps/native-sentry/main-update/recipe.yml b/test/e2e/test-apps/native-sentry/main-update/recipe.yml new file mode 100644 index 00000000..8e3a25db --- /dev/null +++ b/test/e2e/test-apps/native-sentry/main-update/recipe.yml @@ -0,0 +1,4 @@ +description: Native Main Crash (after update) +category: Native (Sentry Uploader) +command: yarn +runTwice: true diff --git a/test/e2e/test-apps/native-sentry/main-update/src/index.html b/test/e2e/test-apps/native-sentry/main-update/src/index.html new file mode 100644 index 00000000..2707fc4f --- /dev/null +++ b/test/e2e/test-apps/native-sentry/main-update/src/index.html @@ -0,0 +1,15 @@ + + + + + + + + + \ No newline at end of file diff --git a/test/e2e/test-apps/native-sentry/main-update/src/main.js b/test/e2e/test-apps/native-sentry/main-update/src/main.js new file mode 100644 index 00000000..b9b112a2 --- /dev/null +++ b/test/e2e/test-apps/native-sentry/main-update/src/main.js @@ -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 ? 'native-sentry-main-update@1.0.0' : 'native-sentry-main-update@2.0.0', + 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(); + }, 500); + } +}); From 97cc51c964360c0674b1e9f0753529a41e077062 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Tue, 6 Jun 2023 12:31:05 +0200 Subject: [PATCH 2/4] Fix test --- test/unit/store.test.ts | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/test/unit/store.test.ts b/test/unit/store.test.ts index 4c82cf36..e4b36956 100644 --- a/test/unit/store.test.ts +++ b/test/unit/store.test.ts @@ -54,11 +54,7 @@ describe('Store', () => { await expectFilesInDirectory(tempDir.name, 0); await store.set({ num: 990, str: 'just a string' }); - // File should not be written after 100ms! - await delay(100); - await expectFilesInDirectory(tempDir.name, 0); - // Should have been written after 1 more second - await delay(1_000); + // Initial write should be immediate await expectFilesInDirectory(tempDir.name, 1); const contents = await readFileAsync(join(tempDir.name, 'test-store.json'), 'utf-8'); From 7f82c129a9197486fb4b99685c49e9cfca932c8a Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Tue, 6 Jun 2023 14:46:45 +0200 Subject: [PATCH 3/4] Extend crash time --- test/e2e/test-apps/native-sentry/main/src/main.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/e2e/test-apps/native-sentry/main/src/main.js b/test/e2e/test-apps/native-sentry/main/src/main.js index ae6a4a7a..c723921e 100644 --- a/test/e2e/test-apps/native-sentry/main/src/main.js +++ b/test/e2e/test-apps/native-sentry/main/src/main.js @@ -30,6 +30,6 @@ app.on('ready', () => { if (process.env.APP_FIRST_RUN) { setTimeout(() => { process.crash(); - }, 500); + }, 1000); } }); From 2f87435cda3ba73703fbf5f0aad44ec4a7048eba Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Wed, 2 Aug 2023 12:06:58 +0200 Subject: [PATCH 4/4] Store the entire default event --- .../integrations/sentry-minidump/index.ts | 87 ++++++++++--------- src/main/store.ts | 15 +--- .../native-sentry/main-update/src/main.js | 2 +- test/unit/store.test.ts | 6 +- 4 files changed, 55 insertions(+), 55 deletions(-) diff --git a/src/main/integrations/sentry-minidump/index.ts b/src/main/integrations/sentry-minidump/index.ts index 8ccb4162..c1f4b9cc 100644 --- a/src/main/integrations/sentry-minidump/index.ts +++ b/src/main/integrations/sentry-minidump/index.ts @@ -16,8 +16,7 @@ import { deleteMinidump, getMinidumpLoader, MinidumpLoader } from './minidump-lo interface PreviousRun { scope: Scope; - release: string; - environment: string; + event?: Event; } /** Sends minidumps via the Sentry uploader */ @@ -47,21 +46,20 @@ export class SentryMinidump implements Integration { this._startCrashReporter(); - const client = getCurrentHub().getClient(); - const options = client?.getOptions() as ElectronMainOptions; - - const currentRelease = options?.release || getDefaultReleaseName(); - const currentEnvironment = options?.environment || getDefaultEnvironment(); - this._scopeStore = new BufferedWriteStore(sentryCachePath, 'scope_v3', { scope: new Scope(), - release: currentRelease, - environment: currentEnvironment, }); // We need to store the scope in a variable here so it can be attached to minidumps this._scopeLastRun = this._scopeStore.get(); + const hub = getCurrentHub(); + const client = hub.getClient(); + const options = client?.getOptions() as ElectronMainOptions; + + const currentRelease = options?.release || getDefaultReleaseName(); + const currentEnvironment = options?.environment || getDefaultEnvironment(); + this._setupScopeListener(currentRelease, currentEnvironment); if (!options?.dsn) { @@ -184,25 +182,30 @@ export class SentryMinidump implements Integration { * Adds a scope listener to persist changes to disk. */ private _setupScopeListener(currentRelease: string, currentEnvironment: string): 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, - release: currentRelease, - environment: currentEnvironment, - }); + 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); } } @@ -211,7 +214,7 @@ export class SentryMinidump implements Integration { * * Returns true if one or more minidumps were found */ - private async _sendNativeCrashes(event: Event): Promise { + private async _sendNativeCrashes(eventIn: Event): Promise { // 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: @@ -224,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'); } @@ -248,28 +253,32 @@ export class SentryMinidump implements Integration { return false; } - const previousRun = await this._scopeLastRun; + // 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); - let newEvent = await storedScope.applyToEvent(event); + 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; } - if (previousRun) { - newEvent.release = previousRun.release; - newEvent.environment = previousRun.environment; - } - for (const minidump of minidumps) { const data = await minidump.load(); if (data) { - captureEvent(newEvent, { + captureEvent(event, { attachments: [ { attachmentType: 'event.minidump', diff --git a/src/main/store.ts b/src/main/store.ts index 6400ddf2..bffb0f3c 100644 --- a/src/main/store.ts +++ b/src/main/store.ts @@ -127,15 +127,8 @@ export class BufferedWriteStore extends Store { * @param id A unique filename to store this data. * @param initial An initial value to initialize data with. * @param throttleTime The minimum time between writes - * @param immediateFirstWrite Whether to write the first set immediately */ - public constructor( - path: string, - id: string, - initial: T, - private readonly _throttleTime: number = 500, - private _immediateFirstWrite = true, - ) { + public constructor(path: string, id: string, initial: T, private readonly _throttleTime: number = 500) { super(path, id, initial); } @@ -143,12 +136,6 @@ export class BufferedWriteStore extends Store { public override async set(data: T): Promise { this._data = data; - // If this is the first write, we write immediately - if (this._immediateFirstWrite) { - this._immediateFirstWrite = false; - return super.set(data); - } - this._pendingWrite = { // We overwrite the data for the pending write so that the latest data is written in the next flush data, diff --git a/test/e2e/test-apps/native-sentry/main-update/src/main.js b/test/e2e/test-apps/native-sentry/main-update/src/main.js index b9b112a2..e2af00c9 100644 --- a/test/e2e/test-apps/native-sentry/main-update/src/main.js +++ b/test/e2e/test-apps/native-sentry/main-update/src/main.js @@ -27,6 +27,6 @@ app.on('ready', () => { if (process.env.APP_FIRST_RUN) { setTimeout(() => { process.crash(); - }, 500); + }, 1000); } }); diff --git a/test/unit/store.test.ts b/test/unit/store.test.ts index e4b36956..4c82cf36 100644 --- a/test/unit/store.test.ts +++ b/test/unit/store.test.ts @@ -54,7 +54,11 @@ describe('Store', () => { await expectFilesInDirectory(tempDir.name, 0); await store.set({ num: 990, str: 'just a string' }); - // Initial write should be immediate + // File should not be written after 100ms! + await delay(100); + await expectFilesInDirectory(tempDir.name, 0); + // Should have been written after 1 more second + await delay(1_000); await expectFilesInDirectory(tempDir.name, 1); const contents = await readFileAsync(join(tempDir.name, 'test-store.json'), 'utf-8');