Skip to content

fix: Don't send JavaScript objects between isolates #4

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jun 5, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,4 @@ node_modules/
build/
lib/
/*.tgz
test/yarn.lock
183 changes: 104 additions & 79 deletions module.cc
Original file line number Diff line number Diff line change
Expand Up @@ -21,78 +21,63 @@ static std::mutex threads_mutex;
// Map to hold all registered threads and their information
static std::unordered_map<v8::Isolate *, ThreadInfo> 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<JsStackFrame>;

// Function to be called when an isolate's execution is interrupted
static void ExecutionInterrupted(Isolate *isolate, void *data) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this throw in any way? If it does, it'll create a deadlock because promise->set_value(frames); can never be called.

Only thing that looks sus is the string conversions with things like utf8_fn.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the string conversions we check for a nullptr and return a default

        v8::String::Utf8Value utf8_fn(isolate, fn_name);
        function_name = *utf8_fn ? *utf8_fn : "?";

auto promise = static_cast<std::promise<Local<Array>> *>(data);
auto promise = static_cast<std::promise<JsStackTrace> *>(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 : "<unknown>";
} else {
filename = "<unknown>";
}

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<Array> CaptureStackTrace(Isolate *isolate) {
std::promise<Local<Array>> promise;
JsStackTrace CaptureStackTrace(Isolate *isolate) {
std::promise<JsStackTrace> promise;
auto future = promise.get_future();

// The v8 isolate must be interrupted to capture the stack trace
Expand All @@ -104,37 +89,77 @@ Local<Array> CaptureStackTrace(Isolate *isolate) {
// Function to capture stack traces from all registered threads
void CaptureStackTraces(const FunctionCallbackInfo<Value> &args) {
auto capture_from_isolate = args.GetIsolate();
auto current_context = capture_from_isolate->GetCurrentContext();

using ThreadResult = std::tuple<std::string, Local<Array>>;
using ThreadResult = std::tuple<std::string, JsStackTrace>;
std::vector<std::future<ThreadResult>> futures;

// We collect the futures into a vec so they can be processed in parallel
std::lock_guard<std::mutex> 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<std::mutex> 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<Object> 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<Array> jsFrames = Array::New(capture_from_isolate, frames.size());
for (size_t i = 0; i < frames.size(); ++i) {
const auto &f = frames[i];
Local<Object> frameObj = Object::New(capture_from_isolate);
frameObj
->Set(current_context,
String::NewFromUtf8(capture_from_isolate, "function",
NewStringType::kInternalized)
.ToLocalChecked(),
String::NewFromUtf8(capture_from_isolate,
f.function_name.c_str(),
NewStringType::kNormal)
.ToLocalChecked())
.Check();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've seen this used like this everywhere so can only assume it's ok to use 🤷‍♂️
https://github.com/search?q=%2FToLocalChecked%5C(%5C)%2F%20%20%2FCheck%5C(%5C)%2F&type=code

frameObj
->Set(current_context,
String::NewFromUtf8(capture_from_isolate, "filename",
NewStringType::kInternalized)
.ToLocalChecked(),
String::NewFromUtf8(capture_from_isolate, f.filename.c_str(),
NewStringType::kNormal)
.ToLocalChecked())
.Check();
frameObj
->Set(current_context,
String::NewFromUtf8(capture_from_isolate, "lineno",
NewStringType::kInternalized)
.ToLocalChecked(),
Integer::New(capture_from_isolate, f.lineno))
.Check();
frameObj
->Set(current_context,
String::NewFromUtf8(capture_from_isolate, "colno",
NewStringType::kInternalized)
.ToLocalChecked(),
Integer::New(capture_from_isolate, f.colno))
.Check();
jsFrames->Set(current_context, static_cast<uint32_t>(i), frameObj)
.Check();
}

result->Set(current_context, key, jsFrames).Check();
}

args.GetReturnValue().Set(result);
Expand Down
27 changes: 0 additions & 27 deletions test/yarn.lock

This file was deleted.