fix: Don't send JavaScript objects between isolates

[timfish]

Once you create v8 objects in an isolate, you cannot return them from another isolate. This PR changes it so we pass a vector of intermediate JsStackFrame structs and construct the v8 JavaScript objects on the correct isolate.

The CI test's that were failing are now passing so this definitely fixes the intermittent issues we were seeing.

[timfish]

@codecov-ai-reviewer review

[codecov-ai]

PR Description

This pull request refactors the stack trace capture mechanism in the native module to improve performance and maintainability. It replaces the direct use of V8 objects within the stack trace capture logic with a C++ data structure, JsStackFrame, to represent stack frames. This allows for better separation of concerns and reduces the overhead of creating and manipulating V8 objects during the interrupt handler.

Click to see more

Key Technical Changes

The key technical changes include:

1. Introduction of JsStackFrame and JsStackTrace: These C++ structures replace the direct use of V8 objects for representing stack frames and stack traces, respectively.
2. Refactoring of ExecutionInterrupted: This function now populates a JsStackTrace instead of directly creating a V8 array. This reduces the amount of V8 API calls within the interrupt handler.
3. Change of Promise Type: The promise used for asynchronous stack trace capture now resolves to a JsStackTrace instead of a Local<Array>.
4. Deferred V8 Object Creation: The conversion from JsStackFrame to V8 objects is now performed outside the interrupt handler in the CaptureStackTraces function, minimizing the time spent in the interrupt.
5. Yarn Lockfile Handling: The test/yarn.lock file is now ignored, as it's a generated file specific to the test environment.

Architecture Decisions

The main architectural decision is to decouple the stack trace representation from the V8 API. This improves the module's resilience to V8 API changes and makes the code easier to test and maintain. The use of std::future and std::async allows for parallel stack trace capture from different isolates, improving overall performance. Scoping the lock guard more narrowly in CaptureStackTraces minimizes contention and improves concurrency.

Dependencies and Interactions

This change primarily affects the internal implementation of stack trace capture. It does not introduce any new external dependencies. It interacts with the V8 API for stack trace retrieval and object creation, but the interaction is now more localized. The RegisterThread and GetThreadsLastSeen functions are not directly affected but rely on the same thread management mechanism.

Risk Considerations

The primary risk is potential performance regressions due to the overhead of copying data between the V8 heap and the C++ data structures. However, this is mitigated by performing the V8 object creation outside the interrupt handler. Thorough testing is required to ensure that the changes do not introduce any memory leaks or race conditions. The UTF-8 conversion from V8 strings to C++ strings could potentially fail, requiring careful error handling.

Notable Implementation Details

A notable implementation detail is the use of v8::String::Utf8Value to convert V8 strings to C++ strings. The code defaults to '?' if the conversion fails. The lock guard in CaptureStackTraces is scoped to only protect access to the threads map, allowing the asynchronous stack trace captures to proceed in parallel. The test/yarn.lock file is added to .gitignore because it's specific to the test environment and contains local file references.

[AbhiPrasad]

last time I wrote c++ I was grinding a uni assignment so hopefully these apply 🙏

[AbhiPrasad]

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.

[timfish]

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 : "?";

[AbhiPrasad]

This crashes the process for it's not defined - https://github.com/v8/v8/blob/2786ffa2b1ed3ea03e9762b281d82983867ffcb0/include/v8-maybe.h#L49. How bad is to to crash the process?

FromMaybe looks safer: https://github.com/v8/v8/blob/2786ffa2b1ed3ea03e9762b281d82983867ffcb0/include/v8-maybe.h#L84

[timfish]

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

[AbhiPrasad]

Let's merge this in for now, we can get further feedback later.