From a7fcea42d28a989da556053152c08a952f1c5383 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Tue, 3 Jun 2025 18:32:22 +0200 Subject: [PATCH 1/2] fix: Don't send JavaScript objects across isolates --- .gitignore | 1 + module.cc | 186 ++++++++++++++++++++++++++++--------------------- test/yarn.lock | 27 ------- 3 files changed, 108 insertions(+), 106 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/module.cc b/module.cc index f1536f9..e3e177e 100644 --- a/module.cc +++ b/module.cc @@ -21,78 +21,63 @@ static std::mutex threads_mutex; // Map to hold all registered threads and their information static std::unordered_map threads = {}; +// Structure to hold stack frame information +struct JsStackFrame { + std::string function_name; + std::string filename; + int lineno; + int colno; +}; + +// Type alias for a vector of JsStackFrame +using JsStackTrace = std::vector; + // 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)); - return; - } - - auto frames = Array::New(isolate, stack->GetFrameCount()); - - for (int i = 0; i < stack->GetFrameCount(); i++) { - auto frame = stack->GetFrame(isolate, i); - auto fn_name = frame->GetFunctionName(); - - if (frame->IsEval()) { - fn_name = - String::NewFromUtf8(isolate, "[eval]", NewStringType::kInternalized) - .ToLocalChecked(); - } else if (fn_name.IsEmpty() || fn_name->Length() == 0) { - fn_name = String::NewFromUtf8(isolate, "?", NewStringType::kInternalized) - .ToLocalChecked(); - } else if (frame->IsConstructor()) { - fn_name = String::NewFromUtf8(isolate, "[constructor]", - NewStringType::kInternalized) - .ToLocalChecked(); + JsStackTrace frames; + if (!stack.IsEmpty()) { + 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()) { + function_name = "[eval]"; + } else if (fn_name.IsEmpty() || fn_name->Length() == 0) { + function_name = "?"; + } else if (frame->IsConstructor()) { + function_name = "[constructor]"; + } else { + v8::String::Utf8Value utf8_fn(isolate, fn_name); + function_name = *utf8_fn ? *utf8_fn : "?"; + } + + 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 : ""; + } else { + filename = ""; + } + + int lineno = frame->GetLineNumber(); + int colno = frame->GetColumn(); + + frames.push_back(JsStackFrame{function_name, filename, lineno, colno}); } - - 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(); - - frame_obj - ->Set( - isolate->GetCurrentContext(), - String::NewFromUtf8(isolate, "colno", NewStringType::kInternalized) - .ToLocalChecked(), - Integer::New(isolate, frame->GetColumn())) - .Check(); - - frames->Set(isolate->GetCurrentContext(), i, frame_obj).Check(); } promise->set_value(frames); } // Function to capture the stack trace of a single isolate -Local CaptureStackTrace(Isolate *isolate) { - std::promise> promise; +JsStackTrace CaptureStackTrace(Isolate *isolate) { + std::promise promise; auto future = promise.get_future(); // The v8 isolate must be interrupted to capture the stack trace @@ -105,36 +90,79 @@ 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 - std::lock_guard lock(threads_mutex); - for (auto [thread_isolate, thread_info] : threads) { - if (thread_isolate == capture_from_isolate) - continue; - - auto thread_name = thread_info.thread_name; - - futures.emplace_back(std::async( - std::launch::async, - [thread_name](Isolate *isolate) -> ThreadResult { - return std::make_tuple(thread_name, CaptureStackTrace(isolate)); - }, - thread_isolate)); + { + std::lock_guard lock(threads_mutex); + for (auto [thread_isolate, thread_info] : threads) { + if (thread_isolate == capture_from_isolate) + continue; + auto thread_name = thread_info.thread_name; + + futures.emplace_back(std::async( + std::launch::async, + [thread_name](Isolate *isolate) -> ThreadResult { + return std::make_tuple(thread_name, CaptureStackTrace(isolate)); + }, + thread_isolate)); + } } - // We wait for all futures to complete and collect their results into a - // JavaScript object Local result = Object::New(capture_from_isolate); + for (auto &future : futures) { auto [thread_name, frames] = 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(); + Local jsFrames = + Array::New(capture_from_isolate, static_cast(frames.size())); + for (size_t i = 0; i < frames.size(); ++i) { + const auto &f = frames[i]; + Local frameObj = Object::New(capture_from_isolate); + frameObj + ->Set(capture_from_isolate->GetCurrentContext(), + String::NewFromUtf8(capture_from_isolate, "function", + NewStringType::kInternalized) + .ToLocalChecked(), + String::NewFromUtf8(capture_from_isolate, + f.function_name.c_str(), + NewStringType::kNormal) + .ToLocalChecked()) + .Check(); + frameObj + ->Set(capture_from_isolate->GetCurrentContext(), + String::NewFromUtf8(capture_from_isolate, "filename", + NewStringType::kInternalized) + .ToLocalChecked(), + String::NewFromUtf8(capture_from_isolate, f.filename.c_str(), + NewStringType::kNormal) + .ToLocalChecked()) + .Check(); + frameObj + ->Set(capture_from_isolate->GetCurrentContext(), + String::NewFromUtf8(capture_from_isolate, "lineno", + NewStringType::kInternalized) + .ToLocalChecked(), + Integer::New(capture_from_isolate, f.lineno)) + .Check(); + frameObj + ->Set(capture_from_isolate->GetCurrentContext(), + String::NewFromUtf8(capture_from_isolate, "colno", + NewStringType::kInternalized) + .ToLocalChecked(), + Integer::New(capture_from_isolate, f.colno)) + .Check(); + jsFrames + ->Set(capture_from_isolate->GetCurrentContext(), + static_cast(i), frameObj) + .Check(); + } + + result->Set(capture_from_isolate->GetCurrentContext(), key, jsFrames) + .Check(); } args.GetReturnValue().Set(result); 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== From e05d3871aa0a8a627f9a29b00c3368db561dd826 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Thu, 5 Jun 2025 16:16:56 +0200 Subject: [PATCH 2/2] PR review --- module.cc | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/module.cc b/module.cc index e3e177e..bdb09c0 100644 --- a/module.cc +++ b/module.cc @@ -89,6 +89,7 @@ JsStackTrace CaptureStackTrace(Isolate *isolate) { // Function to capture stack traces from all registered threads void CaptureStackTraces(const FunctionCallbackInfo &args) { auto capture_from_isolate = args.GetIsolate(); + auto current_context = capture_from_isolate->GetCurrentContext(); using ThreadResult = std::tuple; std::vector> futures; @@ -117,13 +118,12 @@ void CaptureStackTraces(const FunctionCallbackInfo &args) { NewStringType::kNormal) .ToLocalChecked(); - Local jsFrames = - Array::New(capture_from_isolate, static_cast(frames.size())); + Local jsFrames = Array::New(capture_from_isolate, frames.size()); for (size_t i = 0; i < frames.size(); ++i) { const auto &f = frames[i]; Local frameObj = Object::New(capture_from_isolate); frameObj - ->Set(capture_from_isolate->GetCurrentContext(), + ->Set(current_context, String::NewFromUtf8(capture_from_isolate, "function", NewStringType::kInternalized) .ToLocalChecked(), @@ -133,7 +133,7 @@ void CaptureStackTraces(const FunctionCallbackInfo &args) { .ToLocalChecked()) .Check(); frameObj - ->Set(capture_from_isolate->GetCurrentContext(), + ->Set(current_context, String::NewFromUtf8(capture_from_isolate, "filename", NewStringType::kInternalized) .ToLocalChecked(), @@ -142,27 +142,24 @@ void CaptureStackTraces(const FunctionCallbackInfo &args) { .ToLocalChecked()) .Check(); frameObj - ->Set(capture_from_isolate->GetCurrentContext(), + ->Set(current_context, String::NewFromUtf8(capture_from_isolate, "lineno", NewStringType::kInternalized) .ToLocalChecked(), Integer::New(capture_from_isolate, f.lineno)) .Check(); frameObj - ->Set(capture_from_isolate->GetCurrentContext(), + ->Set(current_context, String::NewFromUtf8(capture_from_isolate, "colno", NewStringType::kInternalized) .ToLocalChecked(), Integer::New(capture_from_isolate, f.colno)) .Check(); - jsFrames - ->Set(capture_from_isolate->GetCurrentContext(), - static_cast(i), frameObj) + jsFrames->Set(current_context, static_cast(i), frameObj) .Check(); } - result->Set(capture_from_isolate->GetCurrentContext(), key, jsFrames) - .Check(); + result->Set(current_context, key, jsFrames).Check(); } args.GetReturnValue().Set(result);