module: revert module._compile to original state if module is patched

PR-URL: https://github.com/nodejs/node/pull/21573
Fixes: https://github.com/nodejs/node/issues/17396
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

lib/assert.js

@@ -39,7 +39,6 @@ let isDeepEqual;
 let isDeepStrictEqual;
 let parseExpressionAt;
 let findNodeAround;
-let columnOffset = 0;
 let decoder;
 
 function lazyLoadComparison() {
@@ -256,16 +255,6 @@ function getErrMessage(message, fn) {
   const line = call.getLineNumber() - 1;
   let column = call.getColumnNumber() - 1;
 
-  // Line number one reports the wrong column due to being wrapped in a
-  // function. Remove that offset to get the actual call.
-  if (line === 0) {
-    if (columnOffset === 0) {
-      const { wrapper } = require('internal/modules/cjs/loader');
-      columnOffset = wrapper[0].length;
-    }
-    column -= columnOffset;
-  }
-
   const identifier = `${filename}${line}${column}`;
 
   if (errorCache.has(identifier)) {

lib/internal/modules/cjs/helpers.js

@@ -1,6 +1,9 @@
 'use strict';
 
 const { validateString } = require('internal/validators');
+const path = require('path');
+const { pathToFileURL } = require('internal/url');
+const { URL } = require('url');
 
 const {
   CHAR_LINE_FEED,
@@ -145,10 +148,18 @@ function addBuiltinLibsToObject(object) {
   });
 }
 
+function normalizeReferrerURL(referrer) {
+  if (typeof referrer === 'string' && path.isAbsolute(referrer)) {
+    return pathToFileURL(referrer).href;
+  }
+  return new URL(referrer).href;
+}
+
 module.exports = exports = {
   addBuiltinLibsToObject,
   builtinLibs,
   makeRequireFunction,
+  normalizeReferrerURL,
   requireDepth: 0,
   stripBOM,
   stripShebang

lib/internal/modules/cjs/loader.js

@@ -29,7 +29,6 @@ const assert = require('internal/assert');
 const fs = require('fs');
 const internalFS = require('internal/fs/utils');
 const path = require('path');
-const { URL } = require('url');
 const {
   internalModuleReadJSON,
   internalModuleStat
@@ -37,6 +36,7 @@ const {
 const { safeGetenv } = internalBinding('credentials');
 const {
   makeRequireFunction,
+  normalizeReferrerURL,
   requireDepth,
   stripBOM,
   stripShebang
@@ -48,6 +48,7 @@ const experimentalModules = getOptionValue('--experimental-modules');
 const manifest = getOptionValue('--experimental-policy') ?
   require('internal/process/policy').manifest :
   null;
+const { compileFunction } = internalBinding('contextify');
 
 const {
   ERR_INVALID_ARG_VALUE,
@@ -129,15 +130,52 @@ Module._extensions = Object.create(null);
 var modulePaths = [];
 Module.globalPaths = [];
 
-Module.wrap = function(script) {
+let patched = false;
+
+// eslint-disable-next-line func-style
+let wrap = function(script) {
   return Module.wrapper[0] + script + Module.wrapper[1];
 };
 
-Module.wrapper = [
+const wrapper = [
   '(function (exports, require, module, __filename, __dirname) { ',
   '\n});'
 ];
 
+let wrapperProxy = new Proxy(wrapper, {
+  set(target, property, value, receiver) {
+    patched = true;
+    return Reflect.set(target, property, value, receiver);
+  },
+
+  defineProperty(target, property, descriptor) {
+    patched = true;
+    return Object.defineProperty(target, property, descriptor);
+  }
+});
+
+Object.defineProperty(Module, 'wrap', {
+  get() {
+    return wrap;
+  },
+
+  set(value) {
+    patched = true;
+    wrap = value;
+  }
+});
+
+Object.defineProperty(Module, 'wrapper', {
+  get() {
+    return wrapperProxy;
+  },
+
+  set(value) {
+    patched = true;
+    wrapperProxy = value;
+  }
+});
+
 const debug = util.debuglog('module');
 
 Module._debug = util.deprecate(debug, 'Module._debug is deprecated.',
@@ -680,13 +718,6 @@ Module.prototype.require = function(id) {
 // (needed for setting breakpoint when called with --inspect-brk)
 var resolvedArgv;
 
-function normalizeReferrerURL(referrer) {
-  if (typeof referrer === 'string' && path.isAbsolute(referrer)) {
-    return pathToFileURL(referrer).href;
-  }
-  return new URL(referrer).href;
-}
-
 
 // Run the file contents in the correct scope or sandbox. Expose
 // the correct helper variables (require, module, exports) to
@@ -700,13 +731,48 @@ Module.prototype._compile = function(content, filename) {
 
   content = stripShebang(content);
 
-  const compiledWrapper = vm.compileFunction(content, [
+  let compiledWrapper;
+  if (patched) {
+    const wrapper = Module.wrap(content);
+    compiledWrapper = vm.runInThisContext(wrapper, {
+      filename,
+      lineOffset: 0,
+      displayErrors: true,
+      importModuleDynamically: experimentalModules ? async (specifier) => {
+        if (asyncESM === undefined) lazyLoadESM();
+        const loader = await asyncESM.loaderPromise;
+        return loader.import(specifier, normalizeReferrerURL(filename));
+      } : undefined,
+    });
+  } else {
+    compiledWrapper = compileFunction(
+      content,
+      filename,
+      0,
+      0,
+      undefined,
+      false,
+      undefined,
+      [],
+      [
         'exports',
         'require',
         'module',
         '__filename',
         '__dirname',
-  ], { filename });
+      ]
+    );
+    if (experimentalModules) {
+      const { callbackMap } = internalBinding('module_wrap');
+      callbackMap.set(compiledWrapper, {
+        importModuleDynamically: async (specifier) => {
+          if (asyncESM === undefined) lazyLoadESM();
+          const loader = await asyncESM.loaderPromise;
+          return loader.import(specifier, normalizeReferrerURL(filename));
+        }
+      });
+    }
+  }
 
   var inspectorWrapper = null;
   if (process._breakFirstLine && process._eval == null) {

src/env-inl.h

@@ -461,6 +461,9 @@ inline uint32_t Environment::get_next_module_id() {
 inline uint32_t Environment::get_next_script_id() {
   return script_id_counter_++;
 }
+inline uint32_t Environment::get_next_function_id() {
+  return function_id_counter_++;
+}
 
 Environment::ShouldNotAbortOnUncaughtScope::ShouldNotAbortOnUncaughtScope(
     Environment* env)

src/env.cc

@@ -252,6 +252,11 @@ Environment::Environment(IsolateData* isolate_data,
   isolate()->SetPromiseRejectCallback(task_queue::PromiseRejectCallback);
 }
 
+CompileFnEntry::CompileFnEntry(Environment* env, uint32_t id)
+    : env(env), id(id) {
+  env->compile_fn_entries.insert(this);
+}
+
 Environment::~Environment() {
   isolate()->GetHeapProfiler()->RemoveBuildEmbedderGraphCallback(
       BuildEmbedderGraph, this);
@@ -260,6 +265,12 @@ Environment::~Environment() {
   // CleanupHandles() should have removed all of them.
   CHECK(file_handle_read_wrap_freelist_.empty());
 
+  // dispose the Persistent references to the compileFunction
+  // wrappers used in the dynamic import callback
+  for (auto& entry : compile_fn_entries) {
+    delete entry;
+  }
+
   HandleScope handle_scope(isolate());
 
 #if HAVE_INSPECTOR

src/env.h

@@ -450,6 +450,12 @@ struct ContextInfo {
   bool is_default = false;
 };
 
+struct CompileFnEntry {
+  Environment* env;
+  uint32_t id;
+  CompileFnEntry(Environment* env, uint32_t id);
+};
+
 // Listing the AsyncWrap provider types first enables us to cast directly
 // from a provider type to a debug category.
 #define DEBUG_CATEGORY_NAMES(V)                                                \
@@ -720,9 +726,12 @@ class Environment {
   std::unordered_map<uint32_t, loader::ModuleWrap*> id_to_module_map;
   std::unordered_map<uint32_t, contextify::ContextifyScript*>
       id_to_script_map;
+  std::unordered_set<CompileFnEntry*> compile_fn_entries;
+  std::unordered_map<uint32_t, Persistent<v8::Function>> id_to_function_map;
 
   inline uint32_t get_next_module_id();
   inline uint32_t get_next_script_id();
+  inline uint32_t get_next_function_id();
 
   std::unordered_map<std::string, const loader::PackageConfig>
       package_json_cache;
@@ -1006,6 +1015,7 @@ class Environment {
 
   uint32_t module_id_counter_ = 0;
   uint32_t script_id_counter_ = 0;
+  uint32_t function_id_counter_ = 0;
 
   AliasedBuffer<uint32_t, v8::Uint32Array> should_abort_on_uncaught_toggle_;
   int should_not_abort_scope_counter_ = 0;

src/module_wrap.cc

@@ -752,6 +752,8 @@ static MaybeLocal<Promise> ImportModuleDynamically(
   } else if (type == ScriptType::kModule) {
     ModuleWrap* wrap = ModuleWrap::GetFromID(env, id);
     object = wrap->object();
+  } else if (type == ScriptType::kFunction) {
+    object = env->id_to_function_map.find(id)->second.Get(iso);
   } else {
     UNREACHABLE();
   }

src/module_wrap.h

@@ -20,6 +20,7 @@ enum PackageMainCheck : bool {
 enum ScriptType : int {
   kScript,
   kModule,
+  kFunction,
 };
 
 enum HostDefinedOptions : int {

src/node_contextify.cc

@@ -285,6 +285,15 @@ void ContextifyContext::WeakCallback(
   delete context;
 }
 
+void ContextifyContext::WeakCallbackCompileFn(
+    const WeakCallbackInfo<CompileFnEntry>& data) {
+  CompileFnEntry* entry = data.GetParameter();
+  if (entry->env->compile_fn_entries.erase(entry) != 0) {
+    entry->env->id_to_function_map.erase(entry->id);
+    delete entry;
+  }
+}
+
 // static
 ContextifyContext* ContextifyContext::ContextFromContextifiedSandbox(
     Environment* env,
@@ -1027,7 +1036,30 @@ void ContextifyContext::CompileFunction(
       data + cached_data_buf->ByteOffset(), cached_data_buf->ByteLength());
   }
 
-  ScriptOrigin origin(filename, line_offset, column_offset, True(isolate));
+  // Get the function id
+  uint32_t id = env->get_next_function_id();
+
+  // Set host_defined_options
+  Local<PrimitiveArray> host_defined_options =
+      PrimitiveArray::New(isolate, loader::HostDefinedOptions::kLength);
+  host_defined_options->Set(
+      isolate,
+      loader::HostDefinedOptions::kType,
+      Number::New(isolate, loader::ScriptType::kFunction));
+  host_defined_options->Set(
+      isolate, loader::HostDefinedOptions::kID, Number::New(isolate, id));
+
+  ScriptOrigin origin(filename,
+                      line_offset,       // line offset
+                      column_offset,     // column offset
+                      True(isolate),     // is cross origin
+                      Local<Integer>(),  // script id
+                      Local<Value>(),    // source map URL
+                      False(isolate),    // is opaque (?)
+                      False(isolate),    // is WASM
+                      False(isolate),    // is ES Module
+                      host_defined_options);
+
   ScriptCompiler::Source source(code, origin, cached_data);
   ScriptCompiler::CompileOptions options;
   if (source.GetCachedData() == nullptr) {
@@ -1061,38 +1093,45 @@ void ContextifyContext::CompileFunction(
     }
   }
 
-  MaybeLocal<Function> maybe_fun = ScriptCompiler::CompileFunctionInContext(
+  MaybeLocal<Function> maybe_fn = ScriptCompiler::CompileFunctionInContext(
       parsing_context, &source, params.size(), params.data(),
       context_extensions.size(), context_extensions.data(), options);
 
-  Local<Function> fun;
+  if (maybe_fn.IsEmpty()) {
-  if (maybe_fun.IsEmpty() || !maybe_fun.ToLocal(&fun)) {
     DecorateErrorStack(env, try_catch);
     try_catch.ReThrow();
     return;
   }
+  Local<Function> fn = maybe_fn.ToLocalChecked();
+  env->id_to_function_map.emplace(std::piecewise_construct,
+                                  std::make_tuple(id),
+                                  std::make_tuple(isolate, fn));
+  CompileFnEntry* gc_entry = new CompileFnEntry(env, id);
+  env->id_to_function_map[id].SetWeak(gc_entry,
+      WeakCallbackCompileFn,
+      v8::WeakCallbackType::kParameter);
 
   if (produce_cached_data) {
     const std::unique_ptr<ScriptCompiler::CachedData> cached_data(
-        ScriptCompiler::CreateCodeCacheForFunction(fun));
+        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 (fun->Set(
+      if (fn->Set(
           parsing_context,
           env->cached_data_string(),
           buf.ToLocalChecked()).IsNothing()) return;
     }
-    if (fun->Set(
+    if (fn->Set(
         parsing_context,
         env->cached_data_produced_string(),
         Boolean::New(isolate, cached_data_produced)).IsNothing()) return;
   }
 
-  args.GetReturnValue().Set(fun);
+  args.GetReturnValue().Set(fn);
 }
 
 

src/node_contextify.h

@@ -61,6 +61,8 @@ class ContextifyContext {
       const v8::FunctionCallbackInfo<v8::Value>& args);
   static void WeakCallback(
       const v8::WeakCallbackInfo<ContextifyContext>& data);
+  static void WeakCallbackCompileFn(
+      const v8::WeakCallbackInfo<CompileFnEntry>& data);
   static void PropertyGetterCallback(
       v8::Local<v8::Name> property,
       const v8::PropertyCallbackInfo<v8::Value>& args);

test/fixtures/cjs-module-wrap.js

@@ -0,0 +1,9 @@
+const assert = require('assert');
+const m = require('module');
+
+global.mwc = 0;
+m.wrapper[0] += 'global.mwc = (global.mwc || 0 ) + 1;';
+
+require('./not-main-module.js');
+assert.strictEqual(mwc, 1);
+delete global.mwc;

test/parallel/test-module-wrap.js

@@ -0,0 +1,9 @@
+'use strict';
+require('../common');
+const fixtures = require('../common/fixtures');
+const { execFileSync } = require('child_process');
+
+const cjsModuleWrapTest = fixtures.path('cjs-module-wrap.js');
+const node = process.argv[0];
+
+execFileSync(node, [cjsModuleWrapTest], { stdio: 'pipe' });

test/parallel/test-v8-coverage.js

@@ -27,9 +27,9 @@ function nextdir() {
   const fixtureCoverage = getFixtureCoverage('basic.js', coverageDirectory);
   assert.ok(fixtureCoverage);
   // first branch executed.
-  assert.strictEqual(fixtureCoverage.functions[1].ranges[0].count, 1);
+  assert.strictEqual(fixtureCoverage.functions[0].ranges[0].count, 1);
   // second branch did not execute.
-  assert.strictEqual(fixtureCoverage.functions[1].ranges[1].count, 0);
+  assert.strictEqual(fixtureCoverage.functions[0].ranges[1].count, 0);
 }
 
 // Outputs coverage when process.exit(1) exits process.
@@ -43,9 +43,9 @@ function nextdir() {
   const fixtureCoverage = getFixtureCoverage('exit-1.js', coverageDirectory);
   assert.ok(fixtureCoverage, 'coverage not found for file');
   // first branch executed.
-  assert.strictEqual(fixtureCoverage.functions[1].ranges[0].count, 1);
+  assert.strictEqual(fixtureCoverage.functions[0].ranges[0].count, 1);
   // second branch did not execute.
-  assert.strictEqual(fixtureCoverage.functions[1].ranges[1].count, 0);
+  assert.strictEqual(fixtureCoverage.functions[0].ranges[1].count, 0);
 }
 
 // Outputs coverage when process.kill(process.pid, "SIGINT"); exits process.
@@ -61,9 +61,9 @@ function nextdir() {
   const fixtureCoverage = getFixtureCoverage('sigint.js', coverageDirectory);
   assert.ok(fixtureCoverage);
   // first branch executed.
-  assert.strictEqual(fixtureCoverage.functions[1].ranges[0].count, 1);
+  assert.strictEqual(fixtureCoverage.functions[0].ranges[0].count, 1);
   // second branch did not execute.
-  assert.strictEqual(fixtureCoverage.functions[1].ranges[1].count, 0);
+  assert.strictEqual(fixtureCoverage.functions[0].ranges[1].count, 0);
 }
 
 // outputs coverage from subprocess.
@@ -78,9 +78,9 @@ function nextdir() {
                                              coverageDirectory);
   assert.ok(fixtureCoverage);
   // first branch executed.
-  assert.strictEqual(fixtureCoverage.functions[2].ranges[0].count, 1);
+  assert.strictEqual(fixtureCoverage.functions[1].ranges[0].count, 1);
   // second branch did not execute.
-  assert.strictEqual(fixtureCoverage.functions[2].ranges[1].count, 0);
+  assert.strictEqual(fixtureCoverage.functions[1].ranges[1].count, 0);
 }
 
 // outputs coverage from worker.
@@ -95,9 +95,9 @@ function nextdir() {
                                              coverageDirectory);
   assert.ok(fixtureCoverage);
   // first branch executed.
-  assert.strictEqual(fixtureCoverage.functions[2].ranges[0].count, 1);
+  assert.strictEqual(fixtureCoverage.functions[1].ranges[0].count, 1);
   // second branch did not execute.
-  assert.strictEqual(fixtureCoverage.functions[2].ranges[1].count, 0);
+  assert.strictEqual(fixtureCoverage.functions[1].ranges[1].count, 0);
 }
 
 // Does not output coverage if NODE_V8_COVERAGE is empty.
@@ -125,7 +125,7 @@ function nextdir() {
                                              coverageDirectory);
   assert.ok(fixtureCoverage);
   // first branch executed.
-  assert.strictEqual(fixtureCoverage.functions[1].ranges[0].count, 1);
+  assert.strictEqual(fixtureCoverage.functions[0].ranges[0].count, 1);
 }
 
 // Outputs coverage when the coverage directory is not absolute.
@@ -144,9 +144,9 @@ function nextdir() {
                                              absoluteCoverageDirectory);
   assert.ok(fixtureCoverage);
   // first branch executed.
-  assert.strictEqual(fixtureCoverage.functions[1].ranges[0].count, 1);
+  assert.strictEqual(fixtureCoverage.functions[0].ranges[0].count, 1);
   // second branch did not execute.
-  assert.strictEqual(fixtureCoverage.functions[1].ranges[1].count, 0);
+  assert.strictEqual(fixtureCoverage.functions[0].ranges[1].count, 0);
 }
 
 // Extracts the coverage object for a given fixture name.