src: simplify NativeModule caching and remove redundant data

- Remove `NativeModule._source` - the compilation is now entirely
  done in C++ and `process.binding('natives')` is implemented
  directly in the binding loader so there is no need to store
  additional source code strings.
- Instead of using an object as `NativeModule._cached` and insert
  into it after compilation of each native module, simply prebuild
  a JS map filled with all the native modules and infer the
  state of compilation through `mod.loading`/`mod.loaded`.
- Rename `NativeModule.nonInternalExists` to
  `NativeModule.canBeRequiredByUsers` and precompute that
  property for all the native modules during bootstrap instead
  of branching in every require call during runtime. This also fixes
  the bug where `worker_threads` can be made available with
  `--expose-internals`.
- Rename `NativeModule.requireForDeps` to
  `NativeModule.requireWithFallbackInDeps`.
- Add a test to make sure we do not accidentally leak any module
  to the global namespace.

PR-URL: https://github.com/nodejs/node/pull/25352
Reviewed-By: Anna Henningsen <anna@addaleax.net>

lib/internal/bootstrap/cache.js

@@ -7,14 +7,10 @@
 
 const { NativeModule } = require('internal/bootstrap/loaders');
 const {
-  source, getCodeCache, compileFunction
+  getCodeCache, compileFunction
 } = internalBinding('native_module');
 const { hasTracing, hasInspector } = process.binding('config');
 
-const depsModule = Object.keys(source).filter(
-  (key) => NativeModule.isDepsModule(key) || key.startsWith('internal/deps')
-);
-
 // Modules with source code compiled in js2c that
 // cannot be compiled with the code cache.
 const cannotUseCache = [
@@ -29,7 +25,7 @@ const cannotUseCache = [
   // the code cache is also used when compiling these two files.
   'internal/bootstrap/loaders',
   'internal/bootstrap/node'
-].concat(depsModule);
+];
 
 // Skip modules that cannot be required when they are not
 // built into the binary.
@@ -67,11 +63,18 @@ if (!process.versions.openssl) {
   );
 }
 
+const cachableBuiltins = [];
+for (const id of NativeModule.map.keys()) {
+  if (id.startsWith('internal/deps')) {
+    cannotUseCache.push(id);
+  }
+  if (!cannotUseCache.includes(id)) {
+    cachableBuiltins.push(id);
+  }
+}
+
 module.exports = {
-  cachableBuiltins: Object.keys(source).filter(
+  cachableBuiltins,
-    (key) => !cannotUseCache.includes(key)
-  ),
-  getSource(id) { return source[id]; },
   getCodeCache,
   compileFunction,
   cannotUseCache

lib/internal/bootstrap/loaders.js

@@ -158,6 +158,12 @@ let internalBinding;
 // Create this WeakMap in js-land because V8 has no C++ API for WeakMap.
 internalBinding('module_wrap').callbackMap = new WeakMap();
 
+// Think of this as module.exports in this file even though it is not
+// written in CommonJS style.
+const loaderExports = { internalBinding, NativeModule };
+const loaderId = 'internal/bootstrap/loaders';
+const config = internalBinding('config');
+
 // Set up NativeModule.
 function NativeModule(id) {
   this.filename = `${id}.js`;
@@ -167,34 +173,35 @@ function NativeModule(id) {
   this.exportKeys = undefined;
   this.loaded = false;
   this.loading = false;
+  if (id === loaderId) {
+    // Do not expose this to user land even with --expose-internals.
+    this.canBeRequiredByUsers = false;
+  } else if (id.startsWith('internal/')) {
+    this.canBeRequiredByUsers = config.exposeInternals;
+  } else {
+    this.canBeRequiredByUsers = true;
+  }
 }
 
 const {
-  source,
+  moduleIds,
   compileFunction
 } = internalBinding('native_module');
 
-NativeModule._source = source;
+NativeModule.map = new Map();
-NativeModule._cache = {};
+for (var i = 0; i < moduleIds.length; ++i) {
-
+  const id = moduleIds[i];
-const config = internalBinding('config');
+  const mod = new NativeModule(id);
-
+  NativeModule.map.set(id, mod);
-// Think of this as module.exports in this file even though it is not
+}
-// written in CommonJS style.
-const loaderExports = { internalBinding, NativeModule };
-const loaderId = 'internal/bootstrap/loaders';
 
 NativeModule.require = function(id) {
   if (id === loaderId) {
     return loaderExports;
   }
 
-  const cached = NativeModule.getCached(id);
+  const mod = NativeModule.map.get(id);
-  if (cached && (cached.loaded || cached.loading)) {
+  if (!mod) {
-    return cached.exports;
-  }
-
-  if (!NativeModule.exists(id)) {
     // Model the error off the internal/errors.js model, but
     // do not use that module given that it could actually be
     // the one causing the error if there's a bug in Node.js.
@@ -205,60 +212,31 @@ NativeModule.require = function(id) {
     throw err;
   }
 
-  moduleLoadList.push(`NativeModule ${id}`);
+  if (mod.loaded || mod.loading) {
-
+    return mod.exports;
-  const nativeModule = new NativeModule(id);
-
-  nativeModule.cache();
-  nativeModule.compile();
-
-  return nativeModule.exports;
-};
-
-NativeModule.isDepsModule = function(id) {
-  return id.startsWith('node-inspect/') || id.startsWith('v8/');
-};
-
-NativeModule.requireForDeps = function(id) {
-  if (!NativeModule.exists(id)) {
-    id = `internal/deps/${id}`;
   }
-  return NativeModule.require(id);
-};
 
-NativeModule.getCached = function(id) {
+  moduleLoadList.push(`NativeModule ${id}`);
-  return NativeModule._cache[id];
+  mod.compile();
+  return mod.exports;
 };
 
 NativeModule.exists = function(id) {
-  return NativeModule._source.hasOwnProperty(id);
+  return NativeModule.map.has(id);
 };
 
-if (config.exposeInternals) {
+NativeModule.canBeRequiredByUsers = function(id) {
-  NativeModule.nonInternalExists = function(id) {
+  const mod = NativeModule.map.get(id);
-    // Do not expose this to user land even with --expose-internals.
+  return mod && mod.canBeRequiredByUsers;
-    if (id === loaderId) {
+};
-      return false;
-    }
-    return NativeModule.exists(id);
-  };
 
-  NativeModule.isInternal = function(id) {
+// Allow internal modules from dependencies to require
-    // Do not expose this to user land even with --expose-internals.
+// other modules from dependencies by providing fallbacks.
-    return id === loaderId;
+NativeModule.requireWithFallbackInDeps = function(request) {
-  };
+  if (!NativeModule.map.has(request)) {
-} else {
+    request = `internal/deps/${request}`;
-  NativeModule.nonInternalExists = function(id) {
+  }
-    return NativeModule.exists(id) && !NativeModule.isInternal(id);
+  return NativeModule.require(request);
-  };
-
-  NativeModule.isInternal = function(id) {
-    return id.startsWith('internal/');
-  };
-}
-
-NativeModule.getSource = function(id) {
-  return NativeModule._source[id];
 };
 
 const getOwn = (target, property, receiver) => {
@@ -332,13 +310,13 @@ NativeModule.prototype.compile = function() {
 
   try {
     const requireFn = this.id.startsWith('internal/deps/') ?
-      NativeModule.requireForDeps :
+      NativeModule.requireWithFallbackInDeps :
       NativeModule.require;
 
     const fn = compileFunction(id);
     fn(this.exports, requireFn, this, process, internalBinding);
 
-    if (config.experimentalModules && !NativeModule.isInternal(this.id)) {
+    if (config.experimentalModules && this.canBeRequiredByUsers) {
       this.proxifyExports();
     }
 
@@ -348,10 +326,6 @@ NativeModule.prototype.compile = function() {
   }
 };
 
-NativeModule.prototype.cache = function() {
-  NativeModule._cache[this.id] = this;
-};
-
 // Coverage must be turned on early, so that we can collect
 // it for Node.js' own internal libraries.
 if (process.env.NODE_V8_COVERAGE) {

lib/internal/modules/cjs/loader.js

@@ -112,8 +112,12 @@ function Module(id, parent) {
   this.children = [];
 }
 
-const builtinModules = Object.keys(NativeModule._source)
+const builtinModules = [];
-  .filter(NativeModule.nonInternalExists);
+for (const [id, mod] of NativeModule.map) {
+  if (mod.canBeRequiredByUsers) {
+    builtinModules.push(id);
+  }
+}
 
 Object.freeze(builtinModules);
 Module.builtinModules = builtinModules;
@@ -417,7 +421,7 @@ if (isWindows) {
 var indexChars = [ 105, 110, 100, 101, 120, 46 ];
 var indexLen = indexChars.length;
 Module._resolveLookupPaths = function(request, parent, newReturn) {
-  if (NativeModule.nonInternalExists(request)) {
+  if (NativeModule.canBeRequiredByUsers(request)) {
     debug('looking for %j in []', request);
     return (newReturn ? null : [request, []]);
   }
@@ -534,7 +538,7 @@ Module._load = function(request, parent, isMain) {
     return cachedModule.exports;
   }
 
-  if (NativeModule.nonInternalExists(filename)) {
+  if (NativeModule.canBeRequiredByUsers(filename)) {
     debug('load native module %s', request);
     return NativeModule.require(filename);
   }
@@ -567,7 +571,7 @@ function tryModuleLoad(module, filename) {
 }
 
 Module._resolveFilename = function(request, parent, isMain, options) {
-  if (NativeModule.nonInternalExists(request)) {
+  if (NativeModule.canBeRequiredByUsers(request)) {
     return request;
   }
 

lib/internal/modules/esm/default_resolve.js

@@ -53,7 +53,7 @@ const extensionFormatMap = {
 };
 
 function resolve(specifier, parentURL) {
-  if (NativeModule.nonInternalExists(specifier)) {
+  if (NativeModule.canBeRequiredByUsers(specifier)) {
     return {
       url: specifier,
       format: 'builtin'

lib/internal/modules/esm/translators.js

@@ -81,7 +81,7 @@ translators.set('builtin', async (url) => {
   // slice 'node:' scheme
   const id = url.slice(5);
   NativeModule.require(id);
-  const module = NativeModule.getCached(id);
+  const module = NativeModule.map.get(id);
   return createDynamicModule(
     [...module.exportKeys, 'default'], url, (reflect) => {
       debug(`Loading BuiltinModule ${url}`);

src/node_native_module.cc

@@ -80,11 +80,21 @@ void NativeModuleLoader::GetCacheUsage(
   args.GetReturnValue().Set(result);
 }
 
-void NativeModuleLoader::SourceObjectGetter(
+void NativeModuleLoader::ModuleIdsGetter(
     Local<Name> property, const PropertyCallbackInfo<Value>& info) {
   Local<Context> context = info.GetIsolate()->GetCurrentContext();
-  info.GetReturnValue().Set(
+  Isolate* isolate = info.GetIsolate();
-      per_process::native_module_loader.GetSourceObject(context));
+
+  const NativeModuleRecordMap& source_ =
+      per_process::native_module_loader.source_;
+  std::vector<Local<Value>> ids;
+  ids.reserve(source_.size());
+
+  for (auto const& x : source_) {
+    ids.push_back(OneByteString(isolate, x.first.c_str(), x.first.size()));
+  }
+
+  info.GetReturnValue().Set(Array::New(isolate, ids.data(), ids.size()));
 }
 
 void NativeModuleLoader::ConfigStringGetter(
@@ -303,8 +313,8 @@ void NativeModuleLoader::Initialize(Local<Object> target,
             .FromJust());
   CHECK(target
             ->SetAccessor(env->context(),
-                          env->source_string(),
+                          FIXED_ONE_BYTE_STRING(env->isolate(), "moduleIds"),
-                          SourceObjectGetter,
+                          ModuleIdsGetter,
                           nullptr,
                           MaybeLocal<Value>(),
                           DEFAULT,

src/node_native_module.h

@@ -56,11 +56,10 @@ class NativeModuleLoader {
 
  private:
   static void GetCacheUsage(const v8::FunctionCallbackInfo<v8::Value>& args);
-  // Passing map of builtin module source code into JS land as
+  // Passing ids of builtin module source code into JS land as
-  // internalBinding('native_module').source
+  // internalBinding('native_module').moduleIds
-  static void SourceObjectGetter(
+  static void ModuleIdsGetter(v8::Local<v8::Name> property,
-      v8::Local<v8::Name> property,
+                              const v8::PropertyCallbackInfo<v8::Value>& info);
-      const v8::PropertyCallbackInfo<v8::Value>& info);
   // Passing config.gypi into JS land as internalBinding('native_module').config
   static void ConfigStringGetter(
       v8::Local<v8::Name> property,

test/code-cache/test-code-cache.js

@@ -5,15 +5,12 @@
 // and the cache is used when built in modules are compiled.
 // Otherwise, verifies that no cache is used when compiling builtins.
 
-require('../common');
+const { isMainThread } = require('../common');
 const assert = require('assert');
 const {
   cachableBuiltins,
   cannotUseCache
 } = require('internal/bootstrap/cache');
-const {
-  isMainThread
-} = require('worker_threads');
 
 const {
   internalBinding

test/parallel/test-internal-module-require.js

@@ -0,0 +1,112 @@
+'use strict';
+
+// Flags: --expose-internals
+// This verifies that
+// 1. We do not leak internal modules unless the --require-internals option
+//    is on.
+// 2. We do not accidentally leak any modules to the public global scope.
+// 3. Deprecated modules are properly deprecated.
+
+const common = require('../common');
+
+if (!common.isMainThread) {
+  common.skip('Cannot test the existence of --expose-internals from worker');
+}
+
+const assert = require('assert');
+const fork = require('child_process').fork;
+
+const expectedPublicModules = new Set([
+  '_http_agent',
+  '_http_client',
+  '_http_common',
+  '_http_incoming',
+  '_http_outgoing',
+  '_http_server',
+  '_stream_duplex',
+  '_stream_passthrough',
+  '_stream_readable',
+  '_stream_transform',
+  '_stream_wrap',
+  '_stream_writable',
+  '_tls_common',
+  '_tls_wrap',
+  'assert',
+  'async_hooks',
+  'buffer',
+  'child_process',
+  'cluster',
+  'console',
+  'constants',
+  'crypto',
+  'dgram',
+  'dns',
+  'domain',
+  'events',
+  'fs',
+  'http',
+  'http2',
+  'https',
+  'inspector',
+  'module',
+  'net',
+  'os',
+  'path',
+  'perf_hooks',
+  'process',
+  'punycode',
+  'querystring',
+  'readline',
+  'repl',
+  'stream',
+  'string_decoder',
+  'sys',
+  'timers',
+  'tls',
+  'trace_events',
+  'tty',
+  'url',
+  'util',
+  'v8',
+  'vm',
+  'worker_threads',
+  'zlib'
+]);
+
+if (process.argv[2] === 'child') {
+  assert(!process.execArgv.includes('--expose-internals'));
+  process.once('message', ({ allBuiltins }) => {
+    const publicModules = new Set();
+    for (const id of allBuiltins) {
+      if (id.startsWith('internal/')) {
+        common.expectsError(() => {
+          require(id);
+        }, {
+          code: 'MODULE_NOT_FOUND',
+          message: `Cannot find module '${id}'`
+        });
+      } else {
+        require(id);
+        publicModules.add(id);
+      }
+    }
+    assert(allBuiltins.length > publicModules.size);
+    // Make sure all the public modules are available through
+    // require('module').builtinModules
+    assert.deepStrictEqual(
+      publicModules,
+      new Set(require('module').builtinModules)
+    );
+    assert.deepStrictEqual(publicModules, expectedPublicModules);
+  });
+} else {
+  assert(process.execArgv.includes('--expose-internals'));
+  const child = fork(__filename, ['child'], {
+    execArgv: []
+  });
+  const { builtinModules } = require('module');
+  // When --expose-internals is on, require('module').builtinModules
+  // contains internal modules.
+  const message = { allBuiltins: builtinModules };
+  child.send(message);
+}