src: improve CompileFunctionAndCacheResult error handling

legendecas · legendecas · commit 370d044ffd86 · 2025-05-23T13:33:16.000+01:00
diff --git a/src/node_contextify.cc b/src/node_contextify.cc
@@ -365,13 +365,11 @@ void ContextifyContext::CreatePerIsolateProperties(
     IsolateData* isolate_data, Local<ObjectTemplate> target) {
   Isolate* isolate = isolate_data->isolate();
   SetMethod(isolate, target, "makeContext", MakeContext);
-  SetMethod(isolate, target, "compileFunction", CompileFunction);
 }
 
 void ContextifyContext::RegisterExternalReferences(
     ExternalReferenceRegistry* registry) {
   registry->Register(MakeContext);
-  registry->Register(CompileFunction);
   registry->Register(PropertyQueryCallback);
   registry->Register(PropertyGetterCallback);
   registry->Register(PropertySetterCallback);
@@ -1163,22 +1161,6 @@ Maybe<void> StoreCodeCacheResult(
   return JustVoid();
 }
 
-// TODO(RaisinTen): Reuse in ContextifyContext::CompileFunction().
-MaybeLocal<Function> CompileFunction(Local<Context> context,
-                                     Local<String> filename,
-                                     Local<String> content,
-                                     LocalVector<String>* parameters) {
-  ScriptOrigin script_origin(filename, 0, 0, true);
-  ScriptCompiler::Source script_source(content, script_origin);
-
-  return ScriptCompiler::CompileFunction(context,
-                                         &script_source,
-                                         parameters->size(),
-                                         parameters->data(),
-                                         0,
-                                         nullptr);
-}
-
 bool ContextifyScript::InstanceOf(Environment* env,
                                   const Local<Value>& value) {
   return !value.IsEmpty() &&
@@ -1392,7 +1374,19 @@ ContextifyScript::ContextifyScript(Environment* env, Local<Object> object) {
 
 ContextifyScript::~ContextifyScript() {}
 
-void ContextifyContext::CompileFunction(
+void ContextifyFunction::RegisterExternalReferences(
+    ExternalReferenceRegistry* registry) {
+  registry->Register(CompileFunction);
+}
+
+void ContextifyFunction::CreatePerIsolateProperties(
+    IsolateData* isolate_data, Local<ObjectTemplate> target) {
+  Isolate* isolate = isolate_data->isolate();
+
+  SetMethod(isolate, target, "compileFunction", CompileFunction);
+}
+
+void ContextifyFunction::CompileFunction(
     const FunctionCallbackInfo<Value>& args) {
   Environment* env = Environment::GetCurrent(args);
   Isolate* isolate = env->isolate();
@@ -1511,24 +1505,22 @@ void ContextifyContext::CompileFunction(
   }
 
   TryCatchScope try_catch(env);
-  Local<Object> result = CompileFunctionAndCacheResult(env,
-                                                       parsing_context,
-                                                       &source,
-                                                       params,
-                                                       context_extensions,
-                                                       options,
-                                                       produce_cached_data,
-                                                       id_symbol,
-                                                       try_catch);
-
-  if (try_catch.HasCaught() && !try_catch.HasTerminated()) {
+  MaybeLocal<Object> maybe_result =
+      CompileFunctionAndCacheResult(env,
+                                    parsing_context,
+                                    &source,
+                                    params,
+                                    context_extensions,
+                                    options,
+                                    produce_cached_data,
+                                    id_symbol,
+                                    try_catch);
+  Local<Object> result;
+  if (!maybe_result.ToLocal(&result)) {
+    CHECK(try_catch.HasCaught());
     try_catch.ReThrow();
     return;
   }
-
-  if (result.IsEmpty()) {
-    return;
-  }
   args.GetReturnValue().Set(result);
 }
 
@@ -1544,7 +1536,7 @@ static LocalVector<String> GetCJSParameters(IsolateData* data) {
   return result;
 }
 
-Local<Object> ContextifyContext::CompileFunctionAndCacheResult(
+MaybeLocal<Object> ContextifyFunction::CompileFunctionAndCacheResult(
     Environment* env,
     Local<Context> parsing_context,
     ScriptCompiler::Source* source,
@@ -1566,28 +1558,29 @@ Local<Object> ContextifyContext::CompileFunctionAndCacheResult(
 
   Local<Function> fn;
   if (!maybe_fn.ToLocal(&fn)) {
-    if (try_catch.HasCaught() && !try_catch.HasTerminated()) {
+    CHECK(try_catch.HasCaught());
+    if (!try_catch.HasTerminated()) {
       errors::DecorateErrorStack(env, try_catch);
-      return Object::New(env->isolate());
     }
+    return {};
   }
 
   Local<Context> context = env->context();
   if (fn->SetPrivate(context, env->host_defined_option_symbol(), id_symbol)
           .IsNothing()) {
-    return Object::New(env->isolate());
+    return {};
   }
 
   Isolate* isolate = env->isolate();
   Local<Object> result = Object::New(isolate);
   if (result->Set(parsing_context, env->function_string(), fn).IsNothing())
-    return Object::New(env->isolate());
+    return {};
   if (result
           ->Set(parsing_context,
                 env->source_map_url_string(),
                 fn->GetScriptOrigin().SourceMapUrl())
           .IsNothing())
-    return Object::New(env->isolate());
+    return {};
 
   std::unique_ptr<ScriptCompiler::CachedData> new_cached_data;
   if (produce_cached_data) {
@@ -1600,7 +1593,7 @@ Local<Object> ContextifyContext::CompileFunctionAndCacheResult(
                            produce_cached_data,
                            std::move(new_cached_data))
           .IsNothing()) {
-    return Object::New(env->isolate());
+    return {};
   }
 
   return result;
@@ -1974,6 +1967,7 @@ void CreatePerIsolateProperties(IsolateData* isolate_data,
 
   ContextifyContext::CreatePerIsolateProperties(isolate_data, target);
   ContextifyScript::CreatePerIsolateProperties(isolate_data, target);
+  ContextifyFunction::CreatePerIsolateProperties(isolate_data, target);
 
   SetMethod(isolate, target, "startSigintWatchdog", StartSigintWatchdog);
   SetMethod(isolate, target, "stopSigintWatchdog", StopSigintWatchdog);
@@ -2026,6 +2020,7 @@ static void CreatePerContextProperties(Local<Object> target,
 void RegisterExternalReferences(ExternalReferenceRegistry* registry) {
   ContextifyContext::RegisterExternalReferences(registry);
   ContextifyScript::RegisterExternalReferences(registry);
+  ContextifyFunction::RegisterExternalReferences(registry);
 
   registry->Register(CompileFunctionForCJSLoader);
   registry->Register(StartSigintWatchdog);
diff --git a/src/node_contextify.h b/src/node_contextify.h
@@ -144,18 +144,6 @@ class ContextifyContext final : CPPGC_MIXIN(ContextifyContext) {
   static bool IsStillInitializing(const ContextifyContext* ctx);
   static void MakeContext(const v8::FunctionCallbackInfo<v8::Value>& args);
   static void IsContext(const v8::FunctionCallbackInfo<v8::Value>& args);
-  static void CompileFunction(
-      const v8::FunctionCallbackInfo<v8::Value>& args);
-  static v8::Local<v8::Object> CompileFunctionAndCacheResult(
-      Environment* env,
-      v8::Local<v8::Context> parsing_context,
-      v8::ScriptCompiler::Source* source,
-      v8::LocalVector<v8::String> params,
-      v8::LocalVector<v8::Object> context_extensions,
-      v8::ScriptCompiler::CompileOptions options,
-      bool produce_cached_data,
-      v8::Local<v8::Symbol> id_symbol,
-      const errors::TryCatchScope& try_catch);
   static v8::Intercepted PropertyQueryCallback(
       v8::Local<v8::Name> property,
       const v8::PropertyCallbackInfo<v8::Integer>& args);
@@ -234,6 +222,29 @@ class ContextifyScript final : CPPGC_MIXIN(ContextifyScript) {
   v8::TracedReference<v8::UnboundScript> script_;
 };
 
+class ContextifyFunction final {
+ public:
+  static void RegisterExternalReferences(ExternalReferenceRegistry* registry);
+  static void CreatePerIsolateProperties(IsolateData* isolate_data,
+                                         v8::Local<v8::ObjectTemplate> target);
+
+  static void CompileFunction(const v8::FunctionCallbackInfo<v8::Value>& args);
+  static v8::MaybeLocal<v8::Object> CompileFunctionAndCacheResult(
+      Environment* env,
+      v8::Local<v8::Context> parsing_context,
+      v8::ScriptCompiler::Source* source,
+      v8::LocalVector<v8::String> params,
+      v8::LocalVector<v8::Object> context_extensions,
+      v8::ScriptCompiler::CompileOptions options,
+      bool produce_cached_data,
+      v8::Local<v8::Symbol> id_symbol,
+      const errors::TryCatchScope& try_catch);
+
+ private:
+  ContextifyFunction() = delete;
+  ~ContextifyFunction() = delete;
+};
+
 v8::Maybe<void> StoreCodeCacheResult(
     Environment* env,
     v8::Local<v8::Object> target,
@@ -242,12 +253,6 @@ v8::Maybe<void> StoreCodeCacheResult(
     bool produce_cached_data,
     std::unique_ptr<v8::ScriptCompiler::CachedData> new_cached_data);
 
-v8::MaybeLocal<v8::Function> CompileFunction(
-    v8::Local<v8::Context> context,
-    v8::Local<v8::String> filename,
-    v8::Local<v8::String> content,
-    v8::LocalVector<v8::String>* parameters);
-
 }  // namespace contextify
 }  // namespace node
 
diff --git a/src/node_sea.cc b/src/node_sea.cc
@@ -41,6 +41,7 @@ using v8::MaybeLocal;
 using v8::NewStringType;
 using v8::Object;
 using v8::ScriptCompiler;
+using v8::ScriptOrigin;
 using v8::String;
 using v8::Value;
 
@@ -460,16 +461,23 @@ std::optional<std::string> GenerateCodeCache(std::string_view main_path,
           FIXED_ONE_BYTE_STRING(isolate, "__filename"),
           FIXED_ONE_BYTE_STRING(isolate, "__dirname"),
       });
-
-  // TODO(RaisinTen): Using the V8 code cache prevents us from using `import()`
-  // in the SEA code. Support it.
-  // Refs: https://github.com/nodejs/node/pull/48191#discussion_r1213271430
+  ScriptOrigin script_origin(filename, 0, 0, true);
+  ScriptCompiler::Source script_source(content, script_origin);
+  MaybeLocal<Function> maybe_fn =
+      ScriptCompiler::CompileFunction(context,
+                                      &script_source,
+                                      parameters.size(),
+                                      parameters.data(),
+                                      0,
+                                      nullptr);
   Local<Function> fn;
-  if (!contextify::CompileFunction(context, filename, content, &parameters)
-           .ToLocal(&fn)) {
+  if (!maybe_fn.ToLocal(&fn)) {
     return std::nullopt;
   }
 
+  // TODO(RaisinTen): Using the V8 code cache prevents us from using `import()`
+  // in the SEA code. Support it.
+  // Refs: https://github.com/nodejs/node/pull/48191#discussion_r1213271430
   std::unique_ptr<ScriptCompiler::CachedData> cache{
       ScriptCompiler::CreateCodeCacheForFunction(fn)};
   std::string code_cache(cache->data, cache->data + cache->length);
diff --git a/src/node_snapshotable.cc b/src/node_snapshotable.cc
@@ -42,8 +42,11 @@ using v8::HandleScope;
 using v8::Isolate;
 using v8::Local;
 using v8::LocalVector;
+using v8::MaybeLocal;
 using v8::Object;
 using v8::ObjectTemplate;
+using v8::ScriptCompiler;
+using v8::ScriptOrigin;
 using v8::SnapshotCreator;
 using v8::StartupData;
 using v8::String;
@@ -1488,9 +1491,18 @@ void CompileSerializeMain(const FunctionCallbackInfo<Value>& args) {
           FIXED_ONE_BYTE_STRING(isolate, "__filename"),
           FIXED_ONE_BYTE_STRING(isolate, "__dirname"),
       });
+
+  ScriptOrigin script_origin(filename, 0, 0, true);
+  ScriptCompiler::Source script_source(source, script_origin);
+  MaybeLocal<Function> maybe_fn =
+      ScriptCompiler::CompileFunction(context,
+                                      &script_source,
+                                      parameters.size(),
+                                      parameters.data(),
+                                      0,
+                                      nullptr);
   Local<Function> fn;
-  if (contextify::CompileFunction(context, filename, source, &parameters)
-          .ToLocal(&fn)) {
+  if (maybe_fn.ToLocal(&fn)) {
     args.GetReturnValue().Set(fn);
   }
 }
