vm: unify host-defined option generation in vm.compileFunction

Set a default host-defined option for vm.compileFunction so that it's consistent with vm.Script. PR-URL: #50137 Refs: #35375 Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
nodejs · Oct 23, 2023 · a54179f · a54179f
1 parent b6021ab
commit a54179f
Show file tree

Hide file tree

Showing 4 changed files with 46 additions and 24 deletions.
diff --git a/lib/internal/vm.js b/lib/internal/vm.js
@@ -2,12 +2,16 @@
 
 const {
   ArrayPrototypeForEach,
+  Symbol,
 } = primordials;
 
 const {
   compileFunction,
   isContext: _isContext,
 } = internalBinding('contextify');
+const {
+  default_host_defined_options,
+} = internalBinding('symbols');
 const {
   validateArray,
   validateBoolean,
@@ -30,12 +34,27 @@ function isContext(object) {
   return _isContext(object);
 }
 
+function getHostDefinedOptionId(importModuleDynamically, filename) {
+  if (importModuleDynamically !== undefined) {
+    // Check that it's either undefined or a function before we pass
+    // it into the native constructor.
+    validateFunction(importModuleDynamically,
+                     'options.importModuleDynamically');
+  }
+  if (importModuleDynamically === undefined) {
+    // We need a default host defined options that are the same for all
+    // scripts not needing custom module callbacks so that the isolate
+    // compilation cache can be hit.
+    return default_host_defined_options;
+  }
+  return Symbol(filename);
+}
+
 function internalCompileFunction(code, params, options) {
   validateString(code, 'code');
   if (params !== undefined) {
     validateStringArray(params, 'params');
   }
-
   const {
     filename = '',
     columnOffset = 0,
@@ -72,6 +91,8 @@ function internalCompileFunction(code, params, options) {
     validateObject(extension, name, kValidateObjectAllowNullable);
   });
 
+  const hostDefinedOptionId =
+      getHostDefinedOptionId(importModuleDynamically, filename);
   const result = compileFunction(
     code,
     filename,
@@ -82,6 +103,7 @@ function internalCompileFunction(code, params, options) {
     parsingContext,
     contextExtensions,
     params,
+    hostDefinedOptionId,
   );
 
   if (produceCachedData) {
@@ -113,6 +135,7 @@ function internalCompileFunction(code, params, options) {
 }
 
 module.exports = {
+  getHostDefinedOptionId,
   internalCompileFunction,
   isContext,
 };
diff --git a/lib/vm.js b/lib/vm.js
@@ -41,7 +41,6 @@ const {
 const {
   validateBoolean,
   validateBuffer,
-  validateFunction,
   validateInt32,
   validateObject,
   validateOneOf,
@@ -54,6 +53,7 @@ const {
   kVmBreakFirstLineSymbol,
 } = require('internal/util');
 const {
+  getHostDefinedOptionId,
   internalCompileFunction,
   isContext,
 } = require('internal/vm');
@@ -86,12 +86,8 @@ class Script extends ContextifyScript {
     }
     validateBoolean(produceCachedData, 'options.produceCachedData');
 
-    if (importModuleDynamically !== undefined) {
-      // Check that it's either undefined or a function before we pass
-      // it into the native constructor.
-      validateFunction(importModuleDynamically,
-                       'options.importModuleDynamically');
-    }
+    const hostDefinedOptionId =
+        getHostDefinedOptionId(importModuleDynamically, filename);
     // Calling `ReThrow()` on a native TryCatch does not generate a new
     // abort-on-uncaught-exception check. A dummy try/catch in JS land
     // protects against that.
@@ -103,7 +99,7 @@ class Script extends ContextifyScript {
             cachedData,
             produceCachedData,
             parsingContext,
-            importModuleDynamically !== undefined);
+            hostDefinedOptionId);
     } catch (e) {
       throw e; /* node-do-not-add-exception-line */
     }

diff --git a/src/node_contextify.cc b/src/node_contextify.cc
@@ -771,11 +771,11 @@ void ContextifyScript::New(const FunctionCallbackInfo<Value>& args) {
   bool produce_cached_data = false;
   Local<Context> parsing_context = context;
 
-  bool needs_custom_host_defined_options = false;
+  Local<Symbol> id_symbol;
   if (argc > 2) {
     // new ContextifyScript(code, filename, lineOffset, columnOffset,
     //                      cachedData, produceCachedData, parsingContext,
-    //                      needsCustomHostDefinedOptions)
+    //                      hostDefinedOptionId)
     CHECK_EQ(argc, 8);
     CHECK(args[2]->IsNumber());
     line_offset = args[2].As<Int32>()->Value();
@@ -795,9 +795,8 @@ void ContextifyScript::New(const FunctionCallbackInfo<Value>& args) {
       CHECK_NOT_NULL(sandbox);
       parsing_context = sandbox->context();
     }
-    if (args[7]->IsTrue()) {
-      needs_custom_host_defined_options = true;
-    }
+    CHECK(args[7]->IsSymbol());
+    id_symbol = args[7].As<Symbol>();
   }
 
   ContextifyScript* contextify_script =
@@ -821,12 +820,6 @@ void ContextifyScript::New(const FunctionCallbackInfo<Value>& args) {
 
   Local<PrimitiveArray> host_defined_options =
       PrimitiveArray::New(isolate, loader::HostDefinedOptions::kLength);
-  // We need a default host defined options that's the same for all scripts
-  // not needing custom module callbacks for so that the isolate compilation
-  // cache can be hit.
-  Local<Symbol> id_symbol = needs_custom_host_defined_options
-                                ? Symbol::New(isolate, filename)
-                                : env->default_host_defined_options();
   host_defined_options->Set(
       isolate, loader::HostDefinedOptions::kID, id_symbol);
 
@@ -1199,6 +1192,10 @@ void ContextifyContext::CompileFunction(
     params_buf = args[8].As<Array>();
   }
 
+  // Argument 10: host-defined option symbol
+  CHECK(args[9]->IsSymbol());
+  Local<Symbol> id_symbol = args[9].As<Symbol>();
+
   // Read cache from cached data buffer
   ScriptCompiler::CachedData* cached_data = nullptr;
   if (!cached_data_buf.IsEmpty()) {
@@ -1210,7 +1207,6 @@ void ContextifyContext::CompileFunction(
   // Set host_defined_options
   Local<PrimitiveArray> host_defined_options =
       PrimitiveArray::New(isolate, loader::HostDefinedOptions::kLength);
-  Local<Symbol> id_symbol = Symbol::New(isolate, filename);
   host_defined_options->Set(
       isolate, loader::HostDefinedOptions::kID, id_symbol);
 

diff --git a/test/parallel/test-vm-no-dynamic-import-callback.js b/test/parallel/test-vm-no-dynamic-import-callback.js
@@ -1,7 +1,7 @@
 'use strict';
 
-require('../common');
-const { Script } = require('vm');
+const common = require('../common');
+const { Script, compileFunction } = require('vm');
 const assert = require('assert');
 
 assert.rejects(async () => {
@@ -10,4 +10,11 @@ assert.rejects(async () => {
   await imported;
 }, {
   code: 'ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING'
-});
+}).then(common.mustCall());
+
+assert.rejects(async () => {
+  const imported = compileFunction('return import("fs")')();
+  await imported;
+}, {
+  code: 'ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING'
+}).then(common.mustCall());