vm: expose cachedDataRejected for vm.compileFunction

Having this information available is useful for functions just as
it is for scripts. Therefore, expose it in the same way that other
information related to code caching is reported.

As part of this, de-duplify the code for setting the properties on
the C++ side and add proper exception handling to it.

PR-URL: #46320
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>

Author: addaleax

doc/api/vm.md

@@ -962,6 +962,12 @@ const vm = require('node:vm');
 <!-- YAML
 added: v10.10.0
 changes:
+  - version:
+    - REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/46320
+    description: The return value now includes `cachedDataRejected`
+                 with the same semantics as the `vm.Script` version
+                 if the `cachedData` option was passed.
   - version:
     - v17.0.0
     - v16.12.0

lib/internal/vm.js

@@ -90,6 +90,10 @@ function internalCompileFunction(code, params, options) {
     result.function.cachedData = result.cachedData;
   }
 
+  if (typeof result.cachedDataRejected === 'boolean') {
+    result.function.cachedDataRejected = result.cachedDataRejected;
+  }
+
   if (importModuleDynamically !== undefined) {
     validateFunction(importModuleDynamically,
                      'options.importModuleDynamically');

src/node_contextify.cc

@@ -49,6 +49,7 @@ using v8::HandleScope;
 using v8::IndexedPropertyHandlerConfiguration;
 using v8::Int32;
 using v8::Isolate;
+using v8::Just;
 using v8::Local;
 using v8::Maybe;
 using v8::MaybeLocal;
@@ -58,6 +59,7 @@ using v8::MicrotaskQueue;
 using v8::MicrotasksPolicy;
 using v8::Name;
 using v8::NamedPropertyHandlerConfiguration;
+using v8::Nothing;
 using v8::Number;
 using v8::Object;
 using v8::ObjectTemplate;
@@ -857,40 +859,75 @@ void ContextifyScript::New(const FunctionCallbackInfo<Value>& args) {
   }
   contextify_script->script_.Reset(isolate, v8_script);
 
-  Local<Context> env_context = env->context();
-  if (compile_options == ScriptCompiler::kConsumeCodeCache) {
-    args.This()->Set(
-        env_context,
-        env->cached_data_rejected_string(),
-        Boolean::New(isolate, source.GetCachedData()->rejected)).Check();
-  } else if (produce_cached_data) {
-    std::unique_ptr<ScriptCompiler::CachedData> cached_data{
-        ScriptCompiler::CreateCodeCache(v8_script)};
-    bool cached_data_produced = cached_data != nullptr;
-    if (cached_data_produced) {
-      MaybeLocal<Object> buf = Buffer::Copy(
-          env,
-          reinterpret_cast<const char*>(cached_data->data),
-          cached_data->length);
-      args.This()->Set(env_context,
-                       env->cached_data_string(),
-                       buf.ToLocalChecked()).Check();
-    }
-    args.This()->Set(
-        env_context,
-        env->cached_data_produced_string(),
-        Boolean::New(isolate, cached_data_produced)).Check();
+  std::unique_ptr<ScriptCompiler::CachedData> new_cached_data;
+  if (produce_cached_data) {
+    new_cached_data.reset(ScriptCompiler::CreateCodeCache(v8_script));
   }
 
-  args.This()
-      ->Set(env_context,
-            env->source_map_url_string(),
-            v8_script->GetSourceMappingURL())
-      .Check();
+  if (StoreCodeCacheResult(env,
+                           args.This(),
+                           compile_options,
+                           source,
+                           produce_cached_data,
+                           std::move(new_cached_data))
+          .IsNothing()) {
+    return;
+  }
+
+  if (args.This()
+          ->Set(env->context(),
+                env->source_map_url_string(),
+                v8_script->GetSourceMappingURL())
+          .IsNothing())
+    return;
 
   TRACE_EVENT_END0(TRACING_CATEGORY_NODE2(vm, script), "ContextifyScript::New");
 }
 
+Maybe<bool> StoreCodeCacheResult(
+    Environment* env,
+    Local<Object> target,
+    ScriptCompiler::CompileOptions compile_options,
+    const v8::ScriptCompiler::Source& source,
+    bool produce_cached_data,
+    std::unique_ptr<ScriptCompiler::CachedData> new_cached_data) {
+  Local<Context> context;
+  if (!target->GetCreationContext().ToLocal(&context)) {
+    return Nothing<bool>();
+  }
+  if (compile_options == ScriptCompiler::kConsumeCodeCache) {
+    if (target
+            ->Set(
+                context,
+                env->cached_data_rejected_string(),
+                Boolean::New(env->isolate(), source.GetCachedData()->rejected))
+            .IsNothing()) {
+      return Nothing<bool>();
+    }
+  }
+  if (produce_cached_data) {
+    bool cached_data_produced = new_cached_data != nullptr;
+    if (cached_data_produced) {
+      MaybeLocal<Object> buf =
+          Buffer::Copy(env,
+                       reinterpret_cast<const char*>(new_cached_data->data),
+                       new_cached_data->length);
+      if (target->Set(context, env->cached_data_string(), buf.ToLocalChecked())
+              .IsNothing()) {
+        return Nothing<bool>();
+      }
+    }
+    if (target
+            ->Set(context,
+                  env->cached_data_produced_string(),
+                  Boolean::New(env->isolate(), cached_data_produced))
+            .IsNothing()) {
+      return Nothing<bool>();
+    }
+  }
+  return Just(true);
+}
+
 bool ContextifyScript::InstanceOf(Environment* env,
                                   const Local<Value>& value) {
   return !value.IsEmpty() &&
@@ -1242,28 +1279,18 @@ void ContextifyContext::CompileFunction(
           .IsNothing())
     return;
 
+  std::unique_ptr<ScriptCompiler::CachedData> new_cached_data;
   if (produce_cached_data) {
-    const std::unique_ptr<ScriptCompiler::CachedData> cached_data(
-        ScriptCompiler::CreateCodeCacheForFunction(fn));
-    bool cached_data_produced = cached_data != nullptr;
-    if (cached_data_produced) {
-      MaybeLocal<Object> buf = Buffer::Copy(
-          env,
-          reinterpret_cast<const char*>(cached_data->data),
-          cached_data->length);
-      if (result
-              ->Set(parsing_context,
-                    env->cached_data_string(),
-                    buf.ToLocalChecked())
-              .IsNothing())
-        return;
-    }
-    if (result
-            ->Set(parsing_context,
-                  env->cached_data_produced_string(),
-                  Boolean::New(isolate, cached_data_produced))
-            .IsNothing())
-      return;
+    new_cached_data.reset(ScriptCompiler::CreateCodeCacheForFunction(fn));
+  }
+  if (StoreCodeCacheResult(env,
+                           result,
+                           options,
+                           source,
+                           produce_cached_data,
+                           std::move(new_cached_data))
+          .IsNothing()) {
+    return;
   }
 
   args.GetReturnValue().Set(result);

src/node_contextify.h

@@ -199,6 +199,14 @@ class CompiledFnEntry final : public BaseObject {
   static void WeakCallback(const v8::WeakCallbackInfo<CompiledFnEntry>& data);
 };
 
+v8::Maybe<bool> StoreCodeCacheResult(
+    Environment* env,
+    v8::Local<v8::Object> target,
+    v8::ScriptCompiler::CompileOptions compile_options,
+    const v8::ScriptCompiler::Source& source,
+    bool produce_cached_data,
+    std::unique_ptr<v8::ScriptCompiler::CachedData> new_cached_data);
+
 }  // namespace contextify
 }  // namespace node
 

test/parallel/test-vm-basic.js

@@ -323,13 +323,27 @@ const vm = require('vm');
     global
   );
 
-  // Test compileFunction produceCachedData option
-  const result = vm.compileFunction('console.log("Hello, World!")', [], {
-    produceCachedData: true,
-  });
+  {
+    const source = 'console.log("Hello, World!")';
+    // Test compileFunction produceCachedData option
+    const result = vm.compileFunction(source, [], {
+      produceCachedData: true,
+    });
 
-  assert.ok(result.cachedDataProduced);
-  assert.ok(result.cachedData.length > 0);
+    assert.ok(result.cachedDataProduced);
+    assert.ok(result.cachedData.length > 0);
+
+    // Test compileFunction cachedData consumption
+    const result2 = vm.compileFunction(source, [], {
+      cachedData: result.cachedData
+    });
+    assert.strictEqual(result2.cachedDataRejected, false);
+
+    const result3 = vm.compileFunction('console.log("wrong source")', [], {
+      cachedData: result.cachedData
+    });
+    assert.strictEqual(result3.cachedDataRejected, true);
+  }
 
   // Resetting value
   Error.stackTraceLimit = oldLimit;