console: move the inspector console wrapping in a separate file

Move the wrapping of the inspector console in a separate file for clarity. In addition, save the original console from the VM explicitly via an exported property `require('internal/console/inspector').consoleFromVM` that `require('inspector').console` can alias to it later, instead of hanging the original console onto `per_thread.js` during bootstrap and counting on that `per_thread.js` only gets evaluated once and gets cached. PR-URL: #24709 Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
nodejs · Jan 10, 2019 · 6280cda · 6280cda
1 parent 2f1f009
commit 6280cda
Show file tree

Hide file tree

Showing 5 changed files with 84 additions and 55 deletions.
diff --git a/lib/inspector.js b/lib/inspector.js
@@ -12,7 +12,6 @@ const {
 const { validateString } = require('internal/validators');
 const util = require('util');
 const { Connection, open, url } = internalBinding('inspector');
-const { originalConsole } = require('internal/process/per_thread');
 
 if (!Connection)
   throw new ERR_INSPECTOR_NOT_AVAILABLE();
@@ -103,6 +102,8 @@ module.exports = {
   open: (port, host, wait) => open(port, host, !!wait),
   close: process._debugEnd,
   url: url,
-  console: originalConsole,
+  // This is dynamically added during bootstrap,
+  // where the console from the VM is still available
+  console: require('internal/console/inspector').consoleFromVM,
   Session
 };
diff --git a/lib/internal/bootstrap/node.js b/lib/internal/bootstrap/node.js
@@ -121,8 +121,6 @@
 
     const browserGlobals = !process._noBrowserGlobals;
     if (browserGlobals) {
-      // We are setting this here to forward it to the inspector later
-      perThreadSetup.originalConsole = global.console;
       setupGlobalTimeouts();
       setupGlobalConsole();
       setupGlobalURL();
@@ -486,16 +484,25 @@
   }
 
   function setupGlobalConsole() {
-    const originalConsole = global.console;
-    // Setup Node.js global.console.
-    const wrappedConsole = NativeModule.require('console');
+    const consoleFromVM = global.console;
+    const consoleFromNode =
+      NativeModule.require('internal/console/global');
+    // Override global console from the one provided by the VM
+    // to the one implemented by Node.js
     Object.defineProperty(global, 'console', {
       configurable: true,
       enumerable: false,
-      value: wrappedConsole,
+      value: consoleFromNode,
       writable: true
     });
-    setupInspector(originalConsole, wrappedConsole);
+    // TODO(joyeecheung): can we skip this if inspector is not active?
+    if (process.config.variables.v8_enable_inspector) {
+      const inspector =
+        NativeModule.require('internal/console/inspector');
+      inspector.addInspectorApis(consoleFromNode, consoleFromVM);
+      // This will be exposed by `require('inspector').console` later.
+      inspector.consoleFromVM = consoleFromVM;
+    }
   }
 
   function setupGlobalURL() {
@@ -570,41 +577,6 @@
     registerDOMException(DOMException);
   }
 
-  function setupInspector(originalConsole, wrappedConsole) {
-    if (!process.config.variables.v8_enable_inspector) {
-      return;
-    }
-    const CJSModule = NativeModule.require('internal/modules/cjs/loader');
-    const { addCommandLineAPI, consoleCall } = process.binding('inspector');
-    // Setup inspector command line API.
-    const { makeRequireFunction } =
-      NativeModule.require('internal/modules/cjs/helpers');
-    const path = NativeModule.require('path');
-    const cwd = tryGetCwd(path);
-
-    const consoleAPIModule = new CJSModule('<inspector console>');
-    consoleAPIModule.paths =
-        CJSModule._nodeModulePaths(cwd).concat(CJSModule.globalPaths);
-    addCommandLineAPI('require', makeRequireFunction(consoleAPIModule));
-    const config = {};
-    for (const key of Object.keys(wrappedConsole)) {
-      if (!originalConsole.hasOwnProperty(key))
-        continue;
-      // If global console has the same method as inspector console,
-      // then wrap these two methods into one. Native wrapper will preserve
-      // the original stack.
-      wrappedConsole[key] = consoleCall.bind(wrappedConsole,
-                                             originalConsole[key],
-                                             wrappedConsole[key],
-                                             config);
-    }
-    for (const key of Object.keys(originalConsole)) {
-      if (wrappedConsole.hasOwnProperty(key))
-        continue;
-      wrappedConsole[key] = originalConsole[key];
-    }
-  }
-
   function noop() {}
 
   function setupProcessFatal() {
@@ -683,17 +655,6 @@
     }
   }
 
-  function tryGetCwd(path) {
-    try {
-      return process.cwd();
-    } catch {
-      // getcwd(3) can fail if the current working directory has been deleted.
-      // Fall back to the directory name of the (absolute) executable path.
-      // It's not really correct but what are the alternatives?
-      return path.dirname(process.execPath);
-    }
-  }
-
   function wrapForBreakOnFirstLine(source) {
     if (!process._breakFirstLine)
       return source;
@@ -704,6 +665,7 @@
   function evalScript(name, body) {
     const CJSModule = NativeModule.require('internal/modules/cjs/loader');
     const path = NativeModule.require('path');
+    const { tryGetCwd } = NativeModule.require('internal/util');
     const cwd = tryGetCwd(path);
 
     const module = new CJSModule(name);

diff --git a/lib/internal/console/inspector.js b/lib/internal/console/inspector.js
@@ -0,0 +1,53 @@
+'use strict';
+
+const path = require('path');
+const CJSModule = require('internal/modules/cjs/loader');
+const { makeRequireFunction } = require('internal/modules/cjs/helpers');
+const { tryGetCwd } = require('internal/util');
+const { addCommandLineAPI, consoleCall } = process.binding('inspector');
+
+// Wrap a console implemented by Node.js with features from the VM inspector
+function addInspectorApis(consoleFromNode, consoleFromVM) {
+  // Setup inspector command line API.
+  const cwd = tryGetCwd(path);
+  const consoleAPIModule = new CJSModule('<inspector console>');
+  consoleAPIModule.paths =
+      CJSModule._nodeModulePaths(cwd).concat(CJSModule.globalPaths);
+  addCommandLineAPI('require', makeRequireFunction(consoleAPIModule));
+  const config = {};
+
+  // If global console has the same method as inspector console,
+  // then wrap these two methods into one. Native wrapper will preserve
+  // the original stack.
+  for (const key of Object.keys(consoleFromNode)) {
+    if (!consoleFromVM.hasOwnProperty(key))
+      continue;
+    consoleFromNode[key] = consoleCall.bind(consoleFromNode,
+                                            consoleFromVM[key],
+                                            consoleFromNode[key],
+                                            config);
+  }
+
+  // Add additional console APIs from the inspector
+  for (const key of Object.keys(consoleFromVM)) {
+    if (consoleFromNode.hasOwnProperty(key))
+      continue;
+    consoleFromNode[key] = consoleFromVM[key];
+  }
+}
+
+module.exports = {
+  addInspectorApis
+};
+
+// Stores the console from VM, should be set during bootstrap.
+let consoleFromVM;
+
+Object.defineProperty(module.exports, 'consoleFromVM', {
+  get() {
+    return consoleFromVM;
+  },
+  set(val) {
+    consoleFromVM = val;
+  }
+});
diff --git a/lib/internal/util.js b/lib/internal/util.js
@@ -387,6 +387,17 @@ function once(callback) {
   };
 }
 
+function tryGetCwd(path) {
+  try {
+    return process.cwd();
+  } catch {
+    // getcwd(3) can fail if the current working directory has been deleted.
+    // Fall back to the directory name of the (absolute) executable path.
+    // It's not really correct but what are the alternatives?
+    return path.dirname(process.execPath);
+  }
+}
+
 module.exports = {
   assertCrypto,
   cachedResult,
@@ -406,6 +417,7 @@ module.exports = {
   once,
   promisify,
   spliceOne,
+  tryGetCwd,
   removeColors,
 
   // Symbol used to customize promisify conversion

diff --git a/node.gyp b/node.gyp
@@ -98,6 +98,7 @@
       'lib/internal/cluster/worker.js',
       'lib/internal/console/constructor.js',
       'lib/internal/console/global.js',
+      'lib/internal/console/inspector.js',
       'lib/internal/crypto/certificate.js',
       'lib/internal/crypto/cipher.js',
       'lib/internal/crypto/diffiehellman.js',