src: prefer internal fields in ModuleWrap

Use internal fields instead of `v8::Global`s where possible, since they generally come with lower overhead and it’s much harder to introduce memory leaks with them. PR-URL: #34470 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]>
nodejs · Jul 27, 2020 · b12211e · b12211e
1 parent 9b91467
commit b12211e
Show file tree

Hide file tree

Showing 2 changed files with 33 additions and 13 deletions.
diff --git a/src/module_wrap.cc b/src/module_wrap.cc
@@ -55,9 +55,13 @@ ModuleWrap::ModuleWrap(Environment* env,
                        Local<String> url)
   : BaseObject(env, object),
     module_(env->isolate(), module),
-    url_(env->isolate(), url),
     id_(env->get_next_module_id()) {
   env->id_to_module_map.emplace(id_, this);
+
+  Local<Value> undefined = Undefined(env->isolate());
+  object->SetInternalField(kURLSlot, url);
+  object->SetInternalField(kSyntheticEvaluationStepsSlot, undefined);
+  object->SetInternalField(kContextObjectSlot, undefined);
 }
 
 ModuleWrap::~ModuleWrap() {
@@ -73,6 +77,12 @@ ModuleWrap::~ModuleWrap() {
   }
 }
 
+Local<Context> ModuleWrap::context() const {
+  Local<Value> obj = object()->GetInternalField(kContextObjectSlot);
+  if (obj.IsEmpty()) return {};
+  return obj.As<Object>()->CreationContext();
+}
+
 ModuleWrap* ModuleWrap::GetFromModule(Environment* env,
                                       Local<Module> module) {
   auto range = env->hash_to_module_map.equal_range(module->GetIdentityHash());
@@ -220,11 +230,14 @@ void ModuleWrap::New(const FunctionCallbackInfo<Value>& args) {
 
   if (synthetic) {
     obj->synthetic_ = true;
-    obj->synthetic_evaluation_steps_.Reset(
-        env->isolate(), args[3].As<Function>());
+    obj->object()->SetInternalField(kSyntheticEvaluationStepsSlot, args[3]);
   }
 
-  obj->context_.Reset(isolate, context);
+  // Use the extras object as an object whose CreationContext() will be the
+  // original `context`, since the `Context` itself strictly speaking cannot
+  // be stored in an internal field.
+  obj->object()->SetInternalField(kContextObjectSlot,
+      context->GetExtrasBindingObject());
   obj->contextify_context_ = contextify_context;
 
   env->hash_to_module_map.emplace(module->GetIdentityHash(), obj);
@@ -254,7 +267,7 @@ void ModuleWrap::Link(const FunctionCallbackInfo<Value>& args) {
 
   Local<Function> resolver_arg = args[0].As<Function>();
 
-  Local<Context> mod_context = obj->context_.Get(isolate);
+  Local<Context> mod_context = obj->context();
   Local<Module> module = obj->module_.Get(isolate);
 
   const int module_requests_length = module->GetModuleRequestsLength();
@@ -295,7 +308,7 @@ void ModuleWrap::Instantiate(const FunctionCallbackInfo<Value>& args) {
   Isolate* isolate = args.GetIsolate();
   ModuleWrap* obj;
   ASSIGN_OR_RETURN_UNWRAP(&obj, args.This());
-  Local<Context> context = obj->context_.Get(isolate);
+  Local<Context> context = obj->context();
   Local<Module> module = obj->module_.Get(isolate);
   TryCatchScope try_catch(env);
   USE(module->InstantiateModule(context, ResolveCallback));
@@ -318,7 +331,7 @@ void ModuleWrap::Evaluate(const FunctionCallbackInfo<Value>& args) {
   Isolate* isolate = env->isolate();
   ModuleWrap* obj;
   ASSIGN_OR_RETURN_UNWRAP(&obj, args.This());
-  Local<Context> context = obj->context_.Get(isolate);
+  Local<Context> context = obj->context();
   Local<Module> module = obj->module_.Get(isolate);
 
   ContextifyContext* contextify_context = obj->contextify_context_;
@@ -636,8 +649,10 @@ MaybeLocal<Value> ModuleWrap::SyntheticModuleEvaluationStepsCallback(
 
   TryCatchScope try_catch(env);
   Local<Function> synthetic_evaluation_steps =
-      obj->synthetic_evaluation_steps_.Get(isolate);
-  obj->synthetic_evaluation_steps_.Reset();
+      obj->object()->GetInternalField(kSyntheticEvaluationStepsSlot)
+          .As<Function>();
+  obj->object()->SetInternalField(
+      kSyntheticEvaluationStepsSlot, Undefined(isolate));
   MaybeLocal<Value> ret = synthetic_evaluation_steps->Call(context,
       obj->object(), 0, nullptr);
   if (ret.IsEmpty()) {

diff --git a/src/module_wrap.h b/src/module_wrap.h
@@ -32,6 +32,14 @@ enum HostDefinedOptions : int {
 
 class ModuleWrap : public BaseObject {
  public:
+  enum InternalFields {
+    kModuleWrapBaseField = BaseObject::kInternalFieldCount,
+    kURLSlot,
+    kSyntheticEvaluationStepsSlot,
+    kContextObjectSlot,  // Object whose creation context is the target Context
+    kInternalFieldCount
+  };
+
   static void Initialize(v8::Local<v8::Object> target,
                          v8::Local<v8::Value> unused,
                          v8::Local<v8::Context> context,
@@ -42,11 +50,11 @@ class ModuleWrap : public BaseObject {
       v8::Local<v8::Object> meta);
 
   void MemoryInfo(MemoryTracker* tracker) const override {
-    tracker->TrackField("url", url_);
     tracker->TrackField("resolve_cache", resolve_cache_);
   }
 
   inline uint32_t id() { return id_; }
+  v8::Local<v8::Context> context() const;
   static ModuleWrap* GetFromID(node::Environment*, uint32_t id);
 
   SET_MEMORY_INFO_NAME(ModuleWrap)
@@ -85,11 +93,8 @@ class ModuleWrap : public BaseObject {
       v8::Local<v8::Module> referrer);
   static ModuleWrap* GetFromModule(node::Environment*, v8::Local<v8::Module>);
 
-  v8::Global<v8::Function> synthetic_evaluation_steps_;
   v8::Global<v8::Module> module_;
-  v8::Global<v8::String> url_;
   std::unordered_map<std::string, v8::Global<v8::Promise>> resolve_cache_;
-  v8::Global<v8::Context> context_;
   contextify::ContextifyContext* contextify_context_ = nullptr;
   bool synthetic_ = false;
   bool linked_ = false;