From 9cff6f0672894afbebed1ea841a01fdeffc6b2eb Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Tue, 3 Jun 2025 12:23:11 +0200 Subject: [PATCH] fix: Don't send objects across isolates --- .gitignore | 1 + README.md | 82 ++++++++++++++++--------------------------- module.cc | 88 +++++++++++++++++++++-------------------------- package.json | 1 + src/index.ts | 11 ++---- test/e2e.test.mjs | 32 +++++++++++++++-- test/yarn.lock | 27 --------------- yarn.lock | 5 +++ 8 files changed, 106 insertions(+), 141 deletions(-) delete mode 100644 test/yarn.lock diff --git a/.gitignore b/.gitignore index cf7a18c..9e91cfa 100644 --- a/.gitignore +++ b/.gitignore @@ -2,3 +2,4 @@ node_modules/ build/ lib/ /*.tgz +test/yarn.lock diff --git a/README.md b/README.md index 7abdbc5..e5c1011 100644 --- a/README.md +++ b/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)' } ``` diff --git a/module.cc b/module.cc index f1536f9..f9983d6 100644 --- a/module.cc +++ b/module.cc @@ -2,6 +2,7 @@ #include #include #include +#include using namespace v8; using namespace node; @@ -23,76 +24,61 @@ static std::unordered_map threads = {}; // Function to be called when an isolate's execution is interrupted static void ExecutionInterrupted(Isolate *isolate, void *data) { - auto promise = static_cast> *>(data); + auto promise = static_cast *>(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::ostringstream stack_stream; for (int i = 0; i < stack->GetFrameCount(); i++) { auto frame = stack->GetFrame(isolate, i); auto fn_name = frame->GetFunctionName(); + // Build stack trace line in JavaScript format: + // " at functionName(filename:line:column)" + stack_stream << " at "; if (frame->IsEval()) { - fn_name = - String::NewFromUtf8(isolate, "[eval]", NewStringType::kInternalized) - .ToLocalChecked(); + stack_stream << "[eval]"; } else if (fn_name.IsEmpty() || fn_name->Length() == 0) { - fn_name = String::NewFromUtf8(isolate, "?", NewStringType::kInternalized) - .ToLocalChecked(); + stack_stream << "?"; } else if (frame->IsConstructor()) { - fn_name = String::NewFromUtf8(isolate, "[constructor]", - NewStringType::kInternalized) - .ToLocalChecked(); + stack_stream << "[constructor]"; + } else { + v8::String::Utf8Value utf8_fn(isolate, fn_name); + stack_stream << (*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(); + stack_stream << " ("; - frame_obj - ->Set( - isolate->GetCurrentContext(), - String::NewFromUtf8(isolate, "colno", NewStringType::kInternalized) - .ToLocalChecked(), - Integer::New(isolate, frame->GetColumn())) - .Check(); + auto script_name = frame->GetScriptName(); + if (!script_name.IsEmpty()) { + v8::String::Utf8Value utf8_filename(isolate, script_name); + stack_stream << (*utf8_filename ? *utf8_filename : ""); + } else { + stack_stream << ""; + } - frames->Set(isolate->GetCurrentContext(), i, frame_obj).Check(); + int line_number = frame->GetLineNumber(); + int column_number = frame->GetColumn(); + + stack_stream << ":" << line_number << ":" << column_number << ")"; + + if (i < stack->GetFrameCount() - 1) { + stack_stream << "\n"; + } } - promise->set_value(frames); + promise->set_value(stack_stream.str()); } // Function to capture the stack trace of a single isolate -Local CaptureStackTrace(Isolate *isolate) { - std::promise> promise; +std::string CaptureStackTrace(Isolate *isolate) { + std::promise promise; auto future = promise.get_future(); // The v8 isolate must be interrupted to capture the stack trace @@ -105,7 +91,7 @@ Local CaptureStackTrace(Isolate *isolate) { void CaptureStackTraces(const FunctionCallbackInfo &args) { auto capture_from_isolate = args.GetIsolate(); - using ThreadResult = std::tuple>; + using ThreadResult = std::tuple; std::vector> futures; // We collect the futures into a vec so they can be processed in parallel @@ -128,13 +114,17 @@ void CaptureStackTraces(const FunctionCallbackInfo &args) { // JavaScript object Local 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); diff --git a/package.json b/package.json index 21325d8..49bbae1 100644 --- a/package.json +++ b/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", diff --git a/src/index.ts b/src/index.ts index e21d3da..314656b 100644 --- a/src/index.ts +++ b/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; + captureStackTrace(): Record; getThreadsLastSeen(): Record; } @@ -180,7 +173,7 @@ export function registerThread(threadName: string = String(threadId)): void { /** * Captures stack traces for all registered threads. */ -export function captureStackTrace(): Record { +export function captureStackTrace(): Record { return native.captureStackTrace(); } diff --git a/test/e2e.test.mjs b/test/e2e.test.mjs index fba60ee..61ec149 100644 --- a/test/e2e.test.mjs +++ b/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,7 +24,7 @@ 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([ { @@ -24,18 +32,24 @@ 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(/stack-traces.js$/), lineno: expect.any(Number), colno: expect.any(Number), + in_app: true, + module: undefined, }, ])); @@ -45,29 +59,35 @@ 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([ { @@ -75,18 +95,24 @@ 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(/stalled.js$/), lineno: expect.any(Number), colno: expect.any(Number), + in_app: true, + module: undefined, }, ])); diff --git a/test/yarn.lock b/test/yarn.lock deleted file mode 100644 index 1779b92..0000000 --- a/test/yarn.lock +++ /dev/null @@ -1,27 +0,0 @@ -# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY. -# yarn lockfile v1 - - -"@sentry-internal/node-native-stacktrace@file:../sentry-internal-node-native-stacktrace-0.1.0.tgz": - version "0.1.0" - resolved "file:../sentry-internal-node-native-stacktrace-0.1.0.tgz#9f528856b2eecdefad847e171435f0d33d2375f5" - dependencies: - detect-libc "^2.0.4" - node-abi "^4.9.0" - -detect-libc@^2.0.4: - version "2.0.4" - resolved "https://registry.yarnpkg.com/detect-libc/-/detect-libc-2.0.4.tgz#f04715b8ba815e53b4d8109655b6508a6865a7e8" - integrity sha512-3UDv+G9CsCKO1WKMGw9fwq/SWJYbI0c5Y7LU1AXYoDdbhE2AHQ6N6Nb34sG8Fj7T5APy8qXDCKuuIHd1BR0tVA== - -node-abi@^4.9.0: - version "4.9.0" - resolved "https://registry.yarnpkg.com/node-abi/-/node-abi-4.9.0.tgz#ca6dabf7991e54bf3ba6d8d32641e1b84f305263" - integrity sha512-0isb3h+AXUblx5Iv0mnYy2WsErH+dk2e9iXJXdKAtS076Q5hP+scQhp6P4tvDeVlOBlG3ROKvkpQHtbORllq2A== - dependencies: - semver "^7.6.3" - -semver@^7.6.3: - version "7.7.2" - resolved "https://registry.yarnpkg.com/semver/-/semver-7.7.2.tgz#67d99fdcd35cec21e6f8b87a7fd515a33f982b58" - integrity sha512-RF0Fw+rO5AMf9MAyaRXI4AV0Ulj5lMHqVxxdSgiVbixSCXoEmmX/jk0CuJw4+3SqroYO9VoUh+HcuJivvtJemA== diff --git a/yarn.lock b/yarn.lock index 4f2c2ad..a44e7de 100644 --- a/yarn.lock +++ b/yarn.lock @@ -388,6 +388,11 @@ resolved "https://registry.yarnpkg.com/@sentry-internal/typescript/-/typescript-9.22.0.tgz#7bce764807c0bb122126f29af74fc43bf863039c" integrity sha512-pqeMOKuzUwpMXh12ONbWUtpJ9Tey+UYnLgqPVXZEnB4vpZfJ/y4mdHsNaEXxgGLWZ+wvvSdYJB9GsGBl0iSJrQ== +"@sentry/core@^9.22.0": + version "9.25.0" + resolved "https://registry.yarnpkg.com/@sentry/core/-/core-9.25.0.tgz#354b9d500075b0db1decc533c16fe0566a30c21b" + integrity sha512-k0AgzR6RIf6OEwkVz09zer8GcK1s7RothlS1R6Z4x1wAJ+brtx4HqWnbLp05LDNDNrjTzK30HXvuCGGusnZuig== + "@types/estree@1.0.7", "@types/estree@^1.0.0": version "1.0.7" resolved "https://registry.yarnpkg.com/@types/estree/-/estree-1.0.7.tgz#4158d3105276773d5b7695cd4834b1722e4f37a8"