From 9e8629cfbdf85552f8cea72456e64407a63c90f4 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Fri, 8 Sep 2023 13:08:09 +0200 Subject: [PATCH] src: set ModuleWrap internal fields only once There is no need to initialize the internal fields to undefined and then initialize them to something else in the caller. Simply pass the internal fields into the constructor to initialize them just once. PR-URL: https://github.com/nodejs/node/pull/49391 Reviewed-By: Darshan Sen Reviewed-By: Yagiz Nizipli Reviewed-By: Stephen Belanger Reviewed-By: Chengzhong Wu --- src/module_wrap.cc | 40 +++++++++++++++++++++++----------------- src/module_wrap.h | 4 +++- 2 files changed, 26 insertions(+), 18 deletions(-) diff --git a/src/module_wrap.cc b/src/module_wrap.cc index 2e3d444f64cd15..a1b7633a31d1df 100644 --- a/src/module_wrap.cc +++ b/src/module_wrap.cc @@ -52,16 +52,22 @@ using v8::Value; ModuleWrap::ModuleWrap(Environment* env, Local object, Local module, - Local url) - : BaseObject(env, object), - module_(env->isolate(), module), - id_(env->get_next_module_id()) { + Local url, + Local context_object, + Local synthetic_evaluation_step) + : BaseObject(env, object), + module_(env->isolate(), module), + id_(env->get_next_module_id()) { env->id_to_module_map.emplace(id_, this); - Local undefined = Undefined(env->isolate()); object->SetInternalField(kURLSlot, url); - object->SetInternalField(kSyntheticEvaluationStepsSlot, undefined); - object->SetInternalField(kContextObjectSlot, undefined); + object->SetInternalField(kSyntheticEvaluationStepsSlot, + synthetic_evaluation_step); + object->SetInternalField(kContextObjectSlot, context_object); + + if (!synthetic_evaluation_step->IsUndefined()) { + synthetic_ = true; + } } ModuleWrap::~ModuleWrap() { @@ -79,7 +85,9 @@ ModuleWrap::~ModuleWrap() { Local ModuleWrap::context() const { Local obj = object()->GetInternalField(kContextObjectSlot).As(); - if (obj.IsEmpty()) return {}; + // If this fails, there is likely a bug e.g. ModuleWrap::context() is accessed + // before the ModuleWrap constructor completes. + CHECK(obj->IsObject()); return obj.As()->GetCreationContext().ToLocalChecked(); } @@ -227,18 +235,16 @@ void ModuleWrap::New(const FunctionCallbackInfo& args) { return; } - ModuleWrap* obj = new ModuleWrap(env, that, module, url); - - if (synthetic) { - obj->synthetic_ = true; - obj->object()->SetInternalField(kSyntheticEvaluationStepsSlot, args[3]); - } - // Use the extras object as an object whose GetCreationContext() will be the // original `context`, since the `Context` itself strictly speaking cannot // be stored in an internal field. - obj->object()->SetInternalField(kContextObjectSlot, - context->GetExtrasBindingObject()); + Local context_object = context->GetExtrasBindingObject(); + Local synthetic_evaluation_step = + synthetic ? args[3] : Undefined(env->isolate()).As(); + + ModuleWrap* obj = new ModuleWrap( + env, that, module, url, context_object, synthetic_evaluation_step); + obj->contextify_context_ = contextify_context; env->hash_to_module_map.emplace(module->GetIdentityHash(), obj); diff --git a/src/module_wrap.h b/src/module_wrap.h index ce4610a461a2b4..00a22b79a801ab 100644 --- a/src/module_wrap.h +++ b/src/module_wrap.h @@ -72,7 +72,9 @@ class ModuleWrap : public BaseObject { ModuleWrap(Environment* env, v8::Local object, v8::Local module, - v8::Local url); + v8::Local url, + v8::Local context_object, + v8::Local synthetic_evaluation_step); ~ModuleWrap() override; static void New(const v8::FunctionCallbackInfo& args);