Skip to content
Permalink
Browse files

vm: fix gc bug with modules and compiled functions

PR-URL: #28671
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
  • Loading branch information...
devsnek committed Jul 19, 2019
1 parent b7bcfc9 commit 68c83f962e002a47f61648c5aa2adb41dbe6a358
Showing with 69 additions and 46 deletions.
  1. +3 −3 lib/internal/modules/cjs/loader.js
  2. +11 −1 lib/vm.js
  3. +18 −9 src/env.cc
  4. +8 −4 src/env.h
  5. +1 −1 src/module_wrap.cc
  6. +28 −26 src/node_contextify.cc
  7. +0 −2 src/node_contextify.h
@@ -705,7 +705,7 @@ function wrapSafe(filename, content) {
});
}

const compiledWrapper = compileFunction(
const compiled = compileFunction(
content,
filename,
0,
@@ -725,15 +725,15 @@ function wrapSafe(filename, content) {

if (experimentalModules) {
const { callbackMap } = internalBinding('module_wrap');
callbackMap.set(compiledWrapper, {
callbackMap.set(compiled.cacheKey, {
importModuleDynamically: async (specifier) => {
const loader = await asyncESM.loaderPromise;
return loader.import(specifier, normalizeReferrerURL(filename));
}
});
}

return compiledWrapper;
return compiled.function;
}

// Run the file contents in the correct scope or sandbox. Expose
@@ -383,7 +383,7 @@ function compileFunction(code, params, options = {}) {
}
});

return _compileFunction(
const result = _compileFunction(
code,
filename,
lineOffset,
@@ -394,6 +394,16 @@ function compileFunction(code, params, options = {}) {
contextExtensions,
params
);

if (produceCachedData) {
result.function.cachedDataProduced = result.cachedDataProduced;
}

if (result.cachedData) {
result.function.cachedData = result.cachedData;
}

return result.function;
}


@@ -39,13 +39,15 @@ using v8::NewStringType;
using v8::Number;
using v8::Object;
using v8::Private;
using v8::ScriptOrModule;
using v8::SnapshotCreator;
using v8::StackTrace;
using v8::String;
using v8::Symbol;
using v8::TracingController;
using v8::Undefined;
using v8::Value;
using v8::WeakCallbackInfo;
using worker::Worker;

int const Environment::kNodeContextTag = 0x6e6f64;
@@ -385,9 +387,22 @@ Environment::Environment(IsolateData* isolate_data,
CreateProperties();
}

CompileFnEntry::CompileFnEntry(Environment* env, uint32_t id)
: env(env), id(id) {
env->compile_fn_entries.insert(this);
static void WeakCallbackCompiledFn(
const WeakCallbackInfo<CompiledFnEntry>& data) {
CompiledFnEntry* entry = data.GetParameter();
entry->env->id_to_function_map.erase(entry->id);
delete entry;
}

CompiledFnEntry::CompiledFnEntry(Environment* env,
uint32_t id,
Local<ScriptOrModule> script)
: env(env),
id(id),
cache_key(env->isolate(), Object::New(env->isolate())),
script(env->isolate(), script) {
this->script.SetWeak(
this, WeakCallbackCompiledFn, v8::WeakCallbackType::kParameter);
}

Environment::~Environment() {
@@ -398,12 +413,6 @@ Environment::~Environment() {
// CleanupHandles() should have removed all of them.
CHECK(file_handle_read_wrap_freelist_.empty());

// dispose the Persistent references to the compileFunction
// wrappers used in the dynamic import callback
for (auto& entry : compile_fn_entries) {
delete entry;
}

HandleScope handle_scope(isolate());

#if HAVE_INSPECTOR
@@ -179,6 +179,7 @@ constexpr size_t kFsStatsBufferLength =
V(cached_data_produced_string, "cachedDataProduced") \
V(cached_data_rejected_string, "cachedDataRejected") \
V(cached_data_string, "cachedData") \
V(cache_key_string, "cacheKey") \
V(change_string, "change") \
V(channel_string, "channel") \
V(chunks_sent_since_last_write_string, "chunksSentSinceLastWrite") \
@@ -522,10 +523,14 @@ struct ContextInfo {
bool is_default = false;
};

struct CompileFnEntry {
struct CompiledFnEntry {
Environment* env;
uint32_t id;
CompileFnEntry(Environment* env, uint32_t id);
v8::Global<v8::Object> cache_key;
v8::Global<v8::ScriptOrModule> script;
CompiledFnEntry(Environment* env,
uint32_t id,
v8::Local<v8::ScriptOrModule> script);
};

// Listing the AsyncWrap provider types first enables us to cast directly
@@ -1005,8 +1010,7 @@ class Environment : public MemoryRetainer {
std::unordered_map<uint32_t, loader::ModuleWrap*> id_to_module_map;
std::unordered_map<uint32_t, contextify::ContextifyScript*>
id_to_script_map;
std::unordered_set<CompileFnEntry*> compile_fn_entries;
std::unordered_map<uint32_t, v8::Global<v8::Function>> id_to_function_map;
std::unordered_map<uint32_t, CompiledFnEntry*> id_to_function_map;

inline uint32_t get_next_module_id();
inline uint32_t get_next_script_id();
@@ -1041,7 +1041,7 @@ static MaybeLocal<Promise> ImportModuleDynamically(
ModuleWrap* wrap = ModuleWrap::GetFromID(env, id);
object = wrap->object();
} else if (type == ScriptType::kFunction) {
object = env->id_to_function_map.find(id)->second.Get(iso);
object = env->id_to_function_map.find(id)->second->cache_key.Get(iso);
} else {
UNREACHABLE();
}
@@ -66,6 +66,7 @@ using v8::PropertyHandlerFlags;
using v8::Script;
using v8::ScriptCompiler;
using v8::ScriptOrigin;
using v8::ScriptOrModule;
using v8::String;
using v8::Symbol;
using v8::Uint32;
@@ -305,15 +306,6 @@ void ContextifyContext::WeakCallback(
delete context;
}

void ContextifyContext::WeakCallbackCompileFn(
const WeakCallbackInfo<CompileFnEntry>& data) {
CompileFnEntry* entry = data.GetParameter();
if (entry->env->compile_fn_entries.erase(entry) != 0) {
entry->env->id_to_function_map.erase(entry->id);
delete entry;
}
}

// static
ContextifyContext* ContextifyContext::ContextFromContextifiedSandbox(
Environment* env,
@@ -1117,9 +1109,11 @@ void ContextifyContext::CompileFunction(
}
}

Local<ScriptOrModule> script;
MaybeLocal<Function> maybe_fn = ScriptCompiler::CompileFunctionInContext(
parsing_context, &source, params.size(), params.data(),
context_extensions.size(), context_extensions.data(), options);
context_extensions.size(), context_extensions.data(), options,
v8::ScriptCompiler::NoCacheReason::kNoCacheNoReason, &script);

if (maybe_fn.IsEmpty()) {
if (try_catch.HasCaught() && !try_catch.HasTerminated()) {
@@ -1129,13 +1123,17 @@ void ContextifyContext::CompileFunction(
return;
}
Local<Function> fn = maybe_fn.ToLocalChecked();
env->id_to_function_map.emplace(std::piecewise_construct,
std::make_tuple(id),
std::make_tuple(isolate, fn));
CompileFnEntry* gc_entry = new CompileFnEntry(env, id);
env->id_to_function_map[id].SetWeak(gc_entry,
WeakCallbackCompileFn,
v8::WeakCallbackType::kParameter);

CompiledFnEntry* entry = new CompiledFnEntry(env, id, script);
env->id_to_function_map.emplace(id, entry);
Local<Object> cache_key = entry->cache_key.Get(isolate);

Local<Object> result = Object::New(isolate);
if (result->Set(parsing_context, env->function_string(), fn).IsNothing())
return;
if (result->Set(parsing_context, env->cache_key_string(), cache_key)
.IsNothing())
return;

if (produce_cached_data) {
const std::unique_ptr<ScriptCompiler::CachedData> cached_data(
@@ -1146,18 +1144,22 @@ void ContextifyContext::CompileFunction(
env,
reinterpret_cast<const char*>(cached_data->data),
cached_data->length);
if (fn->Set(
parsing_context,
env->cached_data_string(),
buf.ToLocalChecked()).IsNothing()) return;
if (result
->Set(parsing_context,
env->cached_data_string(),
buf.ToLocalChecked())
.IsNothing())
return;
}
if (fn->Set(
parsing_context,
env->cached_data_produced_string(),
Boolean::New(isolate, cached_data_produced)).IsNothing()) return;
if (result
->Set(parsing_context,
env->cached_data_produced_string(),
Boolean::New(isolate, cached_data_produced))
.IsNothing())
return;
}

args.GetReturnValue().Set(fn);
args.GetReturnValue().Set(result);
}

static void StartSigintWatchdog(const FunctionCallbackInfo<Value>& args) {
@@ -63,8 +63,6 @@ class ContextifyContext {
const v8::FunctionCallbackInfo<v8::Value>& args);
static void WeakCallback(
const v8::WeakCallbackInfo<ContextifyContext>& data);
static void WeakCallbackCompileFn(
const v8::WeakCallbackInfo<CompileFnEntry>& data);
static void PropertyGetterCallback(
v8::Local<v8::Name> property,
const v8::PropertyCallbackInfo<v8::Value>& args);

0 comments on commit 68c83f9

Please sign in to comment.
You can’t perform that action at this time.