fix: Don't send objects across isolates

Author: timfish

.gitignore

@@ -2,3 +2,4 @@ node_modules/
 build/
 lib/
 /*.tgz
+test/yarn.lock

README.md

@@ -16,7 +16,9 @@ registerThread();
 Watchdog thread:
 
 ```ts
-const { captureStackTrace } = require("@sentry-internal/node-native-stacktrace");
+const { captureStackTrace } = require(
+  "@sentry-internal/node-native-stacktrace",
+);
 
 const stacks = captureStackTrace();
 console.log(stacks);
@@ -26,58 +28,32 @@ Results in:
 
 ```js
 {
-  '0': [
-    {
-      function: 'from',
-      filename: 'node:buffer',
-      lineno: 298,
-      colno: 28
-    },
-    {
-      function: 'pbkdf2Sync',
-      filename: 'node:internal/crypto/pbkdf2',
-      lineno: 78,
-      colno: 17
-    },
-    {
-      function: 'longWork',
-      filename: '/app/test.js',
-      lineno: 20,
-      colno: 29
-    },
-    {
-      function: '?',
-      filename: '/app/test.js',
-      lineno: 24,
-      colno: 1
-    }
-  ],
-  '2': [
-    {
-      function: 'from',
-      filename: 'node:buffer',
-      lineno: 298,
-      colno: 28
-    },
-    {
-      function: 'pbkdf2Sync',
-      filename: 'node:internal/crypto/pbkdf2',
-      lineno: 78,
-      colno: 17
-    },
-    {
-      function: 'longWork',
-      filename: '/app/worker.js',
-      lineno: 10,
-      colno: 29
-    },
-    {
-      function: '?',
-      filename: '/app/worker.js',
-      lineno: 14,
-      colno: 1
-    }
-  ]
+   '0': '    at from (node:buffer:299:28)\n' +
+    '    at pbkdf2Sync (node:internal/crypto/pbkdf2:78:17)\n' +
+    '    at longWork (/Users/tim/test/test/long-work.js:6:25)\n' +
+    '    at ? (/Users/tim/test/test/stack-traces.js:11:1)\n' +
+    '    at ? (node:internal/modules/cjs/loader:1734:14)\n' +
+    '    at ? (node:internal/modules/cjs/loader:1899:10)\n' +
+    '    at ? (node:internal/modules/cjs/loader:1469:32)\n' +
+    '    at ? (node:internal/modules/cjs/loader:1286:12)\n' +
+    '    at traceSync (node:diagnostics_channel:322:14)\n' +
+    '    at wrapModuleLoad (node:internal/modules/cjs/loader:235:24)\n' +
+    '    at executeUserEntryPoint (node:internal/modules/run_main:152:5)\n' +
+    '    at ? (node:internal/main/run_main_module:33:47)',
+  '2': '    at from (node:buffer:299:28)\n' +
+    '    at pbkdf2Sync (node:internal/crypto/pbkdf2:78:17)\n' +
+    '    at longWork (/Users/tim/test/test/long-work.js:6:25)\n' +
+    '    at ? (/Users/tim/test/test/worker.js:6:1)\n' +
+    '    at ? (node:internal/modules/cjs/loader:1734:14)\n' +
+    '    at ? (node:internal/modules/cjs/loader:1899:10)\n' +
+    '    at ? (node:internal/modules/cjs/loader:1469:32)\n' +
+    '    at ? (node:internal/modules/cjs/loader:1286:12)\n' +
+    '    at traceSync (node:diagnostics_channel:322:14)\n' +
+    '    at wrapModuleLoad (node:internal/modules/cjs/loader:235:24)\n' +
+    '    at executeUserEntryPoint (node:internal/modules/run_main:152:5)\n' +
+    '    at ? (node:internal/main/worker_thread:212:26)\n' +
+    '    at [nodejs.internal.kHybridDispatch] (node:internal/event_target:827:20)\n' +
+    '    at ? (node:internal/per_context/messageport:23:28)'
 }
 ```
 

module.cc

@@ -23,76 +23,62 @@ static std::unordered_map<v8::Isolate *, ThreadInfo> threads = {};
 
 // Function to be called when an isolate's execution is interrupted
 static void ExecutionInterrupted(Isolate *isolate, void *data) {
-  auto promise = static_cast<std::promise<Local<Array>> *>(data);
+  auto promise = static_cast<std::promise<std::string> *>(data);
   auto stack = StackTrace::CurrentStackTrace(isolate, kMaxStackFrames,
                                              StackTrace::kDetailed);
 
   if (stack.IsEmpty()) {
-    promise->set_value(Array::New(isolate, 0));
+    promise->set_value("");
     return;
   }
 
-  auto frames = Array::New(isolate, stack->GetFrameCount());
+  std::string stack_string;
 
   for (int i = 0; i < stack->GetFrameCount(); i++) {
     auto frame = stack->GetFrame(isolate, i);
     auto fn_name = frame->GetFunctionName();
 
+    std::string function_name;
     if (frame->IsEval()) {
-      fn_name =
-          String::NewFromUtf8(isolate, "[eval]", NewStringType::kInternalized)
-              .ToLocalChecked();
+      function_name = "[eval]";
     } else if (fn_name.IsEmpty() || fn_name->Length() == 0) {
-      fn_name = String::NewFromUtf8(isolate, "?", NewStringType::kInternalized)
-                    .ToLocalChecked();
+      function_name = "?";
     } else if (frame->IsConstructor()) {
-      fn_name = String::NewFromUtf8(isolate, "[constructor]",
-                                    NewStringType::kInternalized)
-                    .ToLocalChecked();
+      function_name = "[constructor]";
+    } else {
+      v8::String::Utf8Value utf8_fn(isolate, fn_name);
+      function_name = *utf8_fn ? *utf8_fn : "?";
     }
 
-    auto frame_obj = Object::New(isolate);
-    frame_obj
-        ->Set(isolate->GetCurrentContext(),
-              String::NewFromUtf8(isolate, "function",
-                                  NewStringType::kInternalized)
-                  .ToLocalChecked(),
-              fn_name)
-        .Check();
-
-    frame_obj
-        ->Set(isolate->GetCurrentContext(),
-              String::NewFromUtf8(isolate, "filename",
-                                  NewStringType::kInternalized)
-                  .ToLocalChecked(),
-              frame->GetScriptName())
-        .Check();
-
-    frame_obj
-        ->Set(
-            isolate->GetCurrentContext(),
-            String::NewFromUtf8(isolate, "lineno", NewStringType::kInternalized)
-                .ToLocalChecked(),
-            Integer::New(isolate, frame->GetLineNumber()))
-        .Check();
+    std::string filename;
+    auto script_name = frame->GetScriptName();
+    if (!script_name.IsEmpty()) {
+      v8::String::Utf8Value utf8_filename(isolate, script_name);
+      filename = *utf8_filename ? *utf8_filename : "<unknown>";
+    } else {
+      filename = "<unknown>";
+    }
 
-    frame_obj
-        ->Set(
-            isolate->GetCurrentContext(),
-            String::NewFromUtf8(isolate, "colno", NewStringType::kInternalized)
-                .ToLocalChecked(),
-            Integer::New(isolate, frame->GetColumn()))
-        .Check();
+    int line_number = frame->GetLineNumber();
+    int column_number = frame->GetColumn();
+
+    // Build stack trace line in JavaScript format: "    at functionName
+    // (filename:line:column)"
+    stack_string += "    at " + function_name + " (" + filename + ":" +
+                    std::to_string(line_number) + ":" +
+                    std::to_string(column_number) + ")";
 
-    frames->Set(isolate->GetCurrentContext(), i, frame_obj).Check();
+    if (i < stack->GetFrameCount() - 1) {
+      stack_string += "\n";
+    }
   }
 
-  promise->set_value(frames);
+  promise->set_value(stack_string);
 }
 
 // Function to capture the stack trace of a single isolate
-Local<Array> CaptureStackTrace(Isolate *isolate) {
-  std::promise<Local<Array>> promise;
+std::string CaptureStackTrace(Isolate *isolate) {
+  std::promise<std::string> promise;
   auto future = promise.get_future();
 
   // The v8 isolate must be interrupted to capture the stack trace
@@ -105,7 +91,7 @@ Local<Array> CaptureStackTrace(Isolate *isolate) {
 void CaptureStackTraces(const FunctionCallbackInfo<Value> &args) {
   auto capture_from_isolate = args.GetIsolate();
 
-  using ThreadResult = std::tuple<std::string, Local<Array>>;
+  using ThreadResult = std::tuple<std::string, std::string>;
   std::vector<std::future<ThreadResult>> futures;
 
   // We collect the futures into a vec so they can be processed in parallel
@@ -128,13 +114,17 @@ void CaptureStackTraces(const FunctionCallbackInfo<Value> &args) {
   // JavaScript object
   Local<Object> result = Object::New(capture_from_isolate);
   for (auto &future : futures) {
-    auto [thread_name, frames] = future.get();
+    auto [thread_name, stack_string] = future.get();
 
     auto key = String::NewFromUtf8(capture_from_isolate, thread_name.c_str(),
                                    NewStringType::kNormal)
                    .ToLocalChecked();
 
-    result->Set(capture_from_isolate->GetCurrentContext(), key, frames).Check();
+    auto value = String::NewFromUtf8(capture_from_isolate, stack_string.c_str(),
+                                     NewStringType::kNormal)
+                     .ToLocalChecked();
+
+    result->Set(capture_from_isolate->GetCurrentContext(), key, value).Check();
   }
 
   args.GetReturnValue().Set(result);

package.json

@@ -32,6 +32,7 @@
   },
   "devDependencies": {
     "@sentry-internal/eslint-config-sdk": "^9.22.0",
+    "@sentry/core": "^9.22.0",
     "@types/node": "^18.19.1",
     "@types/node-abi": "^3.0.3",
     "clang-format": "^1.8.0",

src/index.ts

@@ -11,16 +11,9 @@ const arch = process.env['BUILD_ARCH'] || _arch();
 const abi = getAbi(versions.node, 'node');
 const identifier = [platform, arch, stdlib, abi].filter(c => c !== undefined && c !== null).join('-');
 
-type StackFrame = {
-  function: string;
-  filename: string;
-  lineno: number;
-  colno: number;
-};
-
 interface Native {
   registerThread(threadName: string): void;
-  captureStackTrace(): Record<string, StackFrame[]>;
+  captureStackTrace(): Record<string, string>;
   getThreadsLastSeen(): Record<string, number>;
 }
 
@@ -180,7 +173,7 @@ export function registerThread(threadName: string = String(threadId)): void {
 /**
  * Captures stack traces for all registered threads.
  */
-export function captureStackTrace(): Record<string, StackFrame[]> {
+export function captureStackTrace(): Record<string, string> {
   return native.captureStackTrace();
 }
 

test/e2e.test.mjs

@@ -1,9 +1,17 @@
 import { spawnSync } from 'node:child_process';
 import { join } from 'node:path';
+import { createStackParser, nodeStackLineParser  } from '@sentry/core';
 import { beforeAll, describe, expect, test } from 'vitest';
 import { installTarballAsDependency } from './prepare.mjs';
 
 const __dirname = import.meta.dirname || new URL('.', import.meta.url).pathname;
+const defaultStackParser = createStackParser(nodeStackLineParser());
+
+function parseStacks(stacks) {
+  return Object.fromEntries(
+    Object.entries(stacks).map(([id, stack]) => [id, defaultStackParser(stack)]),
+  );
+}
 
 describe('e2e Tests', { timeout: 20000 }, () => {
   beforeAll(() => {
@@ -16,26 +24,32 @@ describe('e2e Tests', { timeout: 20000 }, () => {
 
     expect(result.status).toBe(0);
 
-    const stacks = JSON.parse(result.stdout.toString());
+    const stacks = parseStacks(JSON.parse(result.stdout.toString()));
 
     expect(stacks['0']).toEqual(expect.arrayContaining([
       {
         function: 'pbkdf2Sync',
         filename: expect.any(String),
         lineno: expect.any(Number),
         colno: expect.any(Number),
+        in_app: false,
+        module: undefined,
       },
       {
         function: 'longWork',
         filename: expect.stringMatching(/long-work.js$/),
         lineno: expect.any(Number),
         colno: expect.any(Number),
+        in_app: true,
+        module: undefined,
       },
       {
         function: '?',
         filename: expect.stringMatching(/stack-traces.js$/),
         lineno: expect.any(Number),
         colno: expect.any(Number),
+        in_app: true,
+        module: undefined,
       },
     ]));
 
@@ -45,48 +59,60 @@ describe('e2e Tests', { timeout: 20000 }, () => {
         filename: expect.any(String),
         lineno: expect.any(Number),
         colno: expect.any(Number),
+        in_app: false,
+        module: undefined,
       },
       {
         function: 'longWork',
         filename: expect.stringMatching(/long-work.js$/),
         lineno: expect.any(Number),
         colno: expect.any(Number),
+        in_app: true,
+        module: undefined,
       },
       {
         function: '?',
         filename: expect.stringMatching(/worker.js$/),
         lineno: expect.any(Number),
         colno: expect.any(Number),
+        in_app: true,
+        module: undefined,
       },
     ]));
   });
 
-  test('detect stalled thread', { timeout: 20000 }, () => {
+  test('Detect stalled thread', { timeout: 20000 }, () => {
     const testFile = join(__dirname, 'stalled.js');
     const result = spawnSync('node', [testFile]);
 
     expect(result.status).toBe(0);
 
-    const stacks = JSON.parse(result.stdout.toString());
+    const stacks = parseStacks(JSON.parse(result.stdout.toString()));
 
     expect(stacks['0']).toEqual(expect.arrayContaining([
       {
         function: 'pbkdf2Sync',
         filename: expect.any(String),
         lineno: expect.any(Number),
         colno: expect.any(Number),
+        in_app: false,
+        module: undefined,
       },
       {
         function: 'longWork',
         filename: expect.stringMatching(/long-work.js$/),
         lineno: expect.any(Number),
         colno: expect.any(Number),
+        in_app: true,
+        module: undefined,
       },
       {
         function: '?',
         filename: expect.stringMatching(/stalled.js$/),
         lineno: expect.any(Number),
         colno: expect.any(Number),
+        in_app: true,
+        module: undefined,
       },
     ]));