Skip to content

Commit

Permalink
vm: expose cachedDataRejected for vm.compileFunction
Browse files Browse the repository at this point in the history
Having this information available is useful for functions just as
it is for scripts. Therefore, expose it in the same way that other
information related to code caching is reported.

As part of this, de-duplify the code for setting the properties on
the C++ side and add proper exception handling to it.

PR-URL: #46320
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
  • Loading branch information
addaleax authored and ruyadorno committed Jan 31, 2023
1 parent 88b904c commit 34d70ce
Show file tree
Hide file tree
Showing 5 changed files with 114 additions and 55 deletions.
6 changes: 6 additions & 0 deletions doc/api/vm.md
Original file line number Diff line number Diff line change
Expand Up @@ -962,6 +962,12 @@ const vm = require('node:vm');
<!-- YAML
added: v10.10.0
changes:
- version:
- REPLACEME
pr-url: https://github.com/nodejs/node/pull/46320
description: The return value now includes `cachedDataRejected`
with the same semantics as the `vm.Script` version
if the `cachedData` option was passed.
- version:
- v17.0.0
- v16.12.0
Expand Down
4 changes: 4 additions & 0 deletions lib/internal/vm.js
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,10 @@ function internalCompileFunction(code, params, options) {
result.function.cachedData = result.cachedData;
}

if (typeof result.cachedDataRejected === 'boolean') {
result.function.cachedDataRejected = result.cachedDataRejected;
}

if (importModuleDynamically !== undefined) {
validateFunction(importModuleDynamically,
'options.importModuleDynamically');
Expand Down
125 changes: 76 additions & 49 deletions src/node_contextify.cc
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ using v8::HandleScope;
using v8::IndexedPropertyHandlerConfiguration;
using v8::Int32;
using v8::Isolate;
using v8::Just;
using v8::Local;
using v8::Maybe;
using v8::MaybeLocal;
Expand All @@ -58,6 +59,7 @@ using v8::MicrotaskQueue;
using v8::MicrotasksPolicy;
using v8::Name;
using v8::NamedPropertyHandlerConfiguration;
using v8::Nothing;
using v8::Number;
using v8::Object;
using v8::ObjectTemplate;
Expand Down Expand Up @@ -857,40 +859,75 @@ void ContextifyScript::New(const FunctionCallbackInfo<Value>& args) {
}
contextify_script->script_.Reset(isolate, v8_script);

Local<Context> env_context = env->context();
if (compile_options == ScriptCompiler::kConsumeCodeCache) {
args.This()->Set(
env_context,
env->cached_data_rejected_string(),
Boolean::New(isolate, source.GetCachedData()->rejected)).Check();
} else if (produce_cached_data) {
std::unique_ptr<ScriptCompiler::CachedData> cached_data{
ScriptCompiler::CreateCodeCache(v8_script)};
bool cached_data_produced = cached_data != nullptr;
if (cached_data_produced) {
MaybeLocal<Object> buf = Buffer::Copy(
env,
reinterpret_cast<const char*>(cached_data->data),
cached_data->length);
args.This()->Set(env_context,
env->cached_data_string(),
buf.ToLocalChecked()).Check();
}
args.This()->Set(
env_context,
env->cached_data_produced_string(),
Boolean::New(isolate, cached_data_produced)).Check();
std::unique_ptr<ScriptCompiler::CachedData> new_cached_data;
if (produce_cached_data) {
new_cached_data.reset(ScriptCompiler::CreateCodeCache(v8_script));
}

args.This()
->Set(env_context,
env->source_map_url_string(),
v8_script->GetSourceMappingURL())
.Check();
if (StoreCodeCacheResult(env,
args.This(),
compile_options,
source,
produce_cached_data,
std::move(new_cached_data))
.IsNothing()) {
return;
}

if (args.This()
->Set(env->context(),
env->source_map_url_string(),
v8_script->GetSourceMappingURL())
.IsNothing())
return;

TRACE_EVENT_END0(TRACING_CATEGORY_NODE2(vm, script), "ContextifyScript::New");
}

Maybe<bool> StoreCodeCacheResult(
Environment* env,
Local<Object> target,
ScriptCompiler::CompileOptions compile_options,
const v8::ScriptCompiler::Source& source,
bool produce_cached_data,
std::unique_ptr<ScriptCompiler::CachedData> new_cached_data) {
Local<Context> context;
if (!target->GetCreationContext().ToLocal(&context)) {
return Nothing<bool>();
}
if (compile_options == ScriptCompiler::kConsumeCodeCache) {
if (target
->Set(
context,
env->cached_data_rejected_string(),
Boolean::New(env->isolate(), source.GetCachedData()->rejected))
.IsNothing()) {
return Nothing<bool>();
}
}
if (produce_cached_data) {
bool cached_data_produced = new_cached_data != nullptr;
if (cached_data_produced) {
MaybeLocal<Object> buf =
Buffer::Copy(env,
reinterpret_cast<const char*>(new_cached_data->data),
new_cached_data->length);
if (target->Set(context, env->cached_data_string(), buf.ToLocalChecked())
.IsNothing()) {
return Nothing<bool>();
}
}
if (target
->Set(context,
env->cached_data_produced_string(),
Boolean::New(env->isolate(), cached_data_produced))
.IsNothing()) {
return Nothing<bool>();
}
}
return Just(true);
}

bool ContextifyScript::InstanceOf(Environment* env,
const Local<Value>& value) {
return !value.IsEmpty() &&
Expand Down Expand Up @@ -1242,28 +1279,18 @@ void ContextifyContext::CompileFunction(
.IsNothing())
return;

std::unique_ptr<ScriptCompiler::CachedData> new_cached_data;
if (produce_cached_data) {
const std::unique_ptr<ScriptCompiler::CachedData> cached_data(
ScriptCompiler::CreateCodeCacheForFunction(fn));
bool cached_data_produced = cached_data != nullptr;
if (cached_data_produced) {
MaybeLocal<Object> buf = Buffer::Copy(
env,
reinterpret_cast<const char*>(cached_data->data),
cached_data->length);
if (result
->Set(parsing_context,
env->cached_data_string(),
buf.ToLocalChecked())
.IsNothing())
return;
}
if (result
->Set(parsing_context,
env->cached_data_produced_string(),
Boolean::New(isolate, cached_data_produced))
.IsNothing())
return;
new_cached_data.reset(ScriptCompiler::CreateCodeCacheForFunction(fn));
}
if (StoreCodeCacheResult(env,
result,
options,
source,
produce_cached_data,
std::move(new_cached_data))
.IsNothing()) {
return;
}

args.GetReturnValue().Set(result);
Expand Down
8 changes: 8 additions & 0 deletions src/node_contextify.h
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,14 @@ class CompiledFnEntry final : public BaseObject {
static void WeakCallback(const v8::WeakCallbackInfo<CompiledFnEntry>& data);
};

v8::Maybe<bool> StoreCodeCacheResult(
Environment* env,
v8::Local<v8::Object> target,
v8::ScriptCompiler::CompileOptions compile_options,
const v8::ScriptCompiler::Source& source,
bool produce_cached_data,
std::unique_ptr<v8::ScriptCompiler::CachedData> new_cached_data);

} // namespace contextify
} // namespace node

Expand Down
26 changes: 20 additions & 6 deletions test/parallel/test-vm-basic.js
Original file line number Diff line number Diff line change
Expand Up @@ -323,13 +323,27 @@ const vm = require('vm');
global
);

// Test compileFunction produceCachedData option
const result = vm.compileFunction('console.log("Hello, World!")', [], {
produceCachedData: true,
});
{
const source = 'console.log("Hello, World!")';
// Test compileFunction produceCachedData option
const result = vm.compileFunction(source, [], {
produceCachedData: true,
});

assert.ok(result.cachedDataProduced);
assert.ok(result.cachedData.length > 0);
assert.ok(result.cachedDataProduced);
assert.ok(result.cachedData.length > 0);

// Test compileFunction cachedData consumption
const result2 = vm.compileFunction(source, [], {
cachedData: result.cachedData
});
assert.strictEqual(result2.cachedDataRejected, false);

const result3 = vm.compileFunction('console.log("wrong source")', [], {
cachedData: result.cachedData
});
assert.strictEqual(result3.cachedDataRejected, true);
}

// Resetting value
Error.stackTraceLimit = oldLimit;
Expand Down

0 comments on commit 34d70ce

Please sign in to comment.