src: make CompiledFnEntry a BaseObject

In particular: - Move the class definition to the relevant header file, i.e. `node_contextify.h`. - Make sure that class instances are destroyed on `Environment` teardown. - Make instances of the key object traceable in heap dumps. This is particularly relevant here because our C++ script → map key mapping could introduce memory leaks when the import function metadata refers back to the script in some way. Refs: #28671 PR-URL: #28782 Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Rich Trott <[email protected]>
nodejs · Jul 23, 2019 · c0f24be · c0f24be
1 parent 7b4638c
commit c0f24be
Show file tree

Hide file tree

Showing 5 changed files with 60 additions and 34 deletions.
diff --git a/src/env.cc b/src/env.cc
@@ -39,15 +39,13 @@ using v8::NewStringType;
 using v8::Number;
 using v8::Object;
 using v8::Private;
-using v8::ScriptOrModule;
 using v8::SnapshotCreator;
 using v8::StackTrace;
 using v8::String;
 using v8::Symbol;
 using v8::TracingController;
 using v8::Undefined;
 using v8::Value;
-using v8::WeakCallbackInfo;
 using worker::Worker;
 
 int const Environment::kNodeContextTag = 0x6e6f64;
@@ -387,24 +385,6 @@ Environment::Environment(IsolateData* isolate_data,
   CreateProperties();
 }
 
-static void WeakCallbackCompiledFn(
-    const WeakCallbackInfo<CompiledFnEntry>& data) {
-  CompiledFnEntry* entry = data.GetParameter();
-  entry->env->id_to_function_map.erase(entry->id);
-  delete entry;
-}
-
-CompiledFnEntry::CompiledFnEntry(Environment* env,
-                                 uint32_t id,
-                                 Local<ScriptOrModule> script)
-    : env(env),
-      id(id),
-      cache_key(env->isolate(), Object::New(env->isolate())),
-      script(env->isolate(), script) {
-  this->script.SetWeak(
-      this, WeakCallbackCompiledFn, v8::WeakCallbackType::kParameter);
-}
-
 Environment::~Environment() {
   isolate()->GetHeapProfiler()->RemoveBuildEmbedderGraphCallback(
       BuildEmbedderGraph, this);

diff --git a/src/env.h b/src/env.h
@@ -55,6 +55,7 @@ namespace node {
 
 namespace contextify {
 class ContextifyScript;
+class CompiledFnEntry;
 }
 
 namespace fs {
@@ -355,6 +356,7 @@ constexpr size_t kFsStatsBufferLength = kFsStatsFieldsNumber * 2;
   V(as_callback_data_template, v8::FunctionTemplate)                           \
   V(async_wrap_ctor_template, v8::FunctionTemplate)                            \
   V(async_wrap_object_ctor_template, v8::FunctionTemplate)                     \
+  V(compiled_fn_entry_template, v8::ObjectTemplate)                            \
   V(fd_constructor_template, v8::ObjectTemplate)                               \
   V(fdclose_constructor_template, v8::ObjectTemplate)                          \
   V(filehandlereadwrap_template, v8::ObjectTemplate)                           \
@@ -501,16 +503,6 @@ struct ContextInfo {
   bool is_default = false;
 };
 
-struct CompiledFnEntry {
-  Environment* env;
-  uint32_t id;
-  v8::Global<v8::Object> cache_key;
-  v8::Global<v8::ScriptOrModule> script;
-  CompiledFnEntry(Environment* env,
-                  uint32_t id,
-                  v8::Local<v8::ScriptOrModule> script);
-};
-
 // Listing the AsyncWrap provider types first enables us to cast directly
 // from a provider type to a debug category.
 #define DEBUG_CATEGORY_NAMES(V)                                                \
@@ -995,7 +987,7 @@ class Environment : public MemoryRetainer {
   std::unordered_map<uint32_t, loader::ModuleWrap*> id_to_module_map;
   std::unordered_map<uint32_t, contextify::ContextifyScript*>
       id_to_script_map;
-  std::unordered_map<uint32_t, CompiledFnEntry*> id_to_function_map;
+  std::unordered_map<uint32_t, contextify::CompiledFnEntry*> id_to_function_map;
 
   inline uint32_t get_next_module_id();
   inline uint32_t get_next_script_id();

diff --git a/src/module_wrap.cc b/src/module_wrap.cc
@@ -1041,7 +1041,9 @@ static MaybeLocal<Promise> ImportModuleDynamically(
     ModuleWrap* wrap = ModuleWrap::GetFromID(env, id);
     object = wrap->object();
   } else if (type == ScriptType::kFunction) {
-    object = env->id_to_function_map.find(id)->second->cache_key.Get(iso);
+    auto it = env->id_to_function_map.find(id);
+    CHECK_NE(it, env->id_to_function_map.end());
+    object = it->second->object();
   } else {
     UNREACHABLE();
   }

diff --git a/src/node_contextify.cc b/src/node_contextify.cc
@@ -1124,9 +1124,13 @@ void ContextifyContext::CompileFunction(
   }
   Local<Function> fn = maybe_fn.ToLocalChecked();
 
-  CompiledFnEntry* entry = new CompiledFnEntry(env, id, script);
+  Local<Object> cache_key;
+  if (!env->compiled_fn_entry_template()->NewInstance(
+           context).ToLocal(&cache_key)) {
+    return;
+  }
+  CompiledFnEntry* entry = new CompiledFnEntry(env, cache_key, id, script);
   env->id_to_function_map.emplace(id, entry);
-  Local<Object> cache_key = entry->cache_key.Get(isolate);
 
   Local<Object> result = Object::New(isolate);
   if (result->Set(parsing_context, env->function_string(), fn).IsNothing())
@@ -1162,6 +1166,27 @@ void ContextifyContext::CompileFunction(
   args.GetReturnValue().Set(result);
 }
 
+void CompiledFnEntry::WeakCallback(
+    const WeakCallbackInfo<CompiledFnEntry>& data) {
+  CompiledFnEntry* entry = data.GetParameter();
+  delete entry;
+}
+
+CompiledFnEntry::CompiledFnEntry(Environment* env,
+                                 Local<Object> object,
+                                 uint32_t id,
+                                 Local<ScriptOrModule> script)
+    : BaseObject(env, object),
+      id_(id),
+      script_(env->isolate(), script) {
+  script_.SetWeak(this, WeakCallback, v8::WeakCallbackType::kParameter);
+}
+
+CompiledFnEntry::~CompiledFnEntry() {
+  env()->id_to_function_map.erase(id_);
+  script_.ClearWeak();
+}
+
 static void StartSigintWatchdog(const FunctionCallbackInfo<Value>& args) {
   int ret = SigintWatchdogHelper::GetInstance()->Start();
   args.GetReturnValue().Set(ret == 0);
@@ -1190,6 +1215,14 @@ void Initialize(Local<Object> target,
   // Used in tests.
   env->SetMethodNoSideEffect(
       target, "watchdogHasPendingSigint", WatchdogHasPendingSigint);
+
+  {
+    Local<FunctionTemplate> tpl = FunctionTemplate::New(env->isolate());
+    tpl->SetClassName(FIXED_ONE_BYTE_STRING(env->isolate(), "CompiledFnEntry"));
+    tpl->InstanceTemplate()->SetInternalFieldCount(1);
+
+    env->set_compiled_fn_entry_template(tpl->InstanceTemplate());
+  }
 }
 
 }  // namespace contextify

diff --git a/src/node_contextify.h b/src/node_contextify.h
@@ -133,6 +133,25 @@ class ContextifyScript : public BaseObject {
   uint32_t id_;
 };
 
+class CompiledFnEntry final : public BaseObject {
+ public:
+  SET_NO_MEMORY_INFO()
+  SET_MEMORY_INFO_NAME(CompiledFnEntry)
+  SET_SELF_SIZE(CompiledFnEntry)
+
+  CompiledFnEntry(Environment* env,
+                  v8::Local<v8::Object> object,
+                  uint32_t id,
+                  v8::Local<v8::ScriptOrModule> script);
+  ~CompiledFnEntry();
+
+ private:
+  uint32_t id_;
+  v8::Global<v8::ScriptOrModule> script_;
+
+  static void WeakCallback(const v8::WeakCallbackInfo<CompiledFnEntry>& data);
+};
+
 }  // namespace contextify
 }  // namespace node