inspector: --debug* deprecation and invalidation

PR-URL: #12949Fixes: #12364Fixes: #12630
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>

Author: refack

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);
+}elseif(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){
 consterrors=lazyErrors();
 vardetail;
 if(type!==null&&typeoftype==='object'&&!Array.isArray(type)){
@@ -150,6 +150,7 @@ function setupProcessWarnings() {
 if(process.throwDeprecation)
 throwwarning;
 }
-process.nextTick(doEmitWarning(warning));
+if(now)process.emit('warning',warning);
+elseprocess.nextTick(doEmitWarning(warning));
 };
 }

lib/module.js

@@ -537,7 +537,7 @@ Module.prototype._compile = function(content, filename) {
 });
 
 varinspectorWrapper=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){
-deleteprocess._debugWaitConnect;
+deleteprocess._breakFirstLine;
 inspectorWrapper=process.binding('inspector').callAndPauseOnStart;
 if(!inspectorWrapper){
 constDebug=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 @@
 namespacenode {
 
 namespace {
-#if HAVE_INSPECTOR
 constint 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) { }
 
 boolDebugOptions::ParseOption(constchar* 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;
+ } elseif (option_name == "--debug") {
+ deprecated_debug_ = true;
  } elseif (option_name == "--inspect-brk") {
- enable_inspector = true;
- wait_connect_ = true;
+ inspector_enabled_ = true;
+ break_first_line_ = true;
+ } elseif (option_name == "--debug-brk") {
+ break_first_line_ = true;
+ deprecated_debug_ = true;
  } elseif (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) {
 returnfalse;
  }
 
-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");
-returnfalse;
-#endif
  }
+ inspector_enabled_ = false;
+returnfalse;
+#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) {
 intDebugOptions::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();
 boolParseOption(constchar* argv0, const std::string& option);
-boolinspector_enabled() const {
-#if HAVE_INSPECTOR
-return inspector_enabled_;
-#else
-returnfalse;
-#endif// HAVE_INSPECTOR
+boolinspector_enabled() const { return inspector_enabled_; }
+booldeprecated_invocation() const {
+return deprecated_debug_ &&
+ inspector_enabled_ &&
+ break_first_line_;
  }
-boolToolsServerEnabled();
-boolwait_for_connect() const { return wait_connect_; }
+boolinvalid_invocation() const {
+return deprecated_debug_ && !inspector_enabled_;
+ }
+boolwait_for_connect() const { return break_first_line_; }
  std::string host_name() const { return host_name_; }
 intport() const;
 voidset_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){
-constargs=[inspectorFlag];
+constargs=[].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';
+constcommon=require('../common');
+common.skipIfInspectorDisabled();
+constassert=require('assert');
+consthelper=require('./inspector-helper.js');
+
+functionsetupExpectBreakOnLine(line,url,session,scopeIdCallback){
+returnfunction(message){
+if('Debugger.paused'===message['method']){
+constcallFrame=message['params']['callFrames'][0];
+constlocation=callFrame['location'];
+assert.strictEqual(url,session.scriptUrlForId(location['scriptId']));
+assert.strictEqual(line,location['lineNumber']);
+scopeIdCallback&&
+scopeIdCallback(callFrame['scopeChain'][0]['object']['objectId']);
+returntrue;
+}
+};
+}
+
+functiontestBreakpointOnStart(session){
+constcommands=[
+{'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));
+}
+
+functiontestWaitsForFrontendDisconnect(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);
+}
+
+functionrunTests(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';
+constassert=require('assert');
+constexecFile=require('child_process').execFile;
+constpath=require('path');
+
+constcommon=require('../common');
+common.skipIfInspectorDisabled();
+
+constmainScript=path.join(common.fixturesDir,'loop.js');
+constexpected=
+'`node --debug` and `node --debug-brk` are invalid. '+
+'Please use `node --inspect` or `node --inspect-brk` instead.';
+for(constinvalidArgof['--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';
+constcommon=require('../common');
+common.skipIfInspectorDisabled();
+constassert=require('assert');
+constspawn=require('child_process').spawn;
+
+constscript=common.fixturesDir+'/empty.js';
+
+functiontest(arg){
+constchild=spawn(process.execPath,['--inspect',arg,script]);
+constargStr=child.spawnargs.join(' ');
+constfail=()=>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');