From 34d70ce615109e12f231e730040ac554b2349da7 Mon Sep 17 00:00:00 2001
From: Anna Henningsen <anna.henningsen@mongodb.com>
Date: Wed, 25 Jan 2023 21:06:55 +0100
Subject: [PATCH] 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: https://github.com/nodejs/node/pull/46320
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
---
 doc/api/vm.md                  |   6 ++
 lib/internal/vm.js             |   4 ++
 src/node_contextify.cc         | 125 ++++++++++++++++++++-------------
 src/node_contextify.h          |   8 +++
 test/parallel/test-vm-basic.js |  26 +++++--
 5 files changed, 114 insertions(+), 55 deletions(-)

diff --git a/doc/api/vm.md b/doc/api/vm.md
index 35a5d4f4bafd67..10c1cbefee828c 100644
--- a/doc/api/vm.md
+++ b/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
diff --git a/lib/internal/vm.js b/lib/internal/vm.js
index c9d0d9dd7da522..bf0a7902a17f42 100644
--- a/lib/internal/vm.js
+++ b/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');
diff --git a/src/node_contextify.cc b/src/node_contextify.cc
index 53b947fd1bcf2d..ed552ddd559f51 100644
--- a/src/node_contextify.cc
+++ b/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);
diff --git a/src/node_contextify.h b/src/node_contextify.h
index 87440169503fbd..76c89318bb6cbf 100644
--- a/src/node_contextify.h
+++ b/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
 
diff --git a/test/parallel/test-vm-basic.js b/test/parallel/test-vm-basic.js
index 13d212f0079ca7..f2424128b66e9f 100644
--- a/test/parallel/test-vm-basic.js
+++ b/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;