inspector: --debug* deprecation and invalidation

PR-URL: https://github.com/nodejs/node/pull/12949
Fixes: https://github.com/nodejs/node/issues/12364
Fixes: https://github.com/nodejs/node/issues/12630
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>

lib/internal/bootstrap_node.js

@@ -65,6 +65,20 @@
     });
     process.argv[0] = process.execPath;
 
+    // Handle `--debug*` deprecation and invalidation
+    if (process._invalidDebug) {
+      process.emitWarning(
+        '`node --debug` and `node --debug-brk` are invalid. ' +
+        'Please use `node --inspect` or `node --inspect-brk` instead.',
+        'DeprecationWarning', 'DEP0062', startup, true);
+      process.exit(9);
+    } else if (process._deprecatedDebugBrk) {
+      process.emitWarning(
+        '`node --inspect --debug-brk` is deprecated. ' +
+        'Please use `node --inspect-brk` instead.',
+        'DeprecationWarning', 'DEP0062', startup, true);
+    }
+
     // There are various modes that Node can run in. The most common two
     // are running from a script and running the REPL - but there are a few
     // others like the debugger or running --eval arguments. Here we decide

lib/internal/process/warning.js

@@ -111,7 +111,7 @@ function setupProcessWarnings() {
   // process.emitWarning(error)
   // process.emitWarning(str[, type[, code]][, ctor])
   // process.emitWarning(str[, options])
-  process.emitWarning = function(warning, type, code, ctor) {
+  process.emitWarning = function(warning, type, code, ctor, now) {
     const errors = lazyErrors();
     var detail;
     if (type !== null && typeof type === 'object' && !Array.isArray(type)) {
@@ -150,6 +150,7 @@ function setupProcessWarnings() {
       if (process.throwDeprecation)
         throw warning;
     }
-    process.nextTick(doEmitWarning(warning));
+    if (now) process.emit('warning', warning);
+    else process.nextTick(doEmitWarning(warning));
   };
 }

lib/module.js

@@ -537,7 +537,7 @@ Module.prototype._compile = function(content, filename) {
   });
 
   var inspectorWrapper = null;
-  if (process._debugWaitConnect && process._eval == null) {
+  if (process._breakFirstLine && process._eval == null) {
     if (!resolvedArgv) {
       // we enter the repl if we're not given a filename argument.
       if (process.argv[1]) {
@@ -549,7 +549,7 @@ Module.prototype._compile = function(content, filename) {
 
     // Set breakpoint on module start
     if (filename === resolvedArgv) {
-      delete process._debugWaitConnect;
+      delete process._breakFirstLine;
       inspectorWrapper = process.binding('inspector').callAndPauseOnStart;
       if (!inspectorWrapper) {
         const Debug = vm.runInDebugContext('Debug');

src/node.cc

@@ -3391,9 +3391,29 @@ void SetupProcessObject(Environment* env,
     READONLY_PROPERTY(process, "traceDeprecation", True(env->isolate()));
   }
 
+  // TODO(refack): move the following 4 to `node_config`
+  // --inspect
+  if (debug_options.inspector_enabled()) {
+    READONLY_DONT_ENUM_PROPERTY(process,
+                                "_inspectorEnbale", True(env->isolate()));
+  }
+
   // --inspect-brk
   if (debug_options.wait_for_connect()) {
-    READONLY_PROPERTY(process, "_debugWaitConnect", True(env->isolate()));
+    READONLY_DONT_ENUM_PROPERTY(process,
+                                "_breakFirstLine", True(env->isolate()));
+  }
+
+  // --inspect --debug-brk
+  if (debug_options.deprecated_invocation()) {
+    READONLY_DONT_ENUM_PROPERTY(process,
+                                "_deprecatedDebugBrk", True(env->isolate()));
+  }
+
+  // --debug or, --debug-brk without --inspect
+  if (debug_options.invalid_invocation()) {
+    READONLY_DONT_ENUM_PROPERTY(process,
+                                "_invalidDebug", True(env->isolate()));
   }
 
   // --security-revert flags

src/node_debug_options.cc

@@ -8,9 +8,7 @@
 namespace node {
 
 namespace {
-#if HAVE_INSPECTOR
 const int default_inspector_port = 9229;
-#endif  // HAVE_INSPECTOR
 
 inline std::string remove_brackets(const std::string& host) {
   if (!host.empty() && host.front() == '[' && host.back() == ']')
@@ -56,14 +54,12 @@ std::pair<std::string, int> split_host_port(const std::string& arg) {
 }  // namespace
 
 DebugOptions::DebugOptions() :
-#if HAVE_INSPECTOR
                                inspector_enabled_(false),
-#endif  // HAVE_INSPECTOR
-                               wait_connect_(false), http_enabled_(false),
+                               deprecated_debug_(false),
+                               break_first_line_(false),
                                host_name_("127.0.0.1"), port_(-1) { }
 
 bool DebugOptions::ParseOption(const char* argv0, const std::string& option) {
-  bool enable_inspector = false;
   bool has_argument = false;
   std::string option_name;
   std::string argument;
@@ -81,11 +77,21 @@ bool DebugOptions::ParseOption(const char* argv0, const std::string& option) {
       argument.clear();
   }
 
+  // Note that --debug-port and --debug-brk in conjuction with --inspect
+  // work but are undocumented.
+  // --debug is no longer valid.
+  // Ref: https://github.com/nodejs/node/issues/12630
+  // Ref: https://github.com/nodejs/node/pull/12949
   if (option_name == "--inspect") {
-    enable_inspector = true;
+    inspector_enabled_ = true;
+  } else if (option_name == "--debug") {
+    deprecated_debug_ = true;
   } else if (option_name == "--inspect-brk") {
-    enable_inspector = true;
-    wait_connect_ = true;
+    inspector_enabled_ = true;
+    break_first_line_ = true;
+  } else if (option_name == "--debug-brk") {
+    break_first_line_ = true;
+    deprecated_debug_ = true;
   } else if (option_name == "--debug-port" ||
              option_name == "--inspect-port") {
     if (!has_argument) {
@@ -97,15 +103,14 @@ bool DebugOptions::ParseOption(const char* argv0, const std::string& option) {
     return false;
   }
 
-  if (enable_inspector) {
-#if HAVE_INSPECTOR
-    inspector_enabled_ = true;
-#else
+#if !HAVE_INSPECTOR
+  if (inspector_enabled_) {
     fprintf(stderr,
             "Inspector support is not available with this Node.js build\n");
-    return false;
-#endif
   }
+  inspector_enabled_ = false;
+  return false;
+#endif
 
   // argument can be specified for *any* option to specify host:port
   if (has_argument) {
@@ -124,9 +129,7 @@ bool DebugOptions::ParseOption(const char* argv0, const std::string& option) {
 int DebugOptions::port() const {
   int port = port_;
   if (port < 0) {
-#if HAVE_INSPECTOR
     port = default_inspector_port;
-#endif  // HAVE_INSPECTOR
   }
   return port;
 }

src/node_debug_options.h

@@ -10,25 +10,24 @@ class DebugOptions {
  public:
   DebugOptions();
   bool ParseOption(const char* argv0, const std::string& option);
-  bool inspector_enabled() const {
-#if HAVE_INSPECTOR
-    return inspector_enabled_;
-#else
-    return false;
-#endif  // HAVE_INSPECTOR
+  bool inspector_enabled() const { return inspector_enabled_; }
+  bool deprecated_invocation() const {
+    return deprecated_debug_ &&
+      inspector_enabled_ &&
+      break_first_line_;
   }
-  bool ToolsServerEnabled();
-  bool wait_for_connect() const { return wait_connect_; }
+  bool invalid_invocation() const {
+    return deprecated_debug_ && !inspector_enabled_;
+  }
+  bool wait_for_connect() const { return break_first_line_; }
   std::string host_name() const { return host_name_; }
   int port() const;
   void set_port(int port) { port_ = port; }
 
  private:
-#if HAVE_INSPECTOR
   bool inspector_enabled_;
-#endif  // HAVE_INSPECTOR
-  bool wait_connect_;
-  bool http_enabled_;
+  bool deprecated_debug_;
+  bool break_first_line_;
   std::string host_name_;
   int port_;
 };

test/inspector/inspector-helper.js

@@ -496,9 +496,9 @@ Harness.prototype.kill = function() {
 };
 
 exports.startNodeForInspectorTest = function(callback,
-                                             inspectorFlag = '--inspect-brk',
+                                             inspectorFlags = ['--inspect-brk'],
                                              opt_script_contents) {
-  const args = [inspectorFlag];
+  const args = [].concat(inspectorFlags);
   if (opt_script_contents) {
     args.push('-e', opt_script_contents);
   } else {

test/inspector/test-inspector-debug-brk.js

@@ -0,0 +1,57 @@
+'use strict';
+const common = require('../common');
+common.skipIfInspectorDisabled();
+const assert = require('assert');
+const helper = require('./inspector-helper.js');
+
+function setupExpectBreakOnLine(line, url, session, scopeIdCallback) {
+  return function(message) {
+    if ('Debugger.paused' === message['method']) {
+      const callFrame = message['params']['callFrames'][0];
+      const location = callFrame['location'];
+      assert.strictEqual(url, session.scriptUrlForId(location['scriptId']));
+      assert.strictEqual(line, location['lineNumber']);
+      scopeIdCallback &&
+          scopeIdCallback(callFrame['scopeChain'][0]['object']['objectId']);
+      return true;
+    }
+  };
+}
+
+function testBreakpointOnStart(session) {
+  const commands = [
+    { 'method': 'Runtime.enable' },
+    { 'method': 'Debugger.enable' },
+    { 'method': 'Debugger.setPauseOnExceptions',
+      'params': {'state': 'none'} },
+    { 'method': 'Debugger.setAsyncCallStackDepth',
+      'params': {'maxDepth': 0} },
+    { 'method': 'Profiler.enable' },
+    { 'method': 'Profiler.setSamplingInterval',
+      'params': {'interval': 100} },
+    { 'method': 'Debugger.setBlackboxPatterns',
+      'params': {'patterns': []} },
+    { 'method': 'Runtime.runIfWaitingForDebugger' }
+  ];
+
+  session
+    .sendInspectorCommands(commands)
+    .expectMessages(setupExpectBreakOnLine(0, session.mainScriptPath, session));
+}
+
+function testWaitsForFrontendDisconnect(session, harness) {
+  console.log('[test]', 'Verify node waits for the frontend to disconnect');
+  session.sendInspectorCommands({ 'method': 'Debugger.resume'})
+    .expectStderrOutput('Waiting for the debugger to disconnect...')
+    .disconnect(true);
+}
+
+function runTests(harness) {
+  harness
+    .runFrontendSession([
+      testBreakpointOnStart,
+      testWaitsForFrontendDisconnect
+    ]).expectShutDown(55);
+}
+
+helper.startNodeForInspectorTest(runTests, ['--inspect', '--debug-brk']);

test/parallel/test-inspector-invalid-args.js

@@ -0,0 +1,23 @@
+'use strict';
+const assert = require('assert');
+const execFile = require('child_process').execFile;
+const path = require('path');
+
+const common = require('../common');
+common.skipIfInspectorDisabled();
+
+const mainScript = path.join(common.fixturesDir, 'loop.js');
+const expected =
+  '`node --debug` and `node --debug-brk` are invalid. ' +
+  'Please use `node --inspect` or `node --inspect-brk` instead.';
+for (const invalidArg of ['--debug-brk', '--debug']) {
+  execFile(
+    process.execPath,
+    [ invalidArg, mainScript ],
+    common.mustCall((error, stdout, stderr) => {
+      assert.strictEqual(error.code, 9, `node ${invalidArg} should exit 9`);
+      assert.strictEqual(stderr.includes(expected), true,
+                         `${stderr} should include '${expected}'`);
+    })
+  );
+}

test/sequential/test-debugger-debug-brk.js

@@ -0,0 +1,27 @@
+'use strict';
+const common = require('../common');
+common.skipIfInspectorDisabled();
+const assert = require('assert');
+const spawn = require('child_process').spawn;
+
+const script = common.fixturesDir + '/empty.js';
+
+function test(arg) {
+  const child = spawn(process.execPath, ['--inspect', arg, script]);
+  const argStr = child.spawnargs.join(' ');
+  const fail = () => assert.fail(true, false, `'${argStr}' should not quit`);
+  child.on('exit', fail);
+
+  // give node time to start up the debugger
+  setTimeout(function() {
+    child.removeListener('exit', fail);
+    child.kill();
+  }, 2000);
+
+  process.on('exit', function() {
+    assert(child.killed);
+  });
+}
+
+test('--debug-brk');
+test('--debug-brk=5959');