Repository: getsentry/sentry-electron (ts)

Issue:
Parse minidumps to determine which process they came from
### Problem Statement

Currently we read minidumps from the crashpad directory when we are notified by Electron that a renderer or child process has crashed. We assume that all the minidumps we find correspond the the Electron event. All minidumps we read at startup are assumed to be main process crashes because they cause the process to exit immediately.

Although these assumptions are usually correct, I've personally seen events recorded incorrectly and so have other users (#1045).

### Solution Brainstorm

Using the Rust `minidump` crate, I found that there is crashpad specific stream which contains the following info:

```rust
use minidump::*;

fn main() {
    let dump = minidump::Minidump::read_path("./mini.dmp").unwrap();
    let crashpad_info = dump.get_stream::<MinidumpCrashpadInfo>().unwrap();

    println!(
        "annotation_objects: {:#?}",
        crashpad_info.module_list[0].annotation_objects
    );
}
```
For an Electron renderer minidump this results in:
```
annotation_objects: {
    "num-experiments": String(
        "0",
    ),
    "osarch": String(
        "x86_64",
    ),
    "pid": String(
        "2796",
    ),
    "platform": String(
        "win32",
    ),
    "process_type": String(
        "renderer",
    ),
    "ptype": String(
        "renderer",
    ),
}
```

I've looked at both the [`node-minidump` code](https://github.com/electron/node-minidump) and the Rust [`minidump` crate code](https://github.com/rust-minidump/rust-minidump) and it looks like this shouldn't be overly complex to parse these out. 

If we can access the `process_type` in the minidump we no longer need to guess which process the minidump came from.

Pull request:
feat: Determine Electron process from minidump metadata
- Closes #1047

As described in the above issue:
> Currently we read minidumps from the crashpad directory when we are notified by Electron that a renderer or child process has crashed. We assume that all the minidumps we find correspond the the Electron event. All minidumps we read at startup are assumed to be main process crashes because they cause the process to exit immediately.
>
>Although these assumptions are usually correct, I've personally seen events recorded incorrectly and so have other users (https://github.com/getsentry/sentry-electron/issues/1045).

Rather than fully parse the minidumps as described in the above issue (ie. header + enumerate the streams), we just search for the `process_type` key in the minidump file and pick out the string that follows. This simplified approach is reasonable because Electrons Crashpad is configured to emit minimal minidumps (250-400kb). 

If we can't determine the process type from parsing the minidump, we have a new `event.process` tag of `unknown`. In most cases, these will be for child processes and I've added a test to confirm that if these are found at startup, they are correctly categorised.



Patch:
diff --git a/src/main/integrations/sentry-minidump/index.ts b/src/main/integrations/sentry-minidump/index.ts
index 21e15bc..a1056d5 100644
--- a/src/main/integrations/sentry-minidump/index.ts
+++ b/src/main/integrations/sentry-minidump/index.ts
@@ -13,7 +13,7 @@ import { app, crashReporter } from 'electron';
 
 import { addScopeListener, getScopeData } from '../../../common/scope';
 import { getEventDefaults } from '../../context';
-import { EXIT_REASONS, getSentryCachePath } from '../../electron-normalize';
+import { EXIT_REASONS, getSentryCachePath, usesCrashpad } from '../../electron-normalize';
 import { getRendererProperties, trackRendererProperties } from '../../renderers';
 import { ElectronMainOptions } from '../../sdk';
 import { checkPreviousSession, sessionCrashed } from '../../sessions';
@@ -80,7 +80,10 @@ export const sentryMinidumpIntegration = defineIntegration((options: Options = {
     scopeChanged(getScopeData());
   }
 
-  async function sendNativeCrashes(client: NodeClient, eventIn: Event): Promise<boolean> {
+  async function sendNativeCrashes(
+    client: NodeClient,
+    getEvent: (minidumpProcess: string | undefined) => 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:
@@ -93,26 +96,6 @@ export const sentryMinidumpIntegration = defineIntegration((options: Options = {
     //     about it. Just use the breadcrumbs and context information we have
     //     right now and hope that the delay was not too long.
 
-    const event = eventIn;
-
-    // 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 scopeLastRun;
-      if (previousRun) {
-        if (previousRun.scope) {
-          applyScopeDataToEvent(event, previousRun.scope);
-        }
-
-        event.release = previousRun.event?.release || event.release;
-        event.environment = previousRun.event?.environment || event.environment;
-        event.contexts = previousRun.event?.contexts || event.contexts;
-      }
-    }
-
-    if (!event) {
-      return false;
-    }
-
     if (minidumpsRemaining <= 0) {
       logger.log('Not sending minidumps because the limit has been reached');
     }
@@ -121,9 +104,30 @@ export const sentryMinidumpIntegration = defineIntegration((options: Options = {
     const deleteAll = client.getOptions().enabled === false || minidumpsRemaining <= 0;
 
     let minidumpFound = false;
-    await minidumpLoader?.(deleteAll, (attachment) => {
+
+    await minidumpLoader?.(deleteAll, async (minidumpProcess, attachment) => {
       minidumpFound = true;
 
+      const event = getEvent(minidumpProcess);
+
+      // 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 scopeLastRun;
+        if (previousRun) {
+          if (previousRun.scope) {
+            applyScopeDataToEvent(event, previousRun.scope);
+          }
+
+          event.release = previousRun.event?.release || event.release;
+          event.environment = previousRun.event?.environment || event.environment;
+          event.contexts = previousRun.event?.contexts || event.contexts;
+        }
+      }
+
+      if (!event) {
+        return;
+      }
+
       if (minidumpsRemaining > 0) {
         minidumpsRemaining -= 1;
         captureEvent(event as Event, { attachments: [attachment] });
@@ -140,25 +144,31 @@ export const sentryMinidumpIntegration = defineIntegration((options: Options = {
     details: Partial<Electron.RenderProcessGoneDetails>,
   ): Promise<void> {
     const { getRendererName } = options;
-    const crashedProcess = getRendererName?.(contents) || 'renderer';
 
-    logger.log(`'${crashedProcess}' process '${details.reason}'`);
+    const found = await sendNativeCrashes(client, (minidumpProcess) => {
+      // We only call 'getRendererName' if this was in fact a renderer crash
+      const crashedProcess =
+        (minidumpProcess === 'renderer' && getRendererName ? getRendererName(contents) : minidumpProcess) ||
+        (usesCrashpad() ? 'unknown' : 'renderer');
 
-    const found = await sendNativeCrashes(client, {
-      contexts: {
-        electron: {
-          crashed_url: getRendererProperties(contents.id)?.url || 'unknown',
-          details,
+      logger.log(`'${crashedProcess}' process '${details.reason}'`);
+
+      return {
+        contexts: {
+          electron: {
+            crashed_url: getRendererProperties(contents.id)?.url || 'unknown',
+            details,
+          },
         },
-      },
-      level: 'fatal',
-      // The default is javascript
-      platform: 'native',
-      tags: {
-        'event.environment': 'native',
-        'event.process': crashedProcess,
-        'exit.reason': details.reason,
-      },
+        level: 'fatal',
+        // The default is javascript
+        platform: 'native',
+        tags: {
+          'event.environment': 'native',
+          'event.process': crashedProcess,
+          'exit.reason': details.reason,
+        },
+      };
     });
 
     if (found) {
@@ -173,7 +183,7 @@ export const sentryMinidumpIntegration = defineIntegration((options: Options = {
   ): Promise<void> {
     logger.log(`${details.type} process has ${details.reason}`);
 
-    const found = await sendNativeCrashes(client, {
+    const found = await sendNativeCrashes(client, (minidumpProcess) => ({
       contexts: {
         electron: { details },
       },
@@ -182,11 +192,11 @@ export const sentryMinidumpIntegration = defineIntegration((options: Options = {
       platform: 'native',
       tags: {
         'event.environment': 'native',
-        'event.process': details.type,
+        'event.process': minidumpProcess || details.type,
         'exit.reason': details.reason,
         event_type: 'native',
       },
-    });
+    }));
 
     if (found) {
       sessionCrashed();
@@ -234,14 +244,14 @@ export const sentryMinidumpIntegration = defineIntegration((options: Options = {
 
       // Start to submit recent minidump crashes. This will load breadcrumbs and
       // context information that was cached on disk in the previous app run, prior to the crash.
-      sendNativeCrashes(client, {
+      sendNativeCrashes(client, (minidumpProcess) => ({
         level: 'fatal',
         platform: 'native',
         tags: {
           'event.environment': 'native',
-          'event.process': 'browser',
+          'event.process': minidumpProcess || (usesCrashpad() ? 'unknown' : 'browser'),
         },
-      })
+      }))
         .then((minidumpsFound) =>
           // Check for previous uncompleted session. If a previous session exists
           // and no minidumps were found, its likely an abnormal exit
diff --git a/src/main/integrations/sentry-minidump/minidump-loader.ts b/src/main/integrations/sentry-minidump/minidump-loader.ts
index 94f9508..07092c2 100644
--- a/src/main/integrations/sentry-minidump/minidump-loader.ts
+++ b/src/main/integrations/sentry-minidump/minidump-loader.ts
@@ -26,7 +26,10 @@ function delay(ms: number): Promise<void> {
  * @param deleteAll Whether to just delete all minidumps
  * @param callback A callback to call with the attachment ready to send
  */
-export type MinidumpLoader = (deleteAll: boolean, callback: (attachment: Attachment) => void) => Promise<void>;
+export type MinidumpLoader = (
+  deleteAll: boolean,
+  callback: (processType: string | undefined, attachment: Attachment) => Promise<void>,
+) => Promise<void>;
 
 /**
  * Creates a minidump loader
@@ -75,9 +78,11 @@ export function createMinidumpLoader(
                 break;
               }
 
+              const minidumpProcess = getMinidumpProcessType(data);
+
               logger.log('Sending minidump');
 
-              callback({
+              await callback(minidumpProcess, {
                 attachmentType: 'event.minidump',
                 filename: basename(path),
                 data,
@@ -214,3 +219,52 @@ function breakpadMinidumpLoader(): MinidumpLoader {
 export function getMinidumpLoader(): MinidumpLoader {
   return usesCrashpad() ? crashpadMinidumpLoader() : breakpadMinidumpLoader();
 }
+
+/**
+ * Crashpad includes it's own custom stream in the minidump file that can include metadata. Electron uses this to
+ * include details about the app and process that caused the crash.
+ *
+ * Rather than parse the minidump by reading the header and parsing through all the streams, we can just look for the
+ * 'process_type' key and then pick the string that comes after that.
+ */
+function getMinidumpProcessType(buffer: Buffer): string | undefined {
+  const index = buffer.indexOf('process_type');
+
+  if (index < 0) {
+    return;
+  }
+
+  // start after 'process_type'
+  let start = index + 12;
+
+  // Move start to the first ascii character
+  while ((buffer[start] || 0) < 32) {
+    start++;
+
+    // If we can't find the start in the first 20 bytes, we assume it's not there
+    if (start - index > 20) {
+      return;
+    }
+  }
+
+  let end = start;
+
+  // Move the end of the ascii
+  while ((buffer[end] || -1) >= 32) {
+    end++;
+
+    // If we can't find the end in the first 20 bytes, we assume it's not there
+    if (end - start > 20) {
+      return;
+    }
+  }
+
+  const processType = buffer.subarray(start, end).toString().replace('-process', '');
+
+  // For backwards compatibility
+  if (processType === 'gpu') {
+    return 'GPU';
+  }
+
+  return processType;
+}


Test patch:
diff --git a/test/e2e/test-apps/native-sentry/unknown/event.json b/test/e2e/test-apps/native-sentry/unknown/event.json
new file mode 100644
index 0000000..dff6fbb
--- /dev/null
+++ b/test/e2e/test-apps/native-sentry/unknown/event.json
@@ -0,0 +1,84 @@
+{
+  "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-unknown",
+        "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
+      },
+      "culture": {
+        "locale": "{{locale}}",
+        "timezone": "{{timezone}}"
+      },
+      "node": {
+        "name": "Node",
+        "type": "runtime",
+        "version": "{{version}}"
+      },
+      "os": {
+        "name": "{{platform}}",
+        "version": "{{version}}"
+      },
+      "runtime": {
+        "name": "Electron",
+        "version": "{{version}}"
+      }
+    },
+    "release": "native-sentry-unknown@1.0.0",
+    "environment": "development",
+    "event_id": "{{id}}",
+    "timestamp": 0,
+    "breadcrumbs": [
+      {
+        "category": "console",
+        "level": "log",
+        "message": "main process breadcrumb from second run"
+      }
+    ],
+    "tags": {
+      "event.environment": "native",
+      "event.origin": "electron",
+      "event.process": "unknown",
+      "app-run": "second"
+    }
+  },
+  "attachments": [
+    {
+      "length": 12004,
+      "filename": "0dc9e285-df8d-47b7-8147-85308b54065a.dmp",
+      "attachment_type": "event.minidump"
+    }
+  ]
+}
diff --git a/test/e2e/test-apps/native-sentry/unknown/package.json b/test/e2e/test-apps/native-sentry/unknown/package.json
new file mode 100644
index 0000000..89d3fc9
--- /dev/null
+++ b/test/e2e/test-apps/native-sentry/unknown/package.json
@@ -0,0 +1,8 @@
+{
+  "name": "native-sentry-unknown",
+  "version": "1.0.0",
+  "main": "src/main.js",
+  "dependencies": {
+    "@sentry/electron": "5.6.0"
+  }
+}
diff --git a/test/e2e/test-apps/native-sentry/unknown/recipe.yml b/test/e2e/test-apps/native-sentry/unknown/recipe.yml
new file mode 100644
index 0000000..a218cf9
--- /dev/null
+++ b/test/e2e/test-apps/native-sentry/unknown/recipe.yml
@@ -0,0 +1,5 @@
+description: Native Unknown Crash
+category: Native (Sentry Uploader)
+command: yarn
+condition: usesCrashpad
+runTwice: true
diff --git a/test/e2e/test-apps/native-sentry/unknown/src/index.html b/test/e2e/test-apps/native-sentry/unknown/src/index.html
new file mode 100644
index 0000000..26aca20
--- /dev/null
+++ b/test/e2e/test-apps/native-sentry/unknown/src/index.html
@@ -0,0 +1,15 @@
+<!DOCTYPE html>
+<html>
+  <head>
+    <meta charset="UTF-8" />
+  </head>
+  <body>
+    <script>
+      const { init } = require('@sentry/electron/renderer');
+
+      init({
+        debug: true,
+      });
+    </script>
+  </body>
+</html>
diff --git a/test/e2e/test-apps/native-sentry/unknown/src/main.js b/test/e2e/test-apps/native-sentry/unknown/src/main.js
new file mode 100644
index 0000000..066071b
--- /dev/null
+++ b/test/e2e/test-apps/native-sentry/unknown/src/main.js
@@ -0,0 +1,50 @@
+const path = require('path');
+const { writeFileSync } = require('fs');
+
+const { app, BrowserWindow } = require('electron');
+const { init, getCurrentScope } = require('@sentry/electron/main');
+
+app.commandLine.appendSwitch('enable-crashpad');
+
+const VALID_LOOKING_MINIDUMP = Buffer.from(`MDMP${'X'.repeat(12000)}`);
+
+const minidumpPath = path.join(
+  app.getPath('crashDumps'),
+  process.platform === 'win32' ? 'reports' : 'completed',
+  '0dc9e285-df8d-47b7-8147-85308b54065a.dmp'
+);
+
+init({
+  dsn: '__DSN__',
+  debug: true,
+  autoSessionTracking: false,
+  onFatalError: () => {},
+});
+
+getCurrentScope().setTag('app-run', process.env.APP_FIRST_RUN ? 'first' : 'second');
+
+app.on('ready', () => {
+  const mainWindow = new BrowserWindow({
+    show: false,
+    webPreferences: {
+      nodeIntegration: true,
+      contextIsolation: false,
+    },
+  });
+
+  mainWindow.loadFile(path.join(__dirname, 'index.html'));
+
+  if (process.env.APP_FIRST_RUN) {
+    console.log('main process breadcrumb from first crashing run');
+
+    setTimeout(() => {
+      writeFileSync(minidumpPath, VALID_LOOKING_MINIDUMP);
+
+      setTimeout(() => {
+        process.exit();
+       });
+    }, 2000);
+  } else {
+    console.log('main process breadcrumb from second run');
+  }
+});
diff --git a/test/unit/minidump-loader.test.ts b/test/unit/minidump-loader.test.ts
index 519b0bf..f00587a 100644
--- a/test/unit/minidump-loader.test.ts
+++ b/test/unit/minidump-loader.test.ts
@@ -36,7 +36,7 @@ describe('createMinidumpLoader', () => {
 
       const loader = createMinidumpLoader(() => Promise.resolve([dumpPath]));
 
-      void loader(false, (attachment) => {
+      void loader(false, (_, attachment) => {
         expect(attachment).to.eql({
           data: VALID_LOOKING_MINIDUMP,
           filename: name,
@@ -151,7 +151,7 @@ describe('createMinidumpLoader', () => {
 
       const loader = createMinidumpLoader(() => Promise.resolve([missingPath, dumpPath]));
 
-      void loader(false, (attachment) => {
+      void loader(false, (_, attachment) => {
         expect(attachment.filename).to.eql(name);
 
         setTimeout(() => {