Skip to content

Commit

Permalink
deps: V8: backport 777fa98
Browse files Browse the repository at this point in the history
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 <adamk@chromium.org>
    Reviewed-by: Georg Neis <neis@chromium.org>
    Commit-Queue: Dan Clark <daniec@microsoft.com>
    Cr-Commit-Position: refs/heads/master@{#64438}

Refs: v8/v8@777fa98

PR-URL: nodejs#30062
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
  • Loading branch information
targos authored and aduh95 committed Jan 7, 2020
1 parent 4442359 commit 72493af
Show file tree
Hide file tree
Showing 7 changed files with 114 additions and 19 deletions.
2 changes: 1 addition & 1 deletion common.gypi
Expand Up @@ -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.16',
'v8_embedder_string': '-node.17',

##### V8 defaults for Node.js #####

Expand Down
18 changes: 13 additions & 5 deletions deps/v8/include/v8.h
Expand Up @@ -1385,11 +1385,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<String> export_name,
Local<Value> 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<bool>() if an error was thrown.
*/
V8_WARN_UNUSED_RESULT Maybe<bool> SetSyntheticModuleExport(
Isolate* isolate, Local<String> export_name, Local<Value> 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<String> export_name,
Local<Value> export_value));
};

/**
Expand Down
28 changes: 25 additions & 3 deletions deps/v8/src/api/api.cc
Expand Up @@ -2333,6 +2333,28 @@ Local<Module> Module::CreateSyntheticModule(
i_module_name, i_export_names, evaluation_steps)));
}

Maybe<bool> Module::SetSyntheticModuleExport(Isolate* isolate,
Local<String> export_name,
Local<v8::Value> export_value) {
auto i_isolate = reinterpret_cast<i::Isolate*>(isolate);
i::Handle<i::String> i_export_name = Utils::OpenHandle(*export_name);
i::Handle<i::Object> i_export_value = Utils::OpenHandle(*export_value);
i::Handle<i::Module> 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<bool>(), i::HandleScope);
has_pending_exception =
i::SyntheticModule::SetExport(i_isolate,
i::Handle<i::SyntheticModule>::cast(self),
i_export_name, i_export_value)
.IsNothing();
RETURN_ON_FAILED_EXECUTION_PRIMITIVE(bool);
return Just(true);
}

void Module::SetSyntheticModuleExport(Local<String> export_name,
Local<v8::Value> export_value) {
i::Handle<i::String> i_export_name = Utils::OpenHandle(*export_name);
Expand All @@ -2342,9 +2364,9 @@ void Module::SetSyntheticModuleExport(Local<String> export_name,
"v8::Module::SetSyntheticModuleExport",
"v8::Module::SetSyntheticModuleExport must only be called on "
"a SyntheticModule");
i::SyntheticModule::SetExport(self->GetIsolate(),
i::Handle<i::SyntheticModule>::cast(self),
i_export_name, i_export_value);
i::SyntheticModule::SetExportStrict(self->GetIsolate(),
i::Handle<i::SyntheticModule>::cast(self),
i_export_name, i_export_value);
}

namespace {
Expand Down
1 change: 1 addition & 0 deletions deps/v8/src/logging/counters.h
Expand Up @@ -779,6 +779,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) \
Expand Down
30 changes: 25 additions & 5 deletions deps/v8/src/objects/synthetic-module.cc
Expand Up @@ -17,16 +17,36 @@ namespace internal {

// Implements SetSyntheticModuleBinding:
// https://heycam.github.io/webidl/#setsyntheticmoduleexport
void SyntheticModule::SetExport(Isolate* isolate,
Handle<SyntheticModule> module,
Handle<String> export_name,
Handle<Object> export_value) {
Maybe<bool> SyntheticModule::SetExport(Isolate* isolate,
Handle<SyntheticModule> module,
Handle<String> export_name,
Handle<Object> export_value) {
Handle<ObjectHashTable> exports(module->exports(), isolate);
Handle<Object> 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<bool>();
}

Handle<Cell> export_cell(Handle<Cell>::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<SyntheticModule> module,
Handle<String> export_name,
Handle<Object> export_value) {
Handle<ObjectHashTable> exports(module->exports(), isolate);
Handle<Object> export_object(exports->Lookup(export_name), isolate);
CHECK(export_object->IsCell());
Maybe<bool> set_export_result =
SetExport(isolate, module, export_name, export_value);
CHECK(set_export_result.FromJust());
}

// Implements Synthetic Module Record's ResolveExport concrete method:
Expand Down
18 changes: 15 additions & 3 deletions deps/v8/src/objects/synthetic-module.h
Expand Up @@ -29,9 +29,21 @@ class SyntheticModule : public Module {
DECL_ACCESSORS(export_names, FixedArray)
DECL_ACCESSORS(evaluation_steps, Foreign)

static void SetExport(Isolate* isolate, Handle<SyntheticModule> module,
Handle<String> export_name,
Handle<Object> 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<bool>() if an error was thrown.
static Maybe<bool> SetExport(Isolate* isolate, Handle<SyntheticModule> module,
Handle<String> export_name,
Handle<Object> 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<SyntheticModule> module,
Handle<String> export_name,
Handle<Object> export_value);

// Layout description.
DEFINE_FIELD_OFFSET_CONSTANTS(Module::kHeaderSize,
Expand Down
36 changes: 34 additions & 2 deletions deps/v8/test/cctest/test-api.cc
Expand Up @@ -23628,7 +23628,9 @@ v8::MaybeLocal<Value> SyntheticModuleEvaluationStepsCallbackFail(

v8::MaybeLocal<Value> SyntheticModuleEvaluationStepsCallbackSetExport(
Local<Context> context, Local<Module> module) {
module->SetSyntheticModuleExport(v8_str("test_export"), v8_num(42));
Maybe<bool> set_export_result = module->SetSyntheticModuleExport(
context->GetIsolate(), v8_str("test_export"), v8_num(42));
CHECK(set_export_result.FromJust());
return v8::Undefined(reinterpret_cast<v8::Isolate*>(context->GetIsolate()));
}

Expand Down Expand Up @@ -23835,7 +23837,9 @@ TEST(SyntheticModuleSetExports) {
// undefined.
CHECK(foo_cell->value().IsUndefined());

module->SetSyntheticModuleExport(foo_string, bar_string);
Maybe<bool> 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);
Expand All @@ -23846,6 +23850,34 @@ TEST(SyntheticModuleSetExports) {
->Equals(*v8::Utils::OpenHandle(*bar_string)));
}

TEST(SyntheticModuleSetMissingExport) {
LocalContext env;
v8::Isolate* isolate = env->GetIsolate();
auto i_isolate = reinterpret_cast<i::Isolate*>(isolate);
v8::Isolate::Scope iscope(isolate);
v8::HandleScope scope(isolate);
v8::Local<v8::Context> context = v8::Context::New(isolate);
v8::Context::Scope cscope(context);

Local<String> foo_string = v8_str("foo");
Local<String> bar_string = v8_str("bar");

Local<Module> module = CreateAndInstantiateSyntheticModule(
isolate, v8_str("SyntheticModuleSetExports-TestSyntheticModule"), context,
std::vector<v8::Local<v8::String>>(),
UnexpectedSyntheticModuleEvaluationStepsCallback);

i::Handle<i::SyntheticModule> i_module =
i::Handle<i::SyntheticModule>::cast(v8::Utils::OpenHandle(*module));
i::Handle<i::ObjectHashTable> exports(i_module->exports(), i_isolate);

TryCatch try_catch(isolate);
Maybe<bool> 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;
Expand Down

0 comments on commit 72493af

Please sign in to comment.