Skip to content
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
20 changes: 20 additions & 0 deletions src/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,25 @@ is done executing. `Local` handles can only be allocated on the C++ stack.
Most of the V8 API uses `Local` handles to work with JavaScript values or return
them from functions.

Additionally, according to [V8 public API documentation][`v8::Local<T>`], local handles
(`v8::Local<T>`) should **never** be allocated on the heap.

This disallows heap-allocated data structures containing instances of `v8::Local`
Copy link
Member

@joyeecheung joyeecheung Mar 27, 2025

Choose a reason for hiding this comment

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

Now come to think of it, when it's done we can probably add a rule to cpplint.py to just disallow std::vector<Local> or std::vector<v8::Local> (should be pretty straight-forward to implement).


For example:

```cpp
// Don't do this
std::vector<v8::Local<v8::Value>> v1;
```

Instead, it is recommended to use `v8::LocalVector<T>` provided by V8
for such scenarios:

```cpp
v8::LocalVector<v8::Value> v1(isolate);
```

Whenever a `Local` handle is created, a `v8::HandleScope` or
`v8::EscapableHandleScope` object must exist on the stack. The `Local` is then
added to that scope and deleted along with it.
Expand Down Expand Up @@ -1409,6 +1428,7 @@ static void GetUserInfo(const FunctionCallbackInfo<Value>& args) {
[`v8.h` in Code Search]: https://cs.chromium.org/chromium/src/v8/include/v8.h
[`v8.h` in Node.js]: https://github.com/nodejs/node/blob/HEAD/deps/v8/include/v8.h
[`v8.h` in V8]: https://github.com/v8/v8/blob/HEAD/include/v8.h
[`v8::Local<T>`]: https://v8.github.io/api/head/classv8_1_1Local.html
[`vm` module]: https://nodejs.org/api/vm.html
[binding function]: #binding-functions
[cleanup hooks]: #cleanup-hooks
Expand Down
30 changes: 16 additions & 14 deletions src/node_contextify.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1166,7 +1166,7 @@ Maybe<void> StoreCodeCacheResult(
MaybeLocal<Function> CompileFunction(Local<Context> context,
Local<String> filename,
Local<String> content,
std::vector<Local<String>>* parameters) {
LocalVector<String>* parameters) {
ScriptOrigin script_origin(filename, 0, 0, true);
ScriptCompiler::Source script_source(content, script_origin);

Expand Down Expand Up @@ -1483,7 +1483,7 @@ void ContextifyContext::CompileFunction(
Context::Scope scope(parsing_context);

// Read context extensions from buffer
std::vector<Local<Object>> context_extensions;
LocalVector<Object> context_extensions(isolate);
if (!context_extensions_buf.IsEmpty()) {
for (uint32_t n = 0; n < context_extensions_buf->Length(); n++) {
Local<Value> val;
Expand All @@ -1494,7 +1494,7 @@ void ContextifyContext::CompileFunction(
}

// Read params from params buffer
std::vector<Local<String>> params;
LocalVector<String> params(isolate);
if (!params_buf.IsEmpty()) {
for (uint32_t n = 0; n < params_buf->Length(); n++) {
Local<Value> val;
Expand Down Expand Up @@ -1526,22 +1526,24 @@ void ContextifyContext::CompileFunction(
args.GetReturnValue().Set(result);
}

static std::vector<Local<String>> GetCJSParameters(IsolateData* data) {
return {
data->exports_string(),
data->require_string(),
data->module_string(),
data->__filename_string(),
data->__dirname_string(),
};
static LocalVector<String> GetCJSParameters(IsolateData* data) {
LocalVector<String> result(data->isolate(),
{
data->exports_string(),
data->require_string(),
data->module_string(),
data->__filename_string(),
data->__dirname_string(),
});
return result;
}

Local<Object> ContextifyContext::CompileFunctionAndCacheResult(
Environment* env,
Local<Context> parsing_context,
ScriptCompiler::Source* source,
std::vector<Local<String>> params,
std::vector<Local<Object>> context_extensions,
LocalVector<String> params,
LocalVector<Object> context_extensions,
ScriptCompiler::CompileOptions options,
bool produce_cached_data,
Local<Symbol> id_symbol,
Expand Down Expand Up @@ -1677,7 +1679,7 @@ static MaybeLocal<Function> CompileFunctionForCJSLoader(
options = ScriptCompiler::kConsumeCodeCache;
}

std::vector<Local<String>> params;
LocalVector<String> params(isolate);
if (is_cjs_scope) {
params = GetCJSParameters(env->isolate_data());
}
Expand Down
6 changes: 3 additions & 3 deletions src/node_contextify.h
Original file line number Diff line number Diff line change
Expand Up @@ -149,8 +149,8 @@ class ContextifyContext final : CPPGC_MIXIN(ContextifyContext) {
Environment* env,
v8::Local<v8::Context> parsing_context,
v8::ScriptCompiler::Source* source,
std::vector<v8::Local<v8::String>> params,
std::vector<v8::Local<v8::Object>> context_extensions,
v8::LocalVector<v8::String> params,
v8::LocalVector<v8::Object> context_extensions,
v8::ScriptCompiler::CompileOptions options,
bool produce_cached_data,
v8::Local<v8::Symbol> id_symbol,
Expand Down Expand Up @@ -244,7 +244,7 @@ v8::MaybeLocal<v8::Function> CompileFunction(
v8::Local<v8::Context> context,
v8::Local<v8::String> filename,
v8::Local<v8::String> content,
std::vector<v8::Local<v8::String>>* parameters);
v8::LocalVector<v8::String>* parameters);

} // namespace contextify
} // namespace node
Expand Down
17 changes: 10 additions & 7 deletions src/node_sea.cc
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ using v8::FunctionCallbackInfo;
using v8::HandleScope;
using v8::Isolate;
using v8::Local;
using v8::LocalVector;
using v8::MaybeLocal;
using v8::NewStringType;
using v8::Object;
Expand Down Expand Up @@ -450,13 +451,15 @@ std::optional<std::string> GenerateCodeCache(std::string_view main_path,
return std::nullopt;
}

std::vector<Local<String>> parameters = {
FIXED_ONE_BYTE_STRING(isolate, "exports"),
FIXED_ONE_BYTE_STRING(isolate, "require"),
FIXED_ONE_BYTE_STRING(isolate, "module"),
FIXED_ONE_BYTE_STRING(isolate, "__filename"),
FIXED_ONE_BYTE_STRING(isolate, "__dirname"),
};
LocalVector<String> parameters(
isolate,
{
FIXED_ONE_BYTE_STRING(isolate, "exports"),
FIXED_ONE_BYTE_STRING(isolate, "require"),
FIXED_ONE_BYTE_STRING(isolate, "module"),
FIXED_ONE_BYTE_STRING(isolate, "__filename"),
FIXED_ONE_BYTE_STRING(isolate, "__dirname"),
});

// TODO(RaisinTen): Using the V8 code cache prevents us from using `import()`
// in the SEA code. Support it.
Expand Down
13 changes: 8 additions & 5 deletions src/node_snapshotable.cc
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ using v8::FunctionCallbackInfo;
using v8::HandleScope;
using v8::Isolate;
using v8::Local;
using v8::LocalVector;
using v8::Object;
using v8::ObjectTemplate;
using v8::SnapshotCreator;
Expand Down Expand Up @@ -1479,11 +1480,13 @@ void CompileSerializeMain(const FunctionCallbackInfo<Value>& args) {
Local<Context> context = isolate->GetCurrentContext();
// TODO(joyeecheung): do we need all of these? Maybe we would want a less
// internal version of them.
std::vector<Local<String>> parameters = {
FIXED_ONE_BYTE_STRING(isolate, "require"),
FIXED_ONE_BYTE_STRING(isolate, "__filename"),
FIXED_ONE_BYTE_STRING(isolate, "__dirname"),
};
LocalVector<String> parameters(
isolate,
{
FIXED_ONE_BYTE_STRING(isolate, "require"),
FIXED_ONE_BYTE_STRING(isolate, "__filename"),
FIXED_ONE_BYTE_STRING(isolate, "__dirname"),
});
Local<Function> fn;
if (contextify::CompileFunction(context, filename, source, &parameters)
.ToLocal(&fn)) {
Expand Down
Loading