From 0e86d892432b29ebc29ebe576446e94a5c8aee74 Mon Sep 17 00:00:00 2001 From: legendecas Date: Sun, 10 Jul 2022 08:37:08 +0800 Subject: [PATCH] deps: V8: backport 22698d267667 Original commit message: [module] Fix aborts in terminated async module evaluation SourceTextModule::ExecuteAsyncModule asserts the execution of the module's async function to succeed without exception. However, the problem is that TerminateExecution initiated by embedders is breaking that assumption. The execution can be terminated with an exception and the exception is not catchable by JavaScript. The uncatchable exceptions during the async module evaluation need to be raised to the embedder and not crash the process if possible. Refs: https://github.com/nodejs/node/issues/43182 Change-Id: Ifc152428b95945b6b49a2f70ba35018cfc0ce40b Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3696493 Reviewed-by: Camillo Bruni Commit-Queue: Chengzhong Wu Cr-Commit-Position: refs/heads/main@{#81307} Refs: https://github.com/v8/v8/commit/22698d267667ad36f8b1d2c1f5ec71f54fcea3eb PR-URL: https://github.com/nodejs/node/pull/43751 Backport-PR-URL: https://github.com/nodejs/node/pull/44085 Reviewed-By: Colin Ihrig Reviewed-By: Jiawen Geng Reviewed-By: Gus Caplan --- common.gypi | 2 +- deps/v8/src/builtins/builtins-async-module.cc | 10 +- deps/v8/src/objects/source-text-module.cc | 31 +++-- deps/v8/src/objects/source-text-module.h | 14 ++- deps/v8/test/cctest/test-api.cc | 116 ++++++++++++++++++ 5 files changed, 158 insertions(+), 15 deletions(-) diff --git a/common.gypi b/common.gypi index 6bea7720e06..d68a4c9eafa 100644 --- a/common.gypi +++ b/common.gypi @@ -36,7 +36,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.21', + 'v8_embedder_string': '-node.22', ##### V8 defaults for Node.js ##### diff --git a/deps/v8/src/builtins/builtins-async-module.cc b/deps/v8/src/builtins/builtins-async-module.cc index 7128ad7e9d3..180bbd5df89 100644 --- a/deps/v8/src/builtins/builtins-async-module.cc +++ b/deps/v8/src/builtins/builtins-async-module.cc @@ -12,7 +12,15 @@ namespace internal { BUILTIN(CallAsyncModuleFulfilled) { HandleScope handle_scope(isolate); Handle module(args.at(0)); - SourceTextModule::AsyncModuleExecutionFulfilled(isolate, module); + if (SourceTextModule::AsyncModuleExecutionFulfilled(isolate, module) + .IsNothing()) { + // The evaluation of async module can not throwing a JavaScript observable + // exception. + DCHECK(isolate->has_pending_exception()); + DCHECK_EQ(isolate->pending_exception(), + ReadOnlyRoots(isolate).termination_exception()); + return ReadOnlyRoots(isolate).exception(); + } return ReadOnlyRoots(isolate).undefined_value(); } diff --git a/deps/v8/src/objects/source-text-module.cc b/deps/v8/src/objects/source-text-module.cc index cf1773f2d60..c7764c365a4 100644 --- a/deps/v8/src/objects/source-text-module.cc +++ b/deps/v8/src/objects/source-text-module.cc @@ -766,14 +766,14 @@ MaybeHandle SourceTextModule::Evaluate( return result; } -void SourceTextModule::AsyncModuleExecutionFulfilled( +Maybe SourceTextModule::AsyncModuleExecutionFulfilled( Isolate* isolate, Handle module) { // 1. If module.[[Status]] is evaluated, then if (module->status() == kErrored) { // a. Assert: module.[[EvaluationError]] is not empty. DCHECK(!module->exception().IsTheHole(isolate)); // b. Return. - return; + return Just(true); } // 3. Assert: module.[[AsyncEvaluating]] is true. DCHECK(module->IsAsyncEvaluating()); @@ -829,7 +829,9 @@ void SourceTextModule::AsyncModuleExecutionFulfilled( } else if (m->async()) { // ii. Otherwise, if m.[[Async]] is *true*, then // a. Perform ! ExecuteAsyncModule(m). - ExecuteAsyncModule(isolate, m); + // The execution may have been terminated and can not be resumed, so just + // raise the exception. + MAYBE_RETURN(ExecuteAsyncModule(isolate, m), Nothing()); } else { // iii. Otherwise, // a. Let _result_ be m.ExecuteModule(). @@ -863,6 +865,7 @@ void SourceTextModule::AsyncModuleExecutionFulfilled( } // 10. Return undefined. + return Just(true); } void SourceTextModule::AsyncModuleExecutionRejected( @@ -922,8 +925,9 @@ void SourceTextModule::AsyncModuleExecutionRejected( } } -void SourceTextModule::ExecuteAsyncModule(Isolate* isolate, - Handle module) { +// static +Maybe SourceTextModule::ExecuteAsyncModule( + Isolate* isolate, Handle module) { // 1. Assert: module.[[Status]] is "evaluating" or "evaluated". CHECK(module->status() == kEvaluating || module->status() == kEvaluated); @@ -973,9 +977,19 @@ void SourceTextModule::ExecuteAsyncModule(Isolate* isolate, // Note: In V8 we have broken module.ExecuteModule into // ExecuteModule for synchronous module execution and // InnerExecuteAsyncModule for asynchronous execution. - InnerExecuteAsyncModule(isolate, module, capability).ToHandleChecked(); + MaybeHandle ret = + InnerExecuteAsyncModule(isolate, module, capability); + if (ret.is_null()) { + // The evaluation of async module can not throwing a JavaScript observable + // exception. + DCHECK(isolate->has_pending_exception()); + DCHECK_EQ(isolate->pending_exception(), + ReadOnlyRoots(isolate).termination_exception()); + return Nothing(); + } // 13. Return. + return Just(true); } MaybeHandle SourceTextModule::InnerExecuteAsyncModule( @@ -1171,8 +1185,11 @@ MaybeHandle SourceTextModule::InnerModuleEvaluation( // c. If module.[[PendingAsyncDependencies]] is 0, // perform ! ExecuteAsyncModule(_module_). + // The execution may have been terminated and can not be resumed, so just + // raise the exception. if (!module->HasPendingAsyncDependencies()) { - SourceTextModule::ExecuteAsyncModule(isolate, module); + MAYBE_RETURN(SourceTextModule::ExecuteAsyncModule(isolate, module), + MaybeHandle()); } } else { // 15. Otherwise, perform ? module.ExecuteModule(). diff --git a/deps/v8/src/objects/source-text-module.h b/deps/v8/src/objects/source-text-module.h index 6f2a3cd0f72..4bb54b90e17 100644 --- a/deps/v8/src/objects/source-text-module.h +++ b/deps/v8/src/objects/source-text-module.h @@ -53,9 +53,10 @@ class SourceTextModule static int ExportIndex(int cell_index); // Used by builtins to fulfill or reject the promise associated - // with async SourceTextModules. - static void AsyncModuleExecutionFulfilled(Isolate* isolate, - Handle module); + // with async SourceTextModules. Return Nothing if the execution is + // terminated. + static Maybe AsyncModuleExecutionFulfilled( + Isolate* isolate, Handle module); static void AsyncModuleExecutionRejected(Isolate* isolate, Handle module, Handle exception); @@ -204,9 +205,10 @@ class SourceTextModule static V8_WARN_UNUSED_RESULT MaybeHandle ExecuteModule( Isolate* isolate, Handle module); - // Implementation of spec ExecuteAsyncModule. - static void ExecuteAsyncModule(Isolate* isolate, - Handle module); + // Implementation of spec ExecuteAsyncModule. Return Nothing if the execution + // is been terminated. + static V8_WARN_UNUSED_RESULT Maybe ExecuteAsyncModule( + Isolate* isolate, Handle module); static void Reset(Isolate* isolate, Handle module); diff --git a/deps/v8/test/cctest/test-api.cc b/deps/v8/test/cctest/test-api.cc index 6e616c80688..f200cf8e576 100644 --- a/deps/v8/test/cctest/test-api.cc +++ b/deps/v8/test/cctest/test-api.cc @@ -24715,6 +24715,122 @@ TEST(ImportFromSyntheticModuleThrow) { CHECK(try_catch.HasCaught()); } +namespace { + +v8::MaybeLocal ModuleEvaluateTerminateExecutionResolveCallback( + Local context, Local specifier, + Local import_assertions, Local referrer) { + v8::Isolate* isolate = context->GetIsolate(); + + Local url = v8_str("www.test.com"); + Local source_text = v8_str("await Promise.resolve();"); + v8::ScriptOrigin origin(isolate, url, 0, 0, false, -1, Local(), + false, false, true); + v8::ScriptCompiler::Source source(source_text, origin); + Local module = + v8::ScriptCompiler::CompileModule(isolate, &source).ToLocalChecked(); + module + ->InstantiateModule(context, + ModuleEvaluateTerminateExecutionResolveCallback) + .ToChecked(); + + CHECK_EQ(module->GetStatus(), Module::kInstantiated); + return module; +} + +void ModuleEvaluateTerminateExecution( + const v8::FunctionCallbackInfo& args) { + v8::Isolate::GetCurrent()->TerminateExecution(); +} +} // namespace + +TEST(ModuleEvaluateTerminateExecution) { + 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); + + v8::Local terminate_execution = + v8::Function::New(context, ModuleEvaluateTerminateExecution, + v8_str("terminate_execution")) + .ToLocalChecked(); + context->Global() + ->Set(context, v8_str("terminate_execution"), terminate_execution) + .FromJust(); + + Local url = v8_str("www.test.com"); + Local source_text = v8_str( + "terminate_execution();" + "await Promise.resolve();"); + v8::ScriptOrigin origin(isolate, url, 0, 0, false, -1, Local(), + false, false, true); + v8::ScriptCompiler::Source source(source_text, origin); + Local module = + v8::ScriptCompiler::CompileModule(isolate, &source).ToLocalChecked(); + module + ->InstantiateModule(context, + ModuleEvaluateTerminateExecutionResolveCallback) + .ToChecked(); + + CHECK_EQ(module->GetStatus(), Module::kInstantiated); + TryCatch try_catch(isolate); + v8::MaybeLocal completion_value = module->Evaluate(context); + CHECK(completion_value.IsEmpty()); + + CHECK_EQ(module->GetStatus(), Module::kErrored); + CHECK(try_catch.HasCaught()); + CHECK(try_catch.HasTerminated()); +} + +TEST(ModuleEvaluateImportTerminateExecution) { + 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); + + v8::Local terminate_execution = + v8::Function::New(context, ModuleEvaluateTerminateExecution, + v8_str("terminate_execution")) + .ToLocalChecked(); + context->Global() + ->Set(context, v8_str("terminate_execution"), terminate_execution) + .FromJust(); + + Local url = v8_str("www.test.com"); + Local source_text = v8_str( + "import './synthetic.module';" + "terminate_execution();" + "await Promise.resolve();"); + v8::ScriptOrigin origin(isolate, url, 0, 0, false, -1, Local(), + false, false, true); + v8::ScriptCompiler::Source source(source_text, origin); + Local module = + v8::ScriptCompiler::CompileModule(isolate, &source).ToLocalChecked(); + module + ->InstantiateModule(context, + ModuleEvaluateTerminateExecutionResolveCallback) + .ToChecked(); + + CHECK_EQ(module->GetStatus(), Module::kInstantiated); + TryCatch try_catch(isolate); + v8::MaybeLocal completion_value = module->Evaluate(context); + Local promise( + Local::Cast(completion_value.ToLocalChecked())); + CHECK_EQ(promise->State(), v8::Promise::kPending); + isolate->PerformMicrotaskCheckpoint(); + + // The exception thrown by terminate execution is not catchable by JavaScript + // so the promise can not be settled. + CHECK_EQ(promise->State(), v8::Promise::kPending); + CHECK_EQ(module->GetStatus(), Module::kEvaluated); + CHECK(try_catch.HasCaught()); + CHECK(try_catch.HasTerminated()); +} + // Tests that the code cache does not confuse the same source code compiled as a // script and as a module. TEST(CodeCacheModuleScriptMismatch) {