From 674d33cb8c288b0e91d6e9e742e9739b2383e847 Mon Sep 17 00:00:00 2001 From: Gus Caplan Date: Fri, 19 Jul 2019 09:22:56 -0500 Subject: [PATCH 1/2] deps: V8: backport b33af60 Original commit message: [api] Get ScriptOrModule from CompileFunctionInContext Adds a new out param which allows accessing the ScriptOrModule of a function, which allows an embedder such as Node.js to use the function's i::Script lifetime. Refs: https://github.com/nodejs/node-v8/issues/111 Change-Id: I34346d94d76e8f9b8377c97d948673f4b95eb9d5 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1699698 Reviewed-by: Yang Guo Commit-Queue: Yang Guo Cr-Commit-Position: refs/heads/master@{#62830} Refs: https://github.com/v8/v8/commit/b33af60dd9e7e5b2557b9fbf3fdb80209f6db844 Backport-PR-URL: https://github.com/nodejs/node/pull/28779 PR-URL: https://github.com/nodejs/node/pull/28671 Reviewed-By: Anna Henningsen Reviewed-By: Guy Bedford --- common.gypi | 2 +- deps/v8/include/v8.h | 7 ++ deps/v8/src/api.cc | 133 ++++++++++++++++----------- deps/v8/test/cctest/test-compiler.cc | 9 +- 4 files changed, 93 insertions(+), 58 deletions(-) diff --git a/common.gypi b/common.gypi index 36e9745009ee44..1eadd779a34427 100644 --- a/common.gypi +++ b/common.gypi @@ -38,7 +38,7 @@ # Reset this number to 0 on major V8 upgrades. # Increment by one for each non-official patch applied to deps/v8. - 'v8_embedder_string': '-node.15', + 'v8_embedder_string': '-node.16', ##### V8 defaults for Node.js ##### diff --git a/deps/v8/include/v8.h b/deps/v8/include/v8.h index 3682c888cc8563..79c17ad57f8923 100644 --- a/deps/v8/include/v8.h +++ b/deps/v8/include/v8.h @@ -1703,6 +1703,13 @@ class V8_EXPORT ScriptCompiler { CompileOptions options = kNoCompileOptions, NoCacheReason no_cache_reason = kNoCacheNoReason); + static V8_WARN_UNUSED_RESULT MaybeLocal CompileFunctionInContext( + Local context, Source* source, size_t arguments_count, + Local arguments[], size_t context_extension_count, + Local context_extensions[], CompileOptions options, + NoCacheReason no_cache_reason, + Local* script_or_module_out); + /** * Creates and returns code cache for the specified unbound_script. * This will return nullptr if the script cannot be serialized. The diff --git a/deps/v8/src/api.cc b/deps/v8/src/api.cc index f4f3fa309eb556..4127d6735adb28 100644 --- a/deps/v8/src/api.cc +++ b/deps/v8/src/api.cc @@ -2500,69 +2500,92 @@ MaybeLocal ScriptCompiler::CompileFunctionInContext( Local arguments[], size_t context_extension_count, Local context_extensions[], CompileOptions options, NoCacheReason no_cache_reason) { - PREPARE_FOR_EXECUTION(v8_context, ScriptCompiler, CompileFunctionInContext, - Function); - TRACE_EVENT_CALL_STATS_SCOPED(isolate, "v8", "V8.ScriptCompiler"); + return ScriptCompiler::CompileFunctionInContext( + v8_context, source, arguments_count, arguments, context_extension_count, + context_extensions, options, no_cache_reason, nullptr); +} - DCHECK(options == CompileOptions::kConsumeCodeCache || - options == CompileOptions::kEagerCompile || - options == CompileOptions::kNoCompileOptions); +MaybeLocal ScriptCompiler::CompileFunctionInContext( + Local v8_context, Source* source, size_t arguments_count, + Local arguments[], size_t context_extension_count, + Local context_extensions[], CompileOptions options, + NoCacheReason no_cache_reason, + Local* script_or_module_out) { + Local result; - i::Handle context = Utils::OpenHandle(*v8_context); + { + PREPARE_FOR_EXECUTION(v8_context, ScriptCompiler, CompileFunctionInContext, + Function); + TRACE_EVENT_CALL_STATS_SCOPED(isolate, "v8", "V8.ScriptCompiler"); - DCHECK(context->IsNativeContext()); - i::Handle outer_info( - context->empty_function()->shared(), isolate); - - i::Handle fun; - i::Handle arguments_list = - isolate->factory()->NewFixedArray(static_cast(arguments_count)); - for (int i = 0; i < static_cast(arguments_count); i++) { - i::Handle argument = Utils::OpenHandle(*arguments[i]); - if (!IsIdentifier(isolate, argument)) return Local(); - arguments_list->set(i, *argument); - } - - for (size_t i = 0; i < context_extension_count; ++i) { - i::Handle extension = - Utils::OpenHandle(*context_extensions[i]); - if (!extension->IsJSObject()) return Local(); - context = isolate->factory()->NewWithContext( - context, - i::ScopeInfo::CreateForWithScope( - isolate, - context->IsNativeContext() - ? i::Handle::null() - : i::Handle(context->scope_info(), isolate)), - extension); - } + DCHECK(options == CompileOptions::kConsumeCodeCache || + options == CompileOptions::kEagerCompile || + options == CompileOptions::kNoCompileOptions); - i::Compiler::ScriptDetails script_details = GetScriptDetails( - isolate, source->resource_name, source->resource_line_offset, - source->resource_column_offset, source->source_map_url, - source->host_defined_options); + i::Handle context = Utils::OpenHandle(*v8_context); - i::ScriptData* script_data = nullptr; - if (options == kConsumeCodeCache) { - DCHECK(source->cached_data); - // ScriptData takes care of pointer-aligning the data. - script_data = new i::ScriptData(source->cached_data->data, - source->cached_data->length); + DCHECK(context->IsNativeContext()); + + i::Handle arguments_list = + isolate->factory()->NewFixedArray(static_cast(arguments_count)); + for (int i = 0; i < static_cast(arguments_count); i++) { + i::Handle argument = Utils::OpenHandle(*arguments[i]); + if (!IsIdentifier(isolate, argument)) return Local(); + arguments_list->set(i, *argument); + } + + for (size_t i = 0; i < context_extension_count; ++i) { + i::Handle extension = + Utils::OpenHandle(*context_extensions[i]); + if (!extension->IsJSObject()) return Local(); + context = isolate->factory()->NewWithContext( + context, + i::ScopeInfo::CreateForWithScope( + isolate, + context->IsNativeContext() + ? i::Handle::null() + : i::Handle(context->scope_info(), isolate)), + extension); + } + + i::Compiler::ScriptDetails script_details = GetScriptDetails( + isolate, source->resource_name, source->resource_line_offset, + source->resource_column_offset, source->source_map_url, + source->host_defined_options); + + i::ScriptData* script_data = nullptr; + if (options == kConsumeCodeCache) { + DCHECK(source->cached_data); + // ScriptData takes care of pointer-aligning the data. + script_data = new i::ScriptData(source->cached_data->data, + source->cached_data->length); + } + + i::Handle scoped_result; + has_pending_exception = + !i::Compiler::GetWrappedFunction( + Utils::OpenHandle(*source->source_string), arguments_list, context, + script_details, source->resource_options, script_data, options, + no_cache_reason) + .ToHandle(&scoped_result); + if (options == kConsumeCodeCache) { + source->cached_data->rejected = script_data->rejected(); + } + delete script_data; + RETURN_ON_FAILED_EXECUTION(Function); + result = handle_scope.Escape(Utils::CallableToLocal(scoped_result)); } - i::Handle result; - has_pending_exception = - !i::Compiler::GetWrappedFunction( - Utils::OpenHandle(*source->source_string), arguments_list, context, - script_details, source->resource_options, script_data, options, - no_cache_reason) - .ToHandle(&result); - if (options == kConsumeCodeCache) { - source->cached_data->rejected = script_data->rejected(); + if (script_or_module_out != nullptr) { + i::Handle function = + i::Handle::cast(Utils::OpenHandle(*result)); + i::Isolate* isolate = function->GetIsolate(); + i::Handle shared(function->shared(), isolate); + i::Handle script(i::Script::cast(shared->script()), isolate); + *script_or_module_out = v8::Utils::ScriptOrModuleToLocal(script); } - delete script_data; - RETURN_ON_FAILED_EXECUTION(Function); - RETURN_ESCAPED(Utils::CallableToLocal(result)); + + return result; } void ScriptCompiler::ScriptStreamingTask::Run() { data_->task->Run(); } diff --git a/deps/v8/test/cctest/test-compiler.cc b/deps/v8/test/cctest/test-compiler.cc index 17f7a7d851cd76..cb559d8fe21f74 100644 --- a/deps/v8/test/cctest/test-compiler.cc +++ b/deps/v8/test/cctest/test-compiler.cc @@ -647,11 +647,16 @@ TEST(CompileFunctionInContextScriptOrigin) { v8::Integer::New(CcTest::isolate(), 22), v8::Integer::New(CcTest::isolate(), 41)); v8::ScriptCompiler::Source script_source(v8_str("throw new Error()"), origin); + Local script; v8::Local fun = - v8::ScriptCompiler::CompileFunctionInContext(env.local(), &script_source, - 0, nullptr, 0, nullptr) + v8::ScriptCompiler::CompileFunctionInContext( + env.local(), &script_source, 0, nullptr, 0, nullptr, + v8::ScriptCompiler::CompileOptions::kNoCompileOptions, + v8::ScriptCompiler::NoCacheReason::kNoCacheNoReason, &script) .ToLocalChecked(); CHECK(!fun.IsEmpty()); + CHECK(!script.IsEmpty()); + CHECK(script->GetResourceName()->StrictEquals(v8_str("test"))); v8::TryCatch try_catch(CcTest::isolate()); CcTest::isolate()->SetCaptureStackTraceForUncaughtExceptions(true); CHECK(fun->Call(env.local(), env->Global(), 0, nullptr).IsEmpty()); From 7b4638cee03771d9666ad0ba0c494046fe84dd8d Mon Sep 17 00:00:00 2001 From: Gus Caplan Date: Fri, 19 Jul 2019 09:30:09 -0500 Subject: [PATCH 2/2] vm: fix gc bug with modules and compiled functions Backport-PR-URL: https://github.com/nodejs/node/pull/28779 PR-URL: https://github.com/nodejs/node/pull/28671 Reviewed-By: Anna Henningsen Reviewed-By: Guy Bedford --- lib/internal/modules/cjs/loader.js | 5 +-- lib/vm.js | 12 ++++++- src/env.cc | 27 ++++++++++----- src/env.h | 12 ++++--- src/module_wrap.cc | 2 +- src/node_contextify.cc | 54 ++++++++++++++++-------------- src/node_contextify.h | 2 -- 7 files changed, 69 insertions(+), 45 deletions(-) diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index 00bc575b1f7d4b..a70fad83f9a8a7 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -717,7 +717,7 @@ Module.prototype._compile = function(content, filename) { } : undefined, }); } else { - compiledWrapper = compileFunction( + const compiled = compileFunction( content, filename, 0, @@ -736,13 +736,14 @@ Module.prototype._compile = function(content, filename) { ); 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)); } }); } + compiledWrapper = compiled.function; } var inspectorWrapper = null; diff --git a/lib/vm.js b/lib/vm.js index a666a05c8a7c10..ec6614e66146a7 100644 --- a/lib/vm.js +++ b/lib/vm.js @@ -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; } diff --git a/src/env.cc b/src/env.cc index df59eaaa94928f..38d5796f54d6e5 100644 --- a/src/env.cc +++ b/src/env.cc @@ -39,6 +39,7 @@ using v8::NewStringType; using v8::Number; using v8::Object; using v8::Private; +using v8::ScriptOrModule; using v8::SnapshotCreator; using v8::StackTrace; using v8::String; @@ -46,6 +47,7 @@ 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& data) { + CompiledFnEntry* entry = data.GetParameter(); + entry->env->id_to_function_map.erase(entry->id); + delete entry; +} + +CompiledFnEntry::CompiledFnEntry(Environment* env, + uint32_t id, + Local 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 diff --git a/src/env.h b/src/env.h index df389f3beab35b..3665c100d9b965 100644 --- a/src/env.h +++ b/src/env.h @@ -157,6 +157,7 @@ constexpr size_t kFsStatsBufferLength = kFsStatsFieldsNumber * 2; 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") \ @@ -500,10 +501,14 @@ struct ContextInfo { bool is_default = false; }; -struct CompileFnEntry { +struct CompiledFnEntry { Environment* env; uint32_t id; - CompileFnEntry(Environment* env, uint32_t id); + v8::Global cache_key; + v8::Global script; + CompiledFnEntry(Environment* env, + uint32_t id, + v8::Local script); }; // Listing the AsyncWrap provider types first enables us to cast directly @@ -990,8 +995,7 @@ class Environment : public MemoryRetainer { std::unordered_map id_to_module_map; std::unordered_map id_to_script_map; - std::unordered_set compile_fn_entries; - std::unordered_map> id_to_function_map; + std::unordered_map id_to_function_map; inline uint32_t get_next_module_id(); inline uint32_t get_next_script_id(); diff --git a/src/module_wrap.cc b/src/module_wrap.cc index bccfc2d2affb79..7df91bb8704282 100644 --- a/src/module_wrap.cc +++ b/src/module_wrap.cc @@ -1041,7 +1041,7 @@ static MaybeLocal 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(); } diff --git a/src/node_contextify.cc b/src/node_contextify.cc index 5be774fb91af0c..6559e813c9ae8b 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -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& 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 script; MaybeLocal 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 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 cache_key = entry->cache_key.Get(isolate); + + Local 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 cached_data( @@ -1146,18 +1144,22 @@ void ContextifyContext::CompileFunction( env, reinterpret_cast(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& args) { diff --git a/src/node_contextify.h b/src/node_contextify.h index 288b51ef56ac11..73461dc623578e 100644 --- a/src/node_contextify.h +++ b/src/node_contextify.h @@ -63,8 +63,6 @@ class ContextifyContext { const v8::FunctionCallbackInfo& args); static void WeakCallback( const v8::WeakCallbackInfo& data); - static void WeakCallbackCompileFn( - const v8::WeakCallbackInfo& data); static void PropertyGetterCallback( v8::Local property, const v8::PropertyCallbackInfo& args);