module: do not share the internal require function with public loaders

joyeecheung · joyeecheung · commit 84156cf10ed1 · 2019-03-19T04:05:38.000+08:00
This patch removes `NativeModule.require` and `NativeModule.requireWithFallbackInDeps`. The public loaders now have to use a special method `NativeModule.prototype.compileForPublicLoader()` to compile native modules. In addition this patch moves the decisions of proxifying exports and throwing unknown builtin errors entirely to public loaders, and skip those during internal use - therefore `loaders.js`, which is compiled during bootstrap, no longer needs to be aware of the value of `--experimental-modules`. PR-URL: #26549 Reviewed-By: James M Snell <jasnell@gmail.com>
diff --git a/lib/internal/bootstrap/loaders.js b/lib/internal/bootstrap/loaders.js
@@ -19,7 +19,7 @@
 //   can be created using NODE_MODULE_CONTEXT_AWARE_CPP() with the flag
 //   NM_F_LINKED.
 // - internalBinding(): the private internal C++ binding loader, inaccessible
-//   from user land because they are only available from NativeModule.require().
+//   from user land unless through `require('internal/test/binding')`.
 //   These C++ bindings are created using NODE_MODULE_CONTEXT_AWARE_INTERNAL()
 //   and have their nm_flags set to NM_F_INTERNAL.
 //
@@ -42,7 +42,7 @@
 // This file is compiled as if it's wrapped in a function with arguments
 // passed by node::RunBootstrapping()
 /* global process, getLinkedBinding, getInternalBinding */
-/* global experimentalModules, exposeInternals, primordials */
+/* global exposeInternals, primordials */
 
 const {
   Reflect,
@@ -185,27 +185,9 @@ function nativeModuleRequire(id) {
   }
 
   const mod = NativeModule.map.get(id);
-  if (!mod) {
-    // 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.
-    // eslint-disable-next-line no-restricted-syntax
-    const err = new Error(`No such built-in module: ${id}`);
-    err.code = 'ERR_UNKNOWN_BUILTIN_MODULE';
-    err.name = 'Error [ERR_UNKNOWN_BUILTIN_MODULE]';
-    throw err;
-  }
-
-  if (mod.loaded || mod.loading) {
-    return mod.exports;
-  }
-
-  moduleLoadList.push(`NativeModule ${id}`);
-  mod.compile();
-  return mod.exports;
+  return mod.compile();
 }
 
-NativeModule.require = nativeModuleRequire;
 NativeModule.exists = function(id) {
   return NativeModule.map.has(id);
 };
@@ -217,11 +199,25 @@ NativeModule.canBeRequiredByUsers = function(id) {
 
 // Allow internal modules from dependencies to require
 // other modules from dependencies by providing fallbacks.
-NativeModule.requireWithFallbackInDeps = function(request) {
+function requireWithFallbackInDeps(request) {
   if (!NativeModule.map.has(request)) {
     request = `internal/deps/${request}`;
   }
-  return NativeModule.require(request);
+  return nativeModuleRequire(request);
+}
+
+// This is exposed for public loaders
+NativeModule.prototype.compileForPublicLoader = function(needToProxify) {
+  if (!this.canBeRequiredByUsers) {
+    // No code because this is an assertion against bugs
+    // eslint-disable-next-line no-restricted-syntax
+    throw new Error(`Should not compile ${this.id} for public use`);
+  }
+  this.compile();
+  if (needToProxify && !this.exportKeys) {
+    this.proxifyExports();
+  }
+  return this.exports;
 };
 
 const getOwn = (target, property, receiver) => {
@@ -289,26 +285,27 @@ NativeModule.prototype.proxifyExports = function() {
 };
 
 NativeModule.prototype.compile = function() {
-  const id = this.id;
+  if (this.loaded || this.loading) {
+    return this.exports;
+  }
 
+  const id = this.id;
   this.loading = true;
 
   try {
     const requireFn = this.id.startsWith('internal/deps/') ?
-      NativeModule.requireWithFallbackInDeps :
-      NativeModule.require;
+      requireWithFallbackInDeps : nativeModuleRequire;
 
     const fn = compileFunction(id);
     fn(this.exports, requireFn, this, process, internalBinding, primordials);
 
-    if (experimentalModules && this.canBeRequiredByUsers) {
-      this.proxifyExports();
-    }
-
     this.loaded = true;
   } finally {
     this.loading = false;
   }
+
+  moduleLoadList.push(`NativeModule ${id}`);
+  return this.exports;
 };
 
 // This will be passed to internal/bootstrap/node.js.
diff --git a/lib/internal/bootstrap/node.js b/lib/internal/bootstrap/node.js
@@ -33,13 +33,12 @@
 
 // This file is compiled as if it's wrapped in a function with arguments
 // passed by node::RunBootstrapping()
-/* global process, loaderExports, isMainThread, ownsProcessState */
+/* global process, require, internalBinding, isMainThread, ownsProcessState */
 /* global primordials */
 
-const { internalBinding, NativeModule } = loaderExports;
 const { Object, Symbol } = primordials;
 const config = internalBinding('config');
-const { deprecate } = NativeModule.require('internal/util');
+const { deprecate } = require('internal/util');
 
 setupProcessObject();
 
@@ -50,17 +49,17 @@ process.domain = null;
 process._exiting = false;
 
 // Bootstrappers for all threads, including worker threads and main thread
-const perThreadSetup = NativeModule.require('internal/process/per_thread');
+const perThreadSetup = require('internal/process/per_thread');
 // Bootstrappers for the main thread only
 let mainThreadSetup;
 // Bootstrappers for the worker threads only
 let workerThreadSetup;
 if (ownsProcessState) {
-  mainThreadSetup = NativeModule.require(
+  mainThreadSetup = require(
     'internal/process/main_thread_only'
   );
 } else {
-  workerThreadSetup = NativeModule.require(
+  workerThreadSetup = require(
     'internal/process/worker_thread_only'
   );
 }
@@ -116,14 +115,18 @@ if (isMainThread) {
 
 const {
   emitWarning
-} = NativeModule.require('internal/process/warning');
+} = require('internal/process/warning');
 
 process.emitWarning = emitWarning;
 
+const {
+  setupTaskQueue,
+  queueMicrotask
+} = require('internal/process/task_queues');
 const {
   nextTick,
   runNextTicks
-} = NativeModule.require('internal/process/task_queues').setupTaskQueue();
+} = setupTaskQueue();
 
 process.nextTick = nextTick;
 // Used to emulate a tick manually in the JS land.
@@ -161,7 +164,7 @@ if (credentials.implementsPosixCredentials) {
 
 if (isMainThread) {
   const { getStdout, getStdin, getStderr } =
-    NativeModule.require('internal/process/stdio').getMainThreadStdio();
+    require('internal/process/stdio').getMainThreadStdio();
   setupProcessStdio(getStdout, getStdin, getStderr);
 } else {
   const { getStdout, getStdin, getStderr } =
@@ -173,7 +176,7 @@ if (config.hasInspector) {
   const {
     enable,
     disable
-  } = NativeModule.require('internal/inspector_async_hook');
+  } = require('internal/inspector_async_hook');
   internalBinding('inspector').registerAsyncHook(enable, disable);
 }
 
@@ -183,30 +186,27 @@ if (!config.noBrowserGlobals) {
   // https://console.spec.whatwg.org/#console-namespace
   exposeNamespace(global, 'console', createGlobalConsole(global.console));
 
-  const { URL, URLSearchParams } = NativeModule.require('internal/url');
+  const { URL, URLSearchParams } = require('internal/url');
   // https://url.spec.whatwg.org/#url
   exposeInterface(global, 'URL', URL);
   // https://url.spec.whatwg.org/#urlsearchparams
   exposeInterface(global, 'URLSearchParams', URLSearchParams);
 
   const {
     TextEncoder, TextDecoder
-  } = NativeModule.require('internal/encoding');
+  } = require('internal/encoding');
   // https://encoding.spec.whatwg.org/#textencoder
   exposeInterface(global, 'TextEncoder', TextEncoder);
   // https://encoding.spec.whatwg.org/#textdecoder
   exposeInterface(global, 'TextDecoder', TextDecoder);
 
   // https://html.spec.whatwg.org/multipage/webappapis.html#windoworworkerglobalscope
-  const timers = NativeModule.require('timers');
+  const timers = require('timers');
   defineOperation(global, 'clearInterval', timers.clearInterval);
   defineOperation(global, 'clearTimeout', timers.clearTimeout);
   defineOperation(global, 'setInterval', timers.setInterval);
   defineOperation(global, 'setTimeout', timers.setTimeout);
 
-  const {
-    queueMicrotask
-  } = NativeModule.require('internal/process/task_queues');
   defineOperation(global, 'queueMicrotask', queueMicrotask);
 
   // Non-standard extensions:
@@ -265,7 +265,7 @@ Object.defineProperty(process, 'features', {
     fatalException,
     setUncaughtExceptionCaptureCallback,
     hasUncaughtExceptionCaptureCallback
-  } = NativeModule.require('internal/process/execution');
+  } = require('internal/process/execution');
 
   process._fatalException = fatalException;
   process.setUncaughtExceptionCaptureCallback =
@@ -275,7 +275,7 @@ Object.defineProperty(process, 'features', {
 }
 
 function setupProcessObject() {
-  const EventEmitter = NativeModule.require('events');
+  const EventEmitter = require('events');
   const origProcProto = Object.getPrototypeOf(process);
   Object.setPrototypeOf(origProcProto, EventEmitter.prototype);
   EventEmitter.call(process);
@@ -359,7 +359,7 @@ function setupGlobalProxy() {
 }
 
 function setupBuffer() {
-  const { Buffer } = NativeModule.require('buffer');
+  const { Buffer } = require('buffer');
   const bufferBinding = internalBinding('buffer');
 
   // Only after this point can C++ use Buffer::New()
@@ -377,9 +377,9 @@ function setupBuffer() {
 
 function createGlobalConsole(consoleFromVM) {
   const consoleFromNode =
-    NativeModule.require('internal/console/global');
+    require('internal/console/global');
   if (config.hasInspector) {
-    const inspector = NativeModule.require('internal/util/inspector');
+    const inspector = require('internal/util/inspector');
     // This will be exposed by `require('inspector').console` later.
     inspector.consoleFromVM = consoleFromVM;
     // TODO(joyeecheung): postpone this until the first time inspector
diff --git a/lib/internal/errors.js b/lib/internal/errors.js
@@ -967,6 +967,7 @@ E('ERR_UNHANDLED_ERROR',
     if (err === undefined) return msg;
     return `${msg} (${err})`;
   }, Error);
+E('ERR_UNKNOWN_BUILTIN_MODULE', 'No such built-in module: %s', Error);
 E('ERR_UNKNOWN_CREDENTIAL', '%s identifier does not exist: %s', Error);
 E('ERR_UNKNOWN_ENCODING', 'Unknown encoding: %s', TypeError);
 
diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js
@@ -568,8 +568,8 @@ Module._resolveLookupPaths = function(request, parent, newReturn) {
 
 // Check the cache for the requested file.
 // 1. If a module already exists in the cache: return its exports object.
-// 2. If the module is native: call `NativeModule.require()` with the
-//    filename and return the result.
+// 2. If the module is native: call
+//    `NativeModule.prototype.compileForPublicLoader()` and return the exports.
 // 3. Otherwise, create a new module for the file and save it to the cache.
 //    Then have it load  the file contents before returning its exports
 //    object.
@@ -586,9 +586,10 @@ Module._load = function(request, parent, isMain) {
     return cachedModule.exports;
   }
 
-  if (NativeModule.canBeRequiredByUsers(filename)) {
+  const mod = NativeModule.map.get(filename);
+  if (mod && mod.canBeRequiredByUsers) {
     debug('load native module %s', request);
-    return NativeModule.require(filename);
+    return mod.compileForPublicLoader(experimentalModules);
   }
 
   // Don't call updateChildren(), Module constructor already does.
diff --git a/lib/internal/modules/esm/translators.js b/lib/internal/modules/esm/translators.js
@@ -21,7 +21,9 @@ const {
 const { URL } = require('url');
 const { debuglog, promisify } = require('util');
 const esmLoader = require('internal/process/esm_loader');
-
+const {
+  ERR_UNKNOWN_BUILTIN_MODULE
+} = require('internal/errors').codes;
 const readFileAsync = promisify(fs.readFile);
 const readFileSync = fs.readFileSync;
 const StringReplace = FunctionPrototype.call.bind(StringPrototype.replace);
@@ -85,8 +87,11 @@ translators.set('builtin', async (url) => {
   debug(`Translating BuiltinModule ${url}`);
   // slice 'node:' scheme
   const id = url.slice(5);
-  NativeModule.require(id);
   const module = NativeModule.map.get(id);
+  if (!module) {
+    throw new ERR_UNKNOWN_BUILTIN_MODULE(id);
+  }
+  module.compileForPublicLoader(true);
   return createDynamicModule(
     [...module.exportKeys, 'default'], url, (reflect) => {
       debug(`Loading BuiltinModule ${url}`);
diff --git a/src/node.cc b/src/node.cc
@@ -296,8 +296,6 @@ MaybeLocal<Value> RunBootstrapping(Environment* env) {
       env->process_string(),
       FIXED_ONE_BYTE_STRING(isolate, "getLinkedBinding"),
       FIXED_ONE_BYTE_STRING(isolate, "getInternalBinding"),
-      // --experimental-modules
-      FIXED_ONE_BYTE_STRING(isolate, "experimentalModules"),
       // --expose-internals
       FIXED_ONE_BYTE_STRING(isolate, "exposeInternals"),
       env->primordials_string()};
@@ -309,7 +307,6 @@ MaybeLocal<Value> RunBootstrapping(Environment* env) {
       env->NewFunctionTemplate(binding::GetInternalBinding)
           ->GetFunction(context)
           .ToLocalChecked(),
-      Boolean::New(isolate, env->options()->experimental_modules),
       Boolean::New(isolate, env->options()->expose_internals),
       env->primordials()};
 
@@ -331,16 +328,19 @@ MaybeLocal<Value> RunBootstrapping(Environment* env) {
       loader_exports_obj->Get(context, env->require_string()).ToLocalChecked();
   env->set_native_module_require(require.As<Function>());
 
-  // process, loaderExports, isMainThread, ownsProcessState, primordials
+  // process, require, internalBinding, isMainThread,
+  // ownsProcessState, primordials
   std::vector<Local<String>> node_params = {
       env->process_string(),
-      FIXED_ONE_BYTE_STRING(isolate, "loaderExports"),
+      env->require_string(),
+      env->internal_binding_string(),
       FIXED_ONE_BYTE_STRING(isolate, "isMainThread"),
       FIXED_ONE_BYTE_STRING(isolate, "ownsProcessState"),
       env->primordials_string()};
   std::vector<Local<Value>> node_args = {
       process,
-      loader_exports_obj,
+      require,
+      internal_binding_loader,
       Boolean::New(isolate, env->is_main_thread()),
       Boolean::New(isolate, env->owns_process_state()),
       env->primordials()};
