From 42c456089af5aea559ea0f12c189e31e72dbdecf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C3=ABl=20Zasso?= Date: Tue, 22 Oct 2019 15:07:21 +0200 Subject: [PATCH] deps: V8: backport 777fa98 Original commit message: Make SetSyntheticModuleExport throw instead of crash for nonexistent export name Per spec, Module::SetSyntheticModuleExport should throw a ReferenceError when called with an export name that was not supplied when constructing that SyntheticModule. Instead, the current implementation crashes with a failed CHECK(). Add a new Module::SyntheticModuleSetExport that throws (without an ensuing crash) for this case, and deprecate the old Module::SetSyntheticModuleExport. Bug: v8:9828 Change-Id: I3b3d353064c3851882781818099bd8f6ee74c809 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1860996 Reviewed-by: Adam Klein Reviewed-by: Georg Neis Commit-Queue: Dan Clark Cr-Commit-Position: refs/heads/master@{#64438} Refs: https://github.com/v8/v8/commit/777fa98cc47ac32f0fde3d9aafd830949deb5d23 PR-URL: https://github.com/nodejs/node/pull/30062 Reviewed-By: Anna Henningsen Reviewed-By: Gireesh Punathil Reviewed-By: Gus Caplan Reviewed-By: Ruben Bridgewater Reviewed-By: Colin Ihrig Reviewed-By: James M Snell --- common.gypi | 2 +- deps/v8/include/v8.h | 18 +++++++++---- deps/v8/src/api/api.cc | 28 ++++++++++++++++--- deps/v8/src/logging/counters.h | 1 + deps/v8/src/objects/synthetic-module.cc | 30 +++++++++++++++++---- deps/v8/src/objects/synthetic-module.h | 18 ++++++++++--- deps/v8/test/cctest/test-api.cc | 36 +++++++++++++++++++++++-- 7 files changed, 114 insertions(+), 19 deletions(-) diff --git a/common.gypi b/common.gypi index 711b7593062d72..87a84afa4494c6 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.22', + 'v8_embedder_string': '-node.23', ##### V8 defaults for Node.js ##### diff --git a/deps/v8/include/v8.h b/deps/v8/include/v8.h index f79d262a62690e..aa19cf8caadf79 100644 --- a/deps/v8/include/v8.h +++ b/deps/v8/include/v8.h @@ -1473,11 +1473,19 @@ class V8_EXPORT Module { /** * Set this module's exported value for the name export_name to the specified * export_value. This method must be called only on Modules created via - * CreateSyntheticModule. export_name must be one of the export_names that - * were passed in that CreateSyntheticModule call. - */ - void SetSyntheticModuleExport(Local export_name, - Local export_value); + * CreateSyntheticModule. An error will be thrown if export_name is not one + * of the export_names that were passed in that CreateSyntheticModule call. + * Returns Just(true) on success, Nothing() if an error was thrown. + */ + V8_WARN_UNUSED_RESULT Maybe SetSyntheticModuleExport( + Isolate* isolate, Local export_name, Local export_value); + V8_DEPRECATE_SOON( + "Use the preceding SetSyntheticModuleExport with an Isolate parameter, " + "instead of the one that follows. The former will throw a runtime " + "error if called for an export that doesn't exist (as per spec); " + "the latter will crash with a failed CHECK().", + void SetSyntheticModuleExport(Local export_name, + Local export_value)); }; /** diff --git a/deps/v8/src/api/api.cc b/deps/v8/src/api/api.cc index 07467d2af05f38..a0e25505a1ace0 100644 --- a/deps/v8/src/api/api.cc +++ b/deps/v8/src/api/api.cc @@ -2351,6 +2351,28 @@ Local Module::CreateSyntheticModule( i_module_name, i_export_names, evaluation_steps))); } +Maybe Module::SetSyntheticModuleExport(Isolate* isolate, + Local export_name, + Local export_value) { + auto i_isolate = reinterpret_cast(isolate); + i::Handle i_export_name = Utils::OpenHandle(*export_name); + i::Handle i_export_value = Utils::OpenHandle(*export_value); + i::Handle self = Utils::OpenHandle(this); + Utils::ApiCheck(self->IsSyntheticModule(), + "v8::Module::SyntheticModuleSetExport", + "v8::Module::SyntheticModuleSetExport must only be called on " + "a SyntheticModule"); + ENTER_V8_NO_SCRIPT(i_isolate, isolate->GetCurrentContext(), Module, + SetSyntheticModuleExport, Nothing(), i::HandleScope); + has_pending_exception = + i::SyntheticModule::SetExport(i_isolate, + i::Handle::cast(self), + i_export_name, i_export_value) + .IsNothing(); + RETURN_ON_FAILED_EXECUTION_PRIMITIVE(bool); + return Just(true); +} + void Module::SetSyntheticModuleExport(Local export_name, Local export_value) { i::Handle i_export_name = Utils::OpenHandle(*export_name); @@ -2360,9 +2382,9 @@ void Module::SetSyntheticModuleExport(Local export_name, "v8::Module::SetSyntheticModuleExport", "v8::Module::SetSyntheticModuleExport must only be called on " "a SyntheticModule"); - i::SyntheticModule::SetExport(self->GetIsolate(), - i::Handle::cast(self), - i_export_name, i_export_value); + i::SyntheticModule::SetExportStrict(self->GetIsolate(), + i::Handle::cast(self), + i_export_name, i_export_value); } namespace { diff --git a/deps/v8/src/logging/counters.h b/deps/v8/src/logging/counters.h index 4466e0a53bc6ca..35df5ec0384565 100644 --- a/deps/v8/src/logging/counters.h +++ b/deps/v8/src/logging/counters.h @@ -780,6 +780,7 @@ class RuntimeCallTimer final { V(Message_GetStartColumn) \ V(Module_Evaluate) \ V(Module_InstantiateModule) \ + V(Module_SetSyntheticModuleExport) \ V(NumberObject_New) \ V(NumberObject_NumberValue) \ V(Object_CallAsConstructor) \ diff --git a/deps/v8/src/objects/synthetic-module.cc b/deps/v8/src/objects/synthetic-module.cc index 0cca30a37b9e7f..850721ac99d303 100644 --- a/deps/v8/src/objects/synthetic-module.cc +++ b/deps/v8/src/objects/synthetic-module.cc @@ -17,16 +17,36 @@ namespace internal { // Implements SetSyntheticModuleBinding: // https://heycam.github.io/webidl/#setsyntheticmoduleexport -void SyntheticModule::SetExport(Isolate* isolate, - Handle module, - Handle export_name, - Handle export_value) { +Maybe SyntheticModule::SetExport(Isolate* isolate, + Handle module, + Handle export_name, + Handle export_value) { Handle exports(module->exports(), isolate); Handle export_object(exports->Lookup(export_name), isolate); - CHECK(export_object->IsCell()); + + if (!export_object->IsCell()) { + isolate->Throw(*isolate->factory()->NewReferenceError( + MessageTemplate::kModuleExportUndefined, export_name)); + return Nothing(); + } + Handle export_cell(Handle::cast(export_object)); // Spec step 2: Set the mutable binding of export_name to export_value export_cell->set_value(*export_value); + + return Just(true); +} + +void SyntheticModule::SetExportStrict(Isolate* isolate, + Handle module, + Handle export_name, + Handle export_value) { + Handle exports(module->exports(), isolate); + Handle export_object(exports->Lookup(export_name), isolate); + CHECK(export_object->IsCell()); + Maybe set_export_result = + SetExport(isolate, module, export_name, export_value); + CHECK(set_export_result.FromJust()); } // Implements Synthetic Module Record's ResolveExport concrete method: diff --git a/deps/v8/src/objects/synthetic-module.h b/deps/v8/src/objects/synthetic-module.h index 6f3bb0438e9597..77a6eed2761da6 100644 --- a/deps/v8/src/objects/synthetic-module.h +++ b/deps/v8/src/objects/synthetic-module.h @@ -24,9 +24,21 @@ class SyntheticModule DECL_VERIFIER(SyntheticModule) DECL_PRINTER(SyntheticModule) - static void SetExport(Isolate* isolate, Handle module, - Handle export_name, - Handle export_value); + // Set module's exported value for the specified export_name to the specified + // export_value. An error will be thrown if export_name is not one + // of the export_names that were supplied during module construction. + // Returns Just(true) on success, Nothing() if an error was thrown. + static Maybe SetExport(Isolate* isolate, Handle module, + Handle export_name, + Handle export_value); + // The following redundant method should be deleted when the deprecated + // version of v8::SetSyntheticModuleExport is removed. It differs from + // SetExport in that it crashes rather than throwing an error if the caller + // attempts to set an export_name that was not present during construction of + // the module. + static void SetExportStrict(Isolate* isolate, Handle module, + Handle export_name, + Handle export_value); using BodyDescriptor = SubclassBodyDescriptor< Module::BodyDescriptor, diff --git a/deps/v8/test/cctest/test-api.cc b/deps/v8/test/cctest/test-api.cc index 0168b0f85a85c6..25d6e41d0e91c9 100644 --- a/deps/v8/test/cctest/test-api.cc +++ b/deps/v8/test/cctest/test-api.cc @@ -23693,7 +23693,9 @@ v8::MaybeLocal SyntheticModuleEvaluationStepsCallbackFail( v8::MaybeLocal SyntheticModuleEvaluationStepsCallbackSetExport( Local context, Local module) { - module->SetSyntheticModuleExport(v8_str("test_export"), v8_num(42)); + Maybe set_export_result = module->SetSyntheticModuleExport( + context->GetIsolate(), v8_str("test_export"), v8_num(42)); + CHECK(set_export_result.FromJust()); return v8::Undefined(reinterpret_cast(context->GetIsolate())); } @@ -23900,7 +23902,9 @@ TEST(SyntheticModuleSetExports) { // undefined. CHECK(foo_cell->value().IsUndefined()); - module->SetSyntheticModuleExport(foo_string, bar_string); + Maybe set_export_result = + module->SetSyntheticModuleExport(isolate, foo_string, bar_string); + CHECK(set_export_result.FromJust()); // After setting the export the Cell should still have the same idenitity. CHECK_EQ(exports->Lookup(v8::Utils::OpenHandle(*foo_string)), *foo_cell); @@ -23911,6 +23915,34 @@ TEST(SyntheticModuleSetExports) { ->Equals(*v8::Utils::OpenHandle(*bar_string))); } +TEST(SyntheticModuleSetMissingExport) { + LocalContext env; + v8::Isolate* isolate = env->GetIsolate(); + auto i_isolate = reinterpret_cast(isolate); + v8::Isolate::Scope iscope(isolate); + v8::HandleScope scope(isolate); + v8::Local context = v8::Context::New(isolate); + v8::Context::Scope cscope(context); + + Local foo_string = v8_str("foo"); + Local bar_string = v8_str("bar"); + + Local module = CreateAndInstantiateSyntheticModule( + isolate, v8_str("SyntheticModuleSetExports-TestSyntheticModule"), context, + std::vector>(), + UnexpectedSyntheticModuleEvaluationStepsCallback); + + i::Handle i_module = + i::Handle::cast(v8::Utils::OpenHandle(*module)); + i::Handle exports(i_module->exports(), i_isolate); + + TryCatch try_catch(isolate); + Maybe set_export_result = + module->SetSyntheticModuleExport(isolate, foo_string, bar_string); + CHECK(set_export_result.IsNothing()); + CHECK(try_catch.HasCaught()); +} + TEST(SyntheticModuleEvaluationStepsNoThrow) { synthetic_module_callback_count = 0; LocalContext env;