From a8c5b51326981b4dbdb07c9acb7bed1940b4542e Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 28 Nov 2019 22:11:40 +0100 Subject: [PATCH 1/2] deps: V8: cherry-pick ca5b0ec MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Original commit message: [heap] Ensure SyntheticModule is initialized before next allocation Ensure that all fields of `SyntheticModule` are set before creating the exports hash table for it, because the latter may trigger garbage collection, leading to crashes. This has been causing failures in the Node.js CI over the last weeks, after making the creating of synthetic modules part of Node’s startup sequence. (I am generally not very familiar with this part of the V8 code and there might be a better way, or possibly a way to add a reliable regression test, that I am not aware of.) Refs: https://github.com/nodejs/node/issues/30498 Refs: https://github.com/nodejs/node/issues/30648 Change-Id: I32da4b7bd888c6ec1421f34f5bd52e7bad154c1e Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1939752 Commit-Queue: Ulan Degenbaev Reviewed-by: Ulan Degenbaev Cr-Commit-Position: refs/heads/master@{#65247} Refs: https://github.com/v8/v8/commit/ca5b0ec2722d2af4551c01ca78921fa16a26ae72 Fixes: https://github.com/nodejs/node/issues/30498 Fixes: https://github.com/nodejs/node/issues/30648 --- common.gypi | 2 +- deps/v8/src/heap/factory.cc | 10 ++++++---- deps/v8/test/cctest/test-api.cc | 25 +++++++++++++++++++++++++ 3 files changed, 32 insertions(+), 5 deletions(-) diff --git a/common.gypi b/common.gypi index 2e59636b1718f1..4fd333a45de84b 100644 --- a/common.gypi +++ b/common.gypi @@ -39,7 +39,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.20', + 'v8_embedder_string': '-node.21', ##### V8 defaults for Node.js ##### diff --git a/deps/v8/src/heap/factory.cc b/deps/v8/src/heap/factory.cc index 721682f00f26de..7e434ea041866f 100644 --- a/deps/v8/src/heap/factory.cc +++ b/deps/v8/src/heap/factory.cc @@ -3070,20 +3070,22 @@ Handle Factory::NewSyntheticModule( Handle module_name, Handle export_names, v8::Module::SyntheticModuleEvaluationSteps evaluation_steps) { ReadOnlyRoots roots(isolate()); - Handle module( - SyntheticModule::cast(New(synthetic_module_map(), AllocationType::kOld)), - isolate()); + Handle exports = ObjectHashTable::New(isolate(), static_cast(export_names->length())); Handle evaluation_steps_foreign = NewForeign(reinterpret_cast(evaluation_steps)); - module->set_exports(*exports); + + Handle module( + SyntheticModule::cast(New(synthetic_module_map(), AllocationType::kOld)), + isolate()); module->set_hash(isolate()->GenerateIdentityHash(Smi::kMaxValue)); module->set_module_namespace(roots.undefined_value()); module->set_status(Module::kUninstantiated); module->set_exception(roots.the_hole_value()); module->set_name(*module_name); module->set_export_names(*export_names); + module->set_exports(*exports); module->set_evaluation_steps(*evaluation_steps_foreign); return module; } diff --git a/deps/v8/test/cctest/test-api.cc b/deps/v8/test/cctest/test-api.cc index 12faaff39cbfcb..bafdd6ce3b7194 100644 --- a/deps/v8/test/cctest/test-api.cc +++ b/deps/v8/test/cctest/test-api.cc @@ -23918,6 +23918,31 @@ TEST(CreateSyntheticModule) { CHECK_EQ(i_module->status(), i::Module::kInstantiated); } +TEST(CreateSyntheticModuleGC) { + // Try to make sure that CreateSyntheticModule() deals well with a GC + // happening during its execution. + i::FLAG_gc_interval = 10; + i::FLAG_inline_new = false; + + LocalContext env; + v8::Isolate* isolate = env->GetIsolate(); + v8::Isolate::Scope iscope(isolate); + v8::HandleScope scope(isolate); + v8::Local context = v8::Context::New(isolate); + v8::Context::Scope cscope(context); + + std::vector> export_names{v8_str("default")}; + v8::Local module_name = + v8_str("CreateSyntheticModule-TestSyntheticModuleGC"); + + for (int i = 0; i < 200; i++) { + Local module = v8::Module::CreateSyntheticModule( + isolate, module_name, export_names, + UnexpectedSyntheticModuleEvaluationStepsCallback); + USE(module); + } +} + TEST(SyntheticModuleSetExports) { LocalContext env; v8::Isolate* isolate = env->GetIsolate(); From c4f3cdf3a88814420430885cbe8c89d46d61e6b1 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 28 Nov 2019 22:13:46 +0100 Subject: [PATCH 2/2] Revert "test: skip test-domain-error-types in debug mode temporariliy" This reverts commit 6d022c13c52fc0134ed6c79be520be8c84c58cf8. --- test/parallel/parallel.status | 4 ---- 1 file changed, 4 deletions(-) diff --git a/test/parallel/parallel.status b/test/parallel/parallel.status index 00ab4a774d29b9..444cf8d1154c92 100644 --- a/test/parallel/parallel.status +++ b/test/parallel/parallel.status @@ -33,7 +33,3 @@ test-async-hooks-http-parser-destroy: PASS,FLAKY [$system==freebsd] [$system==aix] - -[$mode==debug] -# https://github.com/nodejs/node/issues/30498 -test-domain-error-types: PASS,FLAKY